Commit @0243e0ed710eab5f1c496106d16f601204b10ded

Changseong Kim 2014-03-18

fix the issue 1048.

Cause:
Before referring original issue properties for the first time, to call update() of modifieid issue brings about the issue because of lazy loading.

Solution:
The codes that load original issue properties are moved above calling update() of modified issue.

@0243e0ed710eab5f1c496106d16f601204b10ded

0499487

0243e0e

app/controllers/AbstractPostingApp.java

--- app/controllers/AbstractPostingApp.java

+++ app/controllers/AbstractPostingApp.java


      * @param posting
      * @param postingForm
      * @param redirectTo
-     * @param updatePosting
+     * @param preUpdateHook
      * @return
      */
-    protected static Result editPosting(AbstractPosting original, AbstractPosting posting, Form<? extends AbstractPosting> postingForm, Call redirectTo, Runnable updatePosting) {
+    protected static Result editPosting(AbstractPosting original, AbstractPosting posting, Form<? extends AbstractPosting> postingForm, Call redirectTo, Runnable preUpdateHook) {
         if (postingForm.hasErrors()) {
             return badRequest(ErrorViews.BadRequest.render("error.validation", original.project));
         }

         posting.authorLoginId = original.authorLoginId;
         posting.authorName = original.authorName;
         posting.project = original.project;
-        updatePosting.run();
+        preUpdateHook.run();
         posting.update();
         posting.updateProperties();
 

0499487

0243e0e

app/controllers/IssueApp.java

--- app/controllers/IssueApp.java

+++ app/controllers/IssueApp.java


 
     private static void addAssigneeChangedNotification(Issue modifiedIssue, Issue originalIssue) {
         if(!originalIssue.assignedUserEquals(modifiedIssue.assignee)) {
-            Issue updatedIssue = Issue.finder.byId(originalIssue.id);
             User oldAssignee = null;
             if(hasAssignee(originalIssue)) {
                 oldAssignee = originalIssue.assignee.user;
             }
-            NotificationEvent notiEvent = NotificationEvent.afterAssigneeChanged(oldAssignee, updatedIssue);
+            NotificationEvent notiEvent = NotificationEvent.afterAssigneeChanged(oldAssignee, modifiedIssue);
             IssueEvent.addFromNotificationEvent(notiEvent, modifiedIssue, UserApp.currentUser().loginId);
         }
     }
 
     private static void addStateChangedNotification(Issue modifiedIssue, Issue originalIssue) {
         if(modifiedIssue.state != originalIssue.state) {
-            Issue updatedIssue = Issue.finder.byId(originalIssue.id);
-            NotificationEvent notiEvent = NotificationEvent.afterStateChanged(originalIssue.state, updatedIssue);
+            NotificationEvent notiEvent = NotificationEvent.afterStateChanged(originalIssue.state, modifiedIssue);
             IssueEvent.addFromNotificationEvent(notiEvent, modifiedIssue, UserApp.currentUser().loginId);
         }
     }
 
     private static void addBodyChangedNotification(Issue modifiedIssue, Issue originalIssue) {
         if (!modifiedIssue.body.equals(originalIssue.body)) {
-            Issue updatedIssue = Issue.finder.byId(originalIssue.id);
-            NotificationEvent notiEvent = NotificationEvent.afterIssueBodyChanged(originalIssue.body, updatedIssue);
+            NotificationEvent notiEvent = NotificationEvent.afterIssueBodyChanged(originalIssue.body, modifiedIssue);
             IssueEvent.addFromNotificationEvent(notiEvent, modifiedIssue, UserApp.currentUser().loginId);
         }
     }

 
         Call redirectTo = routes.IssueApp.issue(project.owner, project.name, number);
 
-        // updateIssueBeforeSave.run would be called just before this issue is saved.
+        // preUpdateHook.run would be called just before this issue is updated.
         // It updates some properties only for issues, such as assignee or labels, but not for non-issues.
-        Runnable updateIssueBeforeSave = new Runnable() {
+        Runnable preUpdateHook = new Runnable() {
             @Override
             public void run() {
                 // Below addAll() method is needed to avoid the exception, 'Timeout trying to lock table ISSUE'.

                 issue.voters.addAll(originalIssue.voters);
                 issue.comments = originalIssue.comments;
                 addLabels(issue, request());
+
+                addAssigneeChangedNotification(issue, originalIssue);
+                addStateChangedNotification(issue, originalIssue);
+                addBodyChangedNotification(issue, originalIssue);
             }
         };
 
-        Result result = editPosting(originalIssue, issue, issueForm, redirectTo, updateIssueBeforeSave);
-
-        addAssigneeChangedNotification(issue, originalIssue);
-        addStateChangedNotification(issue, originalIssue);
-        addBodyChangedNotification(issue, originalIssue);
-
-        return result;
+        return editPosting(originalIssue, issue, issueForm, redirectTo, preUpdateHook);
     }
 
     /*

0499487

0243e0e

app/views/issue/partial_comments.scala.html

--- app/views/issue/partial_comments.scala.html

+++ app/views/issue/partial_comments.scala.html


     </li>
     }
     case (event: IssueEvent) => {
-    <li class="event" id="event-@event.id">
-        @defining(User.findByLoginId(event.senderLoginId)) { user =>
-            @event.eventType match {
-                case EventType.ISSUE_STATE_CHANGED => {
-                    <span class="state @event.newValue">@Messages("issue.state." + event.newValue)</span> @Html(Messages("issue.event." + event.newValue, linkToUser(user.loginId, user.name)))
-                }
-                case EventType.ISSUE_ASSIGNEE_CHANGED => {
-                    <span class="state changed">@Messages("issue.state.assigned")</span>
-                    @Html(Messages(assginedMesssage(event.newValue, user), linkToUser(user.loginId, user.name), linkToUser(event.newValue,User.findByLoginId(event.newValue).name, true)))
-                }
-                case EventType.ISSUE_REFERRED_FROM_COMMIT => {
-                    <span class="state changed">@Messages("issue.event.referred.title")</span>
-                    @Html(Messages("issue.event.referred",linkToUser(user.loginId, user.name),linkToCommit(event.newValue)))
-                }
-                case EventType.ISSUE_REFERRED_FROM_PULL_REQUEST => {
-                    <span class="state changed">@Messages("issue.event.referred.title")</span>
-                    @defining(PullRequest.findById(Long.valueOf(event.newValue))) { pull =>
-                        @Html(Messages("issue.event.referred",linkToUser(user.loginId, user.name),linkToPullRequest(pull)))
+        @if(event.eventType != EventType.ISSUE_BODY_CHANGED) {
+            <li class="event" id="event-@event.id">
+                @defining(User.findByLoginId(event.senderLoginId)) { user =>
+                    @event.eventType match {
+                        case EventType.ISSUE_STATE_CHANGED => {
+                            <span class="state @event.newValue">@Messages("issue.state." + event.newValue)</span> @Html(Messages("issue.event." + event.newValue, linkToUser(user.loginId, user.name)))
+                        }
+                        case EventType.ISSUE_ASSIGNEE_CHANGED => {
+                            <span class="state changed">@Messages("issue.state.assigned")</span>
+                            @Html(Messages(assginedMesssage(event.newValue, user), linkToUser(user.loginId, user.name), linkToUser(event.newValue,User.findByLoginId(event.newValue).name, true)))
+                        }
+                        case EventType.ISSUE_REFERRED_FROM_COMMIT => {
+                            <span class="state changed">@Messages("issue.event.referred.title")</span>
+                            @Html(Messages("issue.event.referred",linkToUser(user.loginId, user.name),linkToCommit(event.newValue)))
+                        }
+                        case EventType.ISSUE_REFERRED_FROM_PULL_REQUEST => {
+                            <span class="state changed">@Messages("issue.event.referred.title")</span>
+                            @defining(PullRequest.findById(Long.valueOf(event.newValue))) { pull =>
+                                @Html(Messages("issue.event.referred",linkToUser(user.loginId, user.name),linkToPullRequest(pull)))
+                            }
+                        }
+                        case _ => {
+                            @event.newValue by @linkToUser(user.loginId, user.name)
+                        }
                     }
                 }
-                case EventType.ISSUE_BODY_CHANGED => {
-                }
-                case _ => {
-                    @event.newValue by @linkToUser(user.loginId, user.name)
-                }
-            }
+                <span class="date"><a href="#event-@event.id">@utils.TemplateHelper.agoString(JodaDateUtil.ago(event.getDate()))</a></span>
+            </li>
         }
-        <span class="date"><a href="#event-@event.id">@utils.TemplateHelper.agoString(JodaDateUtil.ago(event.getDate()))</a></span>
-    </li>
     }
     }
 }

Add a comment

Open 0
Closed 0

List

...	...	@@ -116,10 +116,10 @@
116	116	* @param posting
117	117	* @param postingForm
118	118	* @param redirectTo
119		- * @param updatePosting
	119	+ * @param preUpdateHook
120	120	* @return
121	121	*/
122		- protected static Result editPosting(AbstractPosting original, AbstractPosting posting, Form<? extends AbstractPosting> postingForm, Call redirectTo, Runnable updatePosting) {
	122	+ protected static Result editPosting(AbstractPosting original, AbstractPosting posting, Form<? extends AbstractPosting> postingForm, Call redirectTo, Runnable preUpdateHook) {
123	123	if (postingForm.hasErrors()) {
124	124	return badRequest(ErrorViews.BadRequest.render("error.validation", original.project));
125	125	}
...	...	@@ -140,7 +140,7 @@
140	140	posting.authorLoginId = original.authorLoginId;
141	141	posting.authorName = original.authorName;
142	142	posting.project = original.project;
143		- updatePosting.run();
	143	+ preUpdateHook.run();
144	144	posting.update();
145	145	posting.updateProperties();
146	146

...	...	@@ -600,28 +600,25 @@
600	600
601	601	private static void addAssigneeChangedNotification(Issue modifiedIssue, Issue originalIssue) {
602	602	if(!originalIssue.assignedUserEquals(modifiedIssue.assignee)) {
603		- Issue updatedIssue = Issue.finder.byId(originalIssue.id);
604	603	User oldAssignee = null;
605	604	if(hasAssignee(originalIssue)) {
606	605	oldAssignee = originalIssue.assignee.user;
607	606	}
608		- NotificationEvent notiEvent = NotificationEvent.afterAssigneeChanged(oldAssignee, updatedIssue);
	607	+ NotificationEvent notiEvent = NotificationEvent.afterAssigneeChanged(oldAssignee, modifiedIssue);
609	608	IssueEvent.addFromNotificationEvent(notiEvent, modifiedIssue, UserApp.currentUser().loginId);
610	609	}
611	610	}
612	611
613	612	private static void addStateChangedNotification(Issue modifiedIssue, Issue originalIssue) {
614	613	if(modifiedIssue.state != originalIssue.state) {
615		- Issue updatedIssue = Issue.finder.byId(originalIssue.id);
616		- NotificationEvent notiEvent = NotificationEvent.afterStateChanged(originalIssue.state, updatedIssue);
	614	+ NotificationEvent notiEvent = NotificationEvent.afterStateChanged(originalIssue.state, modifiedIssue);
617	615	IssueEvent.addFromNotificationEvent(notiEvent, modifiedIssue, UserApp.currentUser().loginId);
618	616	}
619	617	}
620	618
621	619	private static void addBodyChangedNotification(Issue modifiedIssue, Issue originalIssue) {
622	620	if (!modifiedIssue.body.equals(originalIssue.body)) {
623		- Issue updatedIssue = Issue.finder.byId(originalIssue.id);
624		- NotificationEvent notiEvent = NotificationEvent.afterIssueBodyChanged(originalIssue.body, updatedIssue);
	621	+ NotificationEvent notiEvent = NotificationEvent.afterIssueBodyChanged(originalIssue.body, modifiedIssue);
625	622	IssueEvent.addFromNotificationEvent(notiEvent, modifiedIssue, UserApp.currentUser().loginId);
626	623	}
627	624	}
...	...	@@ -660,9 +657,9 @@
660	657
661	658	Call redirectTo = routes.IssueApp.issue(project.owner, project.name, number);
662	659
663		- // updateIssueBeforeSave.run would be called just before this issue is saved.
	660	+ // preUpdateHook.run would be called just before this issue is updated.
664	661	// It updates some properties only for issues, such as assignee or labels, but not for non-issues.
665		- Runnable updateIssueBeforeSave = new Runnable() {
	662	+ Runnable preUpdateHook = new Runnable() {
666	663	@Override
667	664	public void run() {
668	665	// Below addAll() method is needed to avoid the exception, 'Timeout trying to lock table ISSUE'.
...	...	@@ -671,16 +668,14 @@
671	668	issue.voters.addAll(originalIssue.voters);
672	669	issue.comments = originalIssue.comments;
673	670	addLabels(issue, request());
	671	+
	672	+ addAssigneeChangedNotification(issue, originalIssue);
	673	+ addStateChangedNotification(issue, originalIssue);
	674	+ addBodyChangedNotification(issue, originalIssue);
674	675	}
675	676	};
676	677
677		- Result result = editPosting(originalIssue, issue, issueForm, redirectTo, updateIssueBeforeSave);
678		-
679		- addAssigneeChangedNotification(issue, originalIssue);
680		- addStateChangedNotification(issue, originalIssue);
681		- addBodyChangedNotification(issue, originalIssue);
682		-
683		- return result;
	678	+ return editPosting(originalIssue, issue, issueForm, redirectTo, preUpdateHook);
684	679	}
685	680
686	681	/*

...	...	@@ -99,35 +99,35 @@
99	99	</li>
100	100	}
101	101	case (event: IssueEvent) => {
102		- <li class="event" id="event-@event.id">
103		- @defining(User.findByLoginId(event.senderLoginId)) { user =>
104		- @event.eventType match {
105		- case EventType.ISSUE_STATE_CHANGED => {
106		- <span class="state @event.newValue">@Messages("issue.state." + event.newValue)</span> @Html(Messages("issue.event." + event.newValue, linkToUser(user.loginId, user.name)))
107		- }
108		- case EventType.ISSUE_ASSIGNEE_CHANGED => {
109		- <span class="state changed">@Messages("issue.state.assigned")</span>
110		- @Html(Messages(assginedMesssage(event.newValue, user), linkToUser(user.loginId, user.name), linkToUser(event.newValue,User.findByLoginId(event.newValue).name, true)))
111		- }
112		- case EventType.ISSUE_REFERRED_FROM_COMMIT => {
113		- <span class="state changed">@Messages("issue.event.referred.title")</span>
114		- @Html(Messages("issue.event.referred",linkToUser(user.loginId, user.name),linkToCommit(event.newValue)))
115		- }
116		- case EventType.ISSUE_REFERRED_FROM_PULL_REQUEST => {
117		- <span class="state changed">@Messages("issue.event.referred.title")</span>
118		- @defining(PullRequest.findById(Long.valueOf(event.newValue))) { pull =>
119		- @Html(Messages("issue.event.referred",linkToUser(user.loginId, user.name),linkToPullRequest(pull)))
	102	+ @if(event.eventType != EventType.ISSUE_BODY_CHANGED) {
	103	+ <li class="event" id="event-@event.id">
	104	+ @defining(User.findByLoginId(event.senderLoginId)) { user =>
	105	+ @event.eventType match {
	106	+ case EventType.ISSUE_STATE_CHANGED => {
	107	+ <span class="state @event.newValue">@Messages("issue.state." + event.newValue)</span> @Html(Messages("issue.event." + event.newValue, linkToUser(user.loginId, user.name)))
	108	+ }
	109	+ case EventType.ISSUE_ASSIGNEE_CHANGED => {
	110	+ <span class="state changed">@Messages("issue.state.assigned")</span>
	111	+ @Html(Messages(assginedMesssage(event.newValue, user), linkToUser(user.loginId, user.name), linkToUser(event.newValue,User.findByLoginId(event.newValue).name, true)))
	112	+ }
	113	+ case EventType.ISSUE_REFERRED_FROM_COMMIT => {
	114	+ <span class="state changed">@Messages("issue.event.referred.title")</span>
	115	+ @Html(Messages("issue.event.referred",linkToUser(user.loginId, user.name),linkToCommit(event.newValue)))
	116	+ }
	117	+ case EventType.ISSUE_REFERRED_FROM_PULL_REQUEST => {
	118	+ <span class="state changed">@Messages("issue.event.referred.title")</span>
	119	+ @defining(PullRequest.findById(Long.valueOf(event.newValue))) { pull =>
	120	+ @Html(Messages("issue.event.referred",linkToUser(user.loginId, user.name),linkToPullRequest(pull)))
	121	+ }
	122	+ }
	123	+ case _ => {
	124	+ @event.newValue by @linkToUser(user.loginId, user.name)
	125	+ }
120	126	}
121	127	}
122		- case EventType.ISSUE_BODY_CHANGED => {
123		- }
124		- case _ => {
125		- @event.newValue by @linkToUser(user.loginId, user.name)
126		- }
127		- }
	128	+ <span class="date"><a href="#event-@event.id">@utils.TemplateHelper.agoString(JodaDateUtil.ago(event.getDate()))</a></span>
	129	+ </li>
128	130	}
129		- <span class="date"><a href="#event-@event.id">@utils.TemplateHelper.agoString(JodaDateUtil.ago(event.getDate()))</a></span>
130		- </li>
131	131	}
132	132	}
133	133	}

Delete comment