fix: load OldMilestone based on OldMilestoneID, not MilestoneID (#8330)

Fixes #8329

## Checklist

The [contributor guide](https://forgejo.org/docs/next/contributor/) contains information that will be helpful to first time contributors. There also are a few [conditions for merging Pull Requests in Forgejo repositories](https://codeberg.org/forgejo/governance/src/branch/main/PullRequestsAgreement.md). You are also welcome to join the [Forgejo development chatroom](https://matrix.to/#/#forgejo-development:matrix.org).

### Tests

- I added test coverage for Go changes...
  - [ ] in their respective `*_test.go` for unit tests.
  - [x] in the `tests/integration` directory if it involves interactions with a live Forgejo server.

### Documentation

- [x] I did not document these changes and I do not expect someone else to do it.

### Release notes

- [ ] I do not want this change to show in the release notes.
- [x] I want the title to show in the release notes with a link to this pull request.

Reviewed-on: https://codeberg.org/forgejo/forgejo/pulls/8330
Reviewed-by: Robert Wolff <mahlzahn@posteo.de>
Co-authored-by: floss4good <floss4good@disroot.org>
Co-committed-by: floss4good <floss4good@disroot.org>

models/fixtures/comment.yml

@@ -186,10 +186,46 @@
   type: 8 # milestone
   poster_id: 1
   issue_id: 1 # in repo_id 1
-  milestone_id: 10 # not exsting milestone
+  milestone_id: 10 # not existing milestone
   old_milestone_id: 0
   created_unix: 946685080
 
+-
+  id: 2004
+  type: 8 # milestone
+  poster_id: 1
+  issue_id: 1 # in repo_id 1
+  milestone_id: 1
+  old_milestone_id: 10 # not existing (ghost) milestone
+  created_unix: 946685085
+
+-
+  id: 2005
+  type: 8 # milestone
+  poster_id: 1
+  issue_id: 1 # in repo_id 1
+  milestone_id: 10 # not existing (ghost) milestone
+  old_milestone_id: 1
+  created_unix: 946685090
+
+-
+  id: 2006
+  type: 8 # milestone
+  poster_id: 1
+  issue_id: 1 # in repo_id 1
+  milestone_id: 11 # not existing (ghost) milestone
+  old_milestone_id: 10 # not existing (ghost) milestone
+  created_unix: 946685095
+
+-
+  id: 2007
+  type: 8 # milestone
+  poster_id: 1
+  issue_id: 1 # in repo_id 1
+  milestone_id: 0
+  old_milestone_id: 11 # not existing (ghost) milestone
+  created_unix: 946685100
+
 -
   id: 2010
   type: 30 # project

models/issues/comment_list.go

@@ -101,7 +101,7 @@ func (comments CommentList) loadMilestones(ctx context.Context) error {
 		return nil
 	}
 
-	milestoneMaps := make(map[int64]*Milestone, len(milestoneIDs))
+	milestones := make(map[int64]*Milestone, len(milestoneIDs))
 	left := len(milestoneIDs)
 	for left > 0 {
 		limit := db.DefaultMaxInSize
@@ -110,7 +110,7 @@ func (comments CommentList) loadMilestones(ctx context.Context) error {
 		}
 		err := db.GetEngine(ctx).
 			In("id", milestoneIDs[:limit]).
-			Find(&milestoneMaps)
+			Find(&milestones)
 		if err != nil {
 			return err
 		}
@@ -118,8 +118,8 @@ func (comments CommentList) loadMilestones(ctx context.Context) error {
 		milestoneIDs = milestoneIDs[limit:]
 	}
 
-	for _, issue := range comments {
+	for _, comment := range comments {
-		issue.Milestone = milestoneMaps[issue.MilestoneID]
+		comment.Milestone = milestones[comment.MilestoneID]
 	}
 	return nil
 }
@@ -140,7 +140,7 @@ func (comments CommentList) loadOldMilestones(ctx context.Context) error {
 		return nil
 	}
 
-	milestoneMaps := make(map[int64]*Milestone, len(milestoneIDs))
+	milestones := make(map[int64]*Milestone, len(milestoneIDs))
 	left := len(milestoneIDs)
 	for left > 0 {
 		limit := db.DefaultMaxInSize
@@ -149,7 +149,7 @@ func (comments CommentList) loadOldMilestones(ctx context.Context) error {
 		}
 		err := db.GetEngine(ctx).
 			In("id", milestoneIDs[:limit]).
-			Find(&milestoneMaps)
+			Find(&milestones)
 		if err != nil {
 			return err
 		}
@@ -157,8 +157,8 @@ func (comments CommentList) loadOldMilestones(ctx context.Context) error {
 		milestoneIDs = milestoneIDs[limit:]
 	}
 
-	for _, issue := range comments {
+	for _, comment := range comments {
-		issue.OldMilestone = milestoneMaps[issue.MilestoneID]
+		comment.OldMilestone = milestones[comment.OldMilestoneID]
 	}
 	return nil
 }

tests/integration/issue_comment_test.go

@@ -102,11 +102,35 @@ func TestIssueCommentChangeMilestone(t *testing.T) {
 		[]string{"user1 removed this from the milestone2 milestone"},
 		[]string{"/user1", "/user2/repo1/milestone/2"})
 
-	// Deleted milestone
+	// Added milestone that in the meantime was deleted
 	testIssueCommentChangeEvent(t, htmlDoc, "2003",
 		"octicon-milestone", "User One", "/user1",
 		[]string{"user1 added this to the (deleted) milestone"},
 		[]string{"/user1"})
+
+	// Modified milestone - from a meantime deleted one to a valid one
+	testIssueCommentChangeEvent(t, htmlDoc, "2004",
+		"octicon-milestone", "User One", "/user1",
+		[]string{"user1 modified the milestone from (deleted) to milestone1"},
+		[]string{"/user1", "/user2/repo1/milestone/1"})
+
+	// Modified milestone - from a valid one to a meantime deleted one
+	testIssueCommentChangeEvent(t, htmlDoc, "2005",
+		"octicon-milestone", "User One", "/user1",
+		[]string{"user1 modified the milestone from milestone1 to (deleted)"},
+		[]string{"/user1", "/user2/repo1/milestone/1"})
+
+	// Modified milestone - from a meantime deleted one to a meantime deleted one
+	testIssueCommentChangeEvent(t, htmlDoc, "2006",
+		"octicon-milestone", "User One", "/user1",
+		[]string{"user1 modified the milestone from (deleted) to (deleted)"},
+		[]string{"/user1"})
+
+	// Removed milestone that in the meantime was deleted
+	testIssueCommentChangeEvent(t, htmlDoc, "2007",
+		"octicon-milestone", "User One", "/user1",
+		[]string{"user1 removed this from the (deleted) milestone"},
+		[]string{"/user1"})
 }
 
 func TestIssueCommentChangeProject(t *testing.T) {