[HOLD for payment 2022-11-29] [$250] Deleting a comment unresponsive after right click to copy - reported by @gadhiyamanan

[mvtglobally]

If you haven’t already, check out our contributing guidelines for onboarding and email [email protected] to request to join our Slack channel!

---

Action Performed:

1. Go to any chat > long press on any message
2. click copy to clipboard or mark as read
3. before closing popup click on delete comment

Expected Result:

Message should be deleted

Actual Result:

Message is not deleted

Workaround:

unknown

Platform:

Where is this issue occurring?

• Android

Version Number: 1.1.99-0
Reproducible in staging?: Y
Reproducible in production?: Y
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_20220828-132253_One.UI.Home.mp4

Expensify/Expensify Issue URL:
Issue reported by: @gadhiyamanan
Slack conversation: https://expensify.slack.com/archives/C01GTK53T8Q/p1661673321608069

View all open jobs on GitHub

[melvin-bot]

Triggered auto assignment to @RachCHopkins (AutoAssignerTriage), see https://stackoverflow.com/c/expensify/questions/4749 for more details.

[RachCHopkins]

Working on v1.2.0-8

[gadhiyamanan]

@RachCHopkins crash was fixed but still not able to delete message .
https://user-images.githubusercontent.com/54790231/191170377-072d998c-1c20-490d-ad30-50ea1fcf640d.mp4

[RachCHopkins]

I'm unable to reproduce this - has anyone else reported the same problem?

[melvin-bot]

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

[deetergp]

@RachCHopkins I am also unable to reproduce this 🤷 and my message deleted fine. I'm inclined to close it…

[gadhiyamanan]

strange... but I am still not able to delete the message by following the steps
Version(v1.2.3-0)

Go to any chat > long press on any message
click copy to clipboard or mark as read
before closing popup click on delete comment

Screen.Recording.2022-09-21.at.10.36.30.AM.mov

cc: @deetergp

[melvin-bot]

@deetergp Eep! 4 days overdue now. Issues have feelings too...

[gadhiyamanan]

@deetergp still not able to delete message with latest version after mark as read or copy to clipboard
https://user-images.githubusercontent.com/54790231/192721948-a1e0dcf8-bcc5-4e7e-bbe8-0389ddaef997.mp4

[muttmuure]

I've reproduced this on web and iOS.

Reproduction steps are correct.

Adding external label

Copy.and.Delete.not.actioned.mp4

[melvin-bot]

Triggered auto assignment to @muttmuure (External), see https://stackoverflow.com/c/expensify/questions/8582 for more details.

[melvin-bot]

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

[melvin-bot]

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

[0xmiros]

This proposal is mutating the state. From my knowledge of React, this is something that should be completely avoided and can cause hard to track bugs, are there cases where mutations of the state is acceptable? No, as far as I know, but please let me know if I'm wrong about this.

This is usually the case when no need to re-render because of state update.
Of course we can use class member variable (i.e. this.reportID) instead of state variable (i.e. this.state.reportID) to avoid re-render but in this component, it's fine and safe to mutate those directly.

[mananjadhav]

@Pujan92 We have a regression on this issue, that we'll have to fix. Please create a fresh PR to fix the issue and then we can review.

[aldo-expensify]

This proposal is mutating the state. From my knowledge of React, this is something that should be completely avoided and can cause hard to track bugs, are there cases where mutations of the state is acceptable? No, as far as I know, but please let me know if I'm wrong about this.

This is usually the case when no need to re-render because of state update. Of course we can use class member variable (i.e. this.reportID) instead of state variable (i.e. this.state.reportID) to avoid re-render but in this component, it's fine and safe to mutate those directly.

I understand the point that it may be safe in this specific case at this time, but this type of code removes the assumption that the state is immutable. Removing that assumptions open the door for future bugs because other developers won't expect this, also, simple references check are not longer enough for checking equality. In my opinion it is similar to storing your state in window, yes, it may work, but at some point you will have hard to follow buggy code that breaks all the time because it is outside of what we can expect in react.

I'm not very familiarized with class component, but yes, I think we use class members variable for the cases. In functional react, this can be done with useRef because the reference.current is mutable.

Found this in the react documentation that talks about this: https://reactjs.org/docs/react-component.html#state
Seems like the way to go is to use class member variables if you want to do something like that.

[Pujan92]

Sorry for the regression issue. I think updating the hideContextMenu with the check of the callback function which gets passed when there is a delay in context menu action can solve the issue. So if the action is deleting the comment we won't update the state from hideContextMenu method otherwise it(delete modal) won't have the reportID and reportAction.

hideContextMenu(onHideActionCallback) {
        if (_.isFunction(onHideActionCallback)) {
            this.onPopoverHideActionCallback = onHideActionCallback;
            if (!this.state.isDeleteCommentConfirmModalVisible) {
                this.setState({
                    reportID: '0',
                    reportAction: {},
                    selection: '',
                    reportActionDraftMessage: '',
                    isPopoverVisible: false,
                });
            }
        } else {
            this.setState({
                reportID: '0',
                reportAction: {},
                selection: '',
                reportActionDraftMessage: '',
                isPopoverVisible: false,
            });
        }
    }

Recording_1669097982090.webm

[mananjadhav]

@Pujan92 thanks for the quick PR, I'll try to get this one before tomorrow.

[mananjadhav]

@muttmuure Payment date to be removed, as it caused a regression.

[Pujan92]

@Pujan92 thanks for the quick PR, I'll try to get this one before tomorrow.

@mananjadhav Could you plz review once you get some time

[mananjadhav]

Apologies for the delay here. I'll review it by EOD today!

[0xmiros]

I think I am eligible for compensation since my solution was picked up and merged
ref: #12917 (comment)

[mananjadhav]

This was deployed to production but title for the payment date isn't updated.

@muttmuure please note this had a regression so C+ payment has to be adjusted accordingly.

[mananjadhav]

@deetergp @muttmuure quick bump

[Pujan92]

@muttmuure Are we fine to complete this?

[muttmuure]

Thanks for the bump guys - getting this sorted today

[muttmuure]

OK by the logic of Stack Overflow:

@Pujan92 = $250
@0xmiroslav = $250
@mananjadhav = $125

The job is here:

https://www.upwork.com/jobs/~012e1e9aa00357fd02

Please could you apply and we'll get you paid and close this out

[Pujan92]

https://www.upwork.com/jobs/~012e1e9aa00357fd02
Please could you apply and we'll get you paid and close this out

I have already applied for the earlier posted job(https://www.upwork.com/jobs/~01bc6b990f835122e4)

[0xmiros]

@muttmuure applied

[gadhiyamanan]

@muttmuure applied for reporting

[mananjadhav]

Applied @muttmuure

[muttmuure]

@Pujan92 the older job expired, apologies, so you need to apply to the new one.

[Pujan92]

@Pujan92 the older job expired, apologies, so you need to apply to the new one.

@muttmuure actually it was offered and I accepted earlier, milestone is also active within it. Can you plz check once?

[muttmuure]

Oh I see - sure we can do it that way too.

[muttmuure]

Everyone has been paid - closing out. Thanks!