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.12k stars 2.61k forks source link

[$250] Save the world - Principals whisper message in #teachers-unite shows the teacher as "Hidden" #43846

Open lanitochka17 opened 2 weeks ago

lanitochka17 commented 2 weeks 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!


Version Number: 1.4.84-0 Reproducible in staging?: Y Reproducible in production?: Y If this was caught during regression testing, add the test name, ID and link from TestRail: https://expensify.testrail.io/index.php?/tests/view/4635818&group_by=cases:section_id&group_id=229065&group_order=asc Issue reported by: Applause - Internal Team

Action Performed:

  1. Navigate to https://staging.new.expensify.com/
  2. Log in with an expensifail account
  3. Navigate to Account settings - Save the world - I know a teacher
  4. Input anything for the name fields, and an expensifail email address to the email field
  5. Click on the "Let's do this!" button

Expected Result:

The principal added the teacher with a full name and email address so the name or at least, the email address should be shown

Actual Result:

Principals whisper message from Expensify.org Admin in #teachers-unite shows the teacher as "Hidden"

Workaround:

Unknown

Platforms:

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

Screenshots/Videos

Add any screenshot/video evidence

https://github.com/Expensify/App/assets/78819774/6322a374-d95b-4276-a80b-7e4300f54f3d

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~0106b42c9c00ea9afa
  • Upwork Job ID: 1803768783432548906
  • Last Price Increase: 2024-07-04
melvin-bot[bot] commented 2 weeks ago

Triggered auto assignment to @greg-schroeder (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details. Please add this bug to a GH project, as outlined in the SO.

lanitochka17 commented 2 weeks ago

@greg-schroeder FYI I haven't added the External label as I wasn't 100% sure about this issue. Please take a look and add the label if you agree it's a bug and can be handled by external contributors

jainilparikh commented 2 weeks ago

Proposal

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

Teachers name is not visible instead hidden@ is shown.

What is the root cause of that problem?

A. Onyx now defaults to -1 instead of ' ' based on a recent PR. So, for teachers who don't have an expensify account, htmlAttribAccountID is now -1 instead of ''.

When creating the mentions message, we check for this condition:

https://github.com/Expensify/App/blob/b33542383ac2cb8ea1e922aa12dd2265efccd0ab/src/components/HTMLEngineProvider/HTMLRenderers/MentionUserRenderer.tsx#L59

Which was previously false, because htmlAttribAccountID was '' and isEmpty returned true. But now, htmlAttribAccountID is defaulted to -1 so, isEmpty returns false causing the above condition to evaluate to true causing the Hidden message to appear.

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

Replace this line: https://github.com/Expensify/App/blob/b33542383ac2cb8ea1e922aa12dd2265efccd0ab/src/components/HTMLEngineProvider/HTMLRenderers/MentionUserRenderer.tsx#L59

with:

if (!isEmpty(htmlAttribAccountID) && htmlAttribAccountID != '-1') {
}

What alternative solutions did you explore? (Optional)

Tony-MK commented 2 weeks ago

Proposal

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

Save the world - Principals whisper a message in #teachers-unite shows the teacher as "Hidden"

What is the root cause of that problem?

The main root cause of the problem is that when the teacher is referred, the referTeachersUniteVolunteer function is called to send the data the backend without saving the personalDetails in the onyx.

However, unlike the addSchoolPrincipal function, the referTeachersUniteVolunteer function doesn't optimistically add the personalDetails of the referred teacher in the onyx.

Furthermore, the response from the backend in the report action only includes <mention-user accountID=\"17571571\"\/> in the message without sending back the personalDetails like below.

Screenshot 2024-06-18 at 12 35 40

Therefore, when the accountID is not found in the personalDetails, it shows "Hidden".

Only, when the OpenReport API is called is when the personalDetails of the referred teacher is synced with the onyx.

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

We should make the referTeachersUniteVolunteer function consistent with the addSchoolPrincipal function by optimistically adding the referred teacher's personalDetails in the onyx.

This will also improve the offline experience when a teacher is being referred when the user is offline.

Therefore, the optimisticData, successData, and failureData should look like the addSchoolPrincipal function, mainly the personalDetails to avoid the new accountID from showing "Hidden", since the user's data it is not found in the personalDetails.

melvin-bot[bot] commented 2 weeks ago

Job added to Upwork: https://www.upwork.com/jobs/~0106b42c9c00ea9afa

melvin-bot[bot] commented 2 weeks ago

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

melvin-bot[bot] commented 1 week ago

@greg-schroeder, @rushatgabhane Eep! 4 days overdue now. Issues have feelings too...

greg-schroeder commented 1 week ago

Bump on proposal review @rushatgabhane

rushatgabhane commented 1 week ago

I like @Tony-MK's proposal https://github.com/Expensify/App/issues/43846#issuecomment-2175692058

rushatgabhane commented 1 week ago

🎀👀🎀

melvin-bot[bot] commented 1 week ago

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

muttmuure commented 1 week ago

This is not a #newdot-quality bug - the feature is not unreliable, slow or performing redundant or unexpected API calls, it just doesn't work as designed

melvin-bot[bot] commented 1 week ago

📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸

melvin-bot[bot] commented 6 days ago

@youssef-lr @greg-schroeder @rushatgabhane 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] commented 6 days ago

@youssef-lr, @greg-schroeder, @rushatgabhane Eep! 4 days overdue now. Issues have feelings too...

melvin-bot[bot] commented 4 days ago

@youssef-lr, @greg-schroeder, @rushatgabhane 6 days overdue. This is scarier than being forced to listen to Vogon poetry!

greg-schroeder commented 4 days ago

@youssef-lr can you confirm the contributor assignment here?

greg-schroeder commented 4 days ago

I believe that is this one: https://github.com/Expensify/App/issues/43846#issuecomment-2190208844

melvin-bot[bot] commented 3 days ago

📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸