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

Project Management: Prompt user to link GitHub account to WordPress.org profile #21221

Merged
merged 14 commits into from
Apr 1, 2020

Conversation

aduth
Copy link
Member

@aduth aduth commented Mar 27, 2020

Related:

This pull request seeks to fix and enhance the existing "First Time Contributor" project management automation to prompt the user to link their GitHub account to their WordPress.org profile if necessary for release props credit.

Currently, the prompt is:

Congratulations on your first merged pull request! We'd like to credit you for your contribution in the post announcing the next WordPress release, but we can't find a WordPress.org profile associated with your GitHub account. When you have a moment, visit the following URL and click "link your GitHub account" under "GitHub Username" to link your accounts:

https://profiles.wordpress.org/me/profile/edit/

And if you don't have a WordPress.org account, you can create one on this page:

https://login.wordpress.org/register

Kudos!

(Click to expand for initial prompt proposed) >Congratulations on your first merged pull request! We would like to give you credit for your contribution in the next WordPress release, but we were unable to find a WordPress.org profile associated with your GitHub account. At your convenience, please visit the following URL and click "link your GitHub account" under "GitHub Username" to initiate the process to link your accounts: > >https://profiles.wordpress.org/me/profile/edit/ > >If you do not have a WordPress.org account, you can create one at the following page: > >https://login.wordpress.org/register

Feedback is very welcome for adjustments to the phrasing (I've also applied "Needs Copy Review" label to this pull request). Reviewed at #21221 (comment).

The prompt is presented under the following conditions:

  • This is the first commit merged for the user.
  • A WordPress.org user profile is not known to be associated with their GitHub account.

As noted, this also fixes the "First Time Contributor" task, which was intended to be applying the "First Time Contributor" label to a pull request opened by a new contributor. Unfortunately, due to permission constraints of GitHub Actions, labels cannot be applied by actions if the originating repository is a fork of Gutenberg. Since the vast majority of new contributors will not have branching permissions of this repository, this made the task near-useless. This is resolved by adopting a similar approach of #20058 : The task is run on commit to master, where full permissions are granted. This means that the label is only applied after merge, which is not perfect, but certainly an improvement. Coincidentally, this complements the goal of prompting the user to link their accounts upon merge.

Testing Instructions:

There are unit tests:

npm run test-unit packages/project-management-automation/lib/test/add-first-time-contributor-label.js

But the most accurate test won't be 'til we can have it running live 😅

@aduth aduth added [Type] Enhancement A suggestion for improvement. Needs Copy Review Needs review of user-facing copy (language, phrasing) [Type] Project Management Meta-issues related to project management of Gutenberg [Package] Project management automation /packages/project-management-automation labels Mar 27, 2020
@github-actions
Copy link

github-actions bot commented Mar 27, 2020

Size Change: +7.06 kB (0%)

Total Size: 866 kB

Filename Size Change
build/api-fetch/index.js 3.39 kB +1 B
build/autop/index.js 2.58 kB -3 B (0%)
build/block-directory/index.js 6.02 kB -1 B
build/block-editor/index.js 102 kB +252 B (0%)
build/block-editor/style-rtl.css 11 kB +14 B (0%)
build/block-editor/style.css 11 kB +17 B (0%)
build/block-library/editor-rtl.css 7.21 kB -11 B (0%)
build/block-library/editor.css 7.21 kB -12 B (0%)
build/block-library/index.js 111 kB +502 B (0%)
build/block-library/style-rtl.css 7.5 kB +11 B (0%)
build/block-library/style.css 7.51 kB +9 B (0%)
build/blocks/index.js 57.5 kB +4 B (0%)
build/components/index.js 191 kB +1.61 kB (0%)
build/compose/index.js 6.21 kB +1 B
build/core-data/index.js 10.7 kB +71 B (0%)
build/data-controls/index.js 1.04 kB -1 B
build/data/index.js 8.26 kB +4 B (0%)
build/date/index.js 5.36 kB -2 B (0%)
build/dom/index.js 3.05 kB -1 B
build/edit-post/index.js 92.3 kB +1.33 kB (1%)
build/edit-post/style-rtl.css 8.35 kB -86 B (1%)
build/edit-post/style.css 8.34 kB -85 B (1%)
build/edit-site/index.js 9.04 kB +2.31 kB (25%) 🚨
build/edit-site/style-rtl.css 3.41 kB +508 B (14%) ⚠️
build/edit-site/style.css 3.41 kB +512 B (14%) ⚠️
build/edit-widgets/index.js 4.43 kB +3 B (0%)
build/editor/index.js 42.8 kB +29 B (0%)
build/element/index.js 4.44 kB +1 B
build/format-library/index.js 6.95 kB +1 B
build/hooks/index.js 1.93 kB +1 B
build/html-entities/index.js 621 B -1 B
build/i18n/index.js 3.57 kB +83 B (2%)
build/keyboard-shortcuts/index.js 2.3 kB +3 B (0%)
build/keycodes/index.js 1.7 kB -1 B
build/list-reusable-blocks/index.js 2.99 kB +2 B (0%)
build/media-utils/index.js 4.84 kB +5 B (0%)
build/plugins/index.js 2.54 kB -2 B (0%)
build/primitives/index.js 1.5 kB -2 B (0%)
build/priority-queue/index.js 780 B -1 B
build/redux-routine/index.js 2.83 kB -2 B (0%)
build/rich-text/index.js 14.5 kB +1 B
build/server-side-render/index.js 2.55 kB -2 B (0%)
build/shortcode/index.js 1.7 kB -2 B (0%)
build/token-list/index.js 1.28 kB +1 B
build/url/index.js 4.01 kB +2 B (0%)
build/viewport/index.js 1.6 kB -2 B (0%)
ℹ️ View Unchanged
Filename Size Change
build/a11y/index.js 998 B 0 B
build/annotations/index.js 3.45 kB 0 B
build/blob/index.js 620 B 0 B
build/block-directory/style-rtl.css 760 B 0 B
build/block-directory/style.css 760 B 0 B
build/block-library/theme-rtl.css 669 B 0 B
build/block-library/theme.css 671 B 0 B
build/block-serialization-default-parser/index.js 1.65 kB 0 B
build/block-serialization-spec-parser/index.js 3.1 kB 0 B
build/components/style-rtl.css 15.8 kB 0 B
build/components/style.css 15.7 kB 0 B
build/deprecated/index.js 772 B 0 B
build/dom-ready/index.js 568 B 0 B
build/edit-navigation/index.js 2.4 kB 0 B
build/edit-navigation/style-rtl.css 95 B 0 B
build/edit-navigation/style.css 95 B 0 B
build/edit-widgets/style-rtl.css 2.57 kB 0 B
build/edit-widgets/style.css 2.57 kB 0 B
build/editor/editor-styles-rtl.css 423 B 0 B
build/editor/editor-styles.css 426 B 0 B
build/editor/style-rtl.css 3.38 kB 0 B
build/editor/style.css 3.38 kB 0 B
build/escape-html/index.js 733 B 0 B
build/format-library/style-rtl.css 502 B 0 B
build/format-library/style.css 502 B 0 B
build/is-shallow-equal/index.js 710 B 0 B
build/list-reusable-blocks/style-rtl.css 226 B 0 B
build/list-reusable-blocks/style.css 226 B 0 B
build/notices/index.js 1.57 kB 0 B
build/nux/index.js 3.01 kB 0 B
build/nux/style-rtl.css 616 B 0 B
build/nux/style.css 613 B 0 B
build/warning/index.js 1.14 kB 0 B
build/wordcount/index.js 1.18 kB 0 B

compressed-size-action

@dd32
Copy link
Member

dd32 commented Mar 28, 2020

The API usage looks correct, however, the API will throw a 404 response when the user isn't known.
From what I can tell, got() will probably consider that an error. I think you need to set the throwhttperrors option? But that's just from glancing at the docs.

Please also make sure the request comes with a clear UserAgent that our systems team can differentiate from other random clients to avoid unintentional blocking/rate-limiting.

@michelleweber
Copy link

michelleweber commented Mar 30, 2020

This is great! I'd use a few contractions -- they'll make it sound a little friendlier/less formal -- and tighten up a sentence here and there, but nothing big:

Congratulations on your first merged pull request! We'd like to credit you for your contribution in the post announcing the next WordPress release, but we can't find a WordPress.org profile associated with your GitHub account. When you have a moment, visit the following URL and click "link your GitHub account" under "GitHub Username" to link your accounts:

https://profiles.wordpress.org/me/profile/edit/

And if you don't have a WordPress.org account, you can create one on this page:

https://login.wordpress.org/register

Kudos!

Copy link
Contributor

@desrosj desrosj left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for working on this, @aduth! Can you confirm where this prompt will show up? Reading through the code, it looks like it will be added inline as a comment. Do automated comments notify the person like normal issue/pull request activity?

const author = payload.pull_request.user.login;
if ( payload.ref !== 'refs/heads/master' ) {
debug(
'add-first-time-contributor-label: Commit is not to `master`. Aborting'
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it make sense to display the prompt regardless of branch? If they are opening a PR, it shows they are interested in contributing. Why not catch them the first time regardless of branch?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it make sense to display the prompt regardless of branch? If they are opening a PR, it shows they are interested in contributing. Why not catch them the first time regardless of branch?

Unfortunately, it's not possible, related to this point from the original comment:

due to permission constraints of GitHub Actions, labels cannot be applied by actions if the originating repository is a fork of Gutenberg.

A commit pushed to a branch originating from a fork would fall under these restrictions.

It works for commits to master, since it's within the source repository, committed by someone with permissions to push to master.

@noisysocks
Copy link
Member

Can you confirm where this prompt will show up?

It will show up as a comment by @ghbot in the pull request.

Do automated comments notify the person like normal issue/pull request activity?

Yes, it's as if a real human commented on the pull request.

@aduth
Copy link
Member Author

aduth commented Mar 31, 2020

The API usage looks correct, however, the API will throw a 404 response when the user isn't known.
From what I can tell, got() will probably consider that an error. I think you need to set the throwhttperrors option? But that's just from glancing at the docs.

Thanks. Reflecting on this, I think I may refactor to remove the got dependency altogether, and to lean on the fact that a 404 status code is returned. This is much simpler to do in the base https module, since we don't need to deal with processing the content. The only way the content is used right now is in the debug logging, and I don't think it's strictly necessary.

https://nodejs.org/api/https.html#https_https_get_url_options_callback

Maybe a HEAD request would be most appropriate. It appears to work just as well in my testing.

Please also make sure the request comes with a clear UserAgent that our systems team can differentiate from other random clients to avoid unintentional blocking/rate-limiting.

Will do!

This is great! I'd use a few contractions -- they'll make it sound a little friendlier/less formal -- and tighten up a sentence here and there, but nothing big:

Thanks for the copy review, @michelleweber ! I'll make these adjustments.

Can you confirm where this prompt will show up? Reading through the code, it looks like it will be added inline as a comment. Do automated comments notify the person like normal issue/pull request activity?

What @noisysocks said 😄

I believe it should appear identically as in the bot comment above, #21221 (comment) . Aside from the content itself, that is.

@aduth
Copy link
Member Author

aduth commented Mar 31, 2020

Thanks. Reflecting on this, I think I may refactor to remove the got dependency altogether, and to lean on the fact that a 404 status code is returned. This is much simpler to do in the base https module, since we don't need to deal with processing the content. The only way the content is used right now is in the debug logging, and I don't think it's strictly necessary.

https://nodejs.org/api/https.html#https_https_get_url_options_callback

Maybe a HEAD request would be most appropriate. It appears to work just as well in my testing.

Updated in eb1dff6 (with additional revisions in 83ef4d6, 26ddea3).

Please also make sure the request comes with a clear UserAgent that our systems team can differentiate from other random clients to avoid unintentional blocking/rate-limiting.

Added in 0d338e5.

I went with Gutenberg/project-management-automation. Happy to change if it can be further improved.

This is great! I'd use a few contractions -- they'll make it sound a little friendlier/less formal -- and tighten up a sentence here and there, but nothing big:

Updated the prompt in 6bedde8.

@aduth aduth removed the Needs Copy Review Needs review of user-facing copy (language, phrasing) label Mar 31, 2020
Copy link
Member

@noisysocks noisysocks left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The code looks good to me and Travis CI suggests that your tests and type-checks are passing. There's only one way to see if it actually works 😀🚢

@aduth
Copy link
Member Author

aduth commented Apr 1, 2020

Let's give it a try!

@aduth aduth merged commit 4f144b4 into master Apr 1, 2020
@aduth aduth deleted the add/new-contributor-prompt-account-link branch April 1, 2020 13:56
@github-actions github-actions bot added this to the Gutenberg 7.9 milestone Apr 1, 2020
@aduth
Copy link
Member Author

aduth commented Apr 1, 2020

And... it works! #21240 (comment) 🎉

@noisysocks
Copy link
Member

Nice! I wonder if it's worth including an @ mention of the PR author in the comment text.

@aduth
Copy link
Member Author

aduth commented Apr 1, 2020

Nice! I wonder if it's worth including an @ mention of the PR author in the comment text.

It's a good idea! It might require a small refactoring of the current implementation, since the current text is just a static string, and such a change would need to adjust it to be generated dynamically instead.

@aduth
Copy link
Member Author

aduth commented Apr 3, 2020

Nice! I wonder if it's worth including an @ mention of the PR author in the comment text.

See #21384

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Package] Project management automation /packages/project-management-automation [Type] Enhancement A suggestion for improvement. [Type] Project Management Meta-issues related to project management of Gutenberg
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants