Summary

Advertise GitHub Enterprise support in Power Pack

Review Request #7757 — Created Nov. 6, 2015 and submitted Nov. 16, 2015, 5:43 p.m.

Information

Owner

brennie

Repository

Review Board

Branch

release-2.5.x

Bugs

Depends On

~~7761~~

Blocks

7758

Reviewers

Groups

reviewboard

People

Description

This patch adds support for fake hosting services that Review Board
does not provide, but Power Pack does. These services do not implement
anything, but are instead shown in the "Add Repository" form when Power
Pack is not enabled.

When a fake hosting service is selected in the form, an advertisement
for Power Pack is shown and the form becomes unsubmittable. This is to
ensure that users don't end up getting an error when trying to use the
hosting service that is not actually registered in the database.

A large portion of this change is updating repositoryform.js to bring
it in line with the rest of our JavaScript by prefixing all jQuery
objects with $.

Testing Done

Ran unit tests.

Manually verified that the fake hosting service appeared only when
Power Pack was not enabled.

Files

Issues

Description	From	Last Updated
Should be "Power Pack". For the text, how about: "To use GitHub Enterprise with Review Board, you need to install …	chipx86	Nov. 16, 2015, 5:41 p.m.
Should be "Power Pack". We should also run all these strings through gettext.	david	Nov. 9, 2015, 2:03 p.m.
We can make this text a bit more friendly. How about: GitHub enterprise integration is available with Power Pack, an …	david	Nov. 9, 2015, 2:03 p.m.
Undo this change, please.	david	Nov. 9, 2015, 2 p.m.
Undo this change.	david	Nov. 9, 2015, 2 p.m.
Undo this (for anonymous functions, (...) is next to the keyword)	david	Nov. 9, 2015, 2:01 p.m.
This is all good but should probably be done in a separate commit.	david	Nov. 9, 2015, 2:03 p.m.
Hmm. Would this make more sense in the django template?	david	Nov. 9, 2015, 2:24 p.m.
"alo" -> "also"	david	Nov. 9, 2015, 2:24 p.m.
This indentation change is wrong, but can you just stick this whole string in gettext instead?	david	Nov. 9, 2015, 2:24 p.m.
s/has/have/	david	Nov. 12, 2015, 6:23 p.m.
!isCustom is redundant.	brennie	Nov. 16, 2015, 10:15 a.m.
This should all be outside of the if.	brennie	Nov. 16, 2015, 10:15 a.m.
Unnecessary once the $toolRow.setVisible call is moved.	brennie	Nov. 16, 2015, 10:15 a.m.

Tool: Pyflakes
Processed Files:
    reviewboard/hostingsvcs/fake.py
    reviewboard/scmtools/forms.py

Ignored Files:
    reviewboard/static/rb/js/repositoryform.js
    reviewboard/static/rb/css/ui/forms.less



Tool: PEP8 Style Checker
Processed Files:
    reviewboard/hostingsvcs/fake.py
    reviewboard/scmtools/forms.py

Ignored Files:
    reviewboard/static/rb/js/repositoryform.js
    reviewboard/static/rb/css/ui/forms.less

Diff:

Revision 2 (+280 -166)

Show changes

	reviewboard/hostingsvcs/fake.py
	reviewboard/scmtools/forms.py
	reviewboard/static/rb/css/ui/forms.less
	reviewboard/static/rb/js/repositoryform.js

Tool: Pyflakes
Processed Files:
    reviewboard/hostingsvcs/fake.py
    reviewboard/scmtools/forms.py

Ignored Files:
    reviewboard/static/rb/js/repositoryform.js
    reviewboard/static/rb/css/ui/forms.less



Tool: PEP8 Style Checker
Processed Files:
    reviewboard/hostingsvcs/fake.py
    reviewboard/scmtools/forms.py

Ignored Files:
    reviewboard/static/rb/js/repositoryform.js
    reviewboard/static/rb/css/ui/forms.less

Branch:: release-2.6.x
release-2.5.x

reviewboard/static/rb/js/repositoryform.js (Diff revision 2)
The issue has been resolved. Show all issues
```
Should be "Power Pack". We should also run all these strings through gettext.
```

reviewboard/static/rb/js/repositoryform.js (Diff revision 2)

The issue has been resolved. Show all issues

We can make this text a bit more friendly. How about:

GitHub enterprise integration is available with Power Pack, an extension which also offers powerful reports, document review, and more.

Also if you keep the link to Beanbag's website, you should fix the URL :)

reviewboard/static/rb/js/repositoryform.js (Diff revision 2)
The issue has been resolved. Show all issues
```
Undo this change, please.
```
reviewboard/static/rb/js/repositoryform.js (Diff revision 2)
The issue has been resolved. Show all issues
```
Undo this change.
```
reviewboard/static/rb/js/repositoryform.js (Diff revision 2)
The issue has been resolved. Show all issues
```
Undo this (for anonymous functions, (...) is next to the keyword)
```

reviewboard/static/rb/js/repositoryform.js (Diff revision 2)

The issue has been resolved. Show all issues

This is all good but should probably be done in a separate commit.

brennie

Nov. 9, 2015, 2:01 p.m.

The formatting changes have been moved to https://reviews.reviewboard.org/r/7761/.

Depends On:

~~7761 - Fix up the formatting of the repository form JS~~

Diff:

Revision 3 (+127 -28)

Show changes

	reviewboard/hostingsvcs/fake.py
	reviewboard/scmtools/forms.py
	reviewboard/static/rb/css/ui/forms.less
	reviewboard/static/rb/js/repositoryform.js

Tool: Pyflakes
Processed Files:
    reviewboard/hostingsvcs/fake.py
    reviewboard/scmtools/forms.py

Ignored Files:
    reviewboard/static/rb/js/repositoryform.js
    reviewboard/static/rb/css/ui/forms.less



Tool: PEP8 Style Checker
Processed Files:
    reviewboard/hostingsvcs/fake.py
    reviewboard/scmtools/forms.py

Ignored Files:
    reviewboard/static/rb/js/repositoryform.js
    reviewboard/static/rb/css/ui/forms.less

Diff:

Revision 4 (+134 -28)

Show changes

	reviewboard/hostingsvcs/fake.py
	reviewboard/scmtools/forms.py
	reviewboard/static/rb/css/ui/forms.less
	reviewboard/static/rb/js/repositoryform.js

Tool: Pyflakes
Processed Files:
    reviewboard/hostingsvcs/fake.py
    reviewboard/scmtools/forms.py

Ignored Files:
    reviewboard/static/rb/js/repositoryform.js
    reviewboard/static/rb/css/ui/forms.less



Tool: PEP8 Style Checker
Processed Files:
    reviewboard/hostingsvcs/fake.py
    reviewboard/scmtools/forms.py

Ignored Files:
    reviewboard/static/rb/js/repositoryform.js
    reviewboard/static/rb/css/ui/forms.less

Diff:

Revision 5 (+134 -28)

Show changes

	reviewboard/hostingsvcs/fake.py
	reviewboard/scmtools/forms.py
	reviewboard/static/rb/css/ui/forms.less
	reviewboard/static/rb/js/repositoryform.js

Tool: Pyflakes
Processed Files:
    reviewboard/hostingsvcs/fake.py
    reviewboard/scmtools/forms.py

Ignored Files:
    reviewboard/static/rb/js/repositoryform.js
    reviewboard/static/rb/css/ui/forms.less



Tool: PEP8 Style Checker
Processed Files:
    reviewboard/hostingsvcs/fake.py
    reviewboard/scmtools/forms.py

Ignored Files:
    reviewboard/static/rb/js/repositoryform.js
    reviewboard/static/rb/css/ui/forms.less

Diff:

Revision 6 (+160 -52)

Show changes

	reviewboard/hostingsvcs/fake.py
	reviewboard/scmtools/forms.py
	reviewboard/static/rb/css/ui/forms.less
	reviewboard/static/rb/js/repositoryform.js

Tool: Pyflakes
Processed Files:
    reviewboard/hostingsvcs/fake.py
    reviewboard/scmtools/forms.py

Ignored Files:
    reviewboard/static/rb/js/repositoryform.js
    reviewboard/static/rb/css/ui/forms.less



Tool: PEP8 Style Checker
Processed Files:
    reviewboard/hostingsvcs/fake.py
    reviewboard/scmtools/forms.py

Ignored Files:
    reviewboard/static/rb/js/repositoryform.js
    reviewboard/static/rb/css/ui/forms.less

reviewboard/static/rb/js/repositoryform.js (Diff revision 6)

The issue has been dropped. Show all issues

Hmm. Would this make more sense in the django template?

reviewboard/static/rb/js/repositoryform.js (Diff revision 6)
The issue has been resolved. Show all issues
```
"alo" -> "also"
```

reviewboard/static/rb/js/repositoryform.js (Diff revision 6)

The issue has been resolved. Show all issues

This indentation change is wrong, but can you just stick this whole string in gettext instead?

Diff:

Revision 7 (+160 -53)

Show changes

	reviewboard/hostingsvcs/fake.py
	reviewboard/scmtools/forms.py
	reviewboard/static/rb/css/ui/forms.less
	reviewboard/static/rb/js/repositoryform.js

Tool: Pyflakes
Processed Files:
    reviewboard/hostingsvcs/fake.py
    reviewboard/scmtools/forms.py

Ignored Files:
    reviewboard/static/rb/js/repositoryform.js
    reviewboard/static/rb/css/ui/forms.less



Tool: PEP8 Style Checker
Processed Files:
    reviewboard/hostingsvcs/fake.py
    reviewboard/scmtools/forms.py

Ignored Files:
    reviewboard/static/rb/js/repositoryform.js
    reviewboard/static/rb/css/ui/forms.less

Change Summary:

Rebase

Diff:

Revision 8 (+157 -50)

Show changes

	reviewboard/hostingsvcs/fake.py
	reviewboard/scmtools/forms.py
	reviewboard/static/rb/css/ui/forms.less
	reviewboard/static/rb/js/repositoryform.js

Tool: Pyflakes
Processed Files:
    reviewboard/hostingsvcs/fake.py
    reviewboard/scmtools/forms.py

Ignored Files:
    reviewboard/static/rb/js/repositoryform.js
    reviewboard/static/rb/css/ui/forms.less



Tool: PEP8 Style Checker
Processed Files:
    reviewboard/hostingsvcs/fake.py
    reviewboard/scmtools/forms.py

Ignored Files:
    reviewboard/static/rb/js/repositoryform.js
    reviewboard/static/rb/css/ui/forms.less

Ship it!

reviewboard/static/rb/js/repositoryform.js (Diff revision 8)
The issue has been resolved. Show all issues
```
s/has/have/
```

Repository form with Power Pack disabled (Revision 1)

The issue has been resolved. Show all issues

Should be "Power Pack".

For the text, how about:

"To use GitHub Enterprise with Review Board, you need to install Power Pack.

Power Pack offers support for GitHub Enterprise, Microsoft Team Foundation Server, PDF document review, reporting, and more."

david

Nov. 12, 2015, 3:23 p.m.

This screenshot is old, and I already suggested similar changes. Look at the text in the code, instead.

brennie

Nov. 12, 2015, 6:30 p.m.

I updated the screenshot, Christian. Let me know if you're fine with the new wording.

Change Summary:

Address David's issue. Updated screenshot.

Diff:

Revision 9 (+157 -50)

Show changes

	reviewboard/hostingsvcs/fake.py
	reviewboard/scmtools/forms.py
	reviewboard/static/rb/css/ui/forms.less
	reviewboard/static/rb/js/repositoryform.js

Added Files:

Tool: Pyflakes
Processed Files:
    reviewboard/hostingsvcs/fake.py
    reviewboard/scmtools/forms.py

Ignored Files:
    reviewboard/static/rb/js/repositoryform.js
    reviewboard/static/rb/css/ui/forms.less



Tool: PEP8 Style Checker
Processed Files:
    reviewboard/hostingsvcs/fake.py
    reviewboard/scmtools/forms.py

Ignored Files:
    reviewboard/static/rb/js/repositoryform.js
    reviewboard/static/rb/css/ui/forms.less

Change Summary:

Refactor based off of David's comments on /r/7758

Diff:

Revision 10 (+155 -50)

Show changes

	reviewboard/hostingsvcs/fake.py
	reviewboard/scmtools/forms.py
	reviewboard/static/rb/css/ui/forms.less
	reviewboard/static/rb/js/repositoryform.js

Tool: Pyflakes
Processed Files:
    reviewboard/hostingsvcs/fake.py
    reviewboard/scmtools/forms.py

Ignored Files:
    reviewboard/static/rb/js/repositoryform.js
    reviewboard/static/rb/css/ui/forms.less



Tool: PEP8 Style Checker
Processed Files:
    reviewboard/hostingsvcs/fake.py
    reviewboard/scmtools/forms.py

Ignored Files:
    reviewboard/static/rb/js/repositoryform.js
    reviewboard/static/rb/css/ui/forms.less

Diff:

Revision 11 (+210 -52)

Show changes

	reviewboard/hostingsvcs/fake.py
	reviewboard/scmtools/forms.py
	reviewboard/static/rb/css/ui/forms.less
	reviewboard/static/rb/js/repositoryform.js

Tool: Pyflakes
Processed Files:
    reviewboard/hostingsvcs/fake.py
    reviewboard/scmtools/forms.py

Ignored Files:
    reviewboard/static/rb/js/repositoryform.js
    reviewboard/static/rb/css/ui/forms.less



Tool: PEP8 Style Checker
Processed Files:
    reviewboard/hostingsvcs/fake.py
    reviewboard/scmtools/forms.py

Ignored Files:
    reviewboard/static/rb/js/repositoryform.js
    reviewboard/static/rb/css/ui/forms.less

Diff:

Revision 12 (+154 -51)

Show changes

	reviewboard/hostingsvcs/fake.py
	reviewboard/scmtools/forms.py
	reviewboard/static/rb/css/ui/forms.less
	reviewboard/static/rb/js/repositoryform.js

Tool: Pyflakes
Processed Files:
    reviewboard/hostingsvcs/fake.py
    reviewboard/scmtools/forms.py

Ignored Files:
    reviewboard/static/rb/js/repositoryform.js
    reviewboard/static/rb/css/ui/forms.less



Tool: PEP8 Style Checker
Processed Files:
    reviewboard/hostingsvcs/fake.py
    reviewboard/scmtools/forms.py

Ignored Files:
    reviewboard/static/rb/js/repositoryform.js
    reviewboard/static/rb/css/ui/forms.less

reviewboard/static/rb/js/repositoryform.js (Diff revision 12)
The issue has been resolved. Show all issues
```
!isCustom is redundant.
```

reviewboard/static/rb/js/repositoryform.js (Diff revision 12)

The issue has been resolved. Show all issues

This should all be outside of the if.

reviewboard/static/rb/js/repositoryform.js (Diff revision 12)
The issue has been resolved. Show all issues
```
Unnecessary once the $toolRow.setVisible call is moved.
```

Change Summary:

Address own issues

Diff:

Revision 13 (+151 -51)

Show changes

	reviewboard/hostingsvcs/fake.py
	reviewboard/scmtools/forms.py
	reviewboard/static/rb/css/ui/forms.less
	reviewboard/static/rb/js/repositoryform.js

Tool: Pyflakes
Processed Files:
    reviewboard/hostingsvcs/fake.py
    reviewboard/scmtools/forms.py

Ignored Files:
    reviewboard/static/rb/js/repositoryform.js
    reviewboard/static/rb/css/ui/forms.less



Tool: PEP8 Style Checker
Processed Files:
    reviewboard/hostingsvcs/fake.py
    reviewboard/scmtools/forms.py

Ignored Files:
    reviewboard/static/rb/js/repositoryform.js
    reviewboard/static/rb/css/ui/forms.less

Change Summary:

Address David's issues

Diff:

Revision 14 (+61 -28)

Show changes

	reviewboard/scmtools/fake.py
	reviewboard/scmtools/forms.py
	reviewboard/templates/admin/repository_fields.js

Tool: Pyflakes
Processed Files:
    reviewboard/scmtools/fake.py
    reviewboard/scmtools/forms.py

Ignored Files:
    reviewboard/templates/admin/repository_fields.js



Tool: PEP8 Style Checker
Processed Files:
    reviewboard/scmtools/fake.py
    reviewboard/scmtools/forms.py

Ignored Files:
    reviewboard/templates/admin/repository_fields.js

Status:: Completed
Change Summary:: Pushed to release-2.5.x (b906997)