Skip to content

Add more Links to Comments #25049

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

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

Add more Links to Comments #25049

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
15ac3a8
Add more Links to Comments
JakobDev Jun 2, 2023
ea3ac41
Merge branch 'main' into commentlink
JakobDev Jun 2, 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
16 changes: 16 additions & 0 deletions models/issues/milestone.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -123,6 +123,22 @@ func NewMilestone(m *Milestone) (err error) {
return committer.Commit()
}

// LoadRepo loads milestones repository
func (m *Milestone) LoadRepo(ctx context.Context) (err error) {
if m.Repo == nil && m.RepoID != 0 {
m.Repo, err = repo_model.GetRepositoryByID(ctx, m.RepoID)
if err != nil {
return fmt.Errorf("getRepositoryByID [%d]: %w", m.RepoID, err)
}
}
return nil
}

// Link returns the milestone Link
func (m *Milestone) Link() string {
return fmt.Sprintf("%s/milestone/%d", m.Repo.HTMLURL(), m.ID)
Copy link
Member

Choose a reason for hiding this comment

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

You want m.Repo.Link here.
HTMLURL is absolute while Link should be relative.

Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
return fmt.Sprintf("%s/milestone/%d", m.Repo.HTMLURL(), m.ID)
return fmt.Sprintf("%s/milestone/%d", m.Repo.Link(), m.ID)

}

// HasMilestoneByRepoID returns if the milestone exists in the repository.
func HasMilestoneByRepoID(ctx context.Context, repoID, id int64) (bool, error) {
return db.GetEngine(ctx).ID(id).Where("repo_id=?", repoID).Exist(new(Milestone))
Expand Down
15 changes: 15 additions & 0 deletions routers/web/repo/issue.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1554,6 +1554,21 @@ func ViewIssue(ctx *context.Context) {
if comment.MilestoneID > 0 && comment.Milestone == nil {
comment.Milestone = ghostMilestone
}

if comment.Milestone != nil && comment.Milestone.ID != -1 {
err = comment.Milestone.LoadRepo(ctx)
if err != nil {
ctx.ServerError("LoadMilestoneRepo", err)
return
}
}
if comment.OldMilestone != nil && comment.OldMilestone.ID != -1 {
err = comment.OldMilestone.LoadRepo(ctx)
if err != nil {
ctx.ServerError("LoadMilestoneRepo", err)
return
}
}
Comment on lines +1558 to +1571
Copy link
Member

Choose a reason for hiding this comment

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

I recommend two separate methods Comment#LoadMilestone and Comment#LoadOldMilestone

} else if comment.Type == issues_model.CommentTypeProject {

if err = comment.LoadProject(); err != nil {
Expand Down
18 changes: 13 additions & 5 deletions templates/repo/issue/view_content/comments.tmpl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -195,7 +195,15 @@
{{template "shared/user/avatarlink" dict "Context" $.Context "user" .Poster}}
<span class="text grey muted-links">
{{template "shared/user/authorlink" .Poster}}
{{if gt .OldMilestoneID 0}}{{if gt .MilestoneID 0}}{{$.locale.Tr "repo.issues.change_milestone_at" (.OldMilestone.Name|Escape) (.Milestone.Name|Escape) $createdStr | Safe}}{{else}}{{$.locale.Tr "repo.issues.remove_milestone_at" (.OldMilestone.Name|Escape) $createdStr | Safe}}{{end}}{{else if gt .MilestoneID 0}}{{$.locale.Tr "repo.issues.add_milestone_at" (.Milestone.Name|Escape) $createdStr | Safe}}{{end}}
{{if gt .OldMilestoneID 0}}
{{if gt .MilestoneID 0}}
{{$.locale.Tr "repo.issues.change_milestone_at" (printf `<a href="%s">%s</a>` .OldMilestone.Link (.OldMilestone.Name|Escape)) (printf `<a href="%s">%s</a>` .Milestone.Link (.Milestone.Name|Escape)) $createdStr | Safe}}
{{else}}
{{$.locale.Tr "repo.issues.remove_milestone_at" (printf `<a href="%s">%s</a>` .OldMilestone.Link (.OldMilestone.Name|Escape)) $createdStr | Safe}}
{{end}}
{{else if gt .MilestoneID 0}}
{{$.locale.Tr "repo.issues.add_milestone_at" (printf `<a href="%s">%s</a>` .Milestone.Link (.Milestone.Name|Escape)) $createdStr | Safe}}
{{end}}
</span>
</div>
{{else if eq .Type 9}}
Expand All @@ -209,7 +217,7 @@
{{if eq .Poster.ID .Assignee.ID}}
{{$.locale.Tr "repo.issues.remove_self_assignment" $createdStr | Safe}}
{{else}}
{{$.locale.Tr "repo.issues.remove_assignee_at" (.Poster.GetDisplayName|Escape) $createdStr | Safe}}
{{$.locale.Tr "repo.issues.remove_assignee_at" (printf `<a href="%s">%s</a>` .Poster.HomeLink (.Poster.GetDisplayName|Escape)) $createdStr | Safe}}
Copy link
Member

Choose a reason for hiding this comment

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

We are adding the same exact call that many times that we should perhaps extract it into a separate function.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is only used in this template. Do we really need a own function for for every object something that is used in a single template a few times?

{{end}}
</span>
{{else}}
Expand All @@ -219,7 +227,7 @@
{{if eq .Poster.ID .AssigneeID}}
{{$.locale.Tr "repo.issues.self_assign_at" $createdStr | Safe}}
{{else}}
{{$.locale.Tr "repo.issues.add_assignee_at" (.Poster.GetDisplayName|Escape) $createdStr | Safe}}
{{$.locale.Tr "repo.issues.add_assignee_at" (printf `<a href="%s">%s</a>` .Poster.HomeLink (.Poster.GetDisplayName|Escape)) $createdStr | Safe}}
{{end}}
</span>
{{end}}
Expand Down Expand Up @@ -743,12 +751,12 @@
{{$oldProjectDisplayHtml := "Unknown Project"}}
{{if .OldProject}}
{{$trKey := printf "projects.type-%d.display_name" .OldProject.Type}}
{{$oldProjectDisplayHtml = printf `<span data-tooltip-content="%s">%s</span>` ($.locale.Tr $trKey | Escape) (.OldProject.Title | Escape)}}
{{$oldProjectDisplayHtml = printf `<a href="%s" data-tooltip-content="%s">%s</a>` .OldProject.Link ($.locale.Tr $trKey | Escape) (.OldProject.Title | Escape)}}
{{end}}
{{$newProjectDisplayHtml := "Unknown Project"}}
{{if .Project}}
{{$trKey := printf "projects.type-%d.display_name" .Project.Type}}
{{$newProjectDisplayHtml = printf `<span data-tooltip-content="%s">%s</span>` ($.locale.Tr $trKey | Escape) (.Project.Title | Escape)}}
{{$newProjectDisplayHtml = printf `<a href="%s" data-tooltip-content="%s">%s</a>` .Project.Link ($.locale.Tr $trKey | Escape) (.Project.Title | Escape)}}
{{end}}
{{if and (gt .OldProjectID 0) (gt .ProjectID 0)}}
{{$.locale.Tr "repo.issues.change_project_at" $oldProjectDisplayHtml $newProjectDisplayHtml $createdStr | Safe}}
Expand Down