App icon indicating copy to clipboard operation
App copied to clipboard

[$500] [MEDIUM] Referral - Referral link is incomplete and missing contact details

Open lanitochka17 opened this issue 1 year ago β€’ 68 comments

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


Version Number: 1.4.6.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 Expensify/Expensify Issue URL: Issue reported by: Applause - Internal Team Slack conversation:

Issue found when executing PR https://github.com/Expensify/App/pull/31827

Action Performed:

  1. In Safari or Chrome on Mac OSX, go to staging.new.expensify.com
  2. Create a new user account
  3. Tap Search
  4. Tap on the referral banner at the bottom
  5. Tap Copy referral link
  6. Paste the link

Expected Result:

The referral link is complete

Actual Result:

The referral link is incomplete. The link is missing contact details This issue happens for some accounts but not all

Workaround:

Unknown

Platforms:

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

  • [ ] Android: Native
  • [ ] Android: mWeb Chrome
  • [ ] iOS: Native
  • [ ] iOS: mWeb Safari
  • [x] MacOS: Chrome / Safari
  • [x] MacOS: Desktop

Screenshots/Videos

Add any screenshot/video evidence

https://github.com/Expensify/App/assets/78819774/f3fdab66-45be-4b3e-bb72-e8546d445859

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~01cdb9a4da6bb420f6
  • Upwork Job ID: 1730291484465594368
  • Last Price Increase: 2023-11-30
  • Automatic offers:
    • situchan | Contributor | 28033535
    • tienifr | Contributor | 28135729
Issue OwnerCurrent Issue Owner: @roryabraham

lanitochka17 avatar Nov 30 '23 18:11 lanitochka17

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

melvin-bot[bot] avatar Nov 30 '23 18:11 melvin-bot[bot]

Job added to Upwork: https://www.upwork.com/jobs/~01cdb9a4da6bb420f6

melvin-bot[bot] avatar Nov 30 '23 18:11 melvin-bot[bot]

Bug0 Triage Checklist (Main S/O)

  • [x] This "bug" occurs on a supported platform (ensure Platforms in OP are βœ…)
  • [x] 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
  • [x] 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.
  • [x] 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.
  • [x] I have reviewed and subscribed to the linked Slack conversation to ensure Slack/Github stay in sync

melvin-bot[bot] avatar Nov 30 '23 18:11 melvin-bot[bot]

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

melvin-bot[bot] avatar Nov 30 '23 18:11 melvin-bot[bot]

Proposal

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

Referral link is showing empty email address when we generate referral link

What is the root cause of that problem?

On the referral page src/pages/ReferralDetailsPage.js Page when we are generating the referral url using the generateReferralURL function https://github.com/Expensify/App/blob/56a368b07233a78d0a3603b652798d1d86821885/src/pages/ReferralDetailsPage.js#L57-L59 we are using the account.primaryLogin as a email address and sometimes (when a single email added as a contact method) we has empty primaryLogin value https://github.com/Expensify/App/blob/56a368b07233a78d0a3603b652798d1d86821885/src/pages/ReferralDetailsPage.js#L95

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

Instead of the account.primaryLogin we should use the session.email or we can use the account.primaryLogin || session.email as a email address here

https://github.com/Expensify/App/blob/56a368b07233a78d0a3603b652798d1d86821885/src/pages/ReferralDetailsPage.js#L95 urlToCopy={generateReferralURL(account.primaryLogin || session.email)}

What alternative solutions did you explore? (Optional)

N/A

jayeshmangwani avatar Nov 30 '23 18:11 jayeshmangwani

Proposal

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

The referral link is incomplete. The link is missing contact details This issue happens for some accounts but not all

What is the root cause of that problem?

  1. There's a back-end issue where primaryLogin is not added first time new account created but after logout and re-login, it's added. This has been discussed extensively in this issue.

We decided to fix in the back-end but due to low capacity, we did not fix it there.

So in here, the primaryLogin is empty, causing the issue

  1. There's also another issue where when setting another contact method as default here, the primaryLogin is not updated in optimistic data. This causes the primaryLogin to be outdated in many different places that use it, when we update the default contact method.

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

  1. We should fix this properly in the back-end, if we're still low on capacity, we can optimistically update it with session.email in the relevant login APIs.
  2. Update primaryLogin to newDefaultContactMethod in optimistic data (and restore in failure data) when updating the default contact method

What alternative solutions did you explore? (Optional)

We can consider removing the use of primaryLogin everywhere and replace by session.email, but I'm not sure if there's any hidden regression.

We cannot just fix it in the one place of generateReferralURL since all the bug cases of the 2nd problem will remain.

tienifr avatar Dec 01 '23 03:12 tienifr

Proposal

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

The email details is not being populated to the copied shareable url for users that recently joined the app

What is the root cause of that problem?

I don't think there's a need to fix on the service side, this should be addressed on the FE app.

The root cause lies on the ReferralDetailsPage component. https://github.com/Expensify/App/blob/cc4add5ba055b1d88ef246bf6289092a1344afc5/src/pages/ReferralDetailsPage.js#L57-L59

And how is being called below with the primaryLogin, so it's assuming it's not empty: https://github.com/Expensify/App/blob/cc4add5ba055b1d88ef246bf6289092a1344afc5/src/pages/ReferralDetailsPage.js#L92-L95

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

We should provide the credentials stored on Onyx, these hold the credentials the new user just entered to join the app, and on top of that, we should validate whether the primaryLogin at the account level is not empty, if it is, then we should be able to use the login at the credentials level which holds the primary login method the user recently entered.

What alternative solutions did you explore? (Optional)

We could retrieve a user's session email through Onyx as well

Reminder: Please use plain English, be brief and avoid jargon. Feel free to use images, charts or pseudo-code if necessary. Do not post large multi-line diffs or write walls of text. Do not create PRs unless you have been hired for this job.

chiItepin avatar Dec 03 '23 18:12 chiItepin

Are there more specific reproduction steps that you were able to work out in developing your proposals @jayeshmangwani, @tienifr or @chiItepin? I haven't been able to reproduce through the steps above, but based on @tienifr details it seems like it might be:

  1. In Safari or Chrome on Mac OSX, go to staging.new.expensify.com
  2. Create a new user account
  3. Tap Search
  4. Tap on the referral banner at the bottom
  5. Tap Copy referral link
  6. Paste the link

Is that correct?

conorpendergrast avatar Dec 05 '23 14:12 conorpendergrast

@conorpendergrast Yes, that's exactly right, it only happens for recently created accounts. This explains why the "credentials" entity does have the "login" set in the onyx store

chiItepin avatar Dec 05 '23 15:12 chiItepin

Taking this over based on @ntdiary's request

situchan avatar Dec 05 '23 16:12 situchan

@jayeshmangwani's proposal looks good to me. There was similar issue in the past and we decided to use session.email instead of fixing in backend. πŸŽ€ πŸ‘€ πŸŽ€ C+ reviewed

situchan avatar Dec 05 '23 17:12 situchan

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

melvin-bot[bot] avatar Dec 05 '23 17:12 melvin-bot[bot]

There was similar https://github.com/Expensify/App/issues/19366 in the past and we decided to https://github.com/Expensify/App/pull/28164 instead of fixing in backend. πŸŽ€ πŸ‘€ πŸŽ€ C+ reviewed

@situchan Actually we decided to fix in the back-end but due to low capacity, we did not fix it there but use the front-end fix as a workaround instead.

Front-end fix is not fixing the root cause so I think we should fix in back-end here.

cc @tgolen

tienifr avatar Dec 06 '23 07:12 tienifr

Maybe @roryabraham has better context

situchan avatar Dec 06 '23 07:12 situchan

Oof, that's a long issue. It would be great if you had a TLDR @roryabraham.

tgolen avatar Dec 07 '23 16:12 tgolen

@tgolen, @conorpendergrast Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

melvin-bot[bot] avatar Dec 11 '23 11:12 melvin-bot[bot]

πŸ“£ @situchan πŸŽ‰ An offer has been automatically sent to your Upwork account for the Contributor role πŸŽ‰ Thanks for contributing to the Expensify app!

Offer link Upwork job Please accept the offer and leave a comment on the Github issue letting us know when we can expect a PR to be ready for review πŸ§‘β€πŸ’» Keep in mind: Code of Conduct | Contributing πŸ“–

melvin-bot[bot] avatar Dec 11 '23 12:12 melvin-bot[bot]

@situchan assigned to you as C+

conorpendergrast avatar Dec 11 '23 12:12 conorpendergrast

I don't think using session.email is workaround. Waiting for @roryabraham's confirmation either to use session.email or fix in backend.

situchan avatar Dec 13 '23 15:12 situchan

Rory is still on jury duty for today and tomorrow. I think that's fine. This isn't super urgent.

tgolen avatar Dec 13 '23 16:12 tgolen

@tgolen @conorpendergrast @situchan this issue was created 2 weeks ago. Are we close to approving a proposal? If not, what's blocking us from getting this issue assigned? Don't hesitate to create a thread in #expensify-open-source to align faster in real time. Thanks!

melvin-bot[bot] avatar Dec 14 '23 11:12 melvin-bot[bot]

The back-end "fix" for that issue was implemented, it just ... didn't actually fix the issue.

Sorry if I missed this, but what precisely do we think is the root cause?

roryabraham avatar Dec 17 '23 21:12 roryabraham

@roryabraham Thanks for jumping in!

Sorry if I missed this, but what precisely do we think is the root cause?

Can you check the root cause section of my proposal, the back-end is not returning the primaryLogin after the account is first time created, but after logged out and logged in, it's returned.

tienifr avatar Dec 18 '23 03:12 tienifr

Rory is OoO for jury duty again.

@roryabraham I'll assign you for follow-up when you're back!

conorpendergrast avatar Dec 18 '23 15:12 conorpendergrast

@tgolen @conorpendergrast @roryabraham @situchan this issue is now 3 weeks old. There is one more week left before this issue breaks WAQ and will need to go internal. What needs to happen to get a PR in review this week? Please create a thread in #expensify-open-source to discuss. Thanks!

melvin-bot[bot] avatar Dec 21 '23 11:12 melvin-bot[bot]

Rory is back! Assigning him for follow-up

conorpendergrast avatar Dec 21 '23 12:12 conorpendergrast

@tgolen, @conorpendergrast, @roryabraham, @situchan Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

melvin-bot[bot] avatar Dec 25 '23 11:12 melvin-bot[bot]

Had deja vu creating this PR, but verified that it fixes the issue. The previous implementation of the same thing might have been accidentally removed in a bad merge resolution or something.

roryabraham avatar Dec 28 '23 00:12 roryabraham

Issue not reproducible during KI retests. (First week)

mvtglobally avatar Jan 03 '24 05:01 mvtglobally

@conorpendergrast, @roryabraham, @situchan Whoops! This issue is 2 days overdue. Let's get this updated quick!

melvin-bot[bot] avatar Jan 04 '24 17:01 melvin-bot[bot]