-
Notifications
You must be signed in to change notification settings - Fork 2.8k
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
[NoQA] Fix the internal QA PR section losing its state #9946
Changes from all commits
b82ca7c
0c2f7df
2b4dded
27031e1
e8accc9
8b8f856
0997c5f
5077a27
835ec16
6c8ab93
c25773a
beeefb2
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -122,13 +122,20 @@ const run = function () { | |
'number', | ||
); | ||
|
||
// Get the internalQA PR list, preserving the previous state of `isResolved` | ||
const internalQAPRList = _.sortBy( | ||
currentStagingDeployCashData.internalQAPRList, | ||
'number', | ||
); | ||
Comment on lines
+125
to
+129
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We are first getting the list of the InternalQA PRs from the previous issue body. |
||
|
||
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, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. And then passing as a parameter an array of URLs of only those PRs which are resolved (the checkbox has been ticked) |
||
); | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -132,13 +132,20 @@ const run = function () { | |
'number', | ||
); | ||
|
||
// Get the internalQA PR list, preserving the previous state of `isResolved` | ||
const internalQAPRList = _.sortBy( | ||
currentStagingDeployCashData.internalQAPRList, | ||
'number', | ||
); | ||
|
||
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, | ||
); | ||
|
@@ -418,6 +425,7 @@ class GithubUtils { | |
labels: issue.labels, | ||
PRList: this.getStagingDeployCashPRList(issue), | ||
deployBlockers: this.getStagingDeployCashDeployBlockers(issue), | ||
internalQAPRList: this.getStagingDeployCashInternalQA(issue), | ||
isTimingDashboardChecked: /-\s\[x]\sI checked the \[App Timing Dashboard]/.test(issue.body), | ||
isFirebaseChecked: /-\s\[x]\sI checked \[Firebase Crashlytics]/.test(issue.body), | ||
tag, | ||
|
@@ -452,8 +460,7 @@ class GithubUtils { | |
isAccessible: match[4] === 'x', | ||
}), | ||
); | ||
const internalQAPRList = this.getStagingDeployCashInternalQA(issue); | ||
return _.sortBy(_.union(PRList, internalQAPRList), 'number'); | ||
return _.sortBy(PRList, 'number'); | ||
} | ||
|
||
/** | ||
|
@@ -516,6 +523,7 @@ class GithubUtils { | |
* @param {Array} [accessiblePRList] - The list of PR URLs which have passed the accessability check. | ||
* @param {Array} [deployBlockers] - The list of DeployBlocker URLs. | ||
* @param {Array} [resolvedDeployBlockers] - The list of DeployBlockers URLs which have been resolved. | ||
* @param {Array} [resolvedInternalQAPRs] - The list of Internal QA PR URLs which have been resolved. | ||
* @param {Boolean} [isTimingDashboardChecked] | ||
* @param {Boolean} [isFirebaseChecked] | ||
* @returns {Promise} | ||
|
@@ -527,6 +535,7 @@ class GithubUtils { | |
accessiblePRList = [], | ||
deployBlockers = [], | ||
resolvedDeployBlockers = [], | ||
resolvedInternalQAPRs = [], | ||
isTimingDashboardChecked = false, | ||
isFirebaseChecked = false, | ||
) { | ||
|
@@ -538,6 +547,11 @@ class GithubUtils { | |
); | ||
console.log('Filtering out the following automated pull requests:', automatedPRs); | ||
|
||
// 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' ] | ||
// } | ||
const internalQAPRMap = _.reduce( | ||
_.filter(data, pr => !_.isEmpty(_.findWhere(pr.labels, {name: INTERNAL_QA_LABEL}))), | ||
(map, pr) => { | ||
|
@@ -582,11 +596,13 @@ class GithubUtils { | |
}); | ||
} | ||
|
||
// Internal QA PR list | ||
if (!_.isEmpty(internalQAPRMap)) { | ||
console.log('Found the following verified Internal QA PRs:', resolvedInternalQAPRs); | ||
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]' : '- [ ]'} `; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why is there so much code duplication for creating the internal QA PR list and There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Check out the documentation here |
||
issueBody += `${URL}`; | ||
issueBody += ` -${assigneeMentions}`; | ||
}); | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.