Skip to content

Fix cross-deleted-pr-branches on deleted repositories - #4214

Merged
yakov116 merged 8 commits into
mainfrom
delete_repo
Apr 18, 2021
Merged

Fix cross-deleted-pr-branches on deleted repositories#4214
yakov116 merged 8 commits into
mainfrom
delete_repo

Conversation

@yakov116

@yakov116 yakov116 commented Apr 9, 2021

Copy link
Copy Markdown
Member

Test URLs

Screenshot

image

FYI: GitHub used to display 'unknown repository' they no longer do that

@yakov116 yakov116 added the bug label Apr 9, 2021
@yakov116
yakov116 requested a review from fregante April 9, 2021 14:47
Comment thread source/features/cross-deleted-pr-branches.tsx Outdated
@fregante

This comment has been minimized.

Comment thread source/features/cross-deleted-pr-branches.tsx Outdated
@@ -8,29 +8,29 @@ import features from '.';

function init(): void | false {
const lastBranchAction = select.last('.TimelineItem-body .user-select-contain > span:not(.base-ref)');

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Does this work?

Suggested change
const lastBranchAction = select.last('.TimelineItem-body .user-select-contain > span:not(.base-ref)');
const lastBranchAction = select.last('.TimelineItem-body .user-select-contain > .head-ref');

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

No it will fail here #271

Since the deleted branch does not have .head-ref after its deleted

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Indeed, how about this?

Suggested change
const lastBranchAction = select.last('.TimelineItem-body .user-select-contain > span:not(.base-ref)');
const lastBranchAction = select.last('.TimelineItem-body .commit-ref');

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

No that was not good. We only want if they deleted, that gives any action with a commit ref.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

If a branch has been deleted, there are no other commit refs afterwards, I think

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

In jquery/jquery#769 it selects the base branch but then it's filtered out by the next check (!includes(' deleted '))

Great!

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

In jquery/jquery#769 it selects the base branch but then it's filtered out by the next check (!includes(' deleted '))

Just realized that will cause a return false.

If we add :not(.base-ref) it will exclude it.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Just realized that will cause a return false.

And? If the last event is not a deletion, then the branch hasn't been deleted. That PR is locked, so even if it's deleted, we can't detect that.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Everything is working fine, nothing is broken. The only thing is the feature logger.

As we did the following.

	if (!headReferenceLink && !lastBranchAction) {
		return; // Don't return false, This feature’s CSS already takes care of this
	}

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

@fregante if this is a non-issue, lets merge this.

@yakov116 yakov116 changed the title Fix cross-deleted-pr-branches on delete repositories Fix cross-deleted-pr-branches on deleted repositories Apr 11, 2021
@yakov116
yakov116 requested a review from fregante April 12, 2021 14:39
yakov116 and others added 4 commits April 14, 2021 12:47
@yakov116

Copy link
Copy Markdown
Member Author

This feature really looks good after #4152

@fregante 🚀 ?

@yakov116
yakov116 merged commit cf93baa into main Apr 18, 2021
@yakov116
yakov116 deleted the delete_repo branch April 18, 2021 01:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Development

Successfully merging this pull request may close these issues.

2 participants