-
Notifications
You must be signed in to change notification settings - Fork 36
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
Remove cast to default catalog number formatter #6245
base: production
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good work! I'm surprised -and glad- the change is this minimal 😄!
My primary concern when initially viewing this change was that removing the numeric cast would lead to non-sensical searching for collections using the CatalogNumberNumeric formatter (e.g., for strings '2' > '100'
, the same Issue as #3501 (review)).
I think it's important to note that it's definitely still possible to recreate some "non-sensical" searching and sorting results.
Most importantly in my opinion however, is that the data can be misrepresented in Query Results, which can lead to the confusing results (thankfully not in exports).
The following video demonstrates both of these concerns:
Screen.Recording.2025-02-17.at.11.31.21.AM.mov
It might be confusing to a user why 123
is greater than 2100
, 2
is somehow greater than 123
, and 3
is the largest catalog number.
While it still might be nonsensical, I think the underlying problem here is primarily that catalog numbers which can be coerced to the default collection's format are, regardless of their actual representation in the database.
This also holds true with the API; if you query on catalognumbers greater than "2"
when the collection's format is CatalogNumberNumeric for example, the value is assumed to be "000000002"
rather than "2"
regardless of the value of the formatname
on the queryfield; which might be beneficial in some cases, but harmful in others.
Regardless, this is still a good fix and step in the right direction!
The concerns might not be blockers of the PR or release, I just thought I'd leave a comment describing the behavior so it's known and documented!
(On a side note, if the query results should be ordered in a particular way, should the exported results - like through CSV - have the same sorting? Here is the export of the Query shown above:
Catalog Number Query - Mon Feb 17 2025.csv)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- Verify all COs have their respective type's format
- Re-test Support COT catalog number configurations in the Query Builder #5485
The formatting is fixed, which is great!
Relating to the inconsistency with strings that Jason brought up, I don't think it's caused by this PR--rather, it seems like something that should've been mentioned in the previous PR since this is present in production too.
Here is an example query and its result (on this PR):
And another, where we've changed the query to search for any record greater than the lowest result in the previous...
Now 2024-000001 is greater than 230, when 230 was greater than it before, which is definitely a bit of strange behavior.
If it would be possible, it may be a good idea to still have some kind of filter to automatically only include results that are of the COT selected, perhaps. This is possible by just selecting the name of the COT, but that does not support cases in which you want to query for COs with different COTs.
I think for an example query and expected result, it'd be something like:
Catalog Number -> Greater Than -> 2025-000001 [COT: FossilTest]
OR -> Equal -> 1 [COT: Vertebrate Paleontology]Results:
Any CO that has the COT FossilTest with a catalog number greater than 2025-000001 as well as the CO that has the COT Vertebrate Paleontology
If this is out of the scope of this PR, let me know and I'll approve it--but I do agree with Jason that this behavior should be addressed since it might be strange to users.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- Create a CO query
- Verify all COs have their respective type's format
- Re-test Support COT catalog number configurations in the Query Builder #5485
When hovering over a line from a CO query, the tool-tip no longer shows up. Works in production (shown in video as well). Tested on DBs calvertmarinmuseum
and kufish
.
issue branch: https://kufish20250102prod-issue-6239.test.specifysystems.org/specify/query/new/collectionobject/
production: https://kufish20250102prod-production-2.test.specifysystems.org/specify/query/new/collectionobject/
Screen.Recording.2025-02-17.at.11.29.45.AM.mov
^not finished testing but leaving bug info here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- Verify all COs have their respective type's format
- Re-test Support COT catalog number configurations in the Query Builder #5485
Results are not being filtered properly. Not sure the exact logic here but for some cat # they show up in the results when they shouldn't or they don't show up in results when they should. In this video I query on cat # greater than 2025-001 and you can see that '2025-001' shows up when it should not and then when I query on equal to 2025-001 it doesn't show up when it should. This does not seem to be consistent with every cat # in a db but I was able to recreate on ojsmnh and chadron.
02-17_14.12.mp4
dbs: https://ojsmnh110242024production-issue-6239.test.specifysystems.org/specify/query/191/
https://chadrontest2-issue-6239.test.specifysystems.org/specify/query/new/collectionobject/
@combs-a Thanks! I'm gonna open a different PR for filtering COs by a selected COT formatter since the issue was not introduced by this PR and the scope of that seems bigger than what this PR was intended for. Created an issue for it: #6253 @pashiav I initially thought the tooltip disappearing was a bug but it turns out Specify only displays a tooltip in Queries when the value at a cell does not match its formatted value. Since this PR does not convert catalog numbers to a specific format anymore, the values always match the formatted value and hence there's no tooltip. @emenslin It's strange, I'm not able to recreate the same results as in the video. I get correct results when querying with However, I did see one weird result where |
Created #6254 for filtering COs by COT formatter |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- Verify all COs have their respective type's format
- Re-test Support COT catalog number configurations in the Query Builder #5485
Looks good, all tests passed!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- Verify all COs have their respective type's format
- Re-test Support COT catalog number configurations in the Query Builder #5485
I ran into these issues but they do seem to be similar to my last issue so I assume they will be addressed in #6253 but I wanted to comment them just so they're known.
Fixes #6239
Removes an ancient cast to the default numeric catalog number format in queries. Casting to a format shouldn't really be necessary since we now support different formatters. The issue was caused because queries would convert all catalog number to match the default format whenever a COT with the default formatter was chosen with the gear icon.
Checklist
self-explanatory (or properly documented)
Testing instructions