Expensify / App

Welcome to New Expensify: a complete re-imagination of financial collaboration, centered around chat. Help us build the next generation of Expensify by sharing feedback and contributing to the code.
https://new.expensify.com
MIT License
3.3k stars 2.73k forks source link

[$250] Admin's name is showing as blank after Editing anything on profile page - Reported by @Tushu17 #10891

Closed mvtglobally closed 1 year ago

mvtglobally commented 2 years ago

If you haven’t already, check out our contributing guidelines for onboarding and email contributors@expensify.com to request to join our Slack channel!


Action Performed:

  1. Open website.
  2. Go to settings > profile > edit anything and save
  3. Then go to workspace manage member page and see admin's name.

Expected Result:

Name shouldn't be showing as blank

Actual Result:

It's showing blank until user refreshes the website.

Workaround:

unknown

Platform:

Where is this issue occurring?

Version Number: 1.1.96-0 Reproducible in staging?: Y Reproducible in production?: Email or phone of affected tester (no customers): Logs: https://stackoverflow.com/c/expensify/questions/4856 Notes/Photos/Videos: Any additional supporting documentation

https://user-images.githubusercontent.com/43995119/189038101-228df7d0-eab0-42ba-abcc-0cc36b44fb58.mp4

https://user-images.githubusercontent.com/43995119/189038104-6e37003a-4479-4e7c-91a1-ef172b8b166c.mov

Expensify/Expensify Issue URL: Issue reported by: @Tushu17 Slack conversation: https://expensify.slack.com/archives/C01GTK53T8Q/p1661322811349869

View all open jobs on GitHub

melvin-bot[bot] commented 2 years ago

Triggered auto assignment to @kadiealexander (AutoAssignerTriage), see https://stackoverflow.com/c/expensify/questions/4749 for more details.

varshamb commented 1 year ago

Proposal

User profile has First Name and Last Name already updated

image

Workspace Manage member page then

image

First Name and Last Name removed (made blank) from User profile and Saved.

image

Workspace Manage member page after removing First and Last name:

image

Again after sometime visited Workspace Manage member page. Now login id is displaying instead of Name (as first and last names are empty)

image

Will it be okay if we only display LoginID only once.

image

melvin-bot[bot] commented 1 year ago

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

melvin-bot[bot] commented 1 year ago

@danieldoglas Whoops! This issue is 2 days overdue. Let's get this updated quick!

danieldoglas commented 1 year ago

will look at this tomorrow

melvin-bot[bot] commented 1 year ago

Triggered auto assignment to @stephanieelliott (External), see https://stackoverflow.com/c/expensify/questions/8582 for more details.

danieldoglas commented 1 year ago

We can export this one.

aimane-chnaif commented 1 year ago

Proposal

Issue: When first open the app, displayName is login if firstName, lastName is empty. After updating profile, displayName becomes empty.

https://github.com/Expensify/App/blob/0302ce7096572290a2368004902ab51fa3c6651d/src/libs/actions/User.js#L308-L313 This happens because after updating user profile, bad data (displayName='') comes from socket event subscribed in Pusher and this data overwrites Onyx value with key=personalDetails

log

As seen in console log, displayName is first updated to login (when first name, last name are empty) locally, which is correct. But then displayName is updated to blank ("") after Handled onyxApiUpdate event sent by Pusher

Solution1: It's ideal to fix this in backend. So send displayName as login if firstName, lastName not exist in socket event. I don't have access to this repo so I won't be able to share detailed code proposal here.

Solution2: If it won't be fixed by backend, app needs to show login if displayName is empty This way, we always show login both before and after updating profile, which is consistent in user view. https://github.com/Expensify/App/blob/0302ce7096572290a2368004902ab51fa3c6651d/src/pages/workspace/WorkspaceMembersPage.js#L262

-        text: Str.removeSMSDomain(item.displayName),
+        text: Str.removeSMSDomain(item.displayName || item.login),
stephanieelliott commented 1 year ago

Posted to Upwork: https://www.upwork.com/jobs/~013fb8003f81392003

As the reporter, @Tushu17 has right of first refusal on this one.

Tushu17 commented 1 year ago

Hey @stephanieelliott, the policy has changed now the reporter doesn't get dibs on an issue. Anyone can post a solution.

stephanieelliott commented 1 year ago

Oh cool, thanks @Tushu17!

melvin-bot[bot] commented 1 year ago

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

melvin-bot[bot] commented 1 year ago

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

melvin-bot[bot] commented 1 year ago

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

stephanieelliott commented 1 year ago

Re-applied the External label, it didn't apply the assignments correctly the first time. @rushatgabhane we have a proposal here when you get a chance!

iwiznia commented 1 year ago

Almost sure this is a dupe of https://github.com/Expensify/App/issues/11134 and we can close this

Tushu17 commented 1 year ago

Almost sure this is a dupe of #11134 and we can close this

Hey @iwiznia, This issue was reported and created before #11134. So I think we should close the other issue.

iwiznia commented 1 year ago

Sounds good to me

stephanieelliott commented 1 year ago

Hey @rushatgabhane can you review the proposal when you get a chance? https://github.com/Expensify/App/issues/10891#issuecomment-1250913836

rushatgabhane commented 1 year ago

@varshamb

Will it be okay if we only display LoginID only once

Yeah, that seems alright to me. image

It's kinda weird to show the email twice.

image

cc: @roryabraham

rushatgabhane commented 1 year ago

Anyway, looks like @iwiznia has a fix for it on the backend https://github.com/Expensify/App/issues/11134#issuecomment-1256235725

And the email will be shown twice (which is OK too)

stephanieelliott commented 1 year ago

Cool! Ok, removing the Help Wanted label since we have a fix for this already.

melvin-bot[bot] commented 1 year ago

@iwiznia, @rushatgabhane, @stephanieelliott Whoops! This issue is 2 days overdue. Let's get this updated quick!

aimane-chnaif commented 1 year ago

As seen in console log, displayName is first updated to login (when first name, last name are empty) locally, which is correct. But then displayName is updated to blank ("") after Handled onyxApiUpdate event sent by Pusher

@iwiznia I assume you fixed in backend since I confirmed correct displayName coming here now.

And both issues (this one and https://github.com/Expensify/App/issues/11134) are not reproducible any more.

Can I get payment in either of GHs?

iwiznia commented 1 year ago

I don't think so, because this issue was reported first by @Tushu17, so they would get the reporting payment.

iwiznia commented 1 year ago

Oh, BTW, this is already fixed too. @stephanieelliott reporting payment is needed, please check if you agree with my message above.

aimane-chnaif commented 1 year ago

@iwiznia I am not a reporter but the one who found and explained in detail the root cause of these issues and proposed right solution so that you could fix it in backend

Here's my proposals: https://github.com/Expensify/App/issues/10891#issuecomment-1250913836 https://github.com/Expensify/App/issues/11134#issuecomment-1255320541 And approved by C+ here: https://github.com/Expensify/App/issues/11134#issuecomment-1255360653

rushatgabhane commented 1 year ago

Personal time or resources applied towards investigating a proposal will not guarantee compensation. Compensation is only guaranteed to those who propose a solution and get hired for that job. (source)

cc @aimane-chnaif

aimane-chnaif commented 1 year ago

yes I understand but I think this is edge case. No one could have easily found out that the issue came from pusher socket event. So without my proposal, these GHs would have remained open longer and all C/C+/CME waste time checking or debugging this.

11134 C+ review

@parasharrajat I'd like to hear your input as well regarding this compensation since you reviewed my proposal and handed it over to @iwiznia as a backend fix.

PS: Some issues require backend fix and external contributors don't have access to repo but at least they can provide exact root cause and propose general solution in backend though not detailed code proposal. This case, they are not eligible for bonus? So it means they are never compensated for supporting all backend issues?

iwiznia commented 1 year ago

@rushatgabhane is technically correct, but in this case, there were a few mishaps in the issue, like there were 2 issues created (but only one was needed) and they were marked as external when they were really internal. Due to that and the fact that you did spend time debugging it and helped @parasharrajat, I think we should pay out $250 (same as reporting bonus) in good faith.

One minor note: if the issue is in the backend, we should not attempt to fix it in the frontend. so saying for next time if there's a similar issue, it's better to not add frontend proposals and instead commenting something like:

This issue is coming from the backend, data in the property X of the onyx key Y, that gets set after calling the API command Z is sending bad data: it should not return & symbols html escaped.

Thanks for your help and look forward to working with you again!

@stephanieelliott can you pay @aimane-chnaif $250 please?

aimane-chnaif commented 1 year ago

ok noted. thanks so much @iwiznia

parasharrajat commented 1 year ago

Thanks, @iwiznia for following up. There is something I want to add.

We strictly follow our documented guidelines. There can be exceptions but no one should expect those. If someone feels that something is not done as per the guidelines, we will be happy to help you understand or fix our mistake.

For this issue,

At last, I agree with @iwiznia's decision and look forward to more contributions from you and I am sure there won't be many such cases. Thank you.

stephanieelliott commented 1 year ago

Hey @aimane-chnaif, can you please grab the job on Upwork? We’ll use that to pay you https://www.upwork.com/jobs/~013fb8003f81392003

aimane-chnaif commented 1 year ago

@stephanieelliott applied. thanks

stephanieelliott commented 1 year ago

All paid up!