[NoQA] Fix the internal QA PR section losing its state #9946

mountiny · 2022-07-16T17:09:31Z

Details

The internal QA section of the Staging deploy issue loses its state when the issue is regenerated when new deploy blocker is found or we CP some PR to Staging.

Not ideal.

This PR aims to fix this situation by passing a list of URLs of the InternalQA PRs which were checked off already to the function which generates the new issue body.

I have left comments in the PR to explain how this is fixing the problem and also why the integration test hasn't caught this issue previously.

Fixed Issues

$ https://github.com/Expensify/Expensify/issues/198446

Tests

Automated tests cover this change.

Manual tests:

This PR will have InternalQA label
Once merged and deployed to staging, it should appear in the latest Staging QA issue
Check the PR off in the list signaling that it has been resolved
Create another PR with a dummy change at some markdown file for example
Add CP Staging and InternalQA labels and merge it
Once it is deployed to production it should show up in the InternalQA section too and this PR should preserve its QA state (ie QA done, resolved)

Verify that no errors appear in the JS console

PR Review Checklist

Contributor (PR Author) Checklist

PR Reviewer Checklist

The Contributor+ will copy/paste it into a new comment and complete it after the author checklist is completed

QA Steps

Check the Tests section.

Verify that no errors appear in the JS console

Screenshots

N/A

… of the deployment issue

mountiny · 2022-07-17T01:18:29Z

.github/actions/javascript/createOrUpdateStagingDeploy/createOrUpdateStagingDeploy.js

+            // Get the internalQA PR list, preserving the previous state of `isResolved`
+            const internalQAPRList = _.sortBy(
+                currentStagingDeployCashData.internalQAPRList,
+                'number',
+            );


We are first getting the list of the InternalQA PRs from the previous issue body.

mountiny · 2022-07-17T01:19:13Z

.github/actions/javascript/createOrUpdateStagingDeploy/createOrUpdateStagingDeploy.js

            return GithubUtils.generateStagingDeployCashBody(
                newTag,
                _.pluck(PRList, 'url'),
                _.pluck(_.where(PRList, {isVerified: true}), 'url'),
                _.pluck(_.where(PRList, {isAccessible: true}), 'url'),
                _.pluck(deployBlockers, 'url'),
                _.pluck(_.where(deployBlockers, {isResolved: true}), 'url'),
+                _.pluck(_.where(internalQAPRList, {isResolved: true}), 'url'),
                didVersionChange ? false : currentStagingDeployCashData.isTimingDashboardChecked,
                didVersionChange ? false : currentStagingDeployCashData.isFirebaseChecked,


And then passing as a parameter an array of URLs of only those PRs which are resolved (the checkbox has been ticked)

mountiny · 2022-07-17T01:20:52Z

.github/libs/GithubUtils.js

-                        issueBody += `\r\n${_.contains(verifiedOrNoQAPRs, URL) ? '- [x]' : '- [ ]'} `;
+                        issueBody += `\r\n${_.contains(resolvedInternalQAPRs, URL) ? '- [x]' : '- [ ]'} `;


And finally check if the URL is in the resolvedInternalQAPRs array as the verifiedOrNoQAPRs does not include the necessary InternalQA PRs with isResolved field, hence it was always false and it have reset the checkbox to unchecked state.

mountiny · 2022-07-17T01:22:47Z

tests/unit/GithubUtilsTest.js

@@ -533,7 +534,7 @@ describe('GithubUtils', () => {
        ));

        test('Test some verified internalQA PRs', () => (
-            githubUtils.generateStagingDeployCashBody(tag, [...basePRList, ...internalQAPRList], [internalQAPRList[0]])
+            githubUtils.generateStagingDeployCashBody(tag, [...basePRList, ...internalQAPRList], [], [], [], [], [internalQAPRList[0]])


We need to pass this new argument containing the resolved/verified InternalQA PR to the function in the test to make sure that the checkbox state has been preserved.

The reason this test has been passing before (although this feature clearly does not work) is that we have not been simulating the actual flow as it happens in reality. We have passed in the checked-off internalQA PR as a third argument, which then made it to the verifiedOrNoQAPRs array which then worked, however, normally, the internalQA PR would never make it to the third argument so it always resetted the checkboxes 😢

Okay, makes sense 👍

mountiny · 2022-07-17T01:34:24Z

This should be ready for a review, thank you very much for having a look. I will kindly ask @roryabraham and @yuwenmemon for reviews as you have worked on this before.

roryabraham

This looks good. Mind if we test on the next checklist? Don't want to introduce potential breaking changes or distractions when we seem close to deploying the current (massive) checklist to production.

roryabraham · 2022-07-18T19:11:13Z

tests/unit/GithubUtilsTest.js

@@ -533,7 +534,7 @@ describe('GithubUtils', () => {
        ));

        test('Test some verified internalQA PRs', () => (
-            githubUtils.generateStagingDeployCashBody(tag, [...basePRList, ...internalQAPRList], [internalQAPRList[0]])
+            githubUtils.generateStagingDeployCashBody(tag, [...basePRList, ...internalQAPRList], [], [], [], [], [internalQAPRList[0]])


Okay, makes sense 👍

.github/actions/javascript/checkDeployBlockers/index.js

neil-marcellini

Due to my current limited knowledge about GH actions, I asked a few questions and I'll leave it to others to approve.

.github/actions/javascript/awaitStagingDeploys/index.js

neil-marcellini · 2022-07-18T20:51:16Z

.github/actions/javascript/createOrUpdateStagingDeploy/index.js

                    issueBody += '\r\n\r\n\r\n**Internal QA:**';
                    _.each(internalQAPRMap, (assignees, URL) => {
                        const assigneeMentions = _.reduce(assignees, (memo, assignee) => `${memo} @${assignee}`, '');
-                        issueBody += `\r\n${_.contains(verifiedOrNoQAPRs, URL) ? '- [x]' : '- [ ]'} `;
+                        issueBody += `\r\n${_.contains(resolvedInternalQAPRs, URL) ? '- [x]' : '- [ ]'} `;


Why is there so much code duplication for creating the internal QA PR list and getStagingDeployCashData in these Github actions? I'm very new to Github actions.

Check out the documentation here

mountiny · 2022-07-19T12:45:06Z

.github/actions/javascript/awaitStagingDeploys/index.js

+                // The format of this map is following:
+                // {
+                //    'https://github.com/Expensify/App/pull/9641': [ 'PauloGasparSv', 'kidroca' ],
+                //    'https://github.com/Expensify/App/pull/9642': [ 'mountiny', 'kidroca' ]
+                // }


Added this comment to explain what the format of the map will be as it is not immediately clear imho, which then probably lead to the bug.

.github/actions/javascript/awaitStagingDeploys/index.js

mountiny · 2022-07-19T12:46:52Z

@roryabraham @neil-marcellini Updated this PR with a fix to the additional bug, which I thought for some reason was gone haha. Left comments explaining the changes and why it was failing.

mountiny · 2022-07-19T13:50:00Z

.github/libs/GithubUtils.js

-        const internalQAPRList = this.getStagingDeployCashInternalQA(issue);
-        return _.sortBy(_.union(PRList, internalQAPRList), 'number');
+        return _.sortBy(PRList, 'number');


Actually, this is what was causing the internalQA PRs to be added to the normal PR list as well as the internal QA section. It is not necessary to be added here since the PRs are already fetched and then filtered somewhere else. This should fix that bug.

mountiny · 2022-07-19T14:05:45Z

And yes, we can test it, but it on this checklist!

roryabraham

This LGTM, but also I feel like maybe this code has hit a critical mass of complexity where we need to do some refactoring. If you want we can do the refactoring separately. I'll leave that up to you.

mountiny · 2022-07-19T17:25:19Z

Going to merge and test this using a Test deploy blocker!

2 bugs to test, they have the same testing steps:

Merge this PR and wait a bit to make sure the workflows from this will be used
Make sure the current checklist does not contain the internalQA amongst the normal PRs (which is a big now)
If it does, manually remove it
Check off the internalQA PR which is currently listed in the internal QA section if it is not already (remember to put it back to the original state once testing it done)
Create a test issue and assign a DeployBlocker label to it
This will trigger the workflow to update the checklist with the new deploy blocker

InternalQA is present in the list of other PRs too

We already have one internalQA PR in the current locked checklist
Currently, when the checklist is updated, the internalQA PR will be added to the list of normal PRs which is a bug
Make sure that the internalQA PR is not added to the list of normal PRs

Preserving the checked state of the internalQA PR

The internalQA Pr should be checked off as verified before a new deploy blocker is created
Once the checklist is updated with the new deploy blocker, make sure the PR is still marked as verified

That is it 🎉 happy testing!

mountiny · 2022-07-19T17:30:48Z

Given we will QA it now, internalQA is not correct for this PR, removing and will report here how the testing went!

melvin-bot · 2022-07-19T17:37:04Z

@mountiny looks like this was merged without passing tests. Please add a note explaining why this was done and remove the Emergency label if this is not an emergency.

mountiny · 2022-07-19T17:42:49Z

Not an emergency. Tests were passing.

OSBotify · 2022-07-19T17:43:51Z

✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release.

mountiny · 2022-07-19T18:05:35Z

First part worked: InternalQA is present in the list of other PRs too

Second part did not 😡 Preserving the checked state of the internalQA PR

Looking into the logs

mountiny · 2022-07-19T18:38:35Z

Oh, found the problem, from the logs:

Filtering out the following automated pull requests: [ 'https://github.com/Expensify/App/pull/9997' ]
Found the following Internal QA PRs: {
  'https://github.com/Expensify/App/pull/9641': [ 'PauloGasparSv', 'kidroca' ]
}
Found the following NO QA PRs: [
  'https://github.com/Expensify/App/pull/9913',
  'https://github.com/Expensify/App/pull/9890'
]
Found the following verified Internal QA PRs: [
  'https://github.com/Expensify/App/pull/9641 - @PauloGasparSv @kidroca'
]
Successfully updated StagingDeployCash! 🎉 https://github.com/Expensify/App/issues/9970

You can see the Found the following verified Internal QA PRs are not just URLs, which is wrong. I am putting up a PR to fix it!

mountiny · 2022-07-19T19:28:44Z

The follow up PR fixed it! #10007 🎉

OSBotify · 2022-07-25T18:04:33Z

🚀 Deployed to staging by @mountiny in version: 1.1.86-0 🚀

platform	result
🤖 android 🤖	success ✅
🖥 desktop 🖥	success ✅
🍎 iOS 🍎	success ✅
🕸 web 🕸	success ✅

OSBotify · 2022-08-01T22:12:21Z

🚀 Deployed to production by @yuwenmemon in version: 1.1.86-5 🚀

platform	result
🤖 android 🤖	success ✅
🖥 desktop 🖥	success ✅
🍎 iOS 🍎	success ✅
🕸 web 🕸	success ✅

mountiny added 2 commits July 16, 2022 18:00

Pass the resolved internal QA PRs to the method to create new version…

b82ca7c

… of the deployment issue

Add missing comment for consistency

0c2f7df

mountiny added the InternalQA This pull request required internal QA label Jul 16, 2022

mountiny requested a review from a team as a code owner July 16, 2022 17:09

mountiny self-assigned this Jul 16, 2022

melvin-bot bot requested review from neil-marcellini and removed request for a team July 16, 2022 17:09

mountiny added 3 commits July 16, 2022 18:14

Build the actions

2b4dded

Build the actions after fixing linter

27031e1

Update the test

e8accc9

mountiny commented Jul 17, 2022

View reviewed changes

mountiny added 2 commits July 17, 2022 02:30

Add once console log for debugging

8b8f856

Update the console comment

0997c5f

mountiny changed the title ~~[WIP] Fix the internal QA PR section losing its state~~ Fix the internal QA PR section losing its state Jul 17, 2022

mountiny requested review from yuwenmemon and roryabraham July 17, 2022 01:34

roryabraham previously approved these changes Jul 18, 2022

View reviewed changes

roryabraham reviewed Jul 18, 2022

View reviewed changes

.github/actions/javascript/checkDeployBlockers/index.js Show resolved Hide resolved

neil-marcellini reviewed Jul 18, 2022

View reviewed changes

This comment was marked as outdated.

Sign in to view

mountiny added 3 commits July 19, 2022 13:37

Fix the PR numbers difference

5077a27

Add a comment explaining a shape of the map

835ec16

Build the action

6c8ab93

mountiny dismissed roryabraham’s stale review via 6c8ab93 July 19, 2022 12:40

mountiny requested a review from roryabraham July 19, 2022 12:41

Make the comment more readable

c25773a

mountiny commented Jul 19, 2022

View reviewed changes

.github/actions/javascript/awaitStagingDeploys/index.js Outdated Show resolved Hide resolved

mountiny changed the title ~~Fix the internal QA PR section losing its state~~ [WIP] Fix the internal QA PR section losing its state Jul 19, 2022

Fix the bug with internalQA PRs appearing twice

beeefb2

mountiny changed the title ~~[WIP] Fix the internal QA PR section losing its state~~ Fix the internal QA PR section losing its state Jul 19, 2022

mountiny commented Jul 19, 2022

View reviewed changes

roryabraham approved these changes Jul 19, 2022

View reviewed changes

mountiny mentioned this pull request Jul 19, 2022

Refactor and clean up the createStagingDeployIssue code #9999

Closed

mountiny removed the InternalQA This pull request required internal QA label Jul 19, 2022

mountiny changed the title ~~Fix the internal QA PR section losing its state~~ [NoQA] Fix the internal QA PR section losing its state Jul 19, 2022

mountiny merged commit 728a0f4 into main Jul 19, 2022

mountiny deleted the vit-fixInternalQASection branch July 19, 2022 17:36

melvin-bot bot added the Emergency label Jul 19, 2022

mountiny removed the Emergency label Jul 19, 2022

mountiny mentioned this pull request Jul 19, 2022

[TEST] This is a test deploy blocker #10003

Closed

mountiny mentioned this pull request Jul 19, 2022

[NoQA] Parse URLs correctly from the internalQA section of the staging deploy issue #10007

Merged

93 tasks

OSBotify mentioned this pull request Jul 25, 2022

Deploy Checklist: New Expensify 2022-07-25 #10077

Closed

92 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[NoQA] Fix the internal QA PR section losing its state #9946

[NoQA] Fix the internal QA PR section losing its state #9946

mountiny commented Jul 16, 2022 •

edited

Loading

mountiny Jul 17, 2022

mountiny Jul 17, 2022

mountiny Jul 17, 2022

mountiny Jul 17, 2022

mountiny Jul 17, 2022 •

edited

Loading

roryabraham Jul 18, 2022

mountiny commented Jul 17, 2022

roryabraham left a comment

roryabraham Jul 18, 2022

neil-marcellini left a comment

neil-marcellini Jul 18, 2022

roryabraham Jul 18, 2022

This comment was marked as outdated.

This comment was marked as outdated.

mountiny Jul 19, 2022 •

edited

Loading

mountiny commented Jul 19, 2022

mountiny Jul 19, 2022

mountiny commented Jul 19, 2022

roryabraham left a comment

mountiny commented Jul 19, 2022

mountiny commented Jul 19, 2022

melvin-bot bot commented Jul 19, 2022

mountiny commented Jul 19, 2022

OSBotify commented Jul 19, 2022

mountiny commented Jul 19, 2022

mountiny commented Jul 19, 2022

mountiny commented Jul 19, 2022

OSBotify commented Jul 25, 2022

OSBotify commented Aug 1, 2022

		issueBody += `\r\n${_.contains(verifiedOrNoQAPRs, URL) ? '- [x]' : '- [ ]'} `;
		issueBody += `\r\n${_.contains(resolvedInternalQAPRs, URL) ? '- [x]' : '- [ ]'} `;

[NoQA] Fix the internal QA PR section losing its state #9946

[NoQA] Fix the internal QA PR section losing its state #9946

Conversation

mountiny commented Jul 16, 2022 • edited Loading

Details

Fixed Issues

Tests

PR Review Checklist

Contributor (PR Author) Checklist

PR Reviewer Checklist

QA Steps

Screenshots

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mountiny Jul 17, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mountiny commented Jul 17, 2022

roryabraham left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

neil-marcellini left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment was marked as outdated.

This comment was marked as outdated.

mountiny Jul 19, 2022 • edited Loading

Choose a reason for hiding this comment

mountiny commented Jul 19, 2022

Choose a reason for hiding this comment

mountiny commented Jul 19, 2022

roryabraham left a comment

Choose a reason for hiding this comment

mountiny commented Jul 19, 2022

InternalQA is present in the list of other PRs too

Preserving the checked state of the internalQA PR

mountiny commented Jul 19, 2022

melvin-bot bot commented Jul 19, 2022

mountiny commented Jul 19, 2022

OSBotify commented Jul 19, 2022

mountiny commented Jul 19, 2022

mountiny commented Jul 19, 2022

mountiny commented Jul 19, 2022

OSBotify commented Jul 25, 2022

OSBotify commented Aug 1, 2022

mountiny commented Jul 16, 2022 •

edited

Loading

mountiny Jul 17, 2022 •

edited

Loading

mountiny Jul 19, 2022 •

edited

Loading