Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

user.expensifyNewsStatus is overwritten on page reload #3569

Closed
roryabraham opened this issue Jun 12, 2021 · 14 comments · Fixed by #3717
Closed

user.expensifyNewsStatus is overwritten on page reload #3569

roryabraham opened this issue Jun 12, 2021 · 14 comments · Fixed by #3717
Assignees
Labels
Engineering Improvement Item broken or needs improvement. Weekly KSv2

Comments

@roryabraham
Copy link
Contributor

roryabraham commented Jun 12, 2021

If you haven’t already, check out our contributing guidelines for onboarding and email contributors@expensify.com to request to join our Slack channel!


Action Performed:

  1. Go to Settings -> Preferences

  2. Toggle the Notifications switch:

    image

  3. Open the JS console and go to Application -> LocalStorage -> [host]

  4. Search for the user value in localstorage, and confirm that expensifyNewsStatus is set to true or false

    image

  5. If necessary, toggle it back to true. Verify that the value is set to true in localstorage.

  6. Refresh the page.

Expected Result:

The value in localStorage should remain true. The toggle switch should be active.

Actual Result:

The value in localStorage is 0. The toggle switch is deactivated.

Workaround:

None.

Platform:

Where is this issue occurring?

Web ❌
iOS
Android
Desktop App
Mobile Web

Version Number: Version 1.0.65-0 (1.0.65-0)
Notes/Photos/Videos: Any additional supporting documentation
Expensify/Expensify Issue URL: https://github.com/Expensify/Expensify/issues/167397
Upwork job link: https://www.upwork.com/jobs/~01984a0737ed8ba518.

View all open jobs on Upwork

@roryabraham roryabraham added the AutoAssignerTriage Auto assign issues for triage to an available triage team member label Jun 12, 2021
@MelvinBot
Copy link

Triggered auto assignment to @michaelhaxhiu (AutoAssignerTriage), see https://stackoverflow.com/c/expensify/questions/4749 for more details.

@MelvinBot MelvinBot removed the AutoAssignerTriage Auto assign issues for triage to an available triage team member label Jun 12, 2021
@roryabraham roryabraham added Daily KSv2 Improvement Item broken or needs improvement. labels Jun 12, 2021
@MelvinBot
Copy link

Triggered auto assignment to @Jag96 (Exported), see https://stackoverflow.com/c/expensify/questions/7972 for more details.

@michaelhaxhiu
Copy link
Contributor

Posted on Upwork here - https://www.upwork.com/jobs/~01984a0737ed8ba518.

@parasharrajat
Copy link
Member

parasharrajat commented Jun 15, 2021

@Jag96
It seems the backend is not saving the changes it's returning 0 account: Object { subscribed: 0 } on User.getUserDetails();
So when we toggle it we send the request to the backend, here if the code is 200 we set expensifyNewsStatus to true. But when we refresh User.getUserDetails(); returns 0.

@Jag96
Copy link
Contributor

Jag96 commented Jun 16, 2021

@parasharrajat good find! For context, after looking into this it seems that on the API side we've updated the value of subscribed to be an int rather than a boolean, and it wasn't updated in this codebase. I've created a PR on the API side to fix the issue so that the subscribed value works when sent as a boolean or integer.

It looks like there's still an issue here where the value is being stored as an integer (prop warning), so this issue can remain and the fix can be to update the value that is stored in Onyx so it is a boolean rather than an integer.

checkPropTypes.js:20 Warning: Failed prop type: Invalid prop `user.expensifyNewsStatus` of type `number` supplied to `PreferencesPage`, expected `boolean`.
    in PreferencesPage

cc @jasperhuangg since you're reviewing the API PR

@parasharrajat
Copy link
Member

I agree @Jag96. Please let me know when the PR is merged for the backend. Thanks.

@roryabraham
Copy link
Contributor Author

The API change has been deployed to production.

@parasharrajat
Copy link
Member

@Jag96 If this is still available for external. Then we just need to convert the Integer from API response to boolean before saving it to Onyx.

https://github.com/Expensify/Expensify.cash/blob/408b7e00cf33c16600af0b67b819c764fcb5388a/src/libs/actions/User.js#L72
TO

 Onyx.merge(ONYXKEYS.USER, {loginList, expensifyNewsStatus: !!expensifyNewsStatus});

@michaelhaxhiu
Copy link
Contributor

@parasharrajat yep it's still available for external. I'll let @Jag96 review your proposal here as the best next step

@Jag96
Copy link
Contributor

Jag96 commented Jun 22, 2021

@parasharrajat that solution sounds good to me! I've invited you to the Upwork job, once you accept feel free to create a PR.

@parasharrajat
Copy link
Member

PR is up here #3717

@Jag96 Jag96 changed the title user.expensifyNewsStatus is overwritten on page reload [HOLD for payment June 28th] user.expensifyNewsStatus is overwritten on page reload Jun 22, 2021
@Jag96
Copy link
Contributor

Jag96 commented Jun 22, 2021

Reopening to keep track of payment hold time

@Jag96 Jag96 reopened this Jun 22, 2021
@michaelhaxhiu
Copy link
Contributor

@Jag96 are we good to pay this today or do we want to wait for the latest PR to hit production? It's on staging right now #3717

@Jag96
Copy link
Contributor

Jag96 commented Jun 29, 2021

I think this is fine to pay out now since its a very small change, it's been merged for 7 days, and I just confirmed it's working properly on staging. Paid!

@Jag96 Jag96 closed this as completed Jun 29, 2021
@michaelhaxhiu michaelhaxhiu changed the title [HOLD for payment June 28th] user.expensifyNewsStatus is overwritten on page reload user.expensifyNewsStatus is overwritten on page reload Jun 29, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Engineering Improvement Item broken or needs improvement. Weekly KSv2
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants