Summary:

Convert RepositoryTypeCondition to store SCMTool ID instead of Tool PK.

Review Request #12336 — Created June 3, 2022 and submitted June 13, 2022, 3:16 p.m.

Information
Owner:	david
Repository:	Review Board
Branch:	release-5.0.x
Bugs:
Depends On:
Reviewers
Groups:	reviewboard
People:

Description

One of the last places where we were using the old Tool model was the
"Repository Type" condition for integration configurations. This used
the BaseConditionModelMultipleChoice to show a multiple choice field
with the various registered repository types. This meant that the form
field was populated using the contents of the Tool table, and selected
values were stored as the primary keys of the selected Tool instances.

This change makes it so the RepositoryTypeChoice instead loads its
values from the new SCMTools registry, using the SCMTool ID as the key.
In order to handle any existing integration configurations, a new stage
has been added to the upgrade to look through existing integration
configurations and update them.

Testing Done


Created and edited integration configurations using various usage of

  the "Repository Type" choice. Saw that everything was loaded from and

  saved to the database correctly.
Created several integration configurations using the "Repository Type"

  choice and then ran the upgrade. Saw that old lists of integers

  (Tool PKs) were updated to be lists of strings corresponding to the

  correct SCMTool IDs.
Ran unit tests.

Commits

Summary
	Convert RepositoryTypeCondition to store SCMTool ID instead of Tool PK.

Screenshots

Issues

Description	From	Last Updated
Just realized, here and the other data migration change, that scmtool_id can fail if we can't load the SCMTool for …	chipx86	June 10, 2022, 1:04 p.m.
Two things we can do to optimize this: Skip saving if we don't find a condition to update. Call config.save(update_fields=('settings',)) …	chipx86	June 9, 2022, 2:49 p.m.
We should make the lookup conditional, since it's completely possible that we'll fail to find a matching value.	chipx86	June 10, 2022, 1:04 p.m.
line too long (80 > 79 characters) Column: 80 Error code: E501	reviewbot	June 10, 2022, 1:05 p.m.
local variable 'updated_conditions' is assigned to but never used Column: 21 Error code: F841	reviewbot	June 10, 2022, 1:05 p.m.

flake8 passed.

JSHint passed.

reviewboard/upgrade.py (Diff revision 1)

Show all issues

Just realized, here and the other data migration change, that scmtool_id can fail if we can't load the SCMTool for any reason (extension not installed, legacy tool, etc.).

We should check for this and warn with some sort of instructions. I don't know what that'd look like.

david June 9, 2022, 2:49 p.m.

In that case, the upgrade procedure should fail due to an ImproperlyConfigured getting raised from Tool.get_scmtool_class. I'd be pretty shocked if they hit it here but haven't already hit it in many other places.

That said, perhaps I can go through the existing conditions and just ignore any tools which aren't used there?

chipx86 June 10, 2022, 4:28 p.m.

Maybe.

I hit similar sorts of issues when doing SOS development. Ended up with a database containing the SOS SCMTool and repositories using it, but then used it on a new virtualenv without that version of Power Pack. Things blew up.

There's probably a lot that needs to be done here, but it's worth considering as we extend what's in Power Pack.

reviewboard/upgrade.py (Diff revision 1)

Show all issues

Two things we can do to optimize this:

Skip saving if we don't find a condition to update.
Call config.save(update_fields=('settings',))

Also doesn't look like i or enumerate is needed anymore.

reviewboard/upgrade.py (Diff revision 1)

Show all issues

We should make the lookup conditional, since it's completely possible that we'll fail to find a matching value.

Commits:

	Summary
-		Convert RepositoryTypeCondition to store SCMTool ID instead of Tool PK.
+		Convert RepositoryTypeCondition to store SCMTool ID instead of Tool PK.

Diff:

Revision 2 (+220 -100)

Show changes

	reviewboard/upgrade.py
	reviewboard/reviews/tests/test_conditions.py
	reviewboard/scmtools/conditions.py
	reviewboard/scmtools/management/commands/registerscmtools.py
	reviewboard/scmtools/tests/test_conditions.py

Checks run (1 failed, 1 succeeded)

flake8 failed.

JSHint passed.

flake8

reviewboard/upgrade.py (Diff revision 2)

Show all issues

line too long (80 > 79 characters)

Column: 80
Error code: E501

reviewboard/upgrade.py (Diff revision 2)

Show all issues

local variable 'updated_conditions' is assigned to but never used

Column: 21
Error code: F841

Commits:

	Summary
-		Convert RepositoryTypeCondition to store SCMTool ID instead of Tool PK.
+		Convert RepositoryTypeCondition to store SCMTool ID instead of Tool PK.

Diff:

Revision 3 (+218 -100)

Show changes

	reviewboard/upgrade.py
	reviewboard/reviews/tests/test_conditions.py
	reviewboard/scmtools/conditions.py
	reviewboard/scmtools/management/commands/registerscmtools.py
	reviewboard/scmtools/tests/test_conditions.py

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

Ship it!

```
Ship It!
```

Status: Closed (submitted)

Change Summary:

Pushed to release-5.0.x (2c1d4ad)