[Refactor] onyx key user #8264

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

@@ -113,7 +113,8 @@ function getUserDetails() {
            const loginList = _.where(response.loginList, {partnerName: 'expensify.com'});
            const expensifyNewsStatus = lodashGet(response, 'account.subscribed', true);
            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

@@ -113,7 +113,8 @@ function getUserDetails() {
            const loginList = _.where(response.loginList, {partnerName: 'expensify.com'});
            const expensifyNewsStatus = lodashGet(response, 'account.subscribed', true);
            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

@@ -71,6 +71,9 @@ export default {
    // 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 marcaaron and a team 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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Refactor] onyx key user #8264

[Refactor] onyx key user #8264

mdneyazahmad commented Mar 22, 2022 •

edited

Loading

parasharrajat Mar 22, 2022 •

edited

Loading

marcaaron Mar 22, 2022

parasharrajat Mar 22, 2022

parasharrajat left a comment

marcaaron Mar 22, 2022

marcaaron commented Mar 22, 2022

mdneyazahmad commented Mar 23, 2022

parasharrajat left a comment

Beamanator left a comment

Beamanator Mar 23, 2022

mdneyazahmad commented Mar 24, 2022

parasharrajat left a comment •

edited by marcaaron

Loading

marcaaron left a comment

parasharrajat commented Mar 24, 2022 •

edited

Loading

mdneyazahmad commented Mar 28, 2022

parasharrajat commented Mar 28, 2022

mdneyazahmad commented Mar 28, 2022

parasharrajat left a comment

OSBotify commented Mar 28, 2022

OSBotify commented Mar 30, 2022

OSBotify commented Apr 5, 2022

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

[Refactor] onyx key user #8264

[Refactor] onyx key user #8264

Conversation

mdneyazahmad commented Mar 22, 2022 • edited Loading

Details

Fixed Issues

Tests

PR Review Checklist

Contributor (PR Author) Checklist

PR Reviewer Checklist

QA Steps

Screenshots

Web

Mobile Web

Desktop

iOS

Android

parasharrajat Mar 22, 2022 • edited Loading

Choose a reason for hiding this comment

marcaaron Mar 22, 2022

Choose a reason for hiding this comment

parasharrajat Mar 22, 2022

Choose a reason for hiding this comment

parasharrajat left a comment

Choose a reason for hiding this comment

marcaaron Mar 22, 2022

Choose a reason for hiding this comment

marcaaron commented Mar 22, 2022

mdneyazahmad commented Mar 23, 2022

parasharrajat left a comment

Choose a reason for hiding this comment

Beamanator left a comment

Choose a reason for hiding this comment

Beamanator Mar 23, 2022

Choose a reason for hiding this comment

mdneyazahmad commented Mar 24, 2022

parasharrajat left a comment • edited by marcaaron Loading

Choose a reason for hiding this comment

PR Reviewer Checklist

marcaaron left a comment

Choose a reason for hiding this comment

parasharrajat commented Mar 24, 2022 • edited Loading

mdneyazahmad commented Mar 28, 2022

parasharrajat commented Mar 28, 2022

mdneyazahmad commented Mar 28, 2022

parasharrajat left a comment

Choose a reason for hiding this comment

OSBotify commented Mar 28, 2022

OSBotify commented Mar 30, 2022

OSBotify commented Apr 5, 2022

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