Add `show-open-prs-of-forks` feature #2880

loilo · 2020-03-11T09:27:49Z

This PR adds the new show-open-prs-of-forks feature. It triggers in all your repositories which are forks. Because of the user-centric nature of this feature, I unfortunately can not provide test links.

Repo Headline Hints

This feature adds a hint in the secondary repo headline, linking to your open PRs in the original repo:

Single Pull Request

If there is just one open pull request, the link will directly point there (for this screenshot: mautic/mautic#8106).

Multiple Pull Requests

If there are multiple open pull requests, the link will point to a list of them (for this screenshot: loilo-archive/pull-request-target/pulls?q=is:pr+is:open+sort:updated-desc+author:loilo).

Repo Deletion Warnings

The feature also adds a paragraph to the repo deletion dialog, raising your awareness that there are unmerged outgoing PRs from this repo which you will abandon. (I ran a quick test for this — PRs are not actually deleted when the fork is gone, they're just attributed to an "unknown repo", so the warning will be about the loss of write access to the PR.)

Rules for link targets are the same as in the headline hints layed out above.

Closes #2268

IssueHunt Summary

Referenced issues

This pull request has been submitted to:

#2268: Find open PRs on your forks

IssueHunt has been backed by the following sponsors. Become a sponsor

source/features/show-open-prs-of-forks.tsx

loilo · 2020-03-13T21:52:22Z

I think I've addressed all points from the previous review, except one thing: I could not move the isRepoWithAccess() check outside the function because it always returned false in that case (probably because it relies on a DOM check).

Also, besides the suggested simplification of the findForkedRepo function, I moved it to utils.ts (since it is also used in the forked-to feature) and renamed it to getForkedRepo (to be more in line with naming of other utility functions).

Also, my question about GHE still needs to be sorted out.

source/features/show-open-prs-of-forks.tsx

fregante · 2020-03-13T23:43:11Z

I could not move the isRepoWithAccess() check outside the function

The reason is that you're using nowAndOnAjaxedPages; that only works if you use elementReady somewhere, otherwise it's run too soon.

Since isRepoWithAccess basically just checks for the Settings tab, you could implement that manually, still outside the function, as if(await elementReady('settings tab selector'))

Co-Authored-By: Federico Brigante <github@bfred.it>

loilo · 2020-03-14T00:08:26Z

Sorry, I'm a little bit stumped on your last example. To do that ouside the function, we'd need top-level await (which we don't have, do we?)

Or are you suggesting to put it into a Promise?

const hasSettings = elementReady('settings tab selector');

fregante · 2020-03-14T00:18:09Z

It should go inside the init functions

loilo · 2020-03-14T00:24:49Z

Okay, so that's basically how I had it in the first place. I ultimately outsourced all those checks into the getOpenPullRequestsData function purely as a matter of code deduplication (which is also why the function originally did not have any parameters). I can see how the function currently has too many unrelated responsibilities though.

Going to continue working on this tomorrow, it's getting late here and my coding as well as my writing is getting noticeably worse. 😁

Co-Authored-By: Federico Brigante <github@bfred.it>

loilo · 2020-03-14T16:43:22Z

Okay, I've renamed getOpenPullRequestsData to getRawOpenPullRequestsData which returns Promise<[number, number] | false> to reduce the cache footprint. Init functions still call getOpenPullRequestsData, which is responsible for getting the raw (cached) data and assembles the object structure, including the URL.

- Adds proper support for `nowAndOnAjaxedPages` - Reduces duplication

fregante · 2020-03-16T16:26:18Z

I've renamed getOpenPullRequestsData to getRawOpenPullRequestsData

Good idea! I reorganized the code so "getOpenPullRequestsData" returns the full required data, which is count and url, so they can be used directly without further if/else

I'm resolving the conflicts

fregante

🤩

fregante · 2020-03-25T23:51:41Z

source/libs/utils.ts

 	return {ownerName, repoName};
 };

+export function getForkedRepo(): string | undefined {


This is ambiguous. Is it the repo that was forked or the fork itself?

The correct term would be "source repo". Perhaps getForkSourceRepo would be the correct term.

Can you change this name everywhere? Including const forkedRepo = ... (but not isForkedRepo, that's more clear, I think)

I thought so as well, but I trusted the existing naming (the function existed before, I just moved it to utils).

Yes, I'll take care of it when I'm going to handle #2934.

Hm, there's already a function with that name in the forked-to feature.

const getForkSourceRepo = (): string => getForkedRepo() ?? getRepoURL();

How about

getParentRepo: only works if there’s a parent)

getSourceRepo: always gets the source repo, whether it’s the parent or the current

Sound's good to me. I'll do some refactoring of these, but I think I'll open a separate PR for that.

fregante · 2020-03-25T23:52:38Z

source/libs/utils.ts

 };

+export function getForkedRepo(): string | undefined {
+	return select<HTMLAnchorElement>('.fork-flag a')?.pathname.slice(1);


Also you could use the same meta tags I used in the isForkedRepo since they're available earlier on the page:

<meta name="octolytics-dimension-repository_parent_nwo" content="sindresorhus/refined-github" />

Suggested change

return select<HTMLAnchorElement>('.fork-flag a')?.pathname.slice(1);

return select<HTMLMetaElement>('[name="octolytics-dimension-repository_parent_nwo"]')?.content;

Add show-open-prs-of-forks feature

6a45212

fregante added the enhancement label Mar 11, 2020

fregante requested changes Mar 11, 2020

View reviewed changes

jerone reviewed Mar 11, 2020

View reviewed changes

source/features/show-open-prs-of-forks.tsx Outdated Show resolved Hide resolved

loilo commented Mar 13, 2020

View reviewed changes

source/features/show-open-prs-of-forks.tsx Outdated Show resolved Hide resolved

Address review suggestions

2d52712

loilo requested a review from fregante March 13, 2020 21:52

fregante requested changes Mar 13, 2020

View reviewed changes

Use origin-agnostic PRs URL

9b795fe

Co-Authored-By: Federico Brigante <github@bfred.it>

Try unescaped smart apostrophe

47c8f9c

loilo and others added 4 commits March 14, 2020 08:55

Use more compact cache key for open PR data

2095322

Co-Authored-By: Federico Brigante <github@bfred.it>

Move environment checks out of the getOpenPullRequestsData function

738a420

Minimize cache footprint

ee4133e

Fix link to user's pull requests

699c012

Reorganize

306fbb8

- Adds proper support for `nowAndOnAjaxedPages` - Reduces duplication

fregante and others added 2 commits March 16, 2020 17:30

Merge branch 'master' into open-pr-on-fork

5897e7b

Use pluralize to make the link more readable

275f43f

fregante approved these changes Mar 16, 2020

View reviewed changes

fregante added the needs readme update label Mar 16, 2020

fregante added 2 commits March 17, 2020 00:21

Merge branch 'master' into open-pr-on-fork

102e014

Update readme.md

9c0386e

fregante removed the needs readme update label Mar 16, 2020

fregante merged commit 47bae5f into refined-github:master Mar 16, 2020

fregante reviewed Mar 25, 2020

View reviewed changes

fregante mentioned this pull request Jun 3, 2020

Refined GitHub updates #1137

Closed

yakov116 mentioned this pull request Jun 15, 2020

Add fork-source-link-same-view feature #3230

Merged

	return select<HTMLAnchorElement>('.fork-flag a')?.pathname.slice(1);
	return select<HTMLMetaElement>('[name="octolytics-dimension-repository_parent_nwo"]')?.content;

Uh oh!

Add show-open-prs-of-forks feature #2880

Add show-open-prs-of-forks feature #2880

Uh oh!

Conversation

loilo commented Mar 11, 2020 • edited by issuehunt-oss bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Repo Headline Hints

Single Pull Request

Multiple Pull Requests

Repo Deletion Warnings

Referenced issues

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

loilo commented Mar 13, 2020

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

fregante commented Mar 13, 2020

Uh oh!

loilo commented Mar 14, 2020

Uh oh!

fregante commented Mar 14, 2020

Uh oh!

loilo commented Mar 14, 2020

Uh oh!

loilo commented Mar 14, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

fregante commented Mar 16, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

fregante left a comment

Choose a reason for hiding this comment

Uh oh!

fregante Mar 25, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

loilo Mar 26, 2020

Choose a reason for hiding this comment

Uh oh!

loilo Mar 26, 2020

Choose a reason for hiding this comment

Uh oh!

fregante Mar 26, 2020

Choose a reason for hiding this comment

Uh oh!

loilo Mar 26, 2020

Choose a reason for hiding this comment

Uh oh!

fregante Mar 25, 2020

Choose a reason for hiding this comment

Uh oh!

loilo Mar 26, 2020

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

4 participants

Add `show-open-prs-of-forks` feature #2880

Add `show-open-prs-of-forks` feature #2880

loilo commented Mar 11, 2020 •

edited by issuehunt-oss bot

Loading

loilo commented Mar 14, 2020 •

edited

Loading

fregante commented Mar 16, 2020 •

edited

Loading

fregante Mar 25, 2020 •

edited

Loading