-
-
Notifications
You must be signed in to change notification settings - Fork 5.4k
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
Fix deleted milestone bug #1942
Conversation
32e68ae
to
c145720
Compare
Why delete these comments? Maybe just set milestone_id to zero? |
@lunny Setting |
Just don't load the milestones instead? 😖 Deleting all comments on issue where the milestone doesn't exist is not the solution... |
Why does the comments even link to milestones in the first place?! 😂 |
Right, personally I'd just replace |
c145720
to
a046dff
Compare
When build is fixed than LG-TM |
35ebe79
to
2e5dee4
Compare
Rebased to re-run CI |
models/issue_milestone.go
Outdated
func NewGhostMilestone() *Milestone { | ||
return &Milestone{ | ||
ID: -1, | ||
Name: "Deleted", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be translatable
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
69be790
to
f816ff8
Compare
FWIW, we currently set comments' |
CI build keeps failing due to locking in |
@ethantkoenig I restarted it and LGTM |
models/issue_comment.go
Outdated
@@ -228,28 +228,22 @@ func (c *Comment) LoadLabel() error { | |||
func (c *Comment) LoadMilestone() error { | |||
if c.OldMilestoneID > 0 { | |||
var oldMilestone Milestone | |||
has, err := x.ID(c.OldMilestoneID).Get(&oldMilestone) | |||
has, err := x.ID(c.OldMilestoneID).Get(oldMilestone) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should be &oldMilestone
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@lunny Fixed
LGTM |
* Fix deleted milestone bug * Use locale for ghost milestone name * Fix pointer bug
Fixes a bug where viewing an issue from a deleted milestone resulted in a 500 (because comments from that issue would still refer to the deleted milestone, which gitea would then try to load).
When deleting a milestone, delete all comments referencing that milestone.Introduce "ghost" milestones (similar to "ghost" users) to fix the problem.