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 Independent Subviews via API #5253

Open
wants to merge 123 commits into
base: production
Choose a base branch
from
Open

Conversation

melton-jason
Copy link
Contributor

@melton-jason melton-jason commented Aug 29, 2024

Fixes #114
Alternative implementation of #3125

New Functionality

Independent Subviews

An Independent Subview can be distinguished from its dependent counterpart (for both button/non-button subviews) by a magnifying glass which is used to search existing related records, or a link icon

An independent subview in subform mode with no related records

subview_indepdenet

An independent subview in subform mode with no related records

subview_independent_related

Querying to add to a -to-many independent subview

subview_to_many_add_via_query.mov

Querying to add to a -to-one independent subview

subview_to_one_add_via_query.mov

Unlike dependent records, because the records can exist without each other (hence independent), removing a related record from an independent collection only removes it from the associated relationship and does not delete the record.

💡 Unless a resource is explicitly saved/deleted in a dialog (in which case only that specific resource is modified), no changes are made to any of the related records until the "base" record is saved

Saveblockers propagate to the "base" resource

When the subview is not being rendered as a button, if there are any Saveblockers they will be propagated to the "base" record.

subview_save_blocker.mov

Adding new records to an Independent Subview

Because a viewname can be specified on the Form Definition, there is some special logic used when creating new related records to add to the collection.

When the viewname of the Subview is the same view as the default DataEntry form for the table, then the new resource is directly added to the Collection.

subview_base_form_add.mov

Otherwise, if the viewname for the independent subview is not the default DataEntry view, then a separate dialog will be used to create the new resource

subview_different_form_add.mov

(In the above example, a CollectionObject view called AccessionItems is used)

Version Control

To handle version control and ensure that the related records do not get out of date, in the background Specify will only care about the values of the related objects (CollectionObjects related to an Accession for example) when they are explicitly changed.
In other words, you should only experience an out-of-date error on the "base" record if you attempt to save the "base" record when a change to a related record was already made (i.e., the version has been incremented) and you have made changes to the related record from the "base" record.

In other words, the following is okay (as well as adding related records and then modifying them not in the Independent Subview and then saving the "base" record):

subview_independent_version_ctrl.mov

But the below workflow will result in an error:

subview_version_ctl_error.mov

Loading Indicator while fetching collections

subview_loading.mov

Form Editing Improvements

Previously, when trying to render a -to-many relationship as a Query Combo Box, three unintuitive errors were thrown (see #5253 (review)).

Now, a helpful error is displayed to the user:

Screen.Recording.2024-08-31.at.5.35.11.PM.1.mov

When trying to render a relationship which has no reverse as a Subview, an error is now displayed explaining the situation

Screen.Recording.2024-09-03.at.1.56.38.PM.mov

Previously, if there was a cyclical referencing of Views (e.g., both sides of the relationship are rendered CollectionObject -> accession, Accession -> collectionObjects), then this would cause an infinite loop of rendering loops (See #5253 (review)).

Now behind-the-scenes subviews keep track of which relationships have already been rendered in a specific SubView hierarchy. If a relationship (like RepositoryAgreement -> accessions) would be rendered as a SubView but it already is being rendered, then Specify will not render that relationship.

Here is an example with simplified RepositoryAgreement and Accession forms demonstrating the change:

Screen.Recording.2024-09-03.at.1.47.02.PM.mov

Finding Independent Relationships

You can use the Database Schema viewer in UserTools to find independent relationships on specific tables:

finding_independence.mov

For convenience, i've also compiled a list of every independent relationship in Specify organized by table in a json format: independent_master_list.json

You can open the file in any text editor.

(note that PaleoContext and CollectingEvent may be dependent/independent depending on the Discipline -> IsPaleoContextEmbedded and Collection -> IsEmbeddedCollectingEvent respectively)

Checklist

  • Self-review the PR after opening it to make sure the changes look good
    and self-explanatory (or properly documented)
  • Add automated tests
    • though I'd like to add more complex tests
  • Add relevant issue to release milestone

Testing instructions

When testing Independent -to-many relationships, please test Accession -> collectionObjects and RepositoryAgreement -> accessions. These are pretty popular!

Unless otherwise stated in the testing instruction, a specific step (or chain of steps) should be able to be performed on any Independent Subview (-to-one, -to-many, as button, etc.)

Querying related records

  • Using the QueryBuilder interface, make sure you can only select a single record when adding to a -to-one independent relationship (save the "base" record after adding the record)
  • Using the QueryBuilder interface, make sure you can select multiple records when adding to a -to-many independent relationship(save the "base" record after adding the records)

Creating new related records

  • For an independent subview which uses the default DataEntry view for the table (usually just the name of the table. If viewname is omitted in the subview cell, this view is used by default), ensure that no dialog is used when creating a new associated related record using the independent subview.
  • For an independent subview which uses a different view from the default DataEntry view for the table, ensure that a dialog is displayed and resource creation occurs within that dialog (once the resource is saved, it should be automatically added to the independent subview)
  • For more information and examples, see the Adding new records to an Independent Subview section earlier in this PR

Permissions

  • Ensure the buttons for Viewing, Searching/Querying, Creating, and Removing are only present if the logged in user has the correct permissions (All operations should require read permissions, and Query/Create/Remove should require update permissions on the related record table)

Version control

    1. Record the version and/or timestampModifiedof one or more related records before adding/editing them via an independent subview
    1. Add the related record to an independent subview (or edit/remove the related record if already present in the independent subview) and save the "base" record
  • Ensure that the version has increased by one and that the timestampModified has been updated since the recorded value for each related record
    1. Add one or more related records to an independent subview
    • Do not yet save the "base" record!
    1. In a separate tab, edit one or more of the related records and save them
  • Ensure the "base" record can still be saved
    1. Find an independent subview with one or more related records (add them if needed. "base" record saving is optional at this point)
    1. In a separate tab, edit one or more of the related records and save them
    1. Explicitly modify one or more of the related records which you saved in step 2 in the independent subview
  • Ensure an 'out-of-date' error is raised when the "base" record is saved.

Misc.

  • When working with an independent -to-one relationship (such as CollectionObject -> cataloger), make sure that the 🔍 and ➕ icons are disabled for the Subviewwhen there is one related record
  • When you have one or more related records blocking the deletion of a "base" record, ensure that you can remove the association(s) from the "base" record, save the "base" record, and then delete the "base" record.
  • Find or create a record on the one side of a one-to-many record (i.e., Accession with collectionObjects) with a lot (probably 80+) of related records and make sure a loading indicator is displayed while the Subview is loading.

maxpatiiuk and others added 30 commits March 8, 2023 15:50
Triggered by dfaa5d0 on branch refs/heads/issue-114
Triggered by 1359149 on branch refs/heads/issue-114
This is not likely the behavior we want, so this is something to be discussed.
See related #3127
@melton-jason
Copy link
Contributor Author

melton-jason commented Nov 6, 2024

Hi @specify/ux-testing!
Some good news!

The longstanding Issue mentioned in #5253 (review) and previous reviews (where modifying a record in a Subview which is already rendered) has been fixed!

When rendering/showing a Subview for a particular Subview, the current solution goes through each relationship in the "Subview hierarchy"1 and checks if the reverse relationship is already being rendered (e.g., CollectionObject -> accession is the reverse of Accession -> collectionObjects).
If so, then the resource already being rendered will appear as readonly in the Subview.

Screen.Recording.2024-11-06.at.10.43.39.AM.mov

Another solution considered was to remove the resource already being rendered (i..e, higher in the hierarchy/ being rendered above the Subview) from the Subview. For to-one relationships, this would not render the Subview (e.g., Hide the Accession Subview from CollectionObjects entirely when rendering Accession -> collectionObjects -> accession) and for to-many relationships this would exclude the resource from the "collection" of resources (remove the base CollectionObject from the "collection" of COs when rendering CO -> accession -> collectionObjects)

This solution is better performance-wise and simpler for the code base 😃, but maybe less intuitive?

Let me know which solution is preferred!


Also ran into this: when you create a new CO, add it to an accession, and then delete it before trying to save it instead throws a KeyError. Is this the correct error message, or should it have an out-of-date error instead?

This other Issue mentioned in #5253 (review) has been fixed as well! Now, a proper 404 error is raised whenever the resource can not be fetched (does not exist/ deleted)

Screen.Recording.2024-11-06.at.11.20.31.AM.mov

Footnotes

  1. a list of all relationships and resources being shown/rendered "above" the current form. For an example, consider the following chain of relationships all on a single DataEntry form being rendered by Subviews: Accession -> CollectionObject -> Determination -> Taxon

@melton-jason melton-jason requested a review from a team November 6, 2024 17:24
pashiav
pashiav previously requested changes Nov 6, 2024
Copy link
Contributor

@pashiav pashiav left a comment

Choose a reason for hiding this comment

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

Testing instructions

When testing Independent -to-many relationships, please test Accession -> collectionObjects and RepositoryAgreement -> accessions. These are pretty popular!

Unless otherwise stated in the testing instruction, a specific step (or chain of steps) should be able to be performed on any Independent Subview (-to-one, -to-many, as button, etc.)

Querying related records

  • Using the QueryBuilder interface, make sure you can only select a single record when adding to a -to-one independent relationship (save the "base" record after adding the record)
  • Using the QueryBuilder interface, make sure you can select multiple records when adding to a -to-many independent relationship(save the "base" record after adding the records)

Creating new related records

  • For an independent subview which uses the default DataEntry view for the table (usually just the name of the table. If viewname is omitted in the subview cell, this view is used by default), ensure that no dialog is used when creating a new associated related record using the independent subview.
  • For an independent subview which uses a different view from the default DataEntry view for the table, ensure that a dialog is displayed and resource creation occurs within that dialog (once the resource is saved, it should be automatically added to the independent subview)
  • For more information and examples, see the Adding new records to an Independent Subview section earlier in this PR

Permissions

  • Ensure the buttons for Viewing, Searching/Querying, Creating, and Removing are only present if the logged in user has the correct permissions (All operations should require read permissions, and Query/Create/Remove should require update permissions on the related record table)

Version control

    1. Record the version and/or timestampModifiedof one or more related records before adding/editing them via an independent subview
    1. Add the related record to an independent subview (or edit/remove the related record if already present in the independent subview) and save the "base" record
  • Ensure that the version has increased by one and that the timestampModified has been updated since the recorded value for each related record
    1. Add one or more related records to an independent subview
    • Do not yet save the "base" record!
    1. In a separate tab, edit one or more of the related records and save them
  • Ensure the "base" record can still be saved
    1. Find an independent subview with one or more related records (add them if needed. "base" record saving is optional at this point)
    1. In a separate tab, edit one or more of the related records and save them
    1. Explicitly modify one or more of the related records which you saved in step 2 in the independent subview
  • Ensure an 'out-of-date' error is raised when the "base" record is saved.

Misc.

  • When working with an independent -to-one relationship (such as CollectionObject -> cataloger), make sure that the 🔍 and ➕ icons are disabled for the Subviewwhen there is one related record
  • When you have one or more related records blocking the deletion of a "base" record, ensure that you can remove the association(s) from the "base" record, save the "base" record, save the "base" record, and then delete the "base" record.
  • Find or create a record on the one side of a one-to-many record (i.e., Accession with collectionObjects) with a lot (probably 80+) of related records and make sure a loading indicator is displayed while the Subview is loading. d, and then delete the "base" record.
  • Find or create a record on the one side of a one-to-many record (i.e., Accession with collectionObjects) with a lot (probably 80+) of related records and make sure a loading indicator is displayed while the Subview is loading.

WOOO great job so far Jason!!


From #5253 (comment):

This other Issue mentioned in #5253 (review) has been fixed as well! Now, a proper 404 error is raised whenever the resource can not be fetched (does not exist/ deleted)

Tested this with treatment event but it does not have the same error message as in the video.

  1. Go to Accession
  2. Create a Treatment Event
  3. Go to that Treatment Event's record
  4. Delete the Treatment Event from the db
  5. Go back to base record and try to save
  6. See that error is different than the comment's error

Report: Specify 7 Crash Report - 2024-11-06T19_18_57.169Z.txt
Link to: https://cuic22024-issue-114-backend.test.specifysystems.org/specify/view/accession/60/
Screen Shot 2024-11-06 at 1 54 10 PM

Screen.Recording.2024-11-06.at.1.18.41.PM.mov

Also from #5253 (comment):

This solution is better performance-wise and simpler for the code base 😃, but maybe less intuitive?
Let me know which solution is preferred!

I would prefer the second solution! The user wouldn't have to question why something is read-only.

The current implementation is also fine in my opinion. Though, it would be helpful to add a message clarifying why it is read-only. Something simple that reminds the user that they are already on/editing that form. @specify/ux-testing thoughts?

@pashiav pashiav requested a review from a team November 6, 2024 20:02
Copy link
Collaborator

@lexiclevenger lexiclevenger left a comment

Choose a reason for hiding this comment

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

Testing instructions

When testing Independent -to-many relationships, please test Accession -> collectionObjects and RepositoryAgreement -> accessions. These are pretty popular!

Unless otherwise stated in the testing instruction, a specific step (or chain of steps) should be able to be performed on any Independent Subview (-to-one, -to-many, as button, etc.)

Querying related records

  • Using the QueryBuilder interface, make sure you can only select a single record when adding to a -to-one independent relationship (save the "base" record after adding the record)
  • Using the QueryBuilder interface, make sure you can select multiple records when adding to a -to-many independent relationship(save the "base" record after adding the records)

Creating new related records

  • For an independent subview which uses the default DataEntry view for the table (usually just the name of the table. If viewname is omitted in the subview cell, this view is used by default), ensure that no dialog is used when creating a new associated related record using the independent subview.
  • For an independent subview which uses a different view from the default DataEntry view for the table, ensure that a dialog is displayed and resource creation occurs within that dialog (once the resource is saved, it should be automatically added to the independent subview)
  • For more information and examples, see the Adding new records to an Independent Subview section earlier in this PR

Permissions

  • Ensure the buttons for Viewing, Searching/Querying, Creating, and Removing are only present if the logged in user has the correct permissions (All operations should require read permissions, and Query/Create/Remove should require update permissions on the related record table)

Version control

    1. Record the version and/or timestampModifiedof one or more related records before adding/editing them via an independent subview
    1. Add the related record to an independent subview (or edit/remove the related record if already present in the independent subview) and save the "base" record
  • Ensure that the version has increased by one and that the timestampModified has been updated since the recorded value for each related record
    1. Add one or more related records to an independent subview
    • Do not yet save the "base" record!
    1. In a separate tab, edit one or more of the related records and save them
  • Ensure the "base" record can still be saved
    1. Find an independent subview with one or more related records (add them if needed. "base" record saving is optional at this point)
    1. In a separate tab, edit one or more of the related records and save them
    1. Explicitly modify one or more of the related records which you saved in step 2 in the independent subview
  • Ensure an 'out-of-date' error is raised when the "base" record is saved.

Misc.

  • When working with an independent -to-one relationship (such as CollectionObject -> cataloger), make sure that the 🔍 and ➕ icons are disabled for the Subviewwhen there is one related record
  • When you have one or more related records blocking the deletion of a "base" record, ensure that you can remove the association(s) from the "base" record, save the "base" record, and then delete the "base" record.
  • Find or create a record on the one side of a one-to-many record (i.e., Accession with collectionObjects) with a lot (probably 80+) of related records and make sure a loading indicator is displayed while the Subview is loading.

I get the same error as Pashia after deleting a treatment event or CO from the Accession form but still get the 404 error, as well.

Screen.Recording.2024-11-07.at.4.34.21.PM.mov

From #5253 (comment):

This solution is better performance-wise and simpler for the code base 😃, but maybe less intuitive?

Let me know which solution is preferred!

I prefer the current solution because I think completely hiding the base record from the related subview might be more confusing than making it read-only. I'd like to know what other people think. I definitely think Pashia's idea of adding a message or tooltip would be helpful.

However, this still allows the user to remove the base record from a related subview, which causes both a save conflict and out-of-date error. The second solution might be the way to go if this can't be easily prevented.

Screen.Recording.2024-11-07.at.4.31.45.PM.mov

melton-jason and others added 2 commits November 8, 2024 09:34
FormTable remove buttons now checks for update permission for independent relationships, not delete permission

Prevent removing readonly, already rendered independnet resources in subviews
@melton-jason
Copy link
Contributor Author

Thanks for the reviews!

From #5253 (comment):

This other Issue mentioned in #5253 (review) has been fixed as well! Now, a proper 404 error is raised whenever the resource can not be fetched (does not exist/ deleted)

Tested this with treatment event but it does not have the same error message as in the video.

  1. Go to Accession
  2. Create a Treatment Event
  3. Go to that Treatment Event's record
  4. Delete the Treatment Event from the db
  5. Go back to base record and try to save
  6. See that error is different than the comment's error

Report: Specify 7 Crash Report - 2024-11-06T19_18_57.169Z.txt
Link to: https://cuic22024-issue-114-backend.test.specifysystems.org/specify/view/accession/60/
Screen Shot 2024-11-06 at 1 54 10 PM

Screen.Recording.2024-11-06.at.1.18.41.PM.mov

From #5253 (review)

This Issue will not be fixed in this PR. I've opened an Issue to track the incorrect error message: #5390


I prefer the current solution because I think completely hiding the base record from the related subview might be more confusing than making it read-only. I'd like to know what other people think. I definitely think Pashia's idea of adding a message or tooltip would be helpful.

However, this still allows the user to remove the base record from a related subview, which causes both a save conflict and out-of-date error. The second solution might be the way to go if this can't be easily prevented.

From #5253 (review)

It can be easily prevented! I pushed a fix for this, and tested the functionality locally.
This PR will be merged, but if you encounter any more Issues, don't hesitate to report them!

@melton-jason melton-jason dismissed stale reviews from combs-a, Areyes42, alesan99, and emenslin November 8, 2024 16:23

stale review

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Dev Attention Needed
Development

Successfully merging this pull request may close these issues.

Add ability to add independent resources from the remote side of a one-to-many relationship.