Summary

Summary Table Auto-Update

Review Request #2726 — Created Dec. 1, 2011 and submitted Jan. 8, 2012, 9:57 a.m.

Information

Owner

medanat

Repository

Review Board

Branch

Bugs

Depends On

Reviewers

Groups

reviewboard

People

Description

Summary table auto-update

Update the status of the rows within the summary table and update the counters.

Fixes:

Added an anchor selector to the find statement.
Refactored string concatenation.
Fixed comment style.
Removed white space.
Merged with master/origin.
Changed update method to use gCommentIssueManager.
Changed native DOM transformations to jQuery.
Added base value for parseInt. 
Changed jQuery .html() to .text() where relevant.

Testing Done

Ubuntu 10.04 (Linux): Chrome 14.

Screenshots

Summary Table Auto-Update

Issues

Description	From	Last Updated
You're still just calling updateIssueSummaryTable here. I think instead, you should register a callback via the gCommentIssueManager.registerCallback function. You'll need …	mike_conley	Dec. 11, 2011, 7:27 a.m.
Space after these comment slashes	mike_conley	Dec. 17, 2011, 2:21 a.m.
Instead of using innerHTML, we can use jQuery's .html() instead. While we're at it, we should probably through that string …	mike_conley	Dec. 17, 2011, 2:21 a.m.
Same as above	mike_conley	Dec. 17, 2011, 2:21 a.m.
Same as above, re: .html(), and parseInt.	mike_conley	Dec. 17, 2011, 2:21 a.m.
parseInt should take '10' as the second parameter. This is the base it'll convert to. Browsers are fairly tolerant about …	chipx86	Dec. 18, 2011, 2:09 a.m.
I imagine you want to use text(). Only use html() If you actually want to deal with the raw HTML …	chipx86	Dec. 18, 2011, 2:09 a.m.

I committed the parent change. Can you update this? Also, if you can show any screenshots, that'd help.

ME

medanat Dec. 2, 2011, 2:42 a.m.
```
Update r2726 or r2709?
```

Yazan:

Nice work - just one question below.

-Mike

reviewboard/htdocs/media/rb/js/reviews.js (Diff revision 1)

Hm - I sort of figured we'd just register another callback, instead of inserting this new function.

Is there a particular reason you did it this way instead?

medanat Dec. 2, 2011, 1:02 p.m.

I can't say I'm exactly sure what you're asking here. I inserted this function here to avoid javascript racing events. I figured placing in the success clause of the "requestState" function seemed logical, as you would only want to update the table when the new state is set.

Also, I'm not sure what Christian has asked me to update, any ideas?

mike_conley Dec. 2, 2011, 1:10 p.m.

Yazan:

I gotcha.  I remember when I did the issue code, I knew we wanted to have this table somehow, and that it'd need to react to issue status changes, etc.  That's why I threw in the gCommentIssueManager - it allows you to register callbacks to be fired when a comment state changes.  I figured that for each comment with an issue, we'd want to monitor state changes.

That's how the current issues work, anyhow.  See lines 746-747 in reviews.js to see where they register their callback.

If there's a good reason to have this new function, that's totally cool - but if not,  we might as well use the infrastructure that's there.

-Mike

medanat Dec. 2, 2011, 1:40 p.m.

Mike:

I don't fully understand how "callbacks" work. I wanted to capture the older state of the issue before updating. Also, this function updates things that are not handled by the other functions, such as table row classes and counters.

mike_conley Dec. 3, 2011, 2:46 a.m.

Yazan:

So the idea is that we have this global object, gCommentIssueManager, which has functions in it that allow us to register "callback" functions to be called when comments with a particular id are updated.  The callback is a function that takes a single parameter, which is the new state that the comment issue has transitioned to.

Suppose I want some widget to be notified when a comment with an issue, with ID=251 has it's issue state changed.  I would do:

var myCallback = function(aIssueStatus) {
  alert("Hey, sweet, comment with ID = 251 changed it's status to: " + aIssueStatus);
}

gCommentIssueManager.registerCallback(251, myCallback);

If / when the comment issue's state get's updated (like if I'm the reviewee, and I change the comment issue from open to resolved), then myCallback will automatically get called by the gCommentIssueManager.

Does that make more sense?

-Mike

Change Summary:

Merge branch 'master' into summarytableupdate.

Summary:

Summary table auto-update

Summary Table Autoupdate

Description:

	+	Merge branch 'master' into summarytableupdate
	+
	+	Conflicts:
	+	reviewboard/htdocs/media/rb/css/reviews.less
	+	reviewboard/htdocs/media/rb/js/reviews.js
	+	reviewboard/templates/reviews/review_detail.html
	+
	+	Enhance the Issue Summary table.
	+
	+	The Issue Summary table now uncollapses and navigates to the reviews
	+	when the appropriate link is clicked.
	+
	+	The CSS has been converted to use lesscss format.
	+
	+	Reviewed at http://reviews.reviewboard.org/r/2709/
	+
		Summary table auto-update

		Update the status of the rows within the summary table and update the counters.

~		Refactored css/html elements.
~		Added javascript function to update html element classes and counters.
	~	Fixes:
	~
	+	Added an anchor selector to the find statement.
	+	Refactored string concatenation.
	+	Fixed comment style.
	+
	+	Removed white space.
	+
	+
	+
	+	Fixes:
	+
	+	Fixed javascript uncollapse bug and added comment.
	+	Fixed lesscss style.
	+
	+	Changed css to less css. Changed review_details.html to reflect javascript and css changes.
	+
	+
	+
	+	Enhanced Summary Table
	+
	+	Added javascript to uncollapse and navigate to selected reviews.
	+
	+	Merge branch 'release-1.6.x'
	+
	+
	+
	+	Fix the import path for the fallback PIL Image.
	+
	+	It seems easy for PIL to be installed wrong. When it is installed wrong,
	+	you need to insert Image instead of PIL.Image. Our unit tests attempted
	+	this, but failed, as we tried to import Image from Image. Clearly,
	+	nobody has hit this before now, but I have on my new deployment. This is
	+	now fixed.
	+
	+	Fix unit tests with newer versions of Django.
	+
	+	Newer versions of Django broke our unit tests by changing how the
	+	permissions were created. This broke us because we stored the
	+	auto-generated permissions in our test fixtures, but there's very little
	+	reason for us to do that. They've been removed.
	+
	+	There was also some issue with our Mercurial tests. We included the
	+	hg.json fixture before test_scmtools.json, which meant that the
	+	Mercurial tool definition was created after the repository. Django
	+	appears to be more strict now, so we broke. This was silly anyway, so
	+	now the hg.json repository entry has just been moved into
	+	test_scmtools.json.
	+
	+	Merge branch 'release-1.6.x'
	+
	+
	+
	+	Release Review Board 1.6.3.
	+
	+
	+
	+	Merge branch 'release-1.5.x' into release-1.6.x
	+
	+
	+
	+	Release Review Board 1.5.7.
	+
	+
	+
	+	Fix a comment vulnerability allowing scripts to be loaded.
	+
	+	Due to the way that comments were loaded in, it was possible to
	+	terminate a script and inject a new one while loading the diff viewer.
	+	This isn't believed to have been a problem in the wild, but is certainly
	+	an important one to fix.
	+
	+	We now ensure that the text is escaped at the point where it's being fed
	+	into the JavaScript. It's no longer possible to inject scripts.
	+
	+	Thanks to Damian Johnson for the heads up and for the fix that this
	+	change is based on.
	+
	+	This will be going into 1.5.7 and 1.6.3 releases.
	+
	+	Fix the location that reviewboard.db is stored in when running prepare-dev.

~		This commit also includes changes from r2709 (for some reason).
	~	prepare-dev called into rb-site's sync_database, which changed to the
	+	'reviewboard' directory before running syncdb. This meant that the
	+	initial database would appear in the wrong place. Now we explicitly
	+	force it to be the correct directory before calling syncdb.

Diff:

Revision 2 (+207 -50)

Show changes

	AUTHORS
	contrib/internal/prepare-dev.py
	docs/releasenotes/reviewboard/1.5.7.txt
	docs/releasenotes/reviewboard/1.6.3.txt
	docs/releasenotes/reviewboard/index.txt
	reviewboard/test.py
	reviewboard/accounts/fixtures/test_users.json
	reviewboard/htdocs/media/rb/css/defs.less
	9 more

Change Summary:

Removed diff trash

Summary:

Summary Table Autoupdate

Summary Table Auto-Update

Description:

	+	Cleaned diff trash
	+
	+
	+
		Merge branch 'master' into summarytableupdate

		Conflicts:
		reviewboard/htdocs/media/rb/css/reviews.less
		reviewboard/htdocs/media/rb/js/reviews.js
		reviewboard/templates/reviews/review_detail.html

		Enhance the Issue Summary table.

		The Issue Summary table now uncollapses and navigates to the reviews
		when the appropriate link is clicked.

		The CSS has been converted to use lesscss format.

		Reviewed at http://reviews.reviewboard.org/r/2709/

		Summary table auto-update

		Update the status of the rows within the summary table and update the counters.

		Fixes:

		Added an anchor selector to the find statement.
		Refactored string concatenation.
		Fixed comment style.

		Removed white space.



		Fixes:

		Fixed javascript uncollapse bug and added comment.
		Fixed lesscss style.

		Changed css to less css. Changed review_details.html to reflect javascript and css changes.



		Enhanced Summary Table

		Added javascript to uncollapse and navigate to selected reviews.

		Merge branch 'release-1.6.x'



		Fix the import path for the fallback PIL Image.

		It seems easy for PIL to be installed wrong. When it is installed wrong,
		you need to insert Image instead of PIL.Image. Our unit tests attempted
		this, but failed, as we tried to import Image from Image. Clearly,
		nobody has hit this before now, but I have on my new deployment. This is
		now fixed.

		Fix unit tests with newer versions of Django.

		Newer versions of Django broke our unit tests by changing how the
		permissions were created. This broke us because we stored the
		auto-generated permissions in our test fixtures, but there's very little
		reason for us to do that. They've been removed.

		There was also some issue with our Mercurial tests. We included the
		hg.json fixture before test_scmtools.json, which meant that the
		Mercurial tool definition was created after the repository. Django
		appears to be more strict now, so we broke. This was silly anyway, so
		now the hg.json repository entry has just been moved into
		test_scmtools.json.

		Merge branch 'release-1.6.x'



		Release Review Board 1.6.3.



		Merge branch 'release-1.5.x' into release-1.6.x



		Release Review Board 1.5.7.



		Fix a comment vulnerability allowing scripts to be loaded.

		Due to the way that comments were loaded in, it was possible to
		terminate a script and inject a new one while loading the diff viewer.
		This isn't believed to have been a problem in the wild, but is certainly
		an important one to fix.

		We now ensure that the text is escaped at the point where it's being fed
		into the JavaScript. It's no longer possible to inject scripts.

		Thanks to Damian Johnson for the heads up and for the fix that this
		change is based on.

		This will be going into 1.5.7 and 1.6.3 releases.

		Fix the location that reviewboard.db is stored in when running prepare-dev.

		prepare-dev called into rb-site's sync_database, which changed to the
		'reviewboard' directory before running syncdb. This meant that the
		initial database would appear in the wrong place. Now we explicitly
		force it to be the correct directory before calling syncdb.

Diff:

Revision 3 (+199 -51)

Show changes

	AUTHORS
	contrib/internal/prepare-dev.py
	docs/releasenotes/reviewboard/1.5.7.txt
	docs/releasenotes/reviewboard/1.6.3.txt
	docs/releasenotes/reviewboard/index.txt
	reviewboard/test.py
	reviewboard/accounts/fixtures/test_users.json
	reviewboard/htdocs/media/rb/css/defs.less
	9 more

Hey Yazan:

Hm, there appears to be some extra stuff in this diff that I don't think you added...

Secondly, while you're a step closer to what I was asking for, I was hoping you would use the built in callback functionality of the gCommentIssueManager instead of hard-wiring your updateIssueSummaryTable function call.  Email me if you need any further assistance on that.

Thanks,

-Mike

reviewboard/htdocs/media/rb/js/reviews.js (Diff revision 3)

The issue has been resolved. Show all issues

You're still just calling updateIssueSummaryTable here.

I think instead, you should register a callback via the gCommentIssueManager.registerCallback function.

You'll need to expand the notifyCallbacks helper function on line 162 to pass not only the new issue status, but the old one as well.

medanat Dec. 6, 2011, 11:47 a.m.

I'm only trying to merge master into my branch for now, ran into a little trouble there. Talking to Christian on IRC on fixing my merge. The refactoring and will happen after I merge. I have not changed my code, at all, yet.

Change Summary:

Merged with master

Summary:

Summary Table Auto-Update

Issue Summary table auto update.

Diff:

Revision 4 (+199 -51)

Show changes

	AUTHORS
	contrib/internal/prepare-dev.py
	docs/releasenotes/reviewboard/1.5.7.txt
	docs/releasenotes/reviewboard/1.6.3.txt
	docs/releasenotes/reviewboard/index.txt
	reviewboard/test.py
	reviewboard/accounts/fixtures/test_users.json
	reviewboard/htdocs/media/rb/css/defs.less
	9 more

Yazan:

Hrm, there still seems to be some fragments of non-related changes in this diff...

-Mike

medanat Dec. 10, 2011, 10:42 a.m.

I can't get rid of those for some reason, any suggestions?

Change Summary:

Merged with master/origin

Summary:

Issue Summary table auto update.

Summary Table Auto-Update

Description:

-		Cleaned diff trash
-
-
-
-		Merge branch 'master' into summarytableupdate
-
-		Conflicts:
-		reviewboard/htdocs/media/rb/css/reviews.less
-		reviewboard/htdocs/media/rb/js/reviews.js
-		reviewboard/templates/reviews/review_detail.html
-
-		Enhance the Issue Summary table.
-
-		The Issue Summary table now uncollapses and navigates to the reviews
-		when the appropriate link is clicked.
-
-		The CSS has been converted to use lesscss format.
-
-		Reviewed at http://reviews.reviewboard.org/r/2709/
-
		Summary table auto-update

		Update the status of the rows within the summary table and update the counters.
-
-		Fixes:
-
-		Added an anchor selector to the find statement.
-		Refactored string concatenation.
-		Fixed comment style.
-
-		Removed white space.
-
-
-
-		Fixes:
-
-		Fixed javascript uncollapse bug and added comment.
-		Fixed lesscss style.
-
-		Changed css to less css. Changed review_details.html to reflect javascript and css changes.
-
-
-
-		Enhanced Summary Table
-
-		Added javascript to uncollapse and navigate to selected reviews.
-
-		Merge branch 'release-1.6.x'
-
-
-
-		Fix the import path for the fallback PIL Image.
-
-		It seems easy for PIL to be installed wrong. When it is installed wrong,
-		you need to insert Image instead of PIL.Image. Our unit tests attempted
-		this, but failed, as we tried to import Image from Image. Clearly,
-		nobody has hit this before now, but I have on my new deployment. This is
-		now fixed.
-
-		Fix unit tests with newer versions of Django.
-
-		Newer versions of Django broke our unit tests by changing how the
-		permissions were created. This broke us because we stored the
-		auto-generated permissions in our test fixtures, but there's very little
-		reason for us to do that. They've been removed.
-
-		There was also some issue with our Mercurial tests. We included the
-		hg.json fixture before test_scmtools.json, which meant that the
-		Mercurial tool definition was created after the repository. Django
-		appears to be more strict now, so we broke. This was silly anyway, so
-		now the hg.json repository entry has just been moved into
-		test_scmtools.json.
-
-		Merge branch 'release-1.6.x'
-
-
-
-		Release Review Board 1.6.3.
-
-
-
-		Merge branch 'release-1.5.x' into release-1.6.x
-
-
-
-		Release Review Board 1.5.7.
-
-
-
-		Fix a comment vulnerability allowing scripts to be loaded.
-
-		Due to the way that comments were loaded in, it was possible to
-		terminate a script and inject a new one while loading the diff viewer.
-		This isn't believed to have been a problem in the wild, but is certainly
-		an important one to fix.
-
-		We now ensure that the text is escaped at the point where it's being fed
-		into the JavaScript. It's no longer possible to inject scripts.
-
-		Thanks to Damian Johnson for the heads up and for the fix that this
-		change is based on.
-
-		This will be going into 1.5.7 and 1.6.3 releases.
-
-		Fix the location that reviewboard.db is stored in when running prepare-dev.
-
-		prepare-dev called into rb-site's sync_database, which changed to the
-		'reviewboard' directory before running syncdb. This meant that the
-		initial database would appear in the wrong place. Now we explicitly
-		force it to be the correct directory before calling syncdb.

Diff:

Revision 5 (+39 -8)

Show changes

	reviewboard/htdocs/media/rb/css/reviews.less
	reviewboard/htdocs/media/rb/js/reviews.js
	reviewboard/templates/reviews/review_detail.html

Change Summary:

Changed update method to use gCommentIssueManager.

Description:

		Summary table auto-update

		Update the status of the rows within the summary table and update the counters.
	+
	+	Fixes:
	+
	+	Added an anchor selector to the find statement.
	+	Refactored string concatenation.
	+	Fixed comment style.
	+	Removed white space.
	+	Merged with master/origin.
	+	Changed update method to use gCommentIssueManager.

Diff:

Revision 6 (+46 -11)

Show changes

	reviewboard/htdocs/media/rb/css/reviews.less
	reviewboard/htdocs/media/rb/js/reviews.js
	reviewboard/templates/reviews/review_detail.html

Yazan:

Looking good, and real close.  Just a couple of smaller nits I found on this pass.

Thanks for your work!

-Mike

reviewboard/htdocs/media/rb/js/reviews.js (Diff revision 6)
The issue has been resolved. Show all issues
```
Space after these comment slashes
```

reviewboard/htdocs/media/rb/js/reviews.js (Diff revision 6)

The issue has been resolved. Show all issues

Instead of using innerHTML, we can use jQuery's .html() instead.

While we're at it, we should probably through that string through parseInt - that way, if for some reason the contents of the counter are NOT numeric, parseInt will give us back something we can still decrement.  It might not be the correct number, but it won't halt the script.

reviewboard/htdocs/media/rb/js/reviews.js (Diff revision 6)
The issue has been resolved. Show all issues
```
Same as above
```
reviewboard/htdocs/media/rb/js/reviews.js (Diff revision 6)
The issue has been resolved. Show all issues
```
Same as above, re: .html(), and parseInt.
```

reviewboard/templates/reviews/review_detail.html (Diff revision 6)

We might want to replace these &nbsp;'s with some CSS styling instead...

medanat Dec. 17, 2011, 2:23 a.m.

I added a new class "counter" and played around with the margins to compensate for the &nbsp;'s.

Change Summary:

Changed native DOM transformations to jQuery.

Description:

		Summary table auto-update

		Update the status of the rows within the summary table and update the counters.

		Fixes:

		Added an anchor selector to the find statement.
		Refactored string concatenation.
		Fixed comment style.
		Removed white space.
		Merged with master/origin.
~		Changed update method to use gCommentIssueManager.
	~	Changed update method to use gCommentIssueManager.
	+	Changed native DOM transformations to jQuery.

Diff:

Revision 7 (+51 -11)

Show changes

	reviewboard/htdocs/media/rb/css/reviews.less
	reviewboard/htdocs/media/rb/js/reviews.js
	reviewboard/templates/reviews/review_detail.html

Screenshots:

Summary Table Auto-Update

Two last small things. Looking good though!

reviewboard/htdocs/media/rb/js/reviews.js (Diff revision 7)

The issue has been resolved. Show all issues

parseInt should take '10' as the second parameter. This is the base it'll convert to. Browsers are fairly tolerant about leaving it out, but it should be there.

reviewboard/htdocs/media/rb/js/reviews.js (Diff revision 7)

The issue has been resolved. Show all issues

I imagine you want to use text(). Only use html() If you actually want to deal with the raw HTML code (setting or getting).

Change Summary:

Added base value for parseInt. 
Changed jQuery .html() to .text() where relevant.

Description:

		Summary table auto-update

		Update the status of the rows within the summary table and update the counters.

		Fixes:

		Added an anchor selector to the find statement.
		Refactored string concatenation.
		Fixed comment style.
		Removed white space.
		Merged with master/origin.
		Changed update method to use gCommentIssueManager.
~		Changed native DOM transformations to jQuery.
	~	Changed native DOM transformations to jQuery.
	+	Added base value for parseInt.
	+	Changed jQuery .html() to .text() where relevant.

Diff:

Revision 8 (+51 -11)

Show changes

	reviewboard/htdocs/media/rb/css/reviews.less
	reviewboard/htdocs/media/rb/js/reviews.js
	reviewboard/templates/reviews/review_detail.html

Ship it!

```
Ship It!
```

Status:: Completed
Change Summary:: Pushed to master (56b812e). Thanks!