Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[$500] Room settings-Members added/deleted in a room in offline mode are not grayed out/crossed #31764

Closed
6 tasks done
lanitochka17 opened this issue Nov 22, 2023 · 72 comments
Closed
6 tasks done
Assignees
Labels
Bug Something is broken. Auto assigns a BugZero manager. Engineering External Added to denote the issue can be worked on by a contributor Internal Requires API changes or must be handled by Expensify staff Reviewing Has a PR in review Weekly KSv2

Comments

@lanitochka17
Copy link

lanitochka17 commented Nov 22, 2023

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.2-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:

Action Performed:

  1. Go to URL https://staging.new.expensify.com/
  2. Login with any Expensifail account
  3. Open any room (or create it) -> Members
  4. Turn off the internet connection
  5. Invite some new members
  6. Delete some members

Expected Result:

Members added offline are grayed out
Members removed offline are crossed out
We expect the same behavior as when removing/adding members offline in the workspace

Actual Result:

Members added offline are NOT grayed out
Members removed offline are NOT crossed out

Workaround:

Unknown

Platforms:

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

  • Android: Native
  • Android: mWeb Chrome
  • iOS: Native
  • iOS: mWeb Safari
  • MacOS: Chrome / Safari
  • MacOS: Desktop

Screenshots/Videos

Add any screenshot/video evidence

Bug6287884_1700694133056.Recording__6866.mp4

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~013f3cdcfb2a96a853
  • Upwork Job ID: 1727466899004616704
  • Last Price Increase: 2023-11-22
  • Automatic offers:
    • tienifr | Contributor | 27957669
Issue OwnerCurrent Issue Owner: @deetergp
@lanitochka17 lanitochka17 added External Added to denote the issue can be worked on by a contributor Daily KSv2 Bug Something is broken. Auto assigns a BugZero manager. labels Nov 22, 2023
@melvin-bot melvin-bot bot changed the title Room settings-Members added/deleted in a room in offline mode are not grayed out/crossed [$500] Room settings-Members added/deleted in a room in offline mode are not grayed out/crossed Nov 22, 2023
Copy link

melvin-bot bot commented Nov 22, 2023

Job added to Upwork: https://www.upwork.com/jobs/~013f3cdcfb2a96a853

Copy link

melvin-bot bot commented Nov 22, 2023

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

@melvin-bot melvin-bot bot added the Help Wanted Apply this label when an issue is open to proposals by contributors label Nov 22, 2023
Copy link

melvin-bot bot commented Nov 22, 2023

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

Copy link

melvin-bot bot commented Nov 22, 2023

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

@tienifr
Copy link
Contributor

tienifr commented Nov 23, 2023

Proposal

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

Room settings-Members added/deleted in a room in offline mode are not grayed out/crossed

What is the root cause of that problem?

When adding/deleting room members we did not store pendingAction like what we did with WS member

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

Solution 1: Add new Onyx key roomMembers to store the members in room. When adding new member to room, we set pendingAction: add, when removing member, set pendingAction: delete (like what we did with WS (policyMembers).)

Then add pendingAction to result

Solution 2: To be simpler, we can just add new filed to report_{id} when adding/removing member to store pendingAction

    const participantAccountsOptimistic= _.object(inviteeAccountIDs, Array(inviteeAccountIDs.length).fill({pendingAction: CONST.RED_BRICK_ROAD_PENDING_ACTION.ADD}))

    const participantAccountsSuccess= _.object(inviteeAccountIDs, Array(inviteeAccountIDs.length).fill({pendingAction: null}))

then add participantsOptimistic: participantAccountsOptimistic to optimisticData

participantsOptimistic: participantAccountsSuccess to successData

and generate errors to failureData

Do the same with removing room member

Finally, add pendingAction: participantsOptimistic[accountID].pendingAction to result

We should cover the case members are not in personalDetails

In inviteToRoom function

    const newPersonalDetailsOnyxData = PersonalDetailsUtils.getNewPersonalDetailsOnyxData(inviteeEmails, inviteeAccountIDs);

optimisticData: [
...
...newPersonalDetailsOnyxData.optimisticData
];

successData: [
...
...newPersonalDetailsOnyxData.successData
];

failureData: [
...
...newPersonalDetailsOnyxData.failureData
];

Result

Screen.Recording.2023-11-23.at.11.18.44.mov

@thirtyfoura34
Copy link

Contributor details
Your Expensify account email: thirtyfoura34@gmail.com
Upwork Profile Link: https://www.upwork.com/freelancers/~01903eeeb0b0665a65

Copy link

melvin-bot bot commented Nov 23, 2023

⚠️ Invalid email. Please make sure to create an Expensify account with this email first here.

@thirtyfoura34
Copy link

Contributor details
Your Expensify account email: thirtyfoura34@gmail.com
Upwork Profile Link: https://www.upwork.com/freelancers/~01903eeeb0b0665a65

Copy link

melvin-bot bot commented Nov 23, 2023

⚠️ Missing/invalid email or upwork profile link. Please make sure you add both your Expensify email and Upwork profile link in the format specified.

@thirtyfoura34
Copy link

Contributor details
Your Expensify account email: thirtyfoura34@gmail.com
Upwork Profile Link: https://www.upwork.com/freelancers/~01903eeeb0b0665a65

Copy link

melvin-bot bot commented Nov 23, 2023

✅ Contributor details stored successfully. Thank you for contributing to Expensify!

@melvin-bot melvin-bot bot added the Overdue label Nov 27, 2023
@burczu
Copy link
Contributor

burczu commented Nov 27, 2023

Not overdue, I'll be reviewing this issue soon.

@melvin-bot melvin-bot bot removed the Overdue label Nov 27, 2023
@burczu
Copy link
Contributor

burczu commented Nov 29, 2023

I think we should give a try the Solution 1 from the proposal of @tienifr - doing this the same way as for WS is a good idea as this is actually the same functionality.

🎀 👀 🎀 C+ reviewed

Copy link

melvin-bot bot commented Nov 29, 2023

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

@melvin-bot melvin-bot bot added the Overdue label Dec 2, 2023
@melvin-bot melvin-bot bot removed the Help Wanted Apply this label when an issue is open to proposals by contributors label Dec 4, 2023
Copy link

melvin-bot bot commented Dec 4, 2023

📣 @tienifr 🎉 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 melvin-bot bot removed the Overdue label Dec 4, 2023
@tienifr
Copy link
Contributor

tienifr commented Dec 5, 2023

@deetergp @burczu Thanks for choosing my proposal. In the first solution, I suggest adding roomMembers onyx key like what we did with WS, so we need fix BE as well to return roomMembers when users logout -> login. Should we hold this until BE fix? Thanks

@melvin-bot melvin-bot bot added Monthly KSv2 and removed Weekly KSv2 labels Feb 22, 2024
Copy link

melvin-bot bot commented Feb 22, 2024

This issue has not been updated in over 15 days. @deetergp, @bfitzexpensify, @Christinadobrzyn, @alitoshmatov, @tienifr eroding to Monthly issue.

P.S. Is everyone reading this sure this is really a near-term priority? Be brave: if you disagree, go ahead and close it out. If someone disagrees, they'll reopen it, and if they don't: one less thing to do!

@deetergp
Copy link
Contributor

I think the PR is getting pretty close.

@deetergp deetergp added Weekly KSv2 and removed Monthly KSv2 labels Feb 24, 2024
@deetergp
Copy link
Contributor

deetergp commented Mar 5, 2024

Still some discussion going on in the PR.

@deetergp
Copy link
Contributor

deetergp commented Mar 8, 2024

PR is mostly ready. There are a few merge conflicts that need resolved, but once those are sorted I think it'll be good to go.

@Christinadobrzyn
Copy link
Contributor

working on PR - on hold for #16078

@tienifr
Copy link
Contributor

tienifr commented Mar 21, 2024

@deetergp
To summary on this issue:

I spent time to work on first solution, but we decided to go with the second one. The created PR was almost merged, Unfortunately, it was fixed on other issue, but I still fixed other case(strike though), it would be out of scope. Can I get extra bounty in this case? Thanks cc @Christinadobrzyn

@Christinadobrzyn
Copy link
Contributor

Hi @tienifr! Thanks for reaching out about the extra work - what bonus do you think would be fair for the extra work you did?

@tienifr
Copy link
Contributor

tienifr commented Mar 22, 2024

My suggestion is 500$. The same situation is here. Wdyt? @Christinadobrzyn

@Christinadobrzyn
Copy link
Contributor

Christinadobrzyn commented Mar 22, 2024

Sounds good @tienifr! We'll add a $500 bonus to you on payout (you might need to remind us)

Copy link

melvin-bot bot commented Mar 22, 2024

⚠️ Looks like this issue was linked to a Deploy Blocker here

If you are the assigned CME please investigate whether the linked PR caused a regression and leave a comment with the results.

If a regression has occurred and you are the assigned CM follow the instructions here.

If this regression could have been avoided please consider also proposing a recommendation to the PR checklist so that we can avoid it in the future.

@tienifr
Copy link
Contributor

tienifr commented Mar 25, 2024

It's not my regression

@deetergp
Copy link
Contributor

deetergp commented Mar 25, 2024

It's not my regression

Can you elaborate @tienifr?

@tienifr
Copy link
Contributor

tienifr commented Mar 25, 2024

@deetergp We already discussed in here

@deetergp
Copy link
Contributor

Ah, nice. Thanks for the extra context @tienifr.

@tienifr
Copy link
Contributor

tienifr commented Apr 8, 2024

@deetergp @Christinadobrzyn The PR was merged 3 weeks ago without any regressions. Should we go with payment? Thanks

@Christinadobrzyn
Copy link
Contributor

Christinadobrzyn commented Apr 8, 2024

Yes, thanks for catching that @tienifr - looks like the payment trigger didn't happen.

Payouts due:

Upwork job is here the posting is now closed.

Internal so no regression test. I think this can be closed. Let me know if I missed anything!

@alitoshmatov
Copy link
Contributor

@Christinadobrzyn I worked as C+, I also require payment

@Christinadobrzyn
Copy link
Contributor

Ah so sorry for missing that @alitoshmatov - thank you for letting me know, can you accept the Upwork offer here - https://www.upwork.com/ab/applicants/1777525996418326528/offers

@Christinadobrzyn
Copy link
Contributor

Thanks, @alitoshmatov - I paid out the Upwork job based on this payment summary - #31764 (comment)

Closing! Feel free to let me know if I missed anything else!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Something is broken. Auto assigns a BugZero manager. Engineering External Added to denote the issue can be worked on by a contributor Internal Requires API changes or must be handled by Expensify staff Reviewing Has a PR in review Weekly KSv2
Projects
No open projects
Status: CRITICAL
Development

No branches or pull requests

8 participants