Skip to content

Refactor useResourceValue hook #6261

Merged
CarolineDenis merged 14 commits into
productionfrom
issue-6183
Mar 5, 2025
Merged

Refactor useResourceValue hook #6261
CarolineDenis merged 14 commits into
productionfrom
issue-6183

Conversation

@CarolineDenis

@CarolineDenis CarolineDenis commented Feb 20, 2025

Copy link
Copy Markdown
Contributor

Fixes #6183

Checklist

  • Self-review the PR after opening it to make sure the changes look good and
    self-explanatory (or properly documented)
  • Add automated tests
  • Add relevant issue to release milestone

Testing instructions

A) Issue test

  • Create a collection object type
  • Change the catalog number format to something different to the collection default and save
  • Create a new collection object
  • Assign the newly created collection object type
  • Try to save the collection object record
  • Verify the save button is enabled and you can save

B) General test

  • Verify non-relationship fields of different types behave properly
  • Verify to-one relationships rendered as a QueryComboBox behave properly
  • Ensure the 'Field is required' save blocker is set when applicable
  • Ensure the 'Please match requested format' save blocker is displayed when a value is invalid for a field according to its formatter
  • Ensure fields have their defaults properly set in accordance with their formatter

@CarolineDenis CarolineDenis added this to the 7.10.1 milestone Feb 20, 2025
@CarolineDenis
CarolineDenis requested review from a team and sharadsw February 20, 2025 17:15

@lexiclevenger lexiclevenger left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

  • Verify the save button is enabled and you can save
  • Verify non-relationship fields rendered as a QueryComboBox behave properly
  • Verify to-one relationships rendered as a QueryComboBox behave properly
  • Ensure the 'Field is required' save blocker is set when applicable
  • Ensure the 'Please match requested format' save blocker is displayed when a value is invalid for a field according to its formatter
  • Ensure fields have their defaults properly set in accordance with their formatter

Looks good! However, changing the COType of an existing CO causes the new catalog number to save with the temporary value # instead of auto-incrementing.

Untitled.video.-.Made.with.Clipchamp.mp4

@lexiclevenger
lexiclevenger self-requested a review February 20, 2025 20:23

@lexiclevenger lexiclevenger left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Meant to request changes

@lexiclevenger
lexiclevenger dismissed their stale review February 20, 2025 20:24

Meant to request changes

@combs-a
combs-a requested a review from a team February 21, 2025 20:34
@combs-a

combs-a commented Feb 21, 2025

Copy link
Copy Markdown
Collaborator

Question for testing; could you provide examples of non-relationship fields that can be rendered as a QCBX? Not sure of fields in the schema that matches that description. 😅

@melton-jason

melton-jason commented Feb 24, 2025

Copy link
Copy Markdown
Contributor

Looks good! However, changing the COType of an existing CO causes the new catalog number to save with the temporary value # instead of auto-incrementing.

Untitled.video.-.Made.with.Clipchamp.mp4

From #6261 (review) by @lexiclevenger

Nice catch! Though the explanation here is a little awkward, as there's a couple distinct parts of autonumbering which need explanation 😅

Firstly -as far as I can tell- autonumbering has ever only worked when creating new records, and not when updating old records. This would mean that if you were to use the default CatalogNumberNumeric formatter and save an existing record with a ######### catalogNumber, then Specify wouldn't even try to autonumber the CollectionObject.
I'm unsure if this was an intentional design decision (that existing records would not be autonumbered) or not.
Of course, it is much simpler to only apply autonumbering to new records, so that might be the case (or part of it)?

Secondly, technically the default pattern of the incrementing formatter is a valid value for the formatter! With CatalogNumberNumeric for example, ######### is a valid value for the format, so Specify sees nothing wrong with validating and saving a CollectionObject with the format.
I think this goes hand-in-hand with the aforementioned assumption that only new records will have catalogNumbers which match the incrementing default pattern (###...).

Here's a video recreating this exact Issue in v7.9.6.2!:

cat_num_default.mov

Of course, with the changes relating to changing the catalogNumber when the CollectionObjectType changes, this is probably more apparent.

What would you suggest as a solution?
Here are some potential solutions I came up with, any number of which can be implemented:

  • Allow autonumbering to apply to existing records
  • Once a record exists in the database, treat ##.. as an invalid value for the formatter
  • For existing CollectionObjects, don't change the CatalogNumber when the COT changes

Question for testing; could you provide examples of non-relationship fields that can be rendered as a QCBX? Not sure of fields in the schema that matches that description. 😅

From #6261 (comment) by @combs-a

Hmmm, yes there should be no non-relationships that can be rendered as a QCBX.
I think the testing requirement should probably be something along the lines of "Verify non-relationship fields of different types behave properly" or "Ensure relationships rendered as a field cell are readonly and properly formatted/aggregated"?
I've updated the testing instructions!

@combs-a combs-a left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

A) Issue test

  • Verify the save button is enabled and you can save

B) General test

  • Verify non-relationship fields of different types behave properly
  • Verify to-one relationships rendered as a QueryComboBox behave properly
  • Ensure the 'Field is required' save blocker is set when applicable
  • Ensure the 'Please match requested format' save blocker is displayed when a value is invalid for a field according to its formatter
  • Ensure fields have their defaults properly set in accordance with their formatter

Everything looked good on my end! I used ojsmnh to be clear, we might want to check general testing on other things as well.

I wanted to note down some behavior; when adding a new record from a non-default COT CO, it'll use the format of the previous record. It's not something I'm worried about seeing as the save is blocked, just might be slightly annoying behavior for users.

My thought is that Add should always use the default Catalog Number formatter but this can be in a different issue.

firefox_02-24_09-02.mp4

@pashiav pashiav left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

  • Verify the save button is enabled and you can save
  • Verify non-relationship fields of different types behave properly
  • Verify to-one relationships rendered as a QueryComboBox behave properly
  • Ensure the 'Field is required' save blocker is set when applicable
  • Ensure the 'Please match requested format' save blocker is displayed when a value is invalid for a field according to its formatter
  • Ensure fields have their defaults properly set in accordance with their formatter

Looks good! I tested on calvertmarinemuseum20250206.

I agree with @combs-a's comment. If not within the scope of this PR, should we create an issue and add it to a milestone?

@melton-jason

Copy link
Copy Markdown
Contributor

Haven't made any functional changes to this PR in recent commits (https://github.com/specify/specify7/pull/6261/files/034605aa29ac8a9f17a00bde10f6715b7dce05ad..e5977ae17e182790e4ba495d186614f21d7a3e14), just working on adding automated tests!

@CarolineDenis
CarolineDenis merged commit 6575317 into production Mar 5, 2025
@CarolineDenis
CarolineDenis deleted the issue-6183 branch March 5, 2025 17:28
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.

CO form does not update immediately when adding COT with a different catalog number format

6 participants