App icon indicating copy to clipboard operation
App copied to clipboard

[HOLD for payment 2024-04-25] Profile - Personal details not open on off line mode

Open kavimuru opened this issue 1 year ago • 59 comments

If you haven’t already, check out our contributing guidelines for onboarding and email [email protected] to request to join our Slack channel!


Action Performed:

  1. https://staging.new.expensify.com/
  2. Log in with a HIGH TRAFFIC account.
  3. Click on the profile icon
  4. After login in with a High Traffic account - enable Airplane mode to simulate a connection loss
  5. Click on Personal Details

Expected Result:

Personal details is open and options are visible

Actual Result:

Personal details not open

Workaround:

Can the user still use Expensify without this being fixed? Have you informed them of the workaround?

Platforms:

Which of our officially supported platforms is this issue occurring on?

  • [ ] Android / native
  • [ ] Android / Chrome
  • [ ] iOS / native
  • [ ] iOS / Safari
  • [x] MacOS / Chrome / Safari
  • [ ] MacOS / Desktop

Version Number: 1.3.47-2 Reproducible in staging?: y Reproducible in production?: y If this was caught during regression testing, add the test name, ID and link from TestRail: Email or phone of affected tester (no customers): Logs: https://stackoverflow.com/c/expensify/questions/4856 Notes/Photos/Videos: Any additional supporting documentation

https://github.com/Expensify/App/assets/43996225/ccab7673-7f42-4dfa-b392-3c1ef488c5f4

Expensify/Expensify Issue URL: Issue reported by: applause internal team Slack conversation:

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~015f3b23a3e3d5db40
  • Upwork Job ID: 1686070604511633408
  • Last Price Increase: 2023-07-31
Issue OwnerCurrent Issue Owner: @grgia

kavimuru avatar Jul 29 '23 23:07 kavimuru

Triggered auto assignment to @trjExpensify (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details.

melvin-bot[bot] avatar Jul 29 '23 23:07 melvin-bot[bot]

Bug0 Triage Checklist (Main S/O)

  • [ ] This "bug" occurs on a supported platform (ensure Platforms in OP are ✅)
  • [ ] This bug is not a duplicate report (check E/App issues and #expensify-bugs)
    • If it is, comment with a link to the original report, close the issue and add any novel details to the original issue instead
  • [ ] This bug is reproducible using the reproduction steps in the OP. S/O
    • If the reproduction steps are clear and you're unable to reproduce the bug, check with the reporter and QA first, then close the issue.
    • If the reproduction steps aren't clear and you determine the correct steps, please update the OP.
  • [ ] This issue is filled out as thoroughly and clearly as possible
    • Pay special attention to the title, results, platforms where the bug occurs, and if the bug happens on staging/production.
  • [ ] I have reviewed and subscribed to the linked Slack conversation to ensure Slack/Github stay in sync

melvin-bot[bot] avatar Jul 29 '23 23:07 melvin-bot[bot]

Proposal

Please re-state the problem that we are trying to solve in this issue.

Profile - Personal details not open on offline mode

What is the root cause of that problem?

After we login successfully, openApp API is called but this doesn't return privatePersonalDetails of current user. When we offline, because the openPersonalDetailsPage API hasn't been called yet, props.privatePersonalDetails is undefined, and then the page loading infinitely.

https://github.com/Expensify/App/blob/eb020063789091eb2e3086b5b54af3ead8436645/src/pages/settings/Profile/PersonalDetails/PersonalDetailsInitialPage.js#L86-L89

What changes do you think we should make in order to solve the problem?

We have some suggestions to fix this issue

  1. BE should return privatePersonalDetails of current user in openApp API

  2. In componentDidMount of AuthScreen, we will call PersonalDetails.openPersonalDetailsPage(); to call openPersonalDetailsPage API

  3. If we don't want to do this we should return default false here to loading page not should

lodashGet(props.privatePersonalDetails, 'isLoading', false)

https://github.com/Expensify/App/blob/eb020063789091eb2e3086b5b54af3ead8436645/src/pages/settings/Profile/PersonalDetails/PersonalDetailsInitialPage.js#L86-L89

What alternative solutions did you explore? (Optional)

dukenv0307 avatar Jul 30 '23 04:07 dukenv0307

Proposal

Please re-state the problem that we are trying to solve in this issue.

Going offline before fetching the private personal details and clicking on personal details shows a continuous loading indicator

What is the root cause of that problem?

We fetch the private personal details with this hook here https://github.com/Expensify/App/blob/eb020063789091eb2e3086b5b54af3ead8436645/src/pages/settings/Profile/PersonalDetails/PersonalDetailsInitialPage.js#L59 and the hook does not call api when it is offline https://github.com/Expensify/App/blob/eb020063789091eb2e3086b5b54af3ead8436645/src/hooks/usePrivatePersonalDetails.js#L11-L15 but this here still shows the loading indicator https://github.com/Expensify/App/blob/eb020063789091eb2e3086b5b54af3ead8436645/src/pages/settings/Profile/PersonalDetails/PersonalDetailsInitialPage.js#L86-L88 because the default value is true in the conditional. So loading indicator is shown in cases where isLoading does not exist in privatePersonalDetails as well.

What changes do you think we should make in order to solve the problem?

I think showing loading indicator is fine in this case but we might show an indication that the user is offline especially for smaller screens so that they know the network got disconnected. For larger screens this might not be necessary because the report screen footer shows the offline indicator already. But if we like to cover the rare cases where the report screen shows Hmm it's not there screen, we can show offline indicator in the RHN for large screens also. This would require changing the return statement to

return (
     <>
        <FullscreenLoadingIndicator />
        {props.network.isOffline && <OfflineIndicator containerStyles={[styles.chatItemComposeSecondaryRow, styles.ml5, styles.mtAuto]} />}       
     </>)

We change this in other pages like AddressPage, DateOfBirth, LegalName pages also.

What alternative solutions did you explore? (Optional)

c3024 avatar Jul 30 '23 04:07 c3024

Oh hm, interesting! @zanyrenney @twisterdotcom @Beamanator - what was the implementation of this from the account settings doc?

If the personal details pages require being online, we should be using the full page blocking form (offline pattern D) in that case?

trjExpensify avatar Jul 31 '23 11:07 trjExpensify

This issue only happens when we don't go to this page before we go offline. If we go to this page at least once in online mode, this will not show loading when we offline.

dukenv0307 avatar Jul 31 '23 13:07 dukenv0307

That's helpful to know. I'm keen to hear from the team on the expected behaviour of accessing the personal details pages when offline.

trjExpensify avatar Jul 31 '23 14:07 trjExpensify

No these should be available offline. This must be a regression.

twisterdotcom avatar Jul 31 '23 16:07 twisterdotcom

Cool, moving on! :)

trjExpensify avatar Jul 31 '23 17:07 trjExpensify

Job added to Upwork: https://www.upwork.com/jobs/~015f3b23a3e3d5db40

melvin-bot[bot] avatar Jul 31 '23 17:07 melvin-bot[bot]

Current assignee @trjExpensify is eligible for the External assigner, not assigning anyone new.

melvin-bot[bot] avatar Jul 31 '23 17:07 melvin-bot[bot]

Triggered auto assignment to Contributor-plus team member for initial proposal review - @thesahindia (External)

melvin-bot[bot] avatar Jul 31 '23 17:07 melvin-bot[bot]

Proposal

Please re-state the problem that we are trying to solve in this issue.

if you click at Personal details for the first time, it keeps loading

What is the root cause of that problem?

When you sign in, your personal details are neither retrieved from the backend nor stored in the local storage. Therefore, if you click on 'personal details' for the first time, the app will need to obtain your details and set them in the local storage.

What changes do you think we should make in order to solve the problem?

we should load the private personal details from the back end and set it inside the local storage after you sign in

Check out lines 98-102 of https://github.dev/michaelraoof/Expensify/blob/fix/loadingPersolnalDetailsOfflineMode/src/libs/actions/Welcome.js#L98-L102.


let privatePersonalDetails;
Onyx.connect({
  key: ONYXKEYS.PRIVATE_PERSONAL_DETAILS,
  callback: (val) => (privatePersonalDetails = val),
});

What alternative solutions did you explore? (Optional)

michaelraoof avatar Aug 01 '23 02:08 michaelraoof

📣 @michaelraoof! 📣 Hey, it seems we don’t have your contributor details yet! You'll only have to do this once, and this is how we'll hire you on Upwork. Please follow these steps:

  1. Get the email address used to login to your Expensify account. If you don't already have an Expensify account, create one here. If you have multiple accounts (e.g. one for testing), please use your main account email.
  2. Get the link to your Upwork profile. It's necessary because we only pay via Upwork. You can access it by logging in, and then clicking on your name. It'll look like this. If you don't already have an account, sign up for one here.
  3. Copy the format below and paste it in a comment on this issue. Replace the placeholder text with your actual details. Screen Shot 2022-11-16 at 4 42 54 PM Format:
Contributor details
Your Expensify account email: <REPLACE EMAIL HERE>
Upwork Profile Link: <REPLACE LINK HERE>

melvin-bot[bot] avatar Aug 01 '23 02:08 melvin-bot[bot]

After we login successfully, openApp API is called but this doesn't return privatePersonalDetails of current user.

  1. BE should return privatePersonalDetails of current user in openApp API

Just looked at https://github.com/Expensify/App/issues/23873#issuecomment-1657037105, it seem like we need BE changes

cc: @trjExpensify

thesahindia avatar Aug 01 '23 08:08 thesahindia

Ah, so let's take this internal then!

trjExpensify avatar Aug 01 '23 11:08 trjExpensify

Current assignee @thesahindia is eligible for the Internal assigner, not assigning anyone new.

melvin-bot[bot] avatar Aug 01 '23 11:08 melvin-bot[bot]

Triggered auto assignment to @grgia (Engineering), see https://stackoverflow.com/c/expensify/questions/4319 for more details.

melvin-bot[bot] avatar Aug 01 '23 11:08 melvin-bot[bot]

I'm going OOO next week and will not be able to look at this until after I am back. Switching to weekly

grgia avatar Aug 03 '23 14:08 grgia

Can we pop this back on daily now?

trjExpensify avatar Aug 14 '23 10:08 trjExpensify

Going to put this back on daily, we have a week to go before it breaks WAQ.

trjExpensify avatar Aug 22 '23 13:08 trjExpensify

@trjExpensify @grgia @thesahindia this issue is now 4 weeks old and preventing us from maintaining WAQ. This should now be your highest priority. Please post below what your plan is to get a PR in review ASAP. Thanks!

melvin-bot[bot] avatar Aug 26 '23 12:08 melvin-bot[bot]

Am I correct in understanding:

Problem: When a user logs in and goes offline BEFORE opening the personal details page, their private personal details are not retrieved and the private personal details page loads infinitely?

Solution (internally) What I'm seeing has been proposed: return privatePersonalDetails of current user in openApp API call.

cc @Beamanator @puneetlath could I get a check on this and your insight on what our methodology is for storing private personal details?

grgia avatar Aug 28 '23 09:08 grgia

I don't think this has anything to do with the securing logins project that we did. That was all about how we deal with other people's personal details. Which in this case it seems like we just aren't returning the users own private personal details in openApp, which we definitely can start doing. Do we return all other profile/settings stuff in openApp?

puneetlath avatar Aug 28 '23 15:08 puneetlath

Yeah, I think this is more about the offline approach in the account settings doc implementation when these new API commands were introduced, right @Beamanator?

trjExpensify avatar Aug 30 '23 13:08 trjExpensify

Oof sorry for not responding earlier!!

Yeah in the Account Settings doc we decided it wasn't necessary to pull the user's private personal details in OpenApp - though I can't recall the exact reason why 😬

@zanyrenney @twisterdotcom @cristipaval (team awesome) - do y'all think we should start pulling private personal details in OpenApp as well just so we at least have that data initially, even if it's slightly out of date (assuming the case you signed in at some point and have been offline for some time, when you finally navigate back to your "personal details" page)

Beamanator avatar Aug 30 '23 15:08 Beamanator

Yeah in the Account Settings doc we decided it wasn't necessary to pull the user's private personal details in OpenApp - though I can't recall the exact reason why 😬

Ah, it would be great if a decision like this was documented somewhere in like alt solutions to refer back to. I couldn't find it when taking a quick search through the doc just now.

@zanyrenney @twisterdotcom @cristipaval (team awesome) - do y'all think we should start pulling private personal details in OpenApp as well just so we at least have that data initially, even if it's slightly out of date (assuming the case you signed in at some point and have been offline for some time, when you finally navigate back to your "personal details" page)

@zanyrenney @cristipaval, do you guys agree or recall a reason we didn't do this initially? If there isn't one, let's do it. Of course, let's make sure it doesn't slow down the openApp call considerably or something like that as we proceed with a PR.

trjExpensify avatar Sep 04 '23 09:09 trjExpensify

@grgia, I think we have enough to prepare a PR to fix this. :)

trjExpensify avatar Sep 11 '23 11:09 trjExpensify

@trjExpensify hmm I don't remember there being any reason we elected to not load personal details offline. I've searched around in Slack too and not generating heaps there. I think we can change this and do it, I nothing is coming to mind why not to do it.

zanyrenney avatar Sep 11 '23 14:09 zanyrenney

I don't remember either about a decision made about this. I joined the Account Settings team when the detailed section of the doc was already approved and helped mainly with some implementation of the push to page design in the Account Settings pages.

cristipaval avatar Sep 11 '23 19:09 cristipaval