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

[HOLD for payment 2023-11-21][$1000] Offline - Create task in a room with admin only #22608

Closed
1 of 6 tasks
kavimuru opened this issue Jul 11, 2023 · 122 comments
Closed
1 of 6 tasks
Assignees
Labels
Awaiting Payment Auto-added when associated PR is deployed to production Bug Something is broken. Auto assigns a BugZero manager. Engineering External Added to denote the issue can be worked on by a contributor Reviewing Has a PR in review Weekly KSv2

Comments

@kavimuru
Copy link

kavimuru commented Jul 11, 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!


Action Performed:

  1. Login to two accounts, Alice and Bob, in two separate browser tabs.
  2. Alice creates a workspace and invites Bob as a non-Admin member.
  3. Bob goes to the #announce room for the new workspace.
  4. Bob goes offline.
  5. Alice goes to the #announce room for the new workspace, opens room settings, and under Who can post, sets Admins only.
  6. Since Bob is offline, he does not receive an update that the room has become read-only for non-admins. Therefore, he can still attempt to create a task offline in the #announce room.

Expected result

When Bob comes back online, the task should appear "greyed out" with a "red brick road" error. Any actions Bob attempted to take on that task (such as leaving a reaction), should fail.

Actual Result:

The task appears to have been completed normally for Bob, unless he tries to create a thread with that task as the root, in which case he'll see a 404 page.

Workaround:

Don't do that, Bob!

Platforms:

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

  • Android / native
  • Android / Chrome
  • iOS / native
  • iOS / Safari
  • MacOS / Chrome / Safari
  • MacOS / Desktop

Version Number: 1.3.39-5
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
Notes/Photos/Videos: Any additional supporting documentation

Screen.Recording.2023-07-11.at.00.32.22.mov
Recording.1243.mp4

Expensify/Expensify Issue URL:
Issue reported by: @namhihi237
Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1689010436307099

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~01afb147c594f572e5
  • Upwork Job ID: 1679478215688933376
  • Last Price Increase: 2023-07-13
  • Automatic offers:
    • namhihi237 | Contributor | 26516237
    • namhihi237 | Reporter | 26516238
@kavimuru kavimuru added Daily KSv2 Bug Something is broken. Auto assigns a BugZero manager. labels Jul 11, 2023
@melvin-bot
Copy link

melvin-bot bot commented Jul 11, 2023

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

@melvin-bot
Copy link

melvin-bot bot commented Jul 11, 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

@kavimuru
Copy link
Author

Proposal

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

The task should not be displayed in user B if creation fails or gives an error when back online with room setting admins only

What is the root cause of that problem?

When the user creates a task when offline optimisticData is saved in Onyx with reportAction create a task. But after API returns data with an error after back online, failureData doesn't update the error for this reportAction to explain why the user cannot create a task.

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

  • When the back online API create task will be called, after API returns an error failureData should update the error for report action instead of updating pending action is null like this.
    onyxMethod: Onyx.METHOD.MERGE,
    key: `${ONYXKEYS.COLLECTION.REPORT_ACTIONS}${optimisticTaskReport.reportID}`,
    value: {
        [optimisticTaskCreatedAction.reportActionID]: {
            errors: ErrorUtils.getMicroSecondOnyxError('task.error.genericCreateFailureMessage'),
        }
    },
  • we also need to create a new translation for this error.
genericCreateFailureMessage: "Unexpected error create a task, please try again later"

What alternative solutions did you explore? (Optional)

N/A

@thienlnam
Copy link
Contributor

We should also remove the admins only room from the selector if you are not an admin

@namhihi237
Copy link
Contributor

We should also remove the admins only room from the selector if you are not an admin

@thienlnam I am not clear what you mean, you mean if the user in the room is not an admin then we will remove 'admins only' from the selector

@thienlnam
Copy link
Contributor

thienlnam commented Jul 11, 2023

Oh I see, the task was created by going offline and not getting the update that it's an admin only room. Nvm in that case

@thienlnam thienlnam changed the title Offline - Create task in a room with admin only Offline - Create task and split bill in a room with admin only Jul 11, 2023
@thienlnam
Copy link
Contributor

thienlnam commented Jul 11, 2023

Let's also address the split bill case in this issue
https://expensify.slack.com/archives/C049HHMV9SM/p1689011065028709

EDIT: Split bill case needs some BE updates so we're splitting them into a seperate issue

@thienlnam thienlnam changed the title Offline - Create task and split bill in a room with admin only Offline - Create task in a room with admin only Jul 11, 2023
@isabelastisser isabelastisser added the External Added to denote the issue can be worked on by a contributor label Jul 13, 2023
@melvin-bot melvin-bot bot changed the title Offline - Create task in a room with admin only [$1000] Offline - Create task in a room with admin only Jul 13, 2023
@melvin-bot
Copy link

melvin-bot bot commented Jul 13, 2023

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

@melvin-bot melvin-bot bot added Overdue Help Wanted Apply this label when an issue is open to proposals by contributors labels Jul 13, 2023
@melvin-bot
Copy link

melvin-bot bot commented Jul 13, 2023

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

@melvin-bot
Copy link

melvin-bot bot commented Jul 13, 2023

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

@isabelastisser
Copy link
Contributor

Not overdue!

@melvin-bot melvin-bot bot removed the Overdue label Jul 13, 2023
@parasharrajat
Copy link
Member

parasharrajat commented Jul 13, 2023

QA posted a solution above ✨ ✨ ✨ ✨ .....QA's proposal looks to good to me...
.
.
.
.
.
.
.
.
.
.
.
.
.
.
.
.
.

Just Kidding.

@namhihi237' proposal looks good to me. @thienlnam I am not the expert for optimistic actions but is the proposal expected behaviour?

🎀 👀 🎀 C+ reviewed

@melvin-bot
Copy link

melvin-bot bot commented Jul 13, 2023

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

@thienlnam
Copy link
Contributor

Hmm, we should be displaying an error in the reportAction - but maybe this should be set via pusher in the back-end

@namhihi237
Copy link
Contributor

@thienlnam If we use messages from the backend I think need to Backend return the message in reportActions , currently it's missing.
Screenshot 2023-07-14 at 10 47 28

@thienlnam
Copy link
Contributor

Yeah I'm aware, this will likely have to be an internal issue

@thienlnam thienlnam removed External Added to denote the issue can be worked on by a contributor Help Wanted Apply this label when an issue is open to proposals by contributors labels Jul 14, 2023
@parasharrajat
Copy link
Member

I will check the issue status tomorrow and share the next step. It seems we got stalled due to discussion.

@melvin-bot
Copy link

melvin-bot bot commented Oct 5, 2023

@parasharrajat, @thienlnam, @isabelastisser, @namhihi237 Whoops! This issue is 2 days overdue. Let's get this updated quick!

@parasharrajat
Copy link
Member

Just got back from Vacation, will check asap.

@parasharrajat
Copy link
Member

parasharrajat commented Oct 12, 2023

I will an update tomorrow on this. Let's finalize this asap. I can see that things have changed since we started the PR.

@isabelastisser
Copy link
Contributor

Hey @parasharrajat what's next here?

@parasharrajat
Copy link
Member

parasharrajat commented Oct 20, 2023

Retesting it now.

@namhihi237
Copy link
Contributor

Hi @parasharrajat @thienlnam I just tested on the main
Currently on main: After the task is created with an error, the task report is not deleted when opening and leaving the task report.

Screen.Recording.2023-10-23.at.14.23.30.mov

Fix version: In the task preview, we will show RGB. When RGB is clear we will clear the task preview and task report

Screen.Recording.2023-10-23.at.14.25.22.mov

What do you think about this?

@parasharrajat
Copy link
Member

This looks good to me @namhihi237. But we should also show an error on the Task report. In the fix video above, I can see that there is RBR dot on the task report but there is no error shown while it is open. It should have an error similar to report action.

@parasharrajat
Copy link
Member

Let's get this wrapped up. Can you please update the PR and I will retest?

@namhihi237
Copy link
Contributor

@parasharrajat yes, I updated the PR, please help to retest it.Thanks

@thienlnam
Copy link
Contributor

This looks good to me @namhihi237. But we should also show an error on the Task report. In the fix video above, I can see that there is RBR dot on the task report but there is no error shown while it is open. It should have an error similar to report action.

Agreed with this, it should have an error on the task report so you can clear it there as well

@namhihi237
Copy link
Contributor

@thienlnam Currently, in the task report, when creating an error the backend will return the error "Auth CreateTask returned an error" so we have 2 errors shown here. cc @parasharrajat , What do you think?

Screen.Recording.2023-10-25.at.00.38.48.mov

@thienlnam
Copy link
Contributor

That's fine, it happens in other locations as well

Copy link

melvin-bot bot commented Nov 1, 2023

@parasharrajat, @thienlnam, @isabelastisser, @namhihi237 Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

Copy link

melvin-bot bot commented Nov 3, 2023

@parasharrajat, @thienlnam, @isabelastisser, @namhihi237 Eep! 4 days overdue now. Issues have feelings too...

@thienlnam
Copy link
Contributor

PR almost complete, couple last comments but should be merged next week

@thienlnam thienlnam added Weekly KSv2 and removed Daily KSv2 Waiting for copy User facing verbiage needs polishing labels Nov 4, 2023
@namhihi237
Copy link
Contributor

@isabelastisser PR already deploy production but the label of this issue does not add.

@namhihi237
Copy link
Contributor

Hi @thienlnam can you help to change label and title for payment. Seem auto has an error. Thanks

@thienlnam thienlnam added the Awaiting Payment Auto-added when associated PR is deployed to production label Nov 23, 2023
@thienlnam
Copy link
Contributor

Here's the linked PR #26848

cc @isabelastisser This is ready for payment

@thienlnam thienlnam changed the title [$1000] Offline - Create task in a room with admin only [HOLD for payment 2023-11-21][$1000] Offline - Create task in a room with admin only Nov 23, 2023
@isabelastisser
Copy link
Contributor

Payment summary:

📣 @parasharrajat Please request via NewDot manual requests for the Reviewer role ($1000)
@namhihi237 issue reporter: $50
@namhihi237 Accepted proposal: $1000

Payments made in Upwork, all set!

@namhihi237
Copy link
Contributor

@isabelastisser hi the issue report before i think it should be 250$ for reporter

@isabelastisser
Copy link
Contributor

Thanks for the heads up, @namhihi237 ! I updated the payment amount in Upwork now.

@parasharrajat
Copy link
Member

Payment requested as per #22608 (comment)

@JmillsExpensify
Copy link

$1,000 approved for @parasharrajat based on this comment.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Awaiting Payment Auto-added when associated PR is deployed to production Bug Something is broken. Auto assigns a BugZero manager. Engineering External Added to denote the issue can be worked on by a contributor Reviewing Has a PR in review Weekly KSv2
Projects
None yet
Development

No branches or pull requests

9 participants