Resolve #324: Prevent duplicate org and project names

[seav]

Proposed changes in this pull request

• Resolve Prevent duplicate organization names and duplicate project names within the same organization #324:
  • Add unique=True to the name field of the Organization model
  • Add a unique_together constraint for the organization and name fields of the Project model
  • Generate the migration script for the above data model changes, and then modify the script to add two RunPython operations to deduplicate existing data, if any, in the database, one for organization names and the other for project names. The deduplication is based on the SlugModel algorithm. This migration script was manually tested by running it against the development environment database with the following test data:
    • Two organizations with the same name; expected change is that one of the organizations will have " 1" appended to its name
    • Two projects in the same organization with the same name; expected change is that one of the projects will have " 1" appended to its name
    • Two projects in different organizations with the same name; no change is expected
  • Add validation to check for unique project names in the ProjectAddDetails and ProjectEditDetails forms. This explicit validation is needed because the wizard nature of these forms prevents the unique_together validation from working resulting to IntegrityError exceptions
  • Add a similar validation to ProjectSerializer and suppress the automatic unique_together validation. This explicit validation is needed in place of the implicit validation because the implicit validation does not work due to the organization field being read_only in the serializer which conflicts with the requirement for implicit unique_together validation that the unique_together fields are required=True
  • Add unit tests for the following test cases:
    • OrganizationForm test for adding a new organization with a duplicate name
    • ProjectAddDetails test for adding a new project with a duplicate name from the same organization
    • ProjectAddDetails test for adding a new project with a duplicate name from a different organization
    • ProjectEditDetails test for updating a project with a duplicate name from the same organization
    • OrganizationSerializer test for adding a new organization with a duplicate name
    • OrganizationSerializer test for updating an organization with a duplicate name
    • ProjectSerializer test for adding a new project with a duplicate name from the same organization
    • ProjectSerializer test for adding a new project with a duplicate name from a different organization
    • ProjectSerializer test for updating a project with a duplicate name from the same organization
  • Disable the existing OrganizationForm unit test for testing that the organization slug becomes unique because organization names are now required to be unique which prevents the slug deduplication code from actually working
  • Update the existing add duplicate project name functional tests to test the case where the duplicate name is from the same organization and where the duplicate name is from a different organization
• Improve the restricted organization and project names ("Add" and "New") functionality:
  • The original idea with this restriction is to prevent "add" and "new" slug values. So the restriction was only applied during organization and project creation when the slug is assigned. So users can actually rename existing organizations and projects to "Add" or "New" even though we say to the user that these names are not allowed. This "hidden" feature I think is undesired.
  • Modify the existing name validations in OrganizationForm, OrganizationSerializer, and ProjectSerializer to also check during organization or project update
  • Add validation to ProjectEditDetails
  • Add unit tests for the following test cases:
    • OrganizationForm test for updating an organization with a restricted name
    • ProjectEditDetails test for updating a project with a restricted name
    • OrganizationSerializer test for updating an organization with a restricted name
    • ProjectSerializer test for updating a project with a restricted name

When should this PR be merged

• I suggest that somebody confirm the data migration manual testing described above by doing it prior to merging because this manual testing cannot be automatically done (like in Travis)

Risks

• Hopefully, there should be no production database corruption happening as a result of this update

Follow up actions

• When Sprint 7 is deployed to production, the database migration procedure should be performed as a rehearsal of our procedures even though the production database does not contain any duplicate names as of now
• Note to developers: Django migrate action is needed when you update your local repository

[ian-ross]

This looks ​_almost_ perfect, but I did find one funny thing. In the project add wizard, you test for project name uniqueness in the second ("add details") step of the wizard. What happens if one user gets to that step in the wizard, enters a unique project name, then goes off and has a cup of tea before finishing up the wizard, while another user creates a project with the same name? In this case, the second user's project creation goes through without problems, but when the first user comes to finally finish the project add wizard, they just get dropped back into the first step of the wizard. There's no visible error message (although I'm assuming an exception got thrown somewhere).

The best solution here would probably be to take the user back to step 2 (with an explanatory message), while preserving any choices they'd already made in step 3. So, in this situation, the user would see:

1. STEP 1: Select geometry.
2. STEP 2: Select details (including project name).
3. STEP 3: Set up permissions, then click the "Create" button.
4. STEP 2 again: Error message saying something like "Someone stole your project name!", and perhaps a pre-filled deduplicated name in the name field.
5. STEP 3 again: Set up permissions, with the initial choices on the page being the choices already selected in Initial front-end setup and user authentication flows #3.

If this is easy to do, this seems like the best approach. If not, and you can think of an easier way (you suggested just deduplicating the name and warning the user, which seems like it might be OK too), then do that, since this is a corner case that may never appear.

[seav]

The suggestion of throwing the user back to step 2 seems to be the best solution from the point of view of the user as they are given the chance to choose another name. But I'll have to see if this is easy to implement.

I noticed a weird behavior (see new bug #530) and this might affect meta-step 5 mentioned above wherein the permissions selected at meta-step 3 are preserved.

So my inclination is to just do the deduplication and message the user after the fact as this seems easier to implement and so that #530 can be fixed separately.

[ian-ross]

That sounds like a good plan: go ahead and do the deduplication and I'll look at #530.

[seav]

Apparently, the easiest implementation is to throw the user back to step 2. This is actually the built-in behavior of the wizard. At the end of the wizard, everything gets revalidated and the user gets thrown back to the earliest step that failed the revalidation.

The problem is, the existing code interferes with this behavior. In ProjectAddWizard.get_form_kwargs() (source), we call get_cleaned_data_for_step(), but this revalidates the data and returns None if the data is invalid (documentation). Therefore when the race condition occurs, we get a "NoneType has no get method" error and this ultimately stalls the throw-back behavior.

I solved this by storing the organization slug into the wizard's extra_data storage instead:

Diff for process_step():

-            self.organization = result['details-organization']
+            self.storage.extra_data['organization'] = (
+                result['details-organization'])

Diff for get_form_kwargs():

-                'organization': self.get_cleaned_data_for_step(
-                    'details').get('organization')
+                'organization': self.storage.extra_data['organization'],

Note 1: While it would be nice to provide the user with a "Someone stole your project name!" special message (by storing the fact that the user has already finished step 2 without incident), I think the normal error message is still OK since this event is a rare case anyway.

Note 2: I would have liked to create a functional test for this race condition, but our current functional test framework assumes that there is only a single browser instance. To test the race condition properly, we need two instances to simulate the two racing users. And retrofitting our framework now would be too much work. But I think this would be nice to do in the future to test concurrent users scenarios. (Alternatively, we can surreptitiously insert the stealing project name in the database at the appropriate time by using the Django ORM, but I think this is not clean from the functional test point of view.)

[ian-ross]

OK, this looks good. You can still get weird behaviour if you have two browser tabs open logged in as the same user, but that's pretty much unavoidable, because we're using session-based storage for the wizard steps.