Payment for E/E issue #240970

[mallenexpensify]

Need to pay out
E/E GH - https://github.com/Expensify/Expensify/issues/240970
PR - #13898

$1000 to @eVoloshchak

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~01b5a6fdbdbdead3bf
• Upwork Job ID: 1620608464115429376
• Last Price Increase: 2023-02-01

[melvin-bot]

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

[melvin-bot]

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

[mallenexpensify]

@eVoloshchak can you please accept the job and reply here once you have?
https://www.upwork.com/jobs/~01b5a6fdbdbdead3bf

[eVoloshchak]

Accepted

This PR caused a regression here.
The issue didn't happen on PR branch test but happened only after merge, #14289 deleted the import we were using.
Both branches were working perfectly before the merge, linter checks were successful for both, but there is no linter check after you merge a PR into main.
I could have caught it when reviewing if I tested the changes against latest main, so it's probably worth adding Tested against main item to the reviewer checklist.
However, that still will not prevent similar issues if C+ does the review against main, after which other conflicting PR gets merged, and then the current PR is merged by an Expensify engineer.

So as @aimane-chnaif correctly pointed out

This is a good example why we should re-test after merging into main or merge from main before review and test

I'd say we even need to re-test after merging into main AND merge from main before review

UPD: started a slack thread here

[jayeshmangwani]

@eVoloshchak can you please accept the job and reply here once you have? https://www.upwork.com/jobs/~01b5a6fdbdbdead3bf

@mallenexpensify should I also apply for reporting regression ?

[jayeshmangwani]

@eVoloshchak can you please accept the job and reply here once you have? https://www.upwork.com/jobs/~01b5a6fdbdbdead3bf

@mallenexpensify should I also apply for reporting regression ?

hey @mallenexpensify , can you please help here, I have reported a regression from this PR, am I eligible for report regression here ?

[mallenexpensify]

Sorry for the delay @jayeshmangwani , you should be paid, job is
https://www.upwork.com/jobs/~0148e02c38aea53379

@eVoloshchak , do you think your pay should be reduced because of the regression? There's def grey area between could and should have caught. Is there anything that should be updated in the C+ doc to help prevent similar circumstances in the future?

[eVoloshchak]

do you think your pay should be reduced because of the regression?

*“Should have” = C+ should have caught the bug by fully following the checklist. If C+ skips a step or completed the checklist incompletely, payment will be cut in half.

@mallenexpensify, I could have caught this if I tested against latest main, but there is no checklist item requiring this

Is there anything that should be updated in the C+ doc to help prevent similar circumstances in the future?

Yes. In my opinion, 3 things could be improved

1. Add tested against main item to the reviewer checklist
2. Add the same item to the checklist CME are following when they are merging the PR (since there still can be a case when the regression was introduced after C+ approved the PR, but before it was merged)
3. Run linter check before (or after) the PR was merged. It would have failed in this particular case

[mallenexpensify]

Thanks @eVoloshchak , commented on the Slack thread https://expensify.slack.com/archives/C01GTK53T8Q/p1675909181993929?thread_ts=1675274259.566619&cid=C01GTK53T8Q

[MelvinBot]

@marcochavezf, @eVoloshchak, @mallenexpensify Huh... This is 4 days overdue. Who can take care of this?

[mallenexpensify]

Sorry this is taking forever @eVoloshchak , I'm unsure what to do, pinged @marcochavezf in the Slack thread since he's the assigned engineer https://expensify.slack.com/archives/C01GTK53T8Q/p1676414984579679?thread_ts=1675274259.566619&cid=C01GTK53T8Q

[MelvinBot]

@marcochavezf, @eVoloshchak, @mallenexpensify Eep! 4 days overdue now. Issues have feelings too...

[marcochavezf]

Hi @mallenexpensify, I already added the new checklist item in this PR. I think that was the outstanding issue here, correct?

[mallenexpensify]

I just paid @eVoloshchak , we were going back/forth about adding a step to the PR checklist for 'checking against main'. I need to still do that so leaving this open for another few days til I get it done.

[mallenexpensify]

already added the new checklist item in this #15235.

This I missed this earlier, we're all set here then 👋