fix: tagging author after adding changelog label #243

nlok5923 · 2021-04-01T04:15:08Z

Fixes: #240

Explanation

Fixes tagging author after adding changelog label.

Checklist

I have successfully deployed my own instance of Oppiabot.
- You can find instructions for doing this here.
I have manually tested all the changes made in this PR following the manual tests matrix.

jameesjohn

Thanks, @nlok5923.
I'm not sure how this fixes the issue. Can you explain?

jameesjohn · 2021-04-04T11:30:49Z

lib/checkPullRequestLabels.js

@@ -193,12 +193,13 @@ module.exports.checkChangelogLabel = async function (context) {
  );

  const userName = pullRequest.user.login;
-  const changelogLabel = getChangelogLabelFromPullRequest(pullRequest);
+  const changelogLabel = await getChangelogLabelFromPullRequest(pullRequest);


Why is await added here?

Sir previously without await getChangelogLabelFromPullRequest(pullRequest); returns undefined to changeloglabel because of which Boolean(changelogLabel); return false to hasChangelogLabel so to maintain the synchronous execution i used await.

But getChangelogLabelFromPullRequest does not return a promise. So await doesn't do anything there.

jameesjohn · 2021-04-04T11:31:25Z

lib/checkPullRequestLabels.js

  // If the PR has a changelog label, no action is required.
  if (hasChangelogLabel) {
-    const changelogLabelIsValid = matchChangelogLabelWithRegex(changelogLabel);
+    const changelogLabelIsValid = await matchChangelogLabelWithRegex(


Why is await added here?

Just for testing purpose i added await here removing it .

oppiabot · 2021-04-09T19:27:28Z

Hi @nlok5923. Due to recent changes in the "develop" branch, this PR now has a merge conflict. Please follow this link if you need help resolving the conflict, so that the PR can be merged. Thanks!

up

nlok5923 · 2021-04-11T07:03:03Z

manual testing video: just for testing purpose i am listening to pull request reopen event

tagging.mp4

Revert "up"

oppiabot · 2021-04-11T07:12:07Z

Hi @nlok5923. Due to recent changes in the "develop" branch, this PR now has a merge conflict. Please follow this link if you need help resolving the conflict, so that the PR can be merged. Thanks!

Revert "Revert "up""

nlok5923 · 2021-04-17T16:59:35Z

@jameesjohn @vojtechjelinek the functionality is working as expected.

…agging needed

nlok5923 · 2021-04-19T09:36:53Z

@jameesjohn sir PTAL i had reverted the unwanted changes sir the getChangelogLabelFromPullRequest in utils do not had any return when pr label do not has PR CHANGELOG prefix that's why it's halting the getChangelogLabelFromPullRequest while testing for currently this pr introduces return statement to getChangelogLabelFromPullRequest in utils when pr do not have label with prefix PR CHANGELOG.

vojtechjelinek

Thanks! Left a few comments.

vojtechjelinek · 2021-04-20T13:09:55Z

lib/checkPullRequestLabels.js

  const userName = pullRequest.user.login;
  const changelogLabel = getChangelogLabelFromPullRequest(pullRequest);
  const hasChangelogLabel = Boolean(changelogLabel);
-


unneeded change

vojtechjelinek · 2021-04-20T13:09:59Z

lib/checkPullRequestLabels.js

@@ -191,14 +191,14 @@ module.exports.checkChangelogLabel = async function (context) {
    'RUNNING CHANGELOG LABEL CHECK ON PULL REQUEST ' +
    pullRequestNumber
  );
-


unneeded change

vojtechjelinek · 2021-04-20T13:10:46Z

lib/utils.js

@@ -270,6 +270,8 @@ const getChangelogLabelFromPullRequest = (pullRequest) => {
  );
  if (label) {
    return label.name.trim();
+  } else {
+    return false;


false doesn't make sense here, maybe null

addressed in latest commit.

vojtechjelinek

LGTM!

vojtechjelinek · 2021-04-21T14:48:02Z

@nlok5923 Frontend tests need to be fixed.

nlok5923 · 2021-04-30T02:03:51Z

@vojtechjelinek i had fixed the frontend test.

jameesjohn

I'm not sure I understand how this fixes the issue.
Can you show a video of the working before this change, and another after this change?

nlok5923 · 2021-05-04T12:58:22Z

sure @jameesjohn

nlok5923 · 2021-05-08T13:05:27Z

I'm not sure I understand how this fixes the issue.
Can you show a video of the working before this change, and another after this change?

yeah @jameesjohn this solution is partial actual solution need to listen to pull_request.labelled event to keep a check on whether change lock label added or not.

I had a doubt that when pull request opened it triggers the pull request open event which follow up with changelog label check but what we need to do is keep data updated so will it work listening to pull request label event just after listening to pull request opened event updating check variable and then oppiabot will comment accordingly.

nlok5923 · 2021-05-13T10:22:35Z

This issue is quite rare actually so it's really hard to reproduce it. this issue may only happens if contributor open a pull request at the same time oppiabot should be down to introduce some delay in response and in between response contributor may trigger other event.

but as of now oppiabot is working fine with no downfalls so if contributor opens up a pr he will immediately get a response and he will be having no time to trigger any other event in between so this issue won't occur.

i think we should close this issue what your views @jameesjohn @vojtechjelinek .

jameesjohn · 2021-05-16T09:20:53Z

I think that's fine. What do you think @vojtechjelinek?

vojtechjelinek · 2021-05-18T01:18:39Z

I'm fine with closing this.

nlok5923 · 2021-05-18T14:33:25Z

closing this pr as the issue it's addressed is very rare.

nlok5923 added 4 commits April 1, 2021 09:41

fix tagging author after adding label

f5b6705

fix

00ae681

up

848114c

up

f310a06

jameesjohn self-requested a review April 4, 2021 11:17

jameesjohn suggested changes Apr 4, 2021

View reviewed changes

jameesjohn assigned nlok5923 Apr 4, 2021

reemove redundant changes

2b03f65

nlok5923 requested a review from jameesjohn April 4, 2021 11:54

oppiabot bot added the PR: don't merge - HAS MERGE CONFLICTS label Apr 9, 2021

nlok5923 added 2 commits April 10, 2021 11:04

Merge branch 'tagging' into master

f974eb2

Merge pull request #5 from nlok5923/master

9927ee2

up

oppiabot bot removed the PR: don't merge - HAS MERGE CONFLICTS label Apr 10, 2021

nlok5923 added 2 commits April 11, 2021 12:41

Revert "up"

fbb8bac

Merge pull request #8 from nlok5923/revert-5-master

2d6661b

Revert "up"

oppiabot bot added the PR: don't merge - HAS MERGE CONFLICTS label Apr 11, 2021

nlok5923 added 2 commits April 11, 2021 12:43

Revert "Revert "up""

0949197

Merge pull request #9 from nlok5923/revert-8-revert-5-master

dd42b53

Revert "Revert "up""

oppiabot bot removed the PR: don't merge - HAS MERGE CONFLICTS label Apr 11, 2021

nlok5923 added 2 commits April 19, 2021 15:02

functionality working fine now

3298625

Merge branch 'tagging' of https://github.com/nlok5923/oppiabot into t…

de5ad88

…agging needed

vojtechjelinek requested changes Apr 20, 2021

View reviewed changes

nlok5923 added 3 commits April 20, 2021 19:25

removed unwanted changes

ca40d7e

removed unwanted changes

937ea29

removed unneeded tabs spacing

fbacd76

removed unneeded tabs spacing

c6d9c55

nlok5923 requested a review from vojtechjelinek April 21, 2021 10:24

vojtechjelinek approved these changes Apr 21, 2021

View reviewed changes

fix test

becbb12

nlok5923 assigned jameesjohn and unassigned nlok5923 Apr 30, 2021

jameesjohn reviewed May 3, 2021

View reviewed changes

jameesjohn assigned nlok5923 and unassigned jameesjohn May 3, 2021

nlok5923 requested a review from jameesjohn May 9, 2021 16:52

nlok5923 assigned jameesjohn and vojtechjelinek and unassigned nlok5923 May 13, 2021

nlok5923 closed this May 18, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: tagging author after adding changelog label #243

fix: tagging author after adding changelog label #243

nlok5923 commented Apr 1, 2021

jameesjohn left a comment

jameesjohn Apr 4, 2021

nlok5923 Apr 4, 2021

jameesjohn Apr 16, 2021

jameesjohn Apr 4, 2021

nlok5923 Apr 4, 2021

oppiabot bot commented Apr 9, 2021

nlok5923 commented Apr 11, 2021 •

edited

Loading

oppiabot bot commented Apr 11, 2021

nlok5923 commented Apr 17, 2021

nlok5923 commented Apr 19, 2021

vojtechjelinek left a comment

vojtechjelinek Apr 20, 2021

nlok5923 Apr 21, 2021

vojtechjelinek Apr 20, 2021

nlok5923 Apr 21, 2021

vojtechjelinek Apr 20, 2021

nlok5923 Apr 21, 2021

vojtechjelinek left a comment

vojtechjelinek commented Apr 21, 2021

nlok5923 commented Apr 30, 2021

jameesjohn left a comment

nlok5923 commented May 4, 2021

nlok5923 commented May 8, 2021

nlok5923 commented May 13, 2021

jameesjohn commented May 16, 2021

vojtechjelinek commented May 18, 2021

nlok5923 commented May 18, 2021

fix: tagging author after adding changelog label #243

fix: tagging author after adding changelog label #243

Conversation

nlok5923 commented Apr 1, 2021

Explanation

Checklist

jameesjohn left a comment

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oppiabot bot commented Apr 9, 2021

nlok5923 commented Apr 11, 2021 • edited Loading

oppiabot bot commented Apr 11, 2021

nlok5923 commented Apr 17, 2021

nlok5923 commented Apr 19, 2021

vojtechjelinek left a comment

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vojtechjelinek left a comment

Choose a reason for hiding this comment

vojtechjelinek commented Apr 21, 2021

nlok5923 commented Apr 30, 2021

jameesjohn left a comment

Choose a reason for hiding this comment

nlok5923 commented May 4, 2021

nlok5923 commented May 8, 2021

nlok5923 commented May 13, 2021

jameesjohn commented May 16, 2021

vojtechjelinek commented May 18, 2021

nlok5923 commented May 18, 2021

nlok5923 commented Apr 11, 2021 •

edited

Loading