Skip to content

Conversation

@Areyes42
Copy link
Contributor

@Areyes42 Areyes42 commented Feb 17, 2025

Fixes #5503

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
  • Add relevant documentation (Tester - Dev)

Testing instructions

  1. Create two CollectionObjectTypes with the same Catalog Number Format Name which differs from that of the Collection's default Catalog Number format
  2. Create a new Collection Object query, add Catalog Number to the query, and switch the type from Any to Equal
  3. Try to select both COTs
  • Verify that it selects the proper COT regardless of the Catalog Number format

Triggered by fed7786 on branch refs/heads/issue-5503
@CarolineDenis CarolineDenis added this to the 7.10.2 milestone Feb 21, 2025
@Areyes42 Areyes42 marked this pull request as ready for review February 21, 2025 19:15
@Areyes42 Areyes42 requested a review from a team February 21, 2025 19:15
Copy link
Contributor

@sharadsw sharadsw left a comment

Choose a reason for hiding this comment

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

I like your approach. Functionally your code looks good, just pointing out some minor code smells.

@Areyes42
Copy link
Contributor Author

I like your approach. Functionally your code looks good, just pointing out some minor code smells.

Good point, I've added some helpers to separate the logic and hopefully make it a little more intuitive. Let me know if you think this looks better!

@Areyes42 Areyes42 requested a review from sharadsw February 21, 2025 22:24
Copy link
Contributor

@sharadsw sharadsw 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, nice work!

…er.tsx

Co-authored-by: Sharad S <16229739+sharadsw@users.noreply.github.com>
@CarolineDenis CarolineDenis requested review from a team and CarolineDenis February 24, 2025 14:10
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.

  • Verify that it selects the proper COT regardless of the Catalog Number format

Looks good!

Screen.Recording.2025-02-24.at.11.42.46.AM.mov

@lexiclevenger lexiclevenger requested a review from a team February 24, 2025 17:44
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.

  • Verify that it selects the proper COT regardless of the Catalog Number format

Works as expected!

Copy link
Contributor

@melton-jason melton-jason left a comment

Choose a reason for hiding this comment

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

Nice work on this! I really like the implementation; pretty clever idea! 🚀

I will mention that while the CollectionObjectType the user selects while the page is loaded will always be respected, it will always revert to the first CollectionObjectType in the list with the same format when re-rendered!

Screen.Recording.2025-02-26.at.9.04.25.PM.mov

This is because only the format name (nothing of the CollectionObjectType) is saved to the SpQueryField in the database; when the page is rendered initially, it isn't formatted with the formatterSeparator and index and is just the name of the formatter itself.
So Specify has no clue which CollectionObjectType it can be referring to.

There's also a somewhat smaller Issue where changing the CollectionObjectType to another which shares the same format doesn't trigger a change on the field (because nothing has actually changed on the SpQueryField!)

Screen.Recording.2025-02-26.at.9.40.58.PM.mov

We're relatively having a lot of Issues regarding this feature (#6239, #6253, etc. no thanks to my initial implementation in the first place!), and I think that's usually an indicator to take a step back and re-evaluate the core problem and potential solutions.

@specify/ux-testing, @specify/dev-testing

Instead of having a separate option for each CollectionObjectType, could we instead have an option for each Format and have the title of the option be a combination of all the COT names with that format?

It could look something like this:

Screenshot 2025-02-26 at 9 30 38 PM

Or we can go even more descriptive to try and be as clear as possible: the user isn't selecting/filtering on COT, they're only applying a different formatter for the field:

Screenshot 2025-02-26 at 10 18 36 PM

Alternatively, we could move away from the SpQueryField -> FormatName approach entirely and come up with something different which would allow the user to select an individual COT name.

@emenslin
Copy link
Collaborator

I agree with Jasons comment and I think that grouping them together might make the most sense since it does not necessarily matter which specific one you use. I also think since we don't really have the gear other places aside from when you have something mapped as 'formatter' or 'aggregator' that adding 'Format As: ...' in this instance makes sense but we can go with something else if people have other opinions @specify/ux-testing

@Areyes42
Copy link
Contributor Author

Areyes42 commented Mar 3, 2025

@melton-jason @emenslin

Great suggestions! I've updated the logic to group COTs with identical formatters together. This should mitigate issues related to re-rendering and state changes in the form, since it no longer compares against the same formatter. Also, I've added the "Format As:" identifier to improve clarity, but we can adjust it if it seems too confusing.

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.

  • Verify that it selects the proper COT regardless of the Catalog Number format

Looks good! I ran into an issue where the query results were incorrect (I set the filter to equal and instead it showed all results), I couldn't recreate it but I would love if we could get one more testing review to verify that it is not an issue

@emenslin emenslin requested a review from a team March 4, 2025 16:32
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.

  • Verify that it selects the proper COT regardless of the Catalog Number format=

Looks good!

@emenslin I didn't run into what you mentioned, it's working on my end!

Copy link
Contributor

@melton-jason melton-jason left a comment

Choose a reason for hiding this comment

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

Nice work!
Coming along nicely 👌

@Areyes42 Areyes42 merged commit fe8397d into production Mar 12, 2025
12 checks passed
@Areyes42 Areyes42 deleted the issue-5503 branch March 12, 2025 17:49
@github-project-automation github-project-automation bot moved this from Dev Attention Needed to ✅Done in General Tester Board Mar 12, 2025
@melton-jason melton-jason mentioned this pull request Apr 1, 2025
4 tasks
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.

Query Builder: Can't select a COT for Catalog Number if it shares a formatter with another COT

8 participants