Summary

Advertise TFS support in Power Pack

Review Request #7758 — Created Nov. 6, 2015 and submitted Nov. 23, 2015, 5:42 p.m.

Information

Owner

brennie

Repository

Review Board

Branch

release-2.5.x

Bugs

Depends On

~~7757~~

Reviewers

Groups

reviewboard

People

Description

Add support for fake SCMTools entries that Review Board does not
provide, but Power Pack does. These tools are not implemented; they
simply appear in the Tool dropdown in the "Add Repository" form when
Power Pack is not enabled.

When a fake tool 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 a tool that
is not actually registered in the database.

This patch also moves all of the tool information serialization into
the RepositoryForm instead of the JS template.

Testing Done

Ran unit tests.

Manually verified the following:

The form cannot be submitted when a fake tool is selected.
The Power Pack advertisement shows when a fake tool is selected.
The fake SCM Tool entries do not appear when Power Pack is installed.

Files

Issues

Description	From	Last Updated
We usually use "scmtool" all as one piece.	david	Nov. 12, 2015, 6:31 p.m.
This doesn't seem like a useful comparison.	david	Nov. 12, 2015, 6:32 p.m.
Can we set the text before showing? $powerPackToolAdvert .find('.power-pack-advert-missing') .text($tool.find(':selected').text()) .end() .show();	david	Nov. 12, 2015, 6:35 p.m.
This seems silly and inefficient. These selectors shouldn't match more than one element, right? Let's just do: $repoPathRow.hide(); $repoMirrorPathRow.hide(); $bugTrackerTypeRow.hide();	david	Nov. 12, 2015, 6:35 p.m.
Same here.	david	Nov. 12, 2015, 6:35 p.m.
Missing period. All comments should be complete sentences.	chipx86	Nov. 21, 2015, 1:01 a.m.
Can we call this last_tool_pk?	david	Nov. 16, 2015, 10:18 a.m.
Missing trailing comma.	chipx86	Nov. 21, 2015, 1:01 a.m.
I don't understand the purpose of this variable? Maybe it needs a better name.	chipx86	Nov. 21, 2015, 1:02 a.m.
We should probably have some sanity checking now that we have things that don't exist in the db.	david	Nov. 16, 2015, 10:21 a.m.
Let's move this to one var statement.	chipx86	Nov. 21, 2015, 1:03 a.m.

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

Ignored Files:
    reviewboard/static/rb/js/repositoryform.js
    reviewboard/templates/admin/repository_fields.js



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

Ignored Files:
    reviewboard/static/rb/js/repositoryform.js
    reviewboard/templates/admin/repository_fields.js

Diff:

Revision 2 (+127 -34)

Show changes

	reviewboard/scmtools/fake.py
	reviewboard/scmtools/forms.py
	reviewboard/static/rb/js/repositoryform.js
	reviewboard/templates/admin/repository_fields.js

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

Ignored Files:
    reviewboard/static/rb/js/repositoryform.js
    reviewboard/templates/admin/repository_fields.js



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

Ignored Files:
    reviewboard/static/rb/js/repositoryform.js
    reviewboard/templates/admin/repository_fields.js

Diff:

Revision 3 (+126 -33)

Show changes

	reviewboard/scmtools/fake.py
	reviewboard/scmtools/forms.py
	reviewboard/static/rb/js/repositoryform.js
	reviewboard/templates/admin/repository_fields.js

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

Ignored Files:
    reviewboard/static/rb/js/repositoryform.js
    reviewboard/templates/admin/repository_fields.js



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

Ignored Files:
    reviewboard/static/rb/js/repositoryform.js
    reviewboard/templates/admin/repository_fields.js

Change Summary:

Refactor & fixup

Diff:

Revision 4 (+127 -33)

Show changes

	reviewboard/scmtools/fake.py
	reviewboard/scmtools/forms.py
	reviewboard/static/rb/js/repositoryform.js
	reviewboard/templates/admin/repository_fields.js

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

Ignored Files:
    reviewboard/static/rb/js/repositoryform.js
    reviewboard/templates/admin/repository_fields.js



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

Ignored Files:
    reviewboard/static/rb/js/repositoryform.js
    reviewboard/templates/admin/repository_fields.js

reviewboard/scmtools/forms.py (Diff revision 4)
The issue has been resolved. Show all issues
```
We usually use "scmtool" all as one piece.
```
reviewboard/static/rb/js/repositoryform.js (Diff revision 4)
The issue has been resolved. Show all issues
```
This doesn't seem like a useful comparison.
```

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

The issue has been resolved. Show all issues

Can we set the text before showing?

$powerPackToolAdvert
    .find('.power-pack-advert-missing')
        .text($tool.find(':selected').text())
    .end()
    .show();

brennie Nov. 12, 2015, 4:40 p.m.

I didn't know about .end(). This is exactly what I was looking for!

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

The issue has been resolved. Show all issues

This seems silly and inefficient. These selectors shouldn't match more than one element, right? Let's just do:

$repoPathRow.hide();
$repoMirrorPathRow.hide();
$bugTrackerTypeRow.hide();

reviewboard/static/rb/js/repositoryform.js (Diff revision 4)
The issue has been resolved. Show all issues
```
Same here.
```

Change Summary:

Address David's issues.
Rebase off /r/7757/

Diff:

Revision 5 (+55 -26)

Show changes

	reviewboard/scmtools/fake.py
	reviewboard/scmtools/forms.py
	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



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

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

reviewboard/scmtools/forms.py (Diff revision 5)
The issue has been resolved. Show all issues
```
Can we call this last_tool_pk?
```

reviewboard/scmtools/forms.py (Diff revision 5)

The issue has been resolved. Show all issues

We should probably have some sanity checking now that we have things that don't exist in the db.

reviewboard/scmtools/forms.py (Diff revision 5)
The issue has been resolved. Show all issues
```
Missing period. All comments should be complete sentences.
```
reviewboard/scmtools/forms.py (Diff revision 5)
The issue has been resolved. Show all issues
```
Missing trailing comma.
```
reviewboard/scmtools/forms.py (Diff revision 5)
The issue has been resolved. Show all issues
```
I don't understand the purpose of this variable? Maybe it needs a better name.
```
reviewboard/templates/admin/repository_fields.js (Diff revision 5)
The issue has been resolved. Show all issues
```
Let's move this to one var statement.
```

Change Summary:

Addressed David and Christian's issues.

Diff:

Revision 6 (+66 -29)

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

Ship it!

```
Ship It!
```

Status: Closed (submitted)

Change Summary:

Pushed to release-2.5.x (6673971)