Summary

User is able to specify their own hub in extension configurations.

Review Request #6102 — Created July 13, 2014 and submitted Sept. 8, 2014, 3:54 p.m.

Information

Owner

PeterTran

Repository

rb-extension-pack

Branch

Bugs

Depends On

Blocks

6194

Commit

9ff944c...

Reviewers

Groups

reviewboard

People

Description

User is able to specify their own hub in extension configurations.

Testing Done

A hub server was run locally. Its URL was used in the extension configuration. The local server was used successfully to support the extension.

To test the hub used is actually the one indicated:
Launch the chat app while the local server is not on. The chat should still launch but using the "add friend" link on another browser should not connect the two clients.

To test the default hub is used when no hub is specified, remove any input in the extension configuration screen. Launch the chat app and using another browser connect to the first client with the "Add friend" link. The clients should be able to see each other's mouse movements.

Issues

Description	From	Last Updated
Col: 1 E302 expected 2 blank lines, found 1	reviewbot	July 13, 2014, 9:24 a.m.
Col: 1 E302 expected 2 blank lines, found 1	reviewbot	July 13, 2014, 9:48 a.m.
This isn't used anymore (since 'urlpatterns' is redefined below).	david	July 19, 2014, 10:47 a.m.
Col: 5 E128 continuation line under-indented for visual indent	reviewbot	July 19, 2014, 11:03 a.m.
Col: 2 W292 no newline at end of file	reviewbot	July 19, 2014, 10:45 a.m.
'settings' imported but unused	reviewbot	July 19, 2014, 10:46 a.m.
'patterns' imported but unused	reviewbot	July 19, 2014, 10:46 a.m.
'include' imported but unused	reviewbot	July 19, 2014, 10:46 a.m.
'HeaderDropdownActionHook' imported but unused	reviewbot	July 19, 2014, 10:46 a.m.
This should probably be "TogetherJS", and wrapped in _()	david	July 19, 2014, 11:05 a.m.
This doesn't seem to be used.	david	July 19, 2014, 11:05 a.m.
None of this is used (you're using the built-in configure view now)	david	July 19, 2014, 11:06 a.m.
Col: 1 W191 indentation contains tabs	reviewbot	July 19, 2014, 1:24 p.m.
Col: 2 E126 continuation line over-indented for hanging indent	reviewbot	July 19, 2014, 1:24 p.m.
Col: 1 E101 indentation contains mixed spaces and tabs	reviewbot	July 19, 2014, 1:24 p.m.
Col: 5 E131 continuation line unaligned for hanging indent	reviewbot	July 19, 2014, 1:25 p.m.
All of this could go on the same line.	david	July 19, 2014, 1:42 p.m.
I'm thinking we should probably bundle the togetherjs script and include it in the js bundle. Then we can get …	david	July 27, 2014, 10:46 a.m.
This doesn't seem like a very good label.	david	July 19, 2014, 1:43 p.m.
This should probably be kept inside the TogetherJS.Extension class.	david	July 19, 2014, 1:45 p.m.
Should be 4-space indent.	david	July 19, 2014, 1:46 p.m.
Doesn't this shadow the TogetherJS function?	david	July 19, 2014, 1:49 p.m.
Is this variable used inside TogetherJS's code? Does it really need to be re-defined here?	david	July 19, 2014, 1:53 p.m.
Variable definitions should be the first line in the function. The superclass method should also be called with any arguments: …	david	July 19, 2014, 1:54 p.m.
Col: 80 E501 line too long (89 > 79 characters)	reviewbot	July 27, 2014, 9:06 a.m.
Col: 9 E128 continuation line under-indented for visual indent	reviewbot	July 27, 2014, 9:12 a.m.
Col: 80 E501 line too long (89 > 79 characters)	reviewbot	July 27, 2014, 9:12 a.m.
Col: 9 E128 continuation line under-indented for visual indent	reviewbot	July 27, 2014, 9:12 a.m.
Col: 2 E128 continuation line under-indented for visual indent	reviewbot	July 27, 2014, 9:49 a.m.
Col: 1 W191 indentation contains tabs	reviewbot	July 27, 2014, 9:49 a.m.
Col: 1 E101 indentation contains mixed spaces and tabs	reviewbot	July 27, 2014, 9:27 a.m.
Col: 9 E122 continuation line missing indentation or outdented	reviewbot	July 27, 2014, 9:49 a.m.
Col: 25 E127 continuation line over-indented for visual indent	reviewbot	July 27, 2014, 10:01 a.m.
This should be: urlpatterns = patterns( '', url(r'^$', 'reviewboard.....', { ... }) ) That will fix your ReviewBot problem. When …	chipx86	July 29, 2014, 6 p.m.
Col: 21 E128 continuation line under-indented for visual indent	reviewbot	July 29, 2014, 5:48 p.m.
Col: 21 E128 continuation line under-indented for visual indent	reviewbot	July 29, 2014, 5:48 p.m.
This, and lines 4-8 seem redundant after what's going on in initialize.	mike_conley	July 27, 2014, 12:06 p.m.
Col: 21 E128 continuation line under-indented for visual indent	reviewbot	July 29, 2014, 5:48 p.m.
Col: 5 E124 closing bracket does not match visual indentation	reviewbot	July 29, 2014, 6:08 p.m.
This can be deleted now, right?	david	Aug. 2, 2014, 12:53 p.m.
Add a trailing comma here.	david	Aug. 2, 2014, 12:53 p.m.
'TemplateHook' imported but unused	reviewbot	Aug. 2, 2014, 1:07 p.m.
We shouldn't make this a requirement for people to get up and running. We should make it default to the …	mike_conley	Aug. 7, 2014, 6:36 p.m.
"Copy or write down" -> "Make note of".	mike_conley	Aug. 7, 2014, 6:36 p.m.
Trailing whitespace.	david	Aug. 7, 2014, 5:19 p.m.
This file was in a different change. Can you pull the latest master and rebase your branch?	david	Aug. 7, 2014, 5:19 p.m.
Instead of defining these up here, just define them when they're first assigned.	mike_conley	Aug. 7, 2014, 5:40 p.m.
What happens if hub_url is not defined? The reason I ask, is that we might want to make it easy …	mike_conley	Aug. 7, 2014, 6:36 p.m.
This file shouldn't be called "*-min.js" because it's not minified.	david	Aug. 7, 2014, 5:23 p.m.
Nit - instead of "where TogetherJS is listed", it'd be "where ReviewTogether is listed", or whatever is actually listed in …	mike_conley	Aug. 11, 2014, 7:01 p.m.
You might want to double-check with one of the other mentors that this style of breaking lines is legit. Looks …	mike_conley	Aug. 11, 2014, 8:12 p.m.
reviewboard -> Review Board	mike_conley	Aug. 11, 2014, 7:03 p.m.
let's break the list over two lines: 'source_filenames': ['js/togetherjs.js', 'js/review-together.js',]	mike_conley	Aug. 11, 2014, 7:03 p.m.
ReviewBoard -> Review Board.	mike_conley	Aug. 11, 2014, 7:03 p.m.
I don't think this comment adds much - can be safely removed.	mike_conley	Aug. 11, 2014, 7:03 p.m.
This change should have already landed as part of your change from /r/6066... Are you sure you have an up-to-date …	mike_conley	Aug. 11, 2014, 7:10 p.m.
Like I said in IRC, let's just do: if (settings.hub_url) { // Set the TogetherJSConfig_hubBase variable }	mike_conley	Aug. 11, 2014, 7:04 p.m.
Ok, now I'm really confused... This patch is the base of /r/6194 - I get that... But what is this …	mike_conley	Aug. 12, 2014, 8:15 p.m.
'JSExtension' imported but unused	reviewbot	Aug. 12, 2014, 8:21 p.m.
undefined name 'ReviewTogetherJSExtension'	reviewbot	Aug. 12, 2014, 8:24 p.m.

Tool: PEP8 Style Checker
Processed Files:
    review_together/review_together/forms.py
    review_together/review_together/views.py

Ignored Files:
    review_together/review_together/templates/review_together/configure.html



Tool: Pyflakes
Processed Files:
    review_together/review_together/forms.py
    review_together/review_together/views.py

Ignored Files:
    review_together/review_together/templates/review_together/configure.html

review_together/review_together/forms.py (Diff revision 1)
The issue has been resolved. Show all issues
```
Col: 1
 E302 expected 2 blank lines, found 1
```
review_together/review_together/views.py (Diff revision 1)
The issue has been resolved. Show all issues
```
Col: 1
 E302 expected 2 blank lines, found 1
```

Commit:

c6d120f32e65a77ab4df8d79a5c3cef989ac2b81

4176fc2f2ea27d34768c4895bb698e9a58048ec3

Diff:

	review_together/review_together/forms.py
	review_together/review_together/views.py
	review_together/review_together/templates/review_together/configure.html

	review_together/review_together/admin_urls.py
	review_together/review_together/extension.py
	review_together/review_together/forms.py
	review_together/review_together/views.py
	review_together/review_together/templates/review_together/configure.html

	review_together/README.md
	review_together/review_together/admin_urls.py
	review_together/review_together/extension.py
	review_together/review_together/forms.py
	review_together/review_together/static/css/review-together.less
	review_together/review_together/static/js/together.js
	review_together/review_together/static/js/togetherjs-min.js
	review_together/review_together/templates/review_together/base.html

		A hub server was run locally. Its URL was used in the extension configuration. The local server was used successfully to support the extension.
	+
	+	To test the hub used is actually the one indicated:
	+	Launch the chat app while the local server is not on. The chat should still launch but using the "add friend" link on another browser should not connect the two clients.
	+
	+	To test the default hub is used when no hub is specified, remove any input in the extension configuration screen. Launch the chat app and using another browser connect to the first client with the "Add friend" link. The clients should be able to see each other's mouse movements.