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 #37199][$500] Remove members out of a workspace are not crossed out in offline mode #37109

Closed
1 of 6 tasks
m-natarajan opened this issue Feb 22, 2024 · 31 comments
Closed
1 of 6 tasks
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 Reviewing Has a PR in review Weekly KSv2

Comments

@m-natarajan
Copy link

m-natarajan commented Feb 22, 2024

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.43-3
Reproducible in staging?: y
Reproducible in production?: n
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: @hoangzinh
Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1708519808525439

Action Performed:

  1. Go to Settings -> Workspace -> Open any WS -> Members
  2. Invite several members
  3. Turn off the internet connection
  4. Remove WS members

Expected Result:

Removed members should be strike out

Actual Result:

Notice that members are disappearing instead of 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

Screen.Recording.2024-02-21.at.19.49.26.mov
Recording.2761.mp4

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~016f83f55225363e92
  • Upwork Job ID: 1760776518388736000
  • Last Price Increase: 2024-02-22
  • Automatic offers:
    • mkhutornyi | Contributor | 0
@m-natarajan m-natarajan added DeployBlockerCash This issue or pull request should block deployment 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 Feb 22, 2024
Copy link

melvin-bot bot commented Feb 22, 2024

Job added to Upwork: https://www.upwork.com/jobs/~016f83f55225363e92

@melvin-bot melvin-bot bot changed the title Remove members out of a workspace are not crossed out in offline mode [$500] Remove members out of a workspace are not crossed out in offline mode Feb 22, 2024
@melvin-bot melvin-bot bot added the Help Wanted Apply this label when an issue is open to proposals by contributors label Feb 22, 2024
Copy link

melvin-bot bot commented Feb 22, 2024

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

Copy link

melvin-bot bot commented Feb 22, 2024

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

@github-actions github-actions bot added Engineering Hourly KSv2 and removed Daily KSv2 labels Feb 22, 2024
Copy link
Contributor

👋 Friendly reminder that deploy blockers are time-sensitive ⏱ issues! Check out the open `StagingDeployCash` deploy checklist to see the list of PRs included in this release, then work quickly to do one of the following:

  1. Identify the pull request that introduced this issue and revert it.
  2. Find someone who can quickly fix the issue.
  3. Fix the issue yourself.

Copy link

melvin-bot bot commented Feb 22, 2024

Triggered auto assignment to @francoisl (Engineering), see https://stackoverflowteams.com/c/expensify/questions/9980/ for more details.

@francoisl

This comment was marked as resolved.

@mkhutornyi
Copy link
Contributor

mkhutornyi commented Feb 22, 2024

Proposal

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

Offline removed members are not visible

What is the root cause of that problem?

hoverDimmingValue={1}
hoverStyle={styles.hoveredComponentBG}
dataSet={{[CONST.SELECTION_SCRAPER_HIDDEN_ELEMENT]: true}}
onMouseDown={shouldPreventDefaultFocusOnSelectRow ? (e) => e.preventDefault() : undefined}
nativeID={keyForList}
style={pressableStyle}
>
{({hovered}) => (
<>

In #36403, they changed child of PressableWithFeedback from component (<View />) to function (() => <View />) to get hovered value.
This causes applyStrikeThrough not work in OfflineWithFeedback

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

Change wrapping order like this:

<PressableWithFeedback>
    {hovered =>
        <OfflineWithFeedback>
                ...
        </OfflineWithFeedback>
    }
</PressableWithFeedback>

This is exactly the same root cause as #35702 (comment)

@francoisl
Copy link
Contributor

Yeah good catch, tested locally and it works well. Feel free to submit a PR plese @mkhutornyi.

@melvin-bot melvin-bot bot removed the Help Wanted Apply this label when an issue is open to proposals by contributors label Feb 22, 2024
Copy link

melvin-bot bot commented Feb 22, 2024

📣 @mkhutornyi 🎉 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 📖

@mkhutornyi
Copy link
Contributor

Thanks. Raising PR shortly

@mkhutornyi
Copy link
Contributor

Looks like there's another regression happening on main. Caused by #37000
Text is not crossed out on native.
@francoisl Can this be out of scope or should be fixed here?

@francoisl
Copy link
Contributor

Do you see a way to fix that regression? If you do, that's great and let's fix it too, otherwise we can put a comment in the PR to fix it later. Not a huge deal since that PR is not on staging yet.

@mkhutornyi
Copy link
Contributor

#37000 was kind of huge refactor and I see that it's not straight forward fix.
As it's less severe bug, I think it's better to let author fix it.

@francoisl
Copy link
Contributor

Works for me.

@mkhutornyi
Copy link
Contributor

I commented on offending line - https://github.com/Expensify/App/pull/37000/files#r1500067904

@luacmartins
Copy link
Contributor

This is known #37074 (comment) and will be fixed when we update the remove functionality next

@luacmartins
Copy link
Contributor

luacmartins commented Feb 23, 2024

Removing blocker label. I was gonna close the issue, but I see that we already have a PR up to fix it, so I'll let it go through.

@luacmartins luacmartins removed the DeployBlockerCash This issue or pull request should block deployment label Feb 23, 2024
Copy link

melvin-bot bot commented Feb 26, 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.

@mkhutornyi
Copy link
Contributor

@francoisl based on #37235 (comment), should I continue my PR or will be handled while implementing the remove functionality in Simplified Collect?

@burczu
Copy link
Contributor

burczu commented Feb 27, 2024

@mkhutornyi I've already addressed this issue in my PR regarding removing members on the members page (as stated in the mentioned comment): #37199 - it's still in draft state but it will be published tomorrow.

@mkhutornyi
Copy link
Contributor

@burczu I checked your PR but solution is same as mine.
Please consider @parasharrajat's concern here.

@mkhutornyi
Copy link
Contributor

Ideally, can we fix this in OfflineWithFeedback? So that function as child would work with strikethrough.
I'm concerned similar issue might happen in the future.
This is already 2nd bug. 1st bug was #35702

@parasharrajat
Copy link
Member

let's find a universal solution. It is not good to tackle this problem case by case.

@MitchExpensify
Copy link
Contributor

Should we close seeing as this is going to be fixed here #37074 (comment) and we don't have a universal solution identified yet - What do you think @parasharrajat ?

@parasharrajat
Copy link
Member

I think we can.

@burczu
Copy link
Contributor

burczu commented Feb 28, 2024

Ideally, can we fix this in OfflineWithFeedback? So that function as child would work with strikethrough. I'm concerned similar issue might happen in the future. This is already 2nd bug. 1st bug was #35702

Yeah - I'm working on this right now. Thanks for pointing this out.

@parasharrajat
Copy link
Member

@MitchExpensify Could you please put this issue on HOLD for #37199 or close this it?

@luacmartins luacmartins changed the title [$500] Remove members out of a workspace are not crossed out in offline mode [HOLD #37199][$500] Remove members out of a workspace are not crossed out in offline mode Feb 29, 2024
@luacmartins
Copy link
Contributor

done

@parasharrajat
Copy link
Member

parasharrajat commented Mar 29, 2024

@MitchExpensify This issue is resolved. Please close it and its related job on upwork. I am unsure if any payment is pending or if the contributor was already paid.

  • Contributor created the PR and was hired.
  • During review, I found out that a better solution was being implemented in a new PR so we held the issue.

@parasharrajat
Copy link
Member

Bump @MitchExpensify

@MitchExpensify
Copy link
Contributor

Thanks @parasharrajat, let's close. I don't think any payment is due as nothing was ever merged but please let me know if anyone disagrees

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 Reviewing Has a PR in review Weekly KSv2
Projects
None yet
Development

No branches or pull requests

7 participants