department-of-veterans-affairs / va-mobile-app

"If VA were a company, it would have a flagship mobile app."
https://department-of-veterans-affairs.github.io/va-mobile-app/
13 stars 2 forks source link

BUG - sev-3 - All - Don't do API call for PN prefs if sid is null #7926

Open TKDickson opened 7 months ago

TKDickson commented 7 months ago

What happened?

We do a call for a veteran's PN preferences when they navigate to the Notification preferences page, regardless of if they have a SID or not. When they haven't been registered successfully for push notifications, that call looks like this: /mobile/v0/push/prefs/null and will always fail (should instead be /mobile/v0/push/prefs/######with their ID if it's going to have a chance of succeeding).

Specs:

Steps to Reproduce

Can get this on Android by failing/blocking the /mobile/v0/push/register calls during login (definitely initial login, and subsequent logins if you've never turned on PN), then going to Profile > Settings > Notifications.

Desired behavior

We should just not make a failing call - can we handle that error/edge state better on the notification preferences screen? Ex - check for ID, make a call to register if they don't have one, then call for preferences.

Acceptance Criteria

Bug Severity - BE SURE TO ADD THE SEVERITY LABEL

See [Bug Tracking](https://department-of-veterans-affairs.github.io/va-mobile-app/docs/QA#issue-severity) for details on severity levels

Linked to Story

Screen shot(s) and additional information

Full JSON response for services related to issue (expand/collapse)

Ticket Checklist

TKDickson commented 2 months ago

At my WIP limit; marking as blocked

TKDickson commented 2 months ago

Picking this up, removing blocked label

TKDickson commented 2 months ago

Picking this up, removing blocked label

TKDickson commented 2 months ago

May not be able to test this at all for Android (how charles + webview login don't play nicely together on that platform).

For iOS, I believe we should be displaying the error message from ticket #1656 under the following circumstances, but currently are displaying the PN prefs screen, empty of PN pref toggles (see screenshot after circumstances).

Screenshot of what screen actually looks like:

image.png

We do call the PN registration API call during authentication (or biometric 'session recovery'), so these users probably wouldn't be in PN limbo indefinitely. But we could make their limbo state clearer, for sure.

TKDickson commented 2 months ago

@theodur two things:

theodur commented 3 weeks ago

Assigned Dylan to this since this is now part of the PR for #8253

TKDickson commented 3 weeks ago

Have been blocked on this since Tuesday - there's a reverse proxy server somewhere in the staging notification ecosystem that's not playing nicely. Hopefully resolved today.

TKDickson commented 3 weeks ago

DSVA support thread is resolved and I've confirmed I can successfully register for notifications, as well as fetch preferences, from upstream servers. This is unblocked and I can start working on it tomorrow.

TKDickson commented 2 weeks ago

On a build from this branch from today, we are correctly not calling the preferences endpoint if SID is null (aka they have not yet registered for push notifications)

TKDickson commented 4 days ago

Adding a comment to myself mostly - reconfirming that in the latest RQ branch, everything I wanted, all of the weird behaviors documented originally & in comments in this ticket, are fixed and working as expected. No further testing needed on this ticket, unless mandated by fixes to the other stuff going back and forth.