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

Added helpers to handle non-stringified values when parsing keys from localStorage #662

Merged
merged 3 commits into from
Oct 8, 2021

Conversation

sreeram-venkitesh
Copy link
Contributor

@sreeram-venkitesh sreeram-venkitesh commented Oct 7, 2021

Fixes #661

Changes made

  • Added localStorage helper at javascript/src/helpers/storage.js, similar to what is done in granite
  • Added check inside getFromLocalStorage method to check if existing value is JSON stringified. If not, the value is replaced with null.
  • Replaced all existing localStorage.setItem() and localStorage.getItem() with setToLocalStorage() and getFromLocalStorage() methods from the helper.

@yedhink _a

@bot-bigbinary bot-bigbinary temporarily deployed to wheel-pr-662 October 7, 2021 08:12 Inactive
Copy link
Contributor

@yedhink yedhink left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@sreeram-venkitesh _a

I have added some comments. Please go through them and let me know if you need any clarifications.

@yedhink yedhink changed the title Fix to handle non stringified values when JSON parsing localStorage Added helpers to handle non-stringified values when parsing keys from localStorage Oct 7, 2021
@yedhink
Copy link
Contributor

yedhink commented Oct 7, 2021

@sreeram-venkitesh Also, the title of the PR needs to be in the past tense. We don't follow imperative present tense in commit messages nor PR titles.

I always give this reference to someone who starts new and is working with me: https://github.com/bigbinary/bigbinary-website/pull/781#pullrequestreview-677312016

PS: If we need to discuss or bring something to my attention or need my help, then the review comment can be left unresolved.

@bot-bigbinary bot-bigbinary temporarily deployed to wheel-pr-662 October 7, 2021 15:35 Inactive
@sreeram-venkitesh
Copy link
Contributor Author

@yedhink _r

I have added the following changes as per your comments

  • Added storedValue variable
  • Removed redundant JSON.parse()

I have added a reply in the comment about adding logger.error() statement. Please review it. Thank you

@yedhink
Copy link
Contributor

yedhink commented Oct 8, 2021

@sreeram-venkitesh _a

LGTM.

One last thing, verify the following with this PR:

  • Open Granite in localhost:3000. Log in. But don't log out.
  • Close the server
  • Open Wheel in localhost:3000. Try to log in.
  • Expectation is that we are able to open up wheel without any issues.

Once you've verified, you can assign me using _a syntax. Note: It's _a and not _r. There is no macro for _r. All these macros are defined in our own gitemit bot.

The idea is that we assign the PR back if the merging of the PR now falls in the hand of someone else. Assigning to someone else means my assigned task has been done. For asking queries we don't assign, but rather mention.

cc: @AbhayVAshokan @subins2000 @jagannathBhat @prasidhprabudhan You folks might've encountered a similar error while setting up wheel. Once this PR is merged to wheel, make sure you pull the latest from wheel.

Copy link
Contributor

@yedhink yedhink left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@sreeram-venkitesh _a

Noticed this now. One more minor change is required.

@bot-bigbinary bot-bigbinary temporarily deployed to wheel-pr-662 October 8, 2021 14:14 Inactive
@sreeram-venkitesh
Copy link
Contributor Author

@yedhink _a

Changed the order of the return statement as mentioned in the review, in my last commit.

I also tried the steps you mentioned and tried switching from granite to wheel. Once I started wheel's server, reloading the logged in granite page gave the home screen for wheel. This is the intended behaviour if we already have authToken and authEmail stored from a different project.

Please review the changes. Thank you.

@gitemit gitemit bot assigned yedhink and unassigned sreeram-venkitesh Oct 8, 2021
@yedhink yedhink merged commit d799a09 into master Oct 8, 2021
@yedhink yedhink deleted the 661-handle-non-stringified-localstorage branch October 8, 2021 14:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Handle non stringified auth tokens which are not JSON parseable
3 participants