App icon indicating copy to clipboard operation
App copied to clipboard

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

Open lanitochka17 opened this issue 1 year ago • 53 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.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?

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

Screenshots/Videos

Add any screenshot/video evidence

https://github.com/Expensify/App/assets/78819774/1d84f8fc-42ce-431d-8b8b-f9362bd7eb8d

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 avatar Nov 22 '23 23:11 lanitochka17

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

melvin-bot[bot] avatar Nov 22 '23 23:11 melvin-bot[bot]

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

melvin-bot[bot] avatar Nov 22 '23 23:11 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 Nov 22 '23 23:11 melvin-bot[bot]

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

melvin-bot[bot] avatar Nov 22 '23 23:11 melvin-bot[bot]

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

https://github.com/Expensify/App/assets/113963320/5ab193d1-baa5-4b26-93b3-f4102421cceb

tienifr avatar Nov 23 '23 03:11 tienifr

Contributor details Your Expensify account email: [email protected] Upwork Profile Link: https://www.upwork.com/freelancers/~01903eeeb0b0665a65

thirtyfoura34 avatar Nov 23 '23 03:11 thirtyfoura34

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

melvin-bot[bot] avatar Nov 23 '23 03:11 melvin-bot[bot]

Contributor details Your Expensify account email: [email protected] Upwork Profile Link: https://www.upwork.com/freelancers/~01903eeeb0b0665a65

thirtyfoura34 avatar Nov 23 '23 03:11 thirtyfoura34

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

melvin-bot[bot] avatar Nov 23 '23 03:11 melvin-bot[bot]

Contributor details Your Expensify account email: [email protected] Upwork Profile Link: https://www.upwork.com/freelancers/~01903eeeb0b0665a65

thirtyfoura34 avatar Nov 23 '23 03:11 thirtyfoura34

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

melvin-bot[bot] avatar Nov 23 '23 03:11 melvin-bot[bot]

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

burczu avatar Nov 27 '23 13:11 burczu

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

burczu avatar Nov 29 '23 11:11 burczu

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

melvin-bot[bot] avatar Nov 29 '23 11:11 melvin-bot[bot]

📣 @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[bot] avatar Dec 04 '23 20:12 melvin-bot[bot]

@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

tienifr avatar Dec 05 '23 03:12 tienifr

Ahh, right @tienifr... Thanks for pointing this out.

@bfitzexpensify so I think we should switch to Internal here to get an opinion from BE engineer. What do you think?

burczu avatar Dec 05 '23 10:12 burczu

@deetergp what do you think?

bfitzexpensify avatar Dec 05 '23 19:12 bfitzexpensify

@deetergp @burczu @bfitzexpensify @tienifr 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 06 '23 11:12 melvin-bot[bot]

Hey @burczu, your suggestion sounds like a good one 👍 Do you happen to know which issue it was where we similar changes for Workspaces? If I can find the the internal issue related to those changes, it will make my life a lot easier, and speed up the process considerably.

deetergp avatar Dec 07 '23 00:12 deetergp

@deetergp I don't know to be honest, but maybe @tienifr could help here? Could you point out the places where we do the same with policyMembers?

burczu avatar Dec 11 '23 13:12 burczu

@deetergp I tried, but I did not ensure what was the root PR, may be this one. Hope this helps

tienifr avatar Dec 12 '23 14:12 tienifr

@deetergp @burczu @bfitzexpensify @tienifr 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 13 '23 11:12 melvin-bot[bot]

@deetergp, @burczu, @bfitzexpensify, @tienifr Huh... This is 4 days overdue. Who can take care of this?

melvin-bot[bot] avatar Dec 18 '23 00:12 melvin-bot[bot]

@deetergp, @burczu, @bfitzexpensify, @tienifr Huh... This is 4 days overdue. Who can take care of this?

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

Will try to dig back into this one today.

deetergp avatar Dec 19 '23 16:12 deetergp

@deetergp @burczu @bfitzexpensify @tienifr this issue is now 4 weeks old and preventing us from maintaining WAQ, can you:

  • Decide whether any proposals currently meet our guidelines and can be approved as-is today
  • If no proposals meet that standard, please take this issue internal and treat it as one of your highest priorities
  • If you have any questions, don't hesitate to start a discussion in #expensify-open-source

Thanks!

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

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

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

Hey @bfitzexpensify! I'll be OOO till January 6th, and right after that I'm leaving the C+ role and joining Waves. Could you please assign another C+ engineer to handle this issue from now on? Thanks.

burczu avatar Dec 22 '23 13:12 burczu

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

melvin-bot[bot] avatar Dec 22 '23 13:12 melvin-bot[bot]