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 2024-04-05] [$500] Internal QA checklist not working for external PRs #37691

Closed
roryabraham opened this issue Mar 4, 2024 · 28 comments
Assignees
Labels
Awaiting Payment Auto-added when associated PR is deployed to production Bug Something is broken. Auto assigns a BugZero manager. External Added to denote the issue can be worked on by a contributor Weekly KSv2

Comments

@roryabraham
Copy link
Contributor

roryabraham commented Mar 4, 2024

Problem

The deploy checklist has a section for InternalQA and is meant to tag Expensify engineers to complete the QA for each PR. This failed for this checklist and this PR.

Solution

Fix it! make sure that people responsible for internal QA are correctly tagged and assigned to the issue (right now I don't think they're assigned, just tagged)

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~0149b7b8e3e44c57bd
  • Upwork Job ID: 1764724130794770432
  • Last Price Increase: 2024-03-04
  • Automatic offers:
    • hoangzinh | Reviewer | 0
    • rayane-djouah | Contributor | 0
@roryabraham roryabraham added External Added to denote the issue can be worked on by a contributor Bug Something is broken. Auto assigns a BugZero manager. labels Mar 4, 2024
@roryabraham roryabraham self-assigned this Mar 4, 2024
@melvin-bot melvin-bot bot changed the title Internal QA checklist not working for external PRs [$500] Internal QA checklist not working for external PRs Mar 4, 2024
Copy link

melvin-bot bot commented Mar 4, 2024

Job added to Upwork: https://www.upwork.com/jobs/~0149b7b8e3e44c57bd

@melvin-bot melvin-bot bot added the Help Wanted Apply this label when an issue is open to proposals by contributors label Mar 4, 2024
Copy link

melvin-bot bot commented Mar 4, 2024

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

@melvin-bot melvin-bot bot added the Daily KSv2 label Mar 4, 2024
Copy link

melvin-bot bot commented Mar 4, 2024

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

@roryabraham
Copy link
Contributor Author

roryabraham commented Mar 4, 2024

in a hurry so apologies for the low-quality issue. Going to make this external, and anyone proposing a fix should note that thorough automated tests will be a hard requirement for this issue.

@rayane-djouah
Copy link
Contributor

rayane-djouah commented Mar 4, 2024

Proposal

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

Expensify engineer is not tagged and assigned in Deploy Checklist to complete the QA for PRs with Internal QA.

What is the root cause of that problem?

Here:

const internalQAPRMap = _.reduce(
_.filter(data, (pr) => !_.isEmpty(_.findWhere(pr.labels, {name: CONST.LABELS.INTERNAL_QA}))),
(map, pr) => {
// eslint-disable-next-line no-param-reassign
map[pr.html_url] = _.compact(_.pluck(pr.assignees, 'login'));
return map;
},
{},
);

we create internalQAPRMap using pr.html_url and pr.assignees.login but PRs does not always have assignees and may have external assignees. for example #37088 does not have assignees and it's pr.assignees is an empty array (we can check PR details at https://api.github.com/repos/Expensify/App/pulls/37088 and github action execution for the Deploy Checklist in https://github.com/Expensify/App/actions/runs/8103975562/job/22149654475 for further details)

image

image

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

  1. We should use pr.merged_by.login instead of pr.assignees.login for creating the internalQAPRMap here. and change the naming here.
    or if we want to keep tagging and assigning PR assignees also in addition to PR merger, we can keep current logic and add the PR merger also.
  2. Currently Expensify engineer is just tagged in the Deploy Checklist issue and not assigned for Internal QA of a PR, to assign them as well, we should return a list of assignees taken from internalQAPRMap in here with the issueBody as well and add the Engineers to the assignees when creating the Deploy Checklist issue here
  3. we should add test data in addition to this that have empty assignees and a defined merged_by attribute, and assert that the PR merger is tagged in the issueBody here, we should also assert the returned assignees list. we can also add tests for internal QA PRs in createOrUpdateStagingDeployTest.js if needed.

@roryabraham
Copy link
Contributor Author

sounds good @rayane-djouah 👍🏼

We should use pr.merged_by.login instead of pr.assignees.login for creating the internalQAPRMap here. and change the naming here.

I think that's fine. It's better than using assignee as the person who merged the PR will always be an internal engineer. we don't need to use both, that's just more complicated.

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

melvin-bot bot commented Mar 5, 2024

📣 @hoangzinh 🎉 An offer has been automatically sent to your Upwork account for the Reviewer role 🎉 Thanks for contributing to the Expensify app!

Offer link
Upwork job

Copy link

melvin-bot bot commented Mar 5, 2024

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

@roryabraham
Copy link
Contributor Author

@hoangzinh I'm actually going to opt-out of C+ services for this GitHub Actions improvement.

@roryabraham
Copy link
Contributor Author

@slafortune I don't seem to be able to access the offer link for @hoangzinh. Can you please retract that offer as we don't need C+ here?

@rayane-djouah
Copy link
Contributor

Thank you! Will raise a PR ASAP

@roryabraham
Copy link
Contributor Author

@rayane-djouah any estimate on when the PR will be ready for review? this isn't particularly urgent, so I'm going to change the priority to weekly

@roryabraham roryabraham added Weekly KSv2 and removed Daily KSv2 labels Mar 6, 2024
@rayane-djouah
Copy link
Contributor

an ETA is tomorrow

@roryabraham
Copy link
Contributor Author

It "deploys" immediately in this case, but let's give it a week to monitor for regressions nonetheless

@roryabraham
Copy link
Contributor Author

we should also test merging a PR with the InternalQA label. I added the InternalQA label to the PR we just merged so it can test itself 🙃

@roryabraham roryabraham removed the Awaiting Payment Auto-added when associated PR is deployed to production label Mar 11, 2024
@roryabraham
Copy link
Contributor Author

PR reverted. @rayane-djouah we'll need a redo

@melvin-bot melvin-bot bot added Weekly KSv2 Awaiting Payment Auto-added when associated PR is deployed to production and removed Weekly KSv2 labels Mar 12, 2024
@melvin-bot melvin-bot bot changed the title [$500] Internal QA checklist not working for external PRs [HOLD for payment 2024-03-19] [$500] Internal QA checklist not working for external PRs Mar 12, 2024

This comment was marked as outdated.

This comment was marked as off-topic.

@roryabraham roryabraham removed the Awaiting Payment Auto-added when associated PR is deployed to production label Mar 12, 2024
@roryabraham roryabraham changed the title [HOLD for payment 2024-03-19] [$500] Internal QA checklist not working for external PRs [$500] Internal QA checklist not working for external PRs Mar 12, 2024
@roryabraham
Copy link
Contributor Author

roryabraham commented Mar 12, 2024

No BZ checklists yet, not awaiting payment, not on HOLD. The PR was reverted

@rayane-djouah
Copy link
Contributor

It's working correctly: #39033 🎉

@rayane-djouah
Copy link
Contributor

This should be "Awaiting Payment"

@melvin-bot melvin-bot bot added Weekly KSv2 Awaiting Payment Auto-added when associated PR is deployed to production and removed Weekly KSv2 labels Mar 29, 2024
@melvin-bot melvin-bot bot changed the title [$500] Internal QA checklist not working for external PRs [HOLD for payment 2024-04-05] [$500] Internal QA checklist not working for external PRs Mar 29, 2024
@melvin-bot melvin-bot bot removed the Reviewing Has a PR in review label Mar 29, 2024
Copy link

melvin-bot bot commented Mar 29, 2024

Reviewing label has been removed, please complete the "BugZero Checklist".

Copy link

melvin-bot bot commented Mar 29, 2024

The solution for this issue has been 🚀 deployed to production 🚀 in version 1.4.57-5 and is now subject to a 7-day regression period 📆. Here is the list of pull requests that resolve this issue:

If no regressions arise, payment will be issued on 2024-04-05. 🎊

For reference, here are some details about the assignees on this issue:

Copy link

melvin-bot bot commented Mar 29, 2024

BugZero Checklist: The PR fixing this issue has been merged! The following checklist (instructions) will need to be completed before the issue can be closed:

  • [@roryabraham] The PR that introduced the bug has been identified. Link to the PR:
  • [@roryabraham] The offending PR has been commented on, pointing out the bug it caused and why, so the author and reviewers can learn from the mistake. Link to comment:
  • [@roryabraham] A discussion in #expensify-bugs has been started about whether any other steps should be taken (e.g. updating the PR review checklist) in order to catch this type of bug sooner. Link to discussion:
  • [@rayane-djouah] Determine if we should create a regression test for this bug.
  • [@rayane-djouah] If we decide to create a regression test for the bug, please propose the regression test steps to ensure the same bug will not reach production again.
  • [@slafortune] Link the GH issue for creating/updating the regression test once above steps have been agreed upon:

@rayane-djouah
Copy link
Contributor

The offending PR is #6102
I believe a regression test is unnecessary because this is a GitHub Action, and we have already set up automated tests.

@slafortune
Copy link
Contributor

Paid

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. External Added to denote the issue can be worked on by a contributor Weekly KSv2
Projects
None yet
Development

No branches or pull requests

4 participants