Skip to content

Fix empty project displayed in issue sidebar #25802

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

Merged
merged 9 commits into from
Jul 12, 2023
Merged

Fix empty project displayed in issue sidebar #25802

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
ccce338
fix
yp05327 Jul 10, 2023
50b9574
add nil check
yp05327 Jul 10, 2023
e76a717
improve unit test
yp05327 Jul 10, 2023
6c4eef4
Update routers/web/org/projects.go
yp05327 Jul 11, 2023
901d374
Update routers/web/repo/projects.go
yp05327 Jul 11, 2023
0168d6b
Merge branch 'main' into fix-empty-project-in-issue-sidebar
yp05327 Jul 12, 2023
ec6000e
Merge branch 'main' into fix-empty-project-in-issue-sidebar
GiteaBot Jul 12, 2023
6d3529b
Merge branch 'main' into fix-empty-project-in-issue-sidebar
GiteaBot Jul 12, 2023
632f284
Merge branch 'main' into fix-empty-project-in-issue-sidebar
GiteaBot Jul 12, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 1 addition & 3 deletions models/issues/issue_list_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -67,9 +67,7 @@ func TestIssueList_LoadAttributes(t *testing.T) {
if issue.ID == int64(1) {
assert.Equal(t, int64(400), issue.TotalTrackedTime)
assert.NotNil(t, issue.Project)
} else if issue.ID == int64(2) {
assert.Equal(t, int64(3682), issue.TotalTrackedTime)
assert.Nil(t, issue.Project)
assert.Equal(t, int64(1), issue.Project.ID)
} else {
assert.Nil(t, issue.Project)
}
Expand Down
9 changes: 5 additions & 4 deletions models/issues/issue_project.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,13 +16,14 @@ import (
func (issue *Issue) LoadProject(ctx context.Context) (err error) {
if issue.Project == nil {
var p project_model.Project
if _, err = db.GetEngine(ctx).Table("project").
has, err := db.GetEngine(ctx).Table("project").

This comment was marked as outdated.

Sign in to view

Copy link
Member

@delvh delvh Jul 10, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

  1. has is almost idiomatic within the gitea code
  2. res is not what's returned here: The function returns bool, err, not <any>, err to show whether the pointer got a new value (see 3.)
  3. The actual result is set within the function as a pointer is explicitly passed: The &p means that the variable p is changed directly.

Copy link
Member

@delvh delvh Jul 10, 2023

Choose a reason for hiding this comment

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

The only potential alternative to has that would make sense and is also used throughout the code is exists which is probably a better name for it.

Copy link
Member

@denyskon denyskon Jul 10, 2023

Choose a reason for hiding this comment

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

Oh okay, didn't know that it is used in Gitea 😆

Join("INNER", "project_issue", "project.id=project_issue.project_id").
Where("project_issue.issue_id = ?", issue.ID).
Get(&p); err != nil {
Where("project_issue.issue_id = ?", issue.ID).Get(&p)
if err != nil {
return err
} else if has {
Copy link
Member

@puni9869 puni9869 Jul 10, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Suggested change
} else if has {
}
if has {

Not a blocker. But these are two different checks

Copy link
Contributor Author

@yp05327 yp05327 Jul 10, 2023

Choose a reason for hiding this comment

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

They look the same to me. 🤔

Copy link
Member

@puni9869 puni9869 Jul 10, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Technically you are 💯 correct.

From the readability point of viewif ... else if ... else should be act on same variable.
Here if block is related to err so else if also point to err
and for has it should be a different if block.

PS: No blocker my side. Just a thought. :)

issue.Project = &p
}
issue.Project = &p
}
return err
}
Expand Down
45 changes: 45 additions & 0 deletions models/issues/issue_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ import (
repo_model "code.gitea.io/gitea/models/repo"
"code.gitea.io/gitea/models/unittest"
user_model "code.gitea.io/gitea/models/user"
"code.gitea.io/gitea/modules/setting"
"code.gitea.io/gitea/modules/util"

"github.com/stretchr/testify/assert"
Expand Down Expand Up @@ -539,3 +540,47 @@ func TestCountIssues(t *testing.T) {
assert.NoError(t, err)
assert.EqualValues(t, 18, count)
}

func TestIssueLoadAttributes(t *testing.T) {
assert.NoError(t, unittest.PrepareTestDatabase())
setting.Service.EnableTimetracking = true

issueList := issues_model.IssueList{
unittest.AssertExistsAndLoadBean(t, &issues_model.Issue{ID: 1}),
unittest.AssertExistsAndLoadBean(t, &issues_model.Issue{ID: 4}),
}

for _, issue := range issueList {
assert.NoError(t, issue.LoadAttributes(db.DefaultContext))
assert.EqualValues(t, issue.RepoID, issue.Repo.ID)
for _, label := range issue.Labels {
assert.EqualValues(t, issue.RepoID, label.RepoID)
unittest.AssertExistsAndLoadBean(t, &issues_model.IssueLabel{IssueID: issue.ID, LabelID: label.ID})
}
if issue.PosterID > 0 {
assert.EqualValues(t, issue.PosterID, issue.Poster.ID)
}
if issue.AssigneeID > 0 {
assert.EqualValues(t, issue.AssigneeID, issue.Assignee.ID)
}
if issue.MilestoneID > 0 {
assert.EqualValues(t, issue.MilestoneID, issue.Milestone.ID)
}
if issue.IsPull {
assert.EqualValues(t, issue.ID, issue.PullRequest.IssueID)
}
for _, attachment := range issue.Attachments {
assert.EqualValues(t, issue.ID, attachment.IssueID)
}
for _, comment := range issue.Comments {
assert.EqualValues(t, issue.ID, comment.IssueID)
}
if issue.ID == int64(1) {
assert.Equal(t, int64(400), issue.TotalTrackedTime)
assert.NotNil(t, issue.Project)
assert.Equal(t, int64(1), issue.Project.ID)
} else {
assert.Nil(t, issue.Project)
}
}
}
3 changes: 1 addition & 2 deletions routers/web/org/projects.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -438,8 +438,7 @@ func UpdateIssueProject(ctx *context.Context) {
projectID := ctx.FormInt64("id")
for _, issue := range issues {
if issue.Project != nil {
oldProjectID := issue.Project.ID
if oldProjectID == projectID {
if issue.Project.ID == projectID {
continue
}
}
Expand Down
3 changes: 1 addition & 2 deletions routers/web/repo/projects.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -386,8 +386,7 @@ func UpdateIssueProject(ctx *context.Context) {
projectID := ctx.FormInt64("id")
for _, issue := range issues {
if issue.Project != nil {
oldProjectID := issue.Project.ID
if oldProjectID == projectID {
if issue.Project.ID == projectID {
continue
}
}
Expand Down