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.

Sign up for GitHub

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

Jump to bottom

Add priority to protected branch #32286

Open
wants to merge 39 commits into
base: main
Choose a base branch
from
Open

Add priority to protected branch #32286

wants to merge 39 commits into from

Conversation

6543
Copy link
Member

@6543 6543 commented Oct 17, 2024 •
edited
Loading

Solves

Currently for rules to re-order them you have to alter the creation date. so you basicly have to delete and recreate them in the right order. This is more than just inconvinient ...

Solution

Add a new col for prioritization

TODOs:

  • alter models
  • backend func to handle priorities
  • API
  • alter WebUI
  • api tests
  • backend tests
  • fix mssql issue ...

Demo WebUI Video

vid-20241021-211051.mp4

Sponsored by Kithara Software GmbH

@6543 6543 added the type/feature Completely new functionality. Can only be merged if feature freeze is not active. label Oct 17, 2024
@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Oct 17, 2024
@pull-request-size pull-request-size bot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Oct 17, 2024
@github-actions github-actions bot added modifies/go Pull requests that update Go code modifies/migrations labels Oct 17, 2024
@github-actions github-actions bot added the modifies/api This PR adds API routes or modifies them label Oct 17, 2024
@6543 6543 mentioned this pull request Oct 21, 2024
Merged
@github-actions github-actions bot added the modifies/templates This PR modifies the template files label Oct 21, 2024
@6543
Copy link
Member Author

6543 commented Oct 21, 2024

vid-20241021-211051.mp4

@6543 6543 marked this pull request as ready for review November 9, 2024 16:41
@6543 6543 changed the title [WIP] Add priority to protected branch Add priority to protected branch Nov 9, 2024
wxiaoguang
wxiaoguang reviewed Nov 10, 2024
View reviewed changes
web_src/js/features/repo-settings-branches.ts
const newOrder = Array.from(protectedBranchesList.children, (item) => {
const id = item.getAttribute('data-id');
return id ? parseInt(id) : NaN;
}).filter((id) => !Number.isNaN(id));
Copy link
Contributor

@wxiaoguang wxiaoguang Nov 10, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Why the filter is needed? If an item doesn't have data-id, isn't it just a bug?

Copy link
Member Author

Choose a reason for hiding this comment

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

It's not needed but I added it just as savety guard as it does not hurt -> defensive programming (e.g. I expect users to mess up there custom templates)

Copy link
Contributor

Choose a reason for hiding this comment

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

No, that's not defensive programming, that's unclear logic.

You can't do use protectedBranchesList.children directly here, but need to query correct items first.

Copy link
Contributor

@wxiaoguang wxiaoguang Nov 12, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

queryElemChildren(protectedBranchesList, '.item[data-id]') (or use .flex-item[data-id])

Then no need the filter or isNaN trick

wxiaoguang
wxiaoguang reviewed Nov 10, 2024
View reviewed changes
modules/structs/repo_branch.go
@@ -125,3 +128,8 @@ type EditBranchProtectionOption struct {
UnprotectedFilePatterns *string `json:"unprotected_file_patterns"`
BlockAdminMergeOverride *bool `json:"block_admin_merge_override"`
}

// UpdateBranchProtectionPriories a list to update the branch protection rule priorities
type UpdateBranchProtectionPriories []struct {

This comment was marked as outdated.

Sign in to view

wxiaoguang
wxiaoguang reviewed Nov 10, 2024
View reviewed changes
routers/api/v1/repo/branch.go
Comment on lines +1129 to +1132
ids := make([]int64, len(*form))
for i, v := range *form {
ids[i] = v.ID
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Use a proper form struct then no need this trick.

Copy link
Member Author

Choose a reason for hiding this comment

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

for the API i want to use a proper struct so we can extend it later on if we want ...

Copy link
Member Author

@6543 6543 Nov 11, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

... as for the web route, If I can figure it out how to just send an []int64{} directly, I will do so - oh already did so :)

wxiaoguang
wxiaoguang reviewed Nov 10, 2024
View reviewed changes
web_src/js/features/repo-settings-branches.ts
createSortable(protectedBranchesList, {
handle: '.drag-handle',
animation: 150,
onEnd: async () => { // eslint-disable-line @typescript-eslint/no-misused-promises
Copy link
Contributor

Choose a reason for hiding this comment

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

What is misused and why ignored?

Copy link
Member Author

Choose a reason for hiding this comment

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

no idea

I just reused that code

gitea/web_src/js/features/repo-issue-list.ts

Line 199 in f2a6df0

onEnd: pinMoveEnd, // eslint-disable-line @typescript-eslint/no-misused-promises

Copy link
Member Author

Choose a reason for hiding this comment

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

PS: if I remove that the linter fails - but the code works ... so no idea whats going on

Copy link
Contributor

@wxiaoguang wxiaoguang Nov 12, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

The definition is in export interface SortableOptions {, onEnd doesn't accept async.

The proper change is:

onEnd: () => {
    (async () => {
         await ....
    })();
}

But I won't block it if you don't want to make further change.

@pull-request-size pull-request-size bot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Nov 11, 2024
@6543 6543 requested a review from wxiaoguang November 11, 2024 17:21
@lunny lunny added this to the 1.24.0 milestone Nov 11, 2024
wxiaoguang
wxiaoguang reviewed Nov 12, 2024
View reviewed changes
modules/structs/repo_branch.go
@@ -125,3 +128,8 @@ type EditBranchProtectionOption struct {
UnprotectedFilePatterns *string `json:"unprotected_file_patterns"`
BlockAdminMergeOverride *bool `json:"block_admin_merge_override"`
}

// UpdateBranchProtectionPriories a list to update the branch protection rule priorities
type UpdateBranchProtectionPriories []struct {
Copy link
Contributor

Choose a reason for hiding this comment

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

Again, here we need a proper struct, instead of the naked array.

wxiaoguang
wxiaoguang reviewed Nov 12, 2024
View reviewed changes
templates/repo/settings/branches.tmpl
<div class="ui basic primary label">{{.RuleName}}</div>
<div data-id="{{.ID}}">
<div class="flex-item tw-items-center">
<div class="drag-handle tw-cursor-grab">
Copy link
Contributor

@wxiaoguang wxiaoguang Nov 12, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

It should be as simple as

<div class="flex-item tw-items-center item" data-id="{{.ID}} >
</div>

(I do not understand why it's worth to add the extra wrapper)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@wxiaoguang wxiaoguang wxiaoguang left review comments

Assignees
No one assigned
Labels
lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. modifies/api This PR adds API routes or modifies them modifies/frontend modifies/go Pull requests that update Go code modifies/migrations modifies/templates This PR modifies the template files size/L Denotes a PR that changes 100-499 lines, ignoring generated files. type/feature Completely new functionality. Can only be merged if feature freeze is not active.
Projects
None yet
Milestone
1.24.0
Development

Successfully merging this pull request may close these issues.
4 participants
@6543 @wxiaoguang @lunny @GiteaBot