Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow configuring uniqueness rules #4215

Merged
merged 171 commits into from Jan 16, 2024
Merged

Allow configuring uniqueness rules #4215

merged 171 commits into from Jan 16, 2024

Conversation

melton-jason
Copy link
Contributor

Implements #2712

Uniqueness Rules can be configured from the Schema Config, currently found under the Table Aggregation option for a table's Schema Configuration:
Screenshot from 2023-11-15 22-08-10

The interface is table/grid-like.
Screenshot from 2023-11-15 22-21-53

Rules which are defined as constraints on the database level, and thus should not be editable, have all components disabled and remain visible.

Each rule is split into 'groups': the 'fields' and the 'scope'.
The 'fields' represent the values which must be unique in a particular scope.
For example, on CollectionObject there is a rule which has catalogNumber as a unique field and Collection as a scope. Thus, there must be a unique catalogNumber within a Collection.

If a rule is editable, it can be "expanded" to add and/or remove individual unique fields, or delete it from the table's uniqueness rules
Screenshot from 2023-11-15 22-37-45

The Uniqueness Rules have live validation: this is, after every edit to a specific rule, Specify checks to make sure there are no records violating that rule.

If violating records are found, saving the edited Uniqueness Rules is prevented and the option to export all found duplicate values (to a CSV format) for the given fields and scope is presented.
Preferably, an embedded QueryBuilder would be utilized in this case, but it is not currently possible to achieve every Query as needed and thus the export is a necessity.

Screenshot from 2023-11-15 22-40-48

The rules are only 'committed' to the database once the Save button is clicked.
Changes are lost if the Uniqueness Rule dialog is closed or navigated away from before the changes have been Saved.

At the database level, these are implemented in two django migrations from the businessrules app:

  • 0001_initial
    • Creates two tables, uniquenessrule (which contains the containing disciplineid of the rule, the splocalecontaineritem id of the 'scope' and whether the rule is marked as 'readonly' or not) and uniquenessrule_splocalecontaineritem (a many-to-many join table which represents the associated fields for a given uniqueness rule).
    • Note: Practically the distinction between a 'scope' and the 'fields' of a uniqueness rule is solely conceptual. They are the same as far as the operations are performed, and we could easily remove the scope field from uniquenessrule and instead have it as another to-many in uniquenessrule_splocalecontaineritem
  • 0002_default_unique_rules
    • Populates the uniquenessrule and uniquenessrule_splocalecontaineritem with the defaults for Specify, which are maintained in a JSON format within the migration file

melton-jason and others added 30 commits October 5, 2023 00:05
Triggered by 975bf79 on branch refs/heads/uniqueness-rules
 into production"

This reverts commit bd600dc, reversing
changes made to 4cf4ee7.
Triggered by ab30cb6 on branch refs/heads/issue-4083
Triggered by 153ae39 on branch refs/heads/weblate-localization
@melton-jason
Copy link
Contributor Author

melton-jason commented Jan 12, 2024

If you change the scope on a field Without Duplicates and then change the field to something that does have duplicates. You can not export the duplicates.

Screen.Recording.2024-01-11.at.1.13.14.PM.mov

#4215 (review)

At the moment, I am unable to recreate this issue. I will keep an eye open for it, but let me know if other @specify/ux-testing can recreate it.


Only change i would like to request is for the Add Uniqueness Rule button to be moved in-line with the Close and Save buttons on the dialog. Functions and looks fine when rules are present but is a little wonky when grid display is empty.

Screenshot 2024-01-08 at 3 52 20 PM

I have went ahead and gone through with the change as requested in #4215 (review) and moved the Add Uniqueness Rule button to be in-line with the other Dialog Buttons.
For additional context, see my initial response to this request at: #4215 (comment)

Let me know what you all think of the change.

@melton-jason
Copy link
Contributor Author

Uniqueness Rules explicitly scoped to a scoping hierarchy above discipline will now be "global". This means that it will appear in every discipline's uniqueness rules (changes to the rule while in one discipline will apply to all disciplines, unless the rule is "de-elevated" from global status by changing its scope to be in the discipline or below, in which case the rule will be set to that discipline).

To expand on what I mean by "explicitly scoped": any rule that is scoped to the Database, or whose scope traverses through either division or institution. (For example, this means the scope createdByAgent -> division -> address will be considered global because it requires going through the division).

Currently there is no distinction in the UI between "Global" rules and "Discipline" rules, although I assume that would be a desired feature.
Furthermore, the system can be very easily expanded to allow the user to create their own global rules regardless of scope (especially if the distinction between global and discipline rules is made in the UI). Would this be wanted in some cases?

Copy link
Collaborator

@emenslin emenslin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Global uniqueness rules do seem to be working and I like the placement of the add uniqueness rules button so that looks good.
Problem though is that I was able to recreate the issue Carlos mentioned.

@melton-jason
Copy link
Contributor Author

Global uniqueness rules do seem to be working and I like the placement of the add uniqueness rules button so that looks good. Problem though is that I was able to recreate the issue Carlos mentioned.

OK, the issue is now fixed

@melton-jason melton-jason requested review from a team January 12, 2024 16:03
Copy link
Collaborator

@emenslin emenslin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, all issues I found seem to be fixed and I'm not getting any more errors. Good job!

Copy link
Contributor

@grantfitzsimmons grantfitzsimmons left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think there might be a way to improve the UI as the mapper is not the most effective way to represent the selected item for the scope.

Current

Screenshot 2024-01-13 at 9 55 53 AM

Request

image

Suggestions:

  • Use the WorkBench or Query Builder in-line mapper instead of the full mapping interface that shows more information than necessary
  • Move the 'Add' button after the field picker in the same div
  • Remove the redundant explainer at the bottom and only display that in the 'Scope; column when viewed in grid view

Copy link
Contributor

@grantfitzsimmons grantfitzsimmons left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think there might be a way to improve the UI as the mapper is not the most effective way to represent the selected item for the scope.

Current

Screenshot 2024-01-13 at 9 55 53 AM

Request

image

Suggestions:

  • Use the WorkBench or Query Builder in-line mapper instead of the full mapping interface that shows more information than necessary
  • Move the 'Add' button after the field picker in the same div
  • Remove the redundant explainer at the bottom and only display that in the 'Scope; column when viewed in grid view

@melton-jason
Copy link
Contributor Author

Some (small) changes have been made:

  • When checking validation for a uniqueness rule, scopes which have no values (are null) will be ignored.

    • For example if we define a rule on CollectionObject that states a field must be unique to Collection -> Discipline -> Division, any CollectionObject which is not associated with a Collection will not be included in the rule validation. If there is an associated Collection but it does not have an associated Discipline, the Collection Object will not be included in the rule, etc.
  • zero-to-one relationships are now excluded from the Fields and Relationships which can be a Uniqueness Rule Field or Scope.

    • This is because zero-to-one relationships in essence act like a singular -to-many relationship and it would not make sense to allow the 'to-one' side of the relationship to be used as each related model must be unique anyways.
    • The are only two zero-to-one relationships in Specify and both exist on the Locality table: localityDetails and geoCoordDetails
  • The scope of the uniqueness rule in the header columns of the Exported Duplicates CSV has been truncated to be the final relationship in the "relationship path"

  • Any relationship in the Exported Duplicates CSV (truncated scope included) should now have _id appended to the end of it to make clear the associated number is the database ID of the resource

  • The Add button for adding a new Uniqueness Field has been moved in-line with the last Uniqueness Field, and a slight gap has been added between elements for each row of a Uniqueness Field

I agree, that proposal to use a single in-line mapper does look far cleaner than the current approach of using the full mapper. However, given the urgency for this release I would suggest a feature request Issue be made and this be implemented in the future.

@melton-jason melton-jason dismissed stale reviews from realVinayak, bronwyncombs, and carlosmbe January 16, 2024 18:34

The suggested changes have since been made or addressed

@melton-jason melton-jason merged commit a6baf22 into production Jan 16, 2024
9 checks passed
@melton-jason melton-jason deleted the uniqueness-rules branch January 16, 2024 18:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

Allow forcing uniqueness for any field in schema configuration