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

Add default board to new projects, remove uncategorized pseudo-board #29874

Merged
merged 46 commits into from Mar 27, 2024

Conversation

denyskon
Copy link
Member

@denyskon denyskon commented Mar 17, 2024

On creation of an empty project (no template) a default board will be created instead of falling back to the uneditable pseudo-board.

Every project now has to have exactly one default boards. As a consequence, you cannot unset a board as default, instead you have to set another board as default. Existing projects will be modified using a cron job, additionally this check will run every midnight by default.

Deleting the default board is not allowed, you have to set another board as default to do it.

Fixes #29873
Fixes #14679 along the way
Fixes #29853

Co-authored-by: delvh dev.lh@web.de

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Mar 17, 2024
@pull-request-size pull-request-size bot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Mar 17, 2024
@denyskon denyskon requested a review from delvh March 17, 2024 21:46
@github-actions github-actions bot added modifies/go Pull requests that update Go code modifies/docs labels Mar 17, 2024
@denyskon denyskon added type/enhancement An improvement of existing functionality topic/projects labels Mar 17, 2024
@denyskon denyskon added this to the 1.22.0 milestone Mar 17, 2024
@GiteaBot GiteaBot added lgtm/need 1 This PR needs approval from one additional maintainer to be merged. and removed lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. labels Mar 17, 2024
@delvh
Copy link
Member

delvh commented Mar 17, 2024

Wait, the default board is a pseudo-board?
Where is it set?

@denyskon
Copy link
Member Author

@delvh

// represents a board for issues not assigned to one

lng2020

This comment was marked as outdated.

@GiteaBot GiteaBot added lgtm/blocked A maintainer has reservations with the PR and thus it cannot be merged and removed lgtm/need 1 This PR needs approval from one additional maintainer to be merged. labels Mar 18, 2024
@denyskon
Copy link
Member Author

@lng2020 The default logic will apply if all columns are deleted which in fact can easily happen. Also, we cannot possibly do this change retroactively for all existing projects which currently display this default pseudo-column.

lng2020

This comment was marked as outdated.

@lng2020 lng2020 self-requested a review March 18, 2024 05:52
lng2020

This comment was marked as outdated.

@denyskon denyskon requested a review from lng2020 March 18, 2024 08:43
lng2020

This comment was marked as outdated.

@GiteaBot GiteaBot added lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. and removed lgtm/blocked A maintainer has reservations with the PR and thus it cannot be merged labels Mar 18, 2024
lunny
lunny previously requested changes Mar 18, 2024
Copy link
Member

@lunny lunny left a comment

Choose a reason for hiding this comment

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

Do we really need the configuration? Maybe we can hardcode it and make it an option when we think it's necessary.

6543
6543 previously requested changes Mar 26, 2024
// CheckProjectColumnsConsistency ensures there is exactly one default board per project present
func CheckProjectColumnsConsistency(x *xorm.Engine) error {
var projects []project.Project
if err := x.SQL("SELECT DISTINCT `p`.`id`, `p`.`creator_id` FROM `project` `p` WHERE (SELECT COUNT(*) FROM `project_board` `pb` WHERE `pb`.`project_id` = `p`.`id` AND `pb`.`default` = ?) != 1", true).
Copy link
Member

Choose a reason for hiding this comment

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

I'd like to have pagination in this migration!

Copy link
Member Author

Choose a reason for hiding this comment

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

Honestly I have no idea what you mean or how to do it 🙈

Copy link
Member

Choose a reason for hiding this comment

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

Itterate over it in batches ... just look at older migrations

Copy link
Member

Choose a reason for hiding this comment

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

e.g.

limit := setting.Database.IterateBufferSize

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh so the thing I did before 😆

Copy link
Member

Choose a reason for hiding this comment

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

Uh you did?

Copy link
Member

Choose a reason for hiding this comment

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

Yep, and then it was removed again.
Also, I don't quite see the benefit of batching here.
How does it help with anything?

Copy link
Member

Choose a reason for hiding this comment

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

I guess the only reason to batch is when amount of project boards is really massive and the SQL operation would risk running into timeouts maybe.

On the other hand, batched also brings the problem that the operation is not atomic, but if it's idempotent, it wouldn't matter when it's interrupted in the middle.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done

@GiteaBot GiteaBot added lgtm/blocked A maintainer has reservations with the PR and thus it cannot be merged and removed lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. labels Mar 26, 2024
@denyskon denyskon requested a review from 6543 March 27, 2024 18:16
@delvh delvh dismissed 6543’s stale review March 27, 2024 18:32

Implemented

@GiteaBot GiteaBot added lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. and removed lgtm/blocked A maintainer has reservations with the PR and thus it cannot be merged labels Mar 27, 2024
@silverwind
Copy link
Member

@6543 looking good to you?

@6543 6543 enabled auto-merge (squash) March 27, 2024 20:28
@6543 6543 added the reviewed/wait-merge This pull request is part of the merge queue. It will be merged soon. label Mar 27, 2024
@6543 6543 merged commit e516018 into go-gitea:main Mar 27, 2024
26 checks passed
@GiteaBot GiteaBot removed the reviewed/wait-merge This pull request is part of the merge queue. It will be merged soon. label Mar 27, 2024
@denyskon denyskon deleted the projects-default-board branch March 27, 2024 20:55
@denyskon
Copy link
Member Author

Finally 🎉

silverwind added a commit to silverwind/gitea that referenced this pull request Mar 27, 2024
* origin/main: (108 commits)
  Fix table alignment classes (go-gitea#30144)
  Add default board to new projects, remove uncategorized pseudo-board (go-gitea#29874)
  Fix loading spinner on ContextPopup (go-gitea#30145)
  Fix download buttons on branches page (go-gitea#30147)
  Relax generic package filename restrictions (go-gitea#30135)
  Remove jQuery class from the common admin functions (go-gitea#30137)
  Remove jQuery class from the reaction selector (go-gitea#30138)
  Forbid jQuery `.attr` (go-gitea#30116)
  Refactor render (go-gitea#30136)
  Fix: Organization Interface Display Issue (go-gitea#30133)
  Remove jQuery `.attr` from the Fomantic dropdowns (go-gitea#30114)
  Remove jQuery `.attr` from the common admin functions (go-gitea#30115)
  Remove jQuery from the create/rename branch modals (except Fomantic) (go-gitea#30109)
  Remove fomantic label module (go-gitea#30081)
  Fix bug for markdown rendering of blockquote (go-gitea#30130)
  Fix: The interface is broken when modifying  code comments under mobile devices  (go-gitea#30125)
  When the title in the issue has a value, set the text cursor at the end of the text. (go-gitea#30090)
  Load attachments for code comments (go-gitea#30124)
  Upgrade fabric to 6.0.0-beta20 (go-gitea#30121)
  Fix click handler in job-step-summary (go-gitea#30122)
  ...
Limit(limit, start).
Find(&projects); err != nil {
return err
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry but this migration seems broken ....

If the "SELECT" results are stable and never change, this for loop is right.

BUT, by each SELECT and for-loop, the "project boards" are changed. The next SELECT, it should still process the first page results, but not skip start.

For example: 150 projects.

  1. First loop: SELECT gets 1-50 projects and updates them.
  2. Second loop: SELECT gets "the second page" from "51-150", then it gets 101-150 projects.
  3. Then the 51-100 projects are never migrated.

Copy link
Member

Choose a reason for hiding this comment

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

I will send a PR to fix it.

Copy link
Member

Choose a reason for hiding this comment

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

zjjhot added a commit to zjjhot/gitea that referenced this pull request Mar 28, 2024
* giteaofficial/main: (63 commits)
  Apply to become a maintainer (go-gitea#30151)
  Refactor markdown render (go-gitea#30139)
  Drag-and-drop improvements for projects and issue pins (go-gitea#29875)
  Fix table alignment classes (go-gitea#30144)
  Add default board to new projects, remove uncategorized pseudo-board (go-gitea#29874)
  Fix loading spinner on ContextPopup (go-gitea#30145)
  Fix download buttons on branches page (go-gitea#30147)
  Relax generic package filename restrictions (go-gitea#30135)
  Remove jQuery class from the common admin functions (go-gitea#30137)
  Remove jQuery class from the reaction selector (go-gitea#30138)
  Forbid jQuery `.attr` (go-gitea#30116)
  Refactor render (go-gitea#30136)
  Fix: Organization Interface Display Issue (go-gitea#30133)
  Remove jQuery `.attr` from the Fomantic dropdowns (go-gitea#30114)
  Remove jQuery `.attr` from the common admin functions (go-gitea#30115)
  Remove jQuery from the create/rename branch modals (except Fomantic) (go-gitea#30109)
  Remove fomantic label module (go-gitea#30081)
  Fix bug for markdown rendering of blockquote (go-gitea#30130)
  Fix: The interface is broken when modifying  code comments under mobile devices  (go-gitea#30125)
  When the title in the issue has a value, set the text cursor at the end of the text. (go-gitea#30090)
  ...

# Conflicts:
#	templates/user/dashboard/issues.tmpl
@lunny lunny mentioned this pull request Mar 28, 2024
6543 pushed a commit that referenced this pull request Mar 28, 2024
Fix #29874 (comment)

- The migration of v292 will miss many projects. These projects will
have no default board. This PR introduced a new migration number and
removed v292 migration.

- This PR also added the missed transactions on project-related
operations.

- Only `SetDefaultBoard` will remove duplicated defaults but not in
`GetDefaultBoard`
DennisRasey pushed a commit to DennisRasey/forgejo that referenced this pull request Mar 30, 2024
Fix go-gitea/gitea#29874 (comment)

- The migration of v292 will miss many projects. These projects will
have no default board. This PR introduced a new migration number and
removed v292 migration.

- This PR also added the missed transactions on project-related
operations.

- Only `SetDefaultBoard` will remove duplicated defaults but not in
`GetDefaultBoard`

(cherry picked from commit 40cdc84b368cce8328b4b49ea5ecf1c5fa040300)
a1012112796 added a commit to a1012112796/gitea that referenced this pull request Apr 25, 2024
- as title
- remove logic about zero `project_board_id` in `project_issue`
  table and fix some related test data. because after go-gitea#29874,
  `project_board_id` willn't be zero
- some small refactoring

Signed-off-by: a1012112796 <1012112796@qq.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. modifies/go Pull requests that update Go code modifies/js modifies/migrations modifies/templates This PR modifies the template files modifies/translation size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. topic/projects type/enhancement An improvement of existing functionality
Projects
None yet
8 participants