Summary

Refactor SCM tool registration using a registry

Review Request #9724 — Created March 3, 2018 and discarded Feb. 28, 2023, 2:29 p.m.

Information

Owner

jtrang

Repository

Review Board

Branch

master

Bugs

Depends On

Reviewers

Groups

reviewboard, students

People

Description

Instead of registering SCM tools in the database, populate a registry

at startup. This allows for more flexibility for users to register and

unregister external tools.
Things done:

- Rename keys in registry to match the names in SCM tools classes

- Created service.py file with API for registering/unregistering tools

- Created test_registration.py to test service.py

- Renamed Tool model to LegacyTool

- Created new class Tool which is not a model. Instantiated by passing

  in an SCM tool name (string). This class has the same properties as

LegacyTool but gets the SCM tool class from the registry.

- Renamed tool field in Repository model to _legacy_tool and add a

scmtool_name field. Created an evolution for this.

- Created tool property in Repository which does a lazy migration

  away from LegacyTool by checking the value of scmtool_name.

- Replace usages of the legacy tool to use scmtool_name

  E.g. tool = LegacyTool.objects.get(name='Subversion') to

scmtool_name='Subversion'

- Refactor RepositoryForm to use scmtool_name

- Other misc refactoring to pass tests in scmtools, hostingsvcs,

webapi, reviews, and diffviewer

- Add migratescmtools.py command.

- Add SCMToolHook extension and unit tests

Testing Done

Currently passing all tests inscmtools, hostingsvcs, webapi,
reviews, and diffviewer.

Tested migratescmtools.py:
- Add local repos at /admin/db/scmtools/repository/add/
- Since the RepositoryForm uses scmtool_name now, manually
update repos in Django shell to be "pre-migration"
- Check that repo scmtool_name and _legacy_tool fields are as
expected post-migration

Unit tests for SCMToolHook extension.

Issues

Description	From	Last Updated
I'm wondering if this affects RBTools? I tried rbt patch to run the RBTools unit tests but applying the patch …	MA maram	March 18, 2018, 10:09 p.m.
There should be one empty line after the doc string.	RI ridum	March 15, 2018, 8:37 p.m.
There should be one empty line after the doc string.	RI ridum	March 15, 2018, 8:38 p.m.
We need a whitespace between the doc String and the code	RI ridum	March 15, 2018, 8:39 p.m.
There should be one empty line after the doc string.	RI ridum	March 15, 2018, 8:39 p.m.
only one empty line is needed after yield	RI ridum	March 15, 2018, 8:40 p.m.
F401 'pkg_resources' imported but unused	reviewbot	March 3, 2018, 10:49 p.m.
F401 'sys' imported but unused	reviewbot	March 3, 2018, 10:49 p.m.
Since this is a big project the class "command" may be a little ambiguious	RI ridum	March 17, 2018, 5:05 p.m.
There should be one empty line after the doc string.	RI ridum	March 15, 2018, 8:44 p.m.
NOTE: For some reason, the diff captured changes in whitespace which is why there are so many line chaneges in …	JT jtrang	March 11, 2018, 10:27 a.m.
Any specific reason to use " over '?	RO rodgerwa	March 15, 2018, 8:53 p.m.
Is there reason to keep this import here and commented out instead of just removing it?	JC jcorrive	March 15, 2018, 8:45 p.m.
Is this commented line of code here for a purpose?	JC jcorrive	March 15, 2018, 8:45 p.m.
F401 'reviewboard.scmtools.service.unregister_scm_tool' imported but unused	reviewbot	March 24, 2018, 10:04 p.m.
F401 'reviewboard.scmtools.service.register_scm_tool' imported but unused	reviewbot	March 24, 2018, 10:04 p.m.
E501 line too long (80 > 79 characters)	reviewbot	March 24, 2018, 10:05 p.m.
Has this been completed?	JQ jque	March 25, 2018, 11:04 a.m.
I think this should be tool?	JQ jque	March 30, 2018, 5:05 p.m.
Might need Args for name	JQ jque	March 30, 2018, 5:05 p.m.
Can remove blank lines	JQ jque	March 30, 2018, 5:06 p.m.
Spacing seemed fine before change	JQ jque	March 30, 2018, 5:07 p.m.
These two lines should be switched so the imports are in alphabetical order.	JC jcorrive	March 31, 2018, 12:25 p.m.
setUp -> setup	FL floriecai	April 2, 2018, 4:20 p.m.
tearDown -> teardown	FL floriecai	April 2, 2018, 4:20 p.m.
trailing white space	FL floriecai	April 2, 2018, 3:57 p.m.
What is reverse relation here?	FL floriecai	April 2, 2018, 4:13 p.m.
cls is a "reserved" (emphasis on the quotes there) word. Consider changing the name? See @classmethods	rpolyano	April 2, 2018, 4:20 p.m.
See previous comment about cls (especially here used as an argument)	rpolyano	April 2, 2018, 4:20 p.m.
tearDown -> teardown	FL floriecai	April 2, 2018, 4:18 p.m.
setup_class()	FL floriecai	April 2, 2018, 4:18 p.m.
Could you explain what the q & Q does? I saw this elsewhere and was confused by the syntax.	FL floriecai	April 2, 2018, 4:12 p.m.

flake8 passed.

JSHint passed.

Description:: Instead of registering SCM tools in the database, populate a registry
at startup. This allows for more flexibility for users to register and
unregister external tools.

Things done:
+ - Rename keys in registry to match the names in SCM tools classes
- Created service.py file with API for registering/unregistering tools
- Created test_registration.py to test service.py
- Renamed Tool model to LegacyTool
- Created new class Tool which is not a model. Instantiated by passing
in an SCM tool name (string). This class has the same properties as
LegacyTool but gets the SCM tool class from the registry.
- Renamed tool field in Repository model to _legacy_tool and add a
scmtool_name field. Created an evolution for this.
- Created tool property in Repository which does a lazy migration
away from LegacyTool by checking the value of scmtool_name.
- Replace usages of the legacy tool to use scmtool_name
E.g. tool = LegacyTool.objects.get(name='Subversion') to
scmtool_name='Subversion'
- Refactor RepositoryForm to use scmtool_name
- Modify Command class in registerscmtools.py. Loop through legacy
tools and update associated repositories to use scmtool_name
- Other misc refactoring to pass tests in scmtools, hostingsvcs,
webapi, reviews, and diffviewer

Testing Done:: ~
Passing tests ing scmtools, hostingsvcs, webapi, reviews,
~ and diffviewer.
~
Currently passing all tests inscmtools, hostingsvcs, webapi,
~ reviews, and diffviewer.

~
NOTE: any tests requiring SSH/tunnels have been skipped so far.
~
Things NOT tested so far:
+ - Any tests requiring SSH/tunnels have been skipped
+ - No testing of the modified Command class

Description:

		Instead of registering SCM tools in the database, populate a registry
		at startup. This allows for more flexibility for users to register and
		unregister external tools.

		Things done:
		- Rename keys in registry to match the names in SCM tools classes
		- Created service.py file with API for registering/unregistering tools
		- Created test_registration.py to test service.py
		- Renamed `Tool` model to `LegacyTool`
		- Created new class `Tool` which is not a model. Instantiated by passing
		in an SCM tool name (string). This class has the same properties as
		`LegacyTool` but gets the SCM tool class from the registry.
		- Renamed `tool` field in `Repository` model to `_legacy_tool` and add a
		`scmtool_name` field. Created an evolution for this.
		- Created `tool` property in `Repository` which does a lazy migration
		away from `LegacyTool` by checking the value of `scmtool_name`.
		- Replace usages of the legacy `tool` to use `scmtool_name`
		E.g. `tool = LegacyTool.objects.get(name='Subversion')` to
		`scmtool_name='Subversion'`
		- Refactor `RepositoryForm` to use `scmtool_name`
-		- Modify `Command` class in `registerscmtools.py`. Loop through legacy
-		tools and update associated repositories to use `scmtool_name`
		- Other misc refactoring to pass tests in `scmtools`, `hostingsvcs`,
		`webapi`, `reviews`, and `diffviewer`

Testing Done:

		Currently passing all tests in`scmtools`, `hostingsvcs`, `webapi`,
		`reviews`, and `diffviewer`.

		Things NOT tested so far:
~		- Any tests requiring SSH/tunnels have been skipped
	~	- Any tests requiring SSH/tunnels have been skipped
-		- No testing of the modified `Command` class

Commit:

420a2150ff3e0752c15812a963c6782a5a2bf430

3ddc389d1ab4c0ae292f3b5eacc55fe5fdcce8b9

Diff:

Revision 2 (+3067 -2804)

Show changes

	setup.py
	docs/manual/fixtures/initial_data.json
	reviewboard/admin/fixtures/initial_evolution_schema.json
	reviewboard/diffviewer/tests/test_forms.py
	reviewboard/hostingsvcs/tests/test_assembla.py
	reviewboard/hostingsvcs/tests/test_beanstalk.py
	reviewboard/hostingsvcs/tests/test_bitbucket.py
	reviewboard/hostingsvcs/tests/test_codebasehq.py
	33 more

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

Description:

		Instead of registering SCM tools in the database, populate a registry
		at startup. This allows for more flexibility for users to register and
		unregister external tools.

		Things done:
		- Rename keys in registry to match the names in SCM tools classes
		- Created service.py file with API for registering/unregistering tools
		- Created test_registration.py to test service.py
		- Renamed `Tool` model to `LegacyTool`
		- Created new class `Tool` which is not a model. Instantiated by passing
		in an SCM tool name (string). This class has the same properties as
		`LegacyTool` but gets the SCM tool class from the registry.
		- Renamed `tool` field in `Repository` model to `_legacy_tool` and add a
		`scmtool_name` field. Created an evolution for this.
		- Created `tool` property in `Repository` which does a lazy migration
		away from `LegacyTool` by checking the value of `scmtool_name`.
		- Replace usages of the legacy `tool` to use `scmtool_name`
		E.g. `tool = LegacyTool.objects.get(name='Subversion')` to
		`scmtool_name='Subversion'`
		- Refactor `RepositoryForm` to use `scmtool_name`
		- Other misc refactoring to pass tests in `scmtools`, `hostingsvcs`,
~		`webapi`, `reviews`, and `diffviewer`
	~	`webapi`, `reviews`, and `diffviewer`
	+	- [WIP] add `migratescmtools.py` command

Testing Done:

		Currently passing all tests in`scmtools`, `hostingsvcs`, `webapi`,
		`reviews`, and `diffviewer`.

		Things NOT tested so far:
~		- Any tests requiring SSH/tunnels have been skipped
	~	- Any tests requiring SSH/tunnels have been skipped
	+	- No testing done for `migratescmtools.py`

Commit:

3ddc389d1ab4c0ae292f3b5eacc55fe5fdcce8b9

cce2968cdb667f4ad77694bb18fd009728e8c8f1

Diff:

Revision 3 (+3090 -2804)

Show changes

	setup.py
	docs/manual/fixtures/initial_data.json
	reviewboard/admin/fixtures/initial_evolution_schema.json
	reviewboard/diffviewer/tests/test_forms.py
	reviewboard/hostingsvcs/tests/test_assembla.py
	reviewboard/hostingsvcs/tests/test_beanstalk.py
	reviewboard/hostingsvcs/tests/test_bitbucket.py
	reviewboard/hostingsvcs/tests/test_codebasehq.py
	34 more

Checks run (1 failed, 1 succeeded)

flake8 failed.

JSHint passed.

flake8

reviewboard/scmtools/management/commands/migratescmtools.py (Diff revision 3)
The issue has been resolved. Show all issues
```
F401 'pkg_resources' imported but unused
```
reviewboard/scmtools/management/commands/migratescmtools.py (Diff revision 3)
The issue has been resolved. Show all issues
```
F401 'sys' imported but unused
```

Commit:

cce2968cdb667f4ad77694bb18fd009728e8c8f1

6d8d11ecf11698fd243156620d92fc02513950bb

Diff:

Revision 4 (+3086 -2804)

Show changes

	setup.py
	docs/manual/fixtures/initial_data.json
	reviewboard/admin/fixtures/initial_evolution_schema.json
	reviewboard/diffviewer/tests/test_forms.py
	reviewboard/hostingsvcs/tests/test_assembla.py
	reviewboard/hostingsvcs/tests/test_beanstalk.py
	reviewboard/hostingsvcs/tests/test_bitbucket.py
	reviewboard/hostingsvcs/tests/test_codebasehq.py
	34 more

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

Description:

		Instead of registering SCM tools in the database, populate a registry
		at startup. This allows for more flexibility for users to register and
		unregister external tools.

		Things done:
		- Rename keys in registry to match the names in SCM tools classes
		- Created service.py file with API for registering/unregistering tools
		- Created test_registration.py to test service.py
		- Renamed `Tool` model to `LegacyTool`
		- Created new class `Tool` which is not a model. Instantiated by passing
		in an SCM tool name (string). This class has the same properties as
		`LegacyTool` but gets the SCM tool class from the registry.
		- Renamed `tool` field in `Repository` model to `_legacy_tool` and add a
		`scmtool_name` field. Created an evolution for this.
		- Created `tool` property in `Repository` which does a lazy migration
		away from `LegacyTool` by checking the value of `scmtool_name`.
		- Replace usages of the legacy `tool` to use `scmtool_name`
		E.g. `tool = LegacyTool.objects.get(name='Subversion')` to
		`scmtool_name='Subversion'`
		- Refactor `RepositoryForm` to use `scmtool_name`
		- Other misc refactoring to pass tests in `scmtools`, `hostingsvcs`,
		`webapi`, `reviews`, and `diffviewer`
~		- [WIP] add `migratescmtools.py` command
	~	- Add `migratescmtools.py` command.

Testing Done:

		Currently passing all tests in`scmtools`, `hostingsvcs`, `webapi`,
		`reviews`, and `diffviewer`.

	+	Tested `migratescmtools.py`:
	+	- Add local repos at `/admin/db/scmtools/repository/add/`
	+	- Since the `RepositoryForm` uses `scmtool_name` now, manually
	+	update repos in Django shell to be "pre-migration"
	+	- Check that repo `scmtool_name` and `_legacy_tool` fields are as
	+	expected post-migration
	+
		Things NOT tested so far:
~		- Any tests requiring SSH/tunnels have been skipped
	~	- Any tests requiring SSH/tunnels have been skipped
-		- No testing done for `migratescmtools.py`

Commit:

6d8d11ecf11698fd243156620d92fc02513950bb

f27aac219a400786800a94258ae1547891c7a99a

Diff:

Revision 5 (+3099 -2804)

Show changes

	setup.py
	docs/manual/fixtures/initial_data.json
	reviewboard/admin/fixtures/initial_evolution_schema.json
	reviewboard/diffviewer/tests/test_forms.py
	reviewboard/hostingsvcs/tests/test_assembla.py
	reviewboard/hostingsvcs/tests/test_beanstalk.py
	reviewboard/hostingsvcs/tests/test_bitbucket.py
	reviewboard/hostingsvcs/tests/test_codebasehq.py
	34 more

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

docs/manual/fixtures/initial_data.json (Diff revision 5)

The issue has been resolved. Show all issues

NOTE: For some reason, the diff captured changes in whitespace which is why there are so many line chaneges in the diff. I only changed the following lines:

"model": "scmtools.legacytool"

and

"scmtool_name": "Test",

```
Mostly just questions.
```

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

Just curious, why are we using lambda's here?

jtrang March 15, 2018, 9:06 p.m.

Ah, I should add a comment from LegacyTool here: "Templates can't access variables on a class properly. It'll attempt to instantiate the class, which will fail without the necessary parameters. So, we use the lambdas as convenient wrappers to do what the template can't do." The templates are supports_raw_file_urls, supports_ticket_auth, etc, which are set in each SCM tool class.

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

Why don't you use a lambda to assign this like the others? 
IE: Move this with the other instance variables and have the "get_scmtool_class" return the assignment?

jtrang March 15, 2018, 9:06 p.m.

This is related to the comment above. The lambdas are wrappers, but here that is unnecessary.

reviewboard/scmtools/models.py (Diff revision 5)
The issue has been resolved. Show all issues
```
Any specific reason to use " over '?
```
1. JT
  
  jtrang March 15, 2018, 8:53 p.m.
  Thanks! Good catch.

The code is generally detailed and easy to understand; but it will be nice to make sure that there should be a blank line after the last """ and the rest of the module/class body.

reviewboard/webapi/resources/repository.py (Diff revisions 2 - 4)
The issue has been resolved. Show all issues
```
There should be one empty line after the doc string.
```
reviewboard/webapi/resources/repository.py (Diff revisions 2 - 4)
The issue has been resolved. Show all issues
```
There should be one empty line after the doc string.
```
reviewboard/webapi/resources/repository.py (Diff revisions 2 - 4)
The issue has been resolved. Show all issues
```
We need a whitespace between the doc String and the code
```
reviewboard/webapi/resources/repository.py (Diff revisions 2 - 4)
The issue has been resolved. Show all issues
```
There should be one empty line after the doc string.
```
reviewboard/webapi/resources/repository.py (Diff revisions 2 - 4)
The issue has been resolved. Show all issues
```
only one empty line is needed after yield
```

reviewboard/scmtools/management/commands/migratescmtools.py (Diff revisions 4 - 5)

The issue has been resolved. Show all issues

Since this is a big project the class "command" may be a little ambiguious

jtrang March 15, 2018, 8:45 p.m.

That's a good point. I'm copying the pattern for other management commands but will double check if the name should be more specific.

jtrang March 24, 2018, 5:49 p.m.

Confirmed that the name "Command" is required.

reviewboard/scmtools/management/commands/migratescmtools.py (Diff revision 4)
The issue has been resolved. Show all issues
```
There should be one empty line after the doc string.
```

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

The issue has been resolved. Show all issues

Is there reason to keep this import here and commented out instead of just removing it?

reviewboard/scmtools/tests/test_bazaar.py (Diff revision 5)
The issue has been resolved. Show all issues
```
Is this commented line of code here for a purpose?
```

Change Summary:

Add whitespace after docstrings and remove some commented lines.

Commit:

f27aac219a400786800a94258ae1547891c7a99a

d5e9d62ed19a74ae668f0dd252c4aa6841f7d5b7

Diff:

Revision 6 (+3102 -2805)

Show changes

	setup.py
	docs/manual/fixtures/initial_data.json
	reviewboard/admin/fixtures/initial_evolution_schema.json
	reviewboard/diffviewer/tests/test_forms.py
	reviewboard/hostingsvcs/repository.py
	reviewboard/hostingsvcs/tests/test_assembla.py
	reviewboard/hostingsvcs/tests/test_beanstalk.py
	reviewboard/hostingsvcs/tests/test_bitbucket.py
	35 more

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

Commit:

d5e9d62ed19a74ae668f0dd252c4aa6841f7d5b7

f74803be8f01d11c78bc0b23e26f7ffcd62cf07a

Diff:

Revision 7 (+3102 -2805)

Show changes

	setup.py
	docs/manual/fixtures/initial_data.json
	reviewboard/admin/fixtures/initial_evolution_schema.json
	reviewboard/diffviewer/tests/test_forms.py
	reviewboard/hostingsvcs/repository.py
	reviewboard/hostingsvcs/tests/test_assembla.py
	reviewboard/hostingsvcs/tests/test_beanstalk.py
	reviewboard/hostingsvcs/tests/test_bitbucket.py
	35 more

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

Description:

		Instead of registering SCM tools in the database, populate a registry
		at startup. This allows for more flexibility for users to register and
		unregister external tools.

		Things done:
		- Rename keys in registry to match the names in SCM tools classes
		- Created service.py file with API for registering/unregistering tools
		- Created test_registration.py to test service.py
		- Renamed `Tool` model to `LegacyTool`
		- Created new class `Tool` which is not a model. Instantiated by passing
		in an SCM tool name (string). This class has the same properties as
		`LegacyTool` but gets the SCM tool class from the registry.
		- Renamed `tool` field in `Repository` model to `_legacy_tool` and add a
		`scmtool_name` field. Created an evolution for this.
		- Created `tool` property in `Repository` which does a lazy migration
		away from `LegacyTool` by checking the value of `scmtool_name`.
		- Replace usages of the legacy `tool` to use `scmtool_name`
		E.g. `tool = LegacyTool.objects.get(name='Subversion')` to
		`scmtool_name='Subversion'`
		- Refactor `RepositoryForm` to use `scmtool_name`
		- Other misc refactoring to pass tests in `scmtools`, `hostingsvcs`,
		`webapi`, `reviews`, and `diffviewer`
~		- Add `migratescmtools.py` command.
	~	- Add `migratescmtools.py` command.
	+	- Add SCMToolHook extension and unit tests

Commit:

f74803be8f01d11c78bc0b23e26f7ffcd62cf07a

1d2bcb684ac2ee97fc0706f4683e8c4bb4c93da9

Diff:

Revision 8 (+3168 -2806)

Show changes

	setup.py
	docs/manual/fixtures/initial_data.json
	reviewboard/admin/fixtures/initial_evolution_schema.json
	reviewboard/diffviewer/tests/test_forms.py
	reviewboard/extensions/hooks.py
	reviewboard/extensions/tests.py
	reviewboard/hostingsvcs/repository.py
	reviewboard/hostingsvcs/tests/test_assembla.py
	37 more

Checks run (1 failed, 1 succeeded)

flake8 failed.

JSHint passed.

flake8

reviewboard/extensions/tests.py (Diff revision 8)
The issue has been resolved. Show all issues
```
F401 'reviewboard.scmtools.service.unregister_scm_tool' imported but unused
```
reviewboard/extensions/tests.py (Diff revision 8)
The issue has been resolved. Show all issues
```
F401 'reviewboard.scmtools.service.register_scm_tool' imported but unused
```
reviewboard/extensions/tests.py (Diff revision 8)
The issue has been resolved. Show all issues
```
E501 line too long (80 > 79 characters)
```

The issue has been dropped. Show all issues

I'm wondering if this affects RBTools? I tried rbt patch to run the RBTools unit tests but applying the patch failed (Do I need to do DB changes?).

rbtools/clients/__init__.py load_scmclients() is my guess on what might be affected, if anything at all...

jtrang March 18, 2018, 10:06 p.m.

Unfortunately, I haven't worked with the rbtools repository, so I'm unclear on what problem you are encountering. Let me investigate a bit, and I will get back to you.

jtrang March 18, 2018, 10:08 p.m.

(although none of my changes are on master, so I don't think it should be affecting you)

david March 19, 2018, noon

RBTools does have SCM interfaces, but it's a completely separate infrastructure.

Commit:

1d2bcb684ac2ee97fc0706f4683e8c4bb4c93da9

fe8c5b62445ac16a9328008beb06855b9c79a403

Diff:

Revision 9 (+3155 -2807)

Show changes

	setup.py
	docs/manual/fixtures/initial_data.json
	reviewboard/admin/fixtures/initial_evolution_schema.json
	reviewboard/diffviewer/tests/test_forms.py
	reviewboard/extensions/hooks.py
	reviewboard/extensions/tests.py
	reviewboard/hostingsvcs/repository.py
	reviewboard/hostingsvcs/tests/test_assembla.py
	37 more

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

Overall great job! Looks good from what I can tell. Mostly just found docstring and spacing issues.

reviewboard/scmtools/models.py (Diff revision 8)
The issue has been resolved. Show all issues
```
Has this been completed?
```
reviewboard/scmtools/service.py (Diff revision 8)
The issue has been dropped. Show all issues
```
I think this should be tool?
```
1. JT
  
  jtrang March 30, 2018, 5:10 p.m.
  The type refers to the :py:class:SCMTool subclass.
reviewboard/scmtools/service.py (Diff revision 8)
The issue has been resolved. Show all issues
```
Might need Args for name
```
reviewboard/scmtools/tests/test_registration.py (Diff revision 8)
The issue has been resolved. Show all issues
```
Can remove blank lines
```
reviewboard/webapi/tests/test_repository.py (Diff revision 8)
The issue has been resolved. Show all issues
```
Spacing seemed fine before change
```

Commit:

fe8c5b62445ac16a9328008beb06855b9c79a403

a1a10a6855694242afde6c06ae6dfc27dba42b48

Diff:

Revision 10 (+3158 -2807)

Show changes

	setup.py
	docs/manual/fixtures/initial_data.json
	reviewboard/admin/fixtures/initial_evolution_schema.json
	reviewboard/diffviewer/tests/test_forms.py
	reviewboard/extensions/hooks.py
	reviewboard/extensions/tests.py
	reviewboard/hostingsvcs/repository.py
	reviewboard/hostingsvcs/tests/test_assembla.py
	37 more

Checks run (2 timed out)

flake8 timed out.

JSHint timed out.

Testing Done:: Currently passing all tests inscmtools, hostingsvcs, webapi,
reviews, and diffviewer.

Tested migratescmtools.py:
- Add local repos at /admin/db/scmtools/repository/add/
- Since the RepositoryForm uses scmtool_name now, manually
update repos in Django shell to be "pre-migration"
- Check that repo scmtool_name and _legacy_tool fields are as
expected post-migration
-
-
Things NOT tested so far:
- - Any tests requiring SSH/tunnels have been skipped

Testing Done:: Currently passing all tests inscmtools, hostingsvcs, webapi,
reviews, and diffviewer.

Tested migratescmtools.py:
- Add local repos at /admin/db/scmtools/repository/add/
- Since the RepositoryForm uses scmtool_name now, manually
update repos in Django shell to be "pre-migration"
- Check that repo scmtool_name and _legacy_tool fields are as
expected post-migration
+
+
Unit tests for SCMToolHook extension.

reviewboard/extensions/tests.py (Diff revision 10)
The issue has been resolved. Show all issues
```
These two lines should be switched so the imports are in alphabetical order.
```

reviewboard/scmtools/service.py (Diff revision 10)

The issue has been dropped. Show all issues

cls is a "reserved" (emphasis on the quotes there) word. Consider changing the name? See @classmethods

reviewboard/scmtools/service.py (Diff revision 10)
The issue has been dropped. Show all issues
```
See previous comment about cls (especially here used as an argument)
```

reviewboard/extensions/tests.py (Diff revision 10)

The issue has been dropped. Show all issues

setUp -> setup

jtrang April 2, 2018, 4:12 p.m.

Yeah, this is a good point. However I'm following the style of the rest of the tests.py file, where setUp is used as well as test_register. But I will double check on Slack.

reviewboard/extensions/tests.py (Diff revision 10)
The issue has been dropped. Show all issues
```
tearDown -> teardown
```
1. JT
  
  jtrang April 2, 2018, 4:12 p.m.
  Same reply as above.
reviewboard/scmtools/fixtures/test_scmtools.json (Diff revision 10)
The issue has been resolved. Show all issues
```
trailing white space
```

reviewboard/scmtools/management/commands/migratescmtools.py (Diff revision 10)

The issue has been resolved. Show all issues

What is reverse relation here?

jtrang April 2, 2018, 4:12 p.m.

For example, in the Repository model there is a field _legacy_tool, which refers to an entry in the LegacyTool model. The "forward" relationship is to get the legacy tool given the repository (e.g. repository._legacy_tool).

The reverse relation allows you to get all repositories given the legacy tool. To do this, you can set a related_name attribute for the foreign key. E.g. _legacy_tool = models.ForeignKey(LegacyTool, related_name='repositories', ...)

So the reverse relation is getting all repositories given a legacy tool using tool.repositories.all().

reviewboard/scmtools/tests/test_registration.py (Diff revision 10)

The issue has been dropped. Show all issues

tearDown -> teardown

jtrang April 2, 2018, 4:12 p.m.

Similar to tests.py the tests in scmtools/tests use both camel case and underscores, e.g. tearDown and test_registration

jtrang April 2, 2018, 4:20 p.m.

Update: tearDown is defined by the superclass TestCase, and we are overriding it here. Thus the name cannot change.

reviewboard/testing/testcase.py (Diff revision 10)

The issue has been dropped. Show all issues

setup_class()

JT

jtrang April 2, 2018, 4:12 p.m.
```
Similar comment as above.
```

jtrang April 2, 2018, 4:20 p.m.

Update: setUpClass is defined by the superclass TestCase, and we are overriding it here. Thus the name cannot change.

reviewboard/webapi/resources/repository.py (Diff revision 10)

The issue has been resolved. Show all issues

Could you explain what the q & Q does? I saw this elsewhere and was confused by the syntax.

jtrang April 2, 2018, 4:12 p.m.

In this function, we want to build a query based on the attributes which the request has. To build this, we use the Q object. This object encapsulates field lookups.

Higher up in the function, we set q = Q() and then do q & Q(...) to keep building the Q object.

More about QuerySet: https://docs.djangoproject.com/en/2.0/topics/db/queries/
More about Q(): https://docs.djangoproject.com/en/1.7/topics/db/queries/#complex-lookups-with-q

Commit:

a1a10a6855694242afde6c06ae6dfc27dba42b48

dfcb8db05e9bb1330388105ede83545dcfa9982a

Diff:

Revision 11 (+3158 -2807)

Show changes

	setup.py
	docs/manual/fixtures/initial_data.json
	reviewboard/admin/fixtures/initial_evolution_schema.json
	reviewboard/diffviewer/tests/test_forms.py
	reviewboard/extensions/hooks.py
	reviewboard/extensions/tests.py
	reviewboard/hostingsvcs/repository.py
	reviewboard/hostingsvcs/tests/test_assembla.py
	37 more

Checks run (2 timed out)

flake8 timed out.

JSHint timed out.

Status:: Discarded

~		Passing tests ing `scmtools`, `hostingsvcs`, `webapi`, `reviews`,
~		and `diffviewer`.
	~	Currently passing all tests in`scmtools`, `hostingsvcs`, `webapi`,
	~	`reviews`, and `diffviewer`.

~		NOTE: any tests requiring SSH/tunnels have been skipped so far.
	~	Things NOT tested so far:
	+	- Any tests requiring SSH/tunnels have been skipped
	+	- No testing of the modified `Command` class