[Refactor] onyx key user by mdneyazahmad · Pull Request #8264 · Expensify/App

mdneyazahmad · 2022-03-22T14:14:18Z

Details

This pr refactors loginList from onyx key user to a separate key loginList.

Fixed Issues

$ #8051

Tests

This pr touches 5 files. ProfilePage and RequestCallPage are the pages affected.

Web (Optional)
1. Open the app on Web browser. Go to Developer console -> Application -> IndexedDB -> OnyxDB. In the sidebar, select http://localhost:8080/ -> keyvaluepairs.
2. Verify that a key named loginList exists and it is an array of login details.
All Platforms
1. Go to the profile page via Settings -> Profile.
2. Verify that you can see saved email and phone number (if any)
3. Go to Request setup call Page via Settings -> Workspace -> ? in header -> Request setup call.
4. Verify that you can see the phone number (if saved).
Verify that no errors appear in the JS console

PR Review Checklist

Contributor (PR Author) Checklist

PR Reviewer Checklist

QA Steps

Web (Optional)
1. Open the app on Web browser. Go to Developer console -> Application -> IndexedDB -> OnyxDB. In the sidebar, select http://localhost:8080/ -> keyvaluepairs.
2. Verify that a key named loginList exists and it is an array of login details.
All Platforms
1. Go to the profile page via Settings -> Profile.
2. Verify that you can see saved email and phone number (if any)
3. Go to Request setup call Page via Settings -> Workspace -> ? in header -> Request setup call.
4. Verify that you can see the phone number (if saved).
Verify that no errors appear in the JS console

Screenshots

Web

web.mp4

Mobile Web

mweb.mp4

Desktop

desktop.mp4

iOS

ios.mp4

Android

android.mp4

src/pages/RequestCallPage.js

src/pages/settings/Profile/ProfilePage.js

parasharrajat · 2022-03-22T14:55:54Z

src/libs/actions/User.js

            const validatedStatus = lodashGet(response, 'account.validated', false);
-            Onyx.merge(ONYXKEYS.USER, {loginList, expensifyNewsStatus: !!expensifyNewsStatus, validated: !!validatedStatus});
+            Onyx.merge(ONYXKEYS.USER, {expensifyNewsStatus: !!expensifyNewsStatus, validated: !!validatedStatus});
+            Onyx.set(ONYXKEYS.USER_LOGIN_LIST, loginList);


~~Prefer merge~~. Not needed.

Wouldn't that make our loginList grow in size with lots of repeat logins?

Yeah, Correct. LodashMerge.....

parasharrajat

Let's get the screenshots of running app after login screen for each platform.

src/ONYXKEYS.js

marcaaron · 2022-03-22T17:33:40Z

src/libs/actions/User.js

            const validatedStatus = lodashGet(response, 'account.validated', false);
-            Onyx.merge(ONYXKEYS.USER, {loginList, expensifyNewsStatus: !!expensifyNewsStatus, validated: !!validatedStatus});
+            Onyx.merge(ONYXKEYS.USER, {expensifyNewsStatus: !!expensifyNewsStatus, validated: !!validatedStatus});
+            Onyx.set(ONYXKEYS.USER_LOGIN_LIST, loginList);


Wouldn't that make our loginList grow in size with lots of repeat logins?

src/pages/RequestCallPage.js

marcaaron · 2022-03-22T17:41:01Z

This pr touches 5 files. ProfilePage and RequestCallPage are the pages affected.
Verify userLoginList exists in browser storage.
Verify that above mentioned pages does not break.

These test steps could be more specific. Manual QA testers are not always going to recognize a flow by its filename. So let's tell them exactly how to visit these pages and what to test when they're on them.

Seems like this is related to a performance issue? Should we add a test to see if things are faster? cc @Beamanator

mdneyazahmad · 2022-03-23T06:22:12Z

Done

parasharrajat

Looks good. But we should clean the userPropTypes.js as well.

Beamanator

Overall looking good, a few more points of feedback:

What about userPropTypes.js? https://github.com/Expensify/App/blob/main/src/pages/settings/userPropTypes.js

I believe we should remove the references to loginList in those prop types

Also I agree with @marcaaron that the testing steps should be improved - Manual QA testers are not always going to recognize a flow by its filename. So let's tell them exactly how to visit these pages and what to test when they're on them.

Beamanator · 2022-03-23T12:47:50Z

src/ONYXKEYS.js

    // Contains all the users settings for the Settings page and sub pages
    USER: 'user',

+    // Contains user login list


Can we be more descriptive here? Something like:

Suggested change

// Contains user login list

// Contains metadata (partner, login, validation date) for all of the user's logins

mdneyazahmad · 2022-03-24T02:23:03Z

Updated

parasharrajat

LGTM.

cc: @Beamanator

PR Reviewer Checklist

🎀 👀 🎀 C+ reviewed

marcaaron

@parasharrajat can you bump us again for review when the QA steps are improved?

https://github.com/Expensify/App/blob/main/PR_REVIEW_GUIDELINES.md#example-of-good-steps

parasharrajat · 2022-03-24T19:36:36Z

I thought this is NO QA but yes.

@mdneyazahmad Please be more detailed in the QA steps.

Verify loginList exists in browser storage.

How?

Verify that ProfilePage (settings -> profile) and RequestCallPage (settings -> workspace -> ? in header -> request setup call) do not break.

How?

Verify that you can see phone no(if saved) on RequestCallPage.

How?

you should specify the exact steps to verify all the above.

mdneyazahmad · 2022-03-28T12:23:19Z

Updated

parasharrajat · 2022-03-28T15:23:00Z

@mdneyazahmad

Web (Optional)
1. Open the app on Web browser. Go to Developer console -> Application -> IndexedDB -> OnyxDB. In the sidebar, select http://localhost:8080 -> keyvaluepairs.
2. Verify that a key named loginList exists and it is an array of login details.
All Platforms
1. Go to the profile page via Settings -> Profile.
2. Verify that you can see saved email and phone number (if any)
3. Go to Request setup call Page via Settings -> Workspace -> ? in header -> Request setup call.
4. Verify that you can see the phone number (if saved).

mdneyazahmad · 2022-03-28T16:37:25Z

@parasharrajat Thank you

parasharrajat

LGTM.

OSBotify · 2022-03-28T18:24:14Z

✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release.

OSBotify · 2022-03-30T20:49:15Z

🚀 Deployed to staging by @Beamanator in version: 1.1.47-0 🚀

platform	result
🤖 android 🤖	success ✅
🖥 desktop 🖥	success ✅
🍎 iOS 🍎	success ✅
🕸 web 🕸	success ✅

OSBotify · 2022-04-05T17:31:22Z

🚀 Deployed to production by @roryabraham in version: 1.1.49-1 🚀

platform	result
🤖 android 🤖	success ✅
🖥 desktop 🖥	success ✅
🍎 iOS 🍎	success ✅
🕸 web 🕸	success ✅

mdneyazahmad added 2 commits March 21, 2022 12:23

refactor: onyx key user

084e0f5

fix: onyx set loginList

3d430c1

mdneyazahmad requested review from a team and marcaaron as code owners March 22, 2022 14:14

melvin-bot bot requested review from Beamanator and parasharrajat and removed request for a team March 22, 2022 14:14

parasharrajat suggested changes Mar 22, 2022

View reviewed changes

parasharrajat reviewed Mar 22, 2022

View reviewed changes

marcaaron assigned mdneyazahmad Mar 22, 2022

marcaaron reviewed Mar 22, 2022

View reviewed changes

mdneyazahmad added 2 commits March 23, 2022 09:03

docs: update comments

d34d307

refactor: change onyx key name

c34ed7b

parasharrajat suggested changes Mar 23, 2022

View reviewed changes

Beamanator requested changes Mar 23, 2022

View reviewed changes

mdneyazahmad added 2 commits March 24, 2022 07:42

docs: update comment

c900938

refactor: remove unused code

bc00572

parasharrajat approved these changes Mar 24, 2022

View reviewed changes

marcaaron self-requested a review March 24, 2022 19:27

marcaaron requested changes Mar 24, 2022

View reviewed changes

parasharrajat approved these changes Mar 28, 2022

View reviewed changes

marcaaron approved these changes Mar 28, 2022

View reviewed changes

marcaaron requested a review from Beamanator March 28, 2022 17:56

Beamanator approved these changes Mar 28, 2022

View reviewed changes

Beamanator merged commit 406599f into Expensify:main Mar 28, 2022

OSBotify mentioned this pull request Mar 30, 2022

Deploy Checklist: New Expensify 2022-03-30 #8378

Closed

47 tasks

botify mentioned this pull request Apr 5, 2022

[HOLD for payment 2022-04-12] Move user.loginList data to new, specialized ONYX key #8051

Closed

	// Contains user login list
	// Contains metadata (partner, login, validation date) for all of the user's logins

Conversation

mdneyazahmad commented Mar 22, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Details

Fixed Issues

Tests

PR Review Checklist

Contributor (PR Author) Checklist

PR Reviewer Checklist

QA Steps

Screenshots

Web

Mobile Web

Desktop

iOS

Android

Uh oh!

Uh oh!

Uh oh!

parasharrajat Mar 22, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

marcaaron Mar 22, 2022

Choose a reason for hiding this comment

Uh oh!

parasharrajat Mar 22, 2022

Choose a reason for hiding this comment

Uh oh!

parasharrajat left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

marcaaron Mar 22, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

marcaaron commented Mar 22, 2022

Uh oh!

mdneyazahmad commented Mar 23, 2022

Uh oh!

parasharrajat left a comment

Choose a reason for hiding this comment

Uh oh!

Beamanator left a comment

Choose a reason for hiding this comment

Uh oh!

Beamanator Mar 23, 2022

Choose a reason for hiding this comment

Uh oh!

mdneyazahmad commented Mar 24, 2022

Uh oh!

parasharrajat left a comment • edited by marcaaron Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

PR Reviewer Checklist

Uh oh!

marcaaron left a comment

Choose a reason for hiding this comment

Uh oh!

parasharrajat commented Mar 24, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mdneyazahmad commented Mar 28, 2022

Uh oh!

parasharrajat commented Mar 28, 2022

Uh oh!

mdneyazahmad commented Mar 28, 2022

Uh oh!

parasharrajat left a comment

Choose a reason for hiding this comment

Uh oh!

OSBotify commented Mar 28, 2022

Uh oh!

OSBotify commented Mar 30, 2022

Uh oh!

OSBotify commented Apr 5, 2022

Uh oh!

Reviewers

Assignees

mdneyazahmad commented Mar 22, 2022 •

edited

Loading

parasharrajat Mar 22, 2022 •

edited

Loading

parasharrajat left a comment •

edited by marcaaron

Loading

parasharrajat commented Mar 24, 2022 •

edited

Loading