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

Fix duplicate issues in default boards #27659

Closed
wants to merge 2 commits into from
Closed

Fix duplicate issues in default boards #27659

wants to merge 2 commits into from

Conversation

edlerd
Copy link

@edlerd edlerd commented Oct 17, 2023

A user reported in #27639 that the same issue appears twice on their project board. This fixes one source of duplication.

Fixes #27639

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Oct 17, 2023
@pull-request-size pull-request-size bot added the size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. label Oct 17, 2023
@edlerd edlerd force-pushed the fix-duplication-issues-in-board branch from cc4d8c3 to e109607 Compare October 17, 2023 12:04
@pull-request-size pull-request-size bot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Oct 17, 2023
@edlerd edlerd force-pushed the fix-duplication-issues-in-board branch from e109607 to 05a6c86 Compare October 17, 2023 12:18
lunny
lunny reviewed Oct 17, 2023
View reviewed changes
models/issues/issue_project.go Outdated
@@ -51,7 +51,7 @@ func (issue *Issue) ProjectBoardID(ctx context.Context) int64 {
func LoadIssuesFromBoard(ctx context.Context, b *project_model.Board) (IssueList, error) {
issueList := make(IssueList, 0, 10)

if b.ID != 0 {
if b.ID != 0 && !(b.ID == -1 && b.Default) {
Copy link
Member

Choose a reason for hiding this comment

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

Use constant variable but -1 directly.

Copy link
Author

Choose a reason for hiding this comment

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

I don't understand what you mean. Can you please clarify?

Copy link
Member

@delvh delvh Oct 17, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

What lunny means is you should use a variable instead of the constant.
I'd even go a step further, and the whole new check should be extracted into its own function.

Copy link
Member

@lng2020 lng2020 Oct 17, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

The code used -1 since the Kanban board was introduced in 2020. Maybe should do some refactor later.

Copy link
Author

Choose a reason for hiding this comment

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

I can simplify the check to b.ID > 0 to cover all cases.

@edlerd edlerd force-pushed the fix-duplication-issues-in-board branch 2 times, most recently from 034ec3d to 6a8fc29 Compare October 18, 2023 21:05
lng2020
lng2020 reviewed Oct 20, 2023
View reviewed changes
Copy link
Member

@lng2020 lng2020 left a comment •
edited
Loading

Choose a reason for hiding this comment

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

I doubt that it will fix the issue. The wrapper function already uses transactions, no need for transactions again.
and according to the user-provided screenshot, I think it's probably because of other functions. (Though haven't found out why yet)

@edlerd
Fix duplicate issues in default boards
c1afe0e 
User reported in go-gitea#27639 that the same issue appears twice on their board. This fixes the source of duplication.

Fixes go-gitea#27639

Signed-off-by: David Edler <david.edler@canonical.com>
@edlerd edlerd force-pushed the fix-duplication-issues-in-board branch from 6a8fc29 to c1afe0e Compare October 20, 2023 08:30
@pull-request-size pull-request-size bot added size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Oct 20, 2023
@6543
Copy link
Member

6543 commented Oct 20, 2023

this pull has no diff anymore.

looks like #27705 did address this also

@lng2020
Copy link
Member

lng2020 commented Oct 20, 2023 •
edited
Loading

But the issue is not fixed. Change b.ID != 0 to b.ID > 0 has no thing to do with the issue.
Probably the issue is about other CRUD operations that should be a transaction.

@lunny
Copy link
Member

lunny commented Oct 22, 2023

Maybe open another PR or send some changes.

@lunny
Copy link
Member

lunny commented Nov 1, 2023

Looks like this PR is empty now.

@delvh
Copy link
Member

delvh commented Jan 11, 2024

This PR seems dead.

@delvh delvh closed this Jan 11, 2024
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Mar 12, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@lng2020 lng2020 lng2020 left review comments

@delvh delvh delvh left review comments

@lunny lunny lunny left review comments

Assignees
No one assigned
Labels
lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Same issue shows twice in project's column
6 participants
@edlerd @6543 @lng2020 @lunny @delvh @GiteaBot