React Animal History - Search By Id#1085
Conversation
| 8. Permissions: Folder Read Permission and dataset read permissions | ||
| 9. Metrics: Report and filter usage. |
There was a problem hiding this comment.
Are these requirements or non-requiremnents? I'm unclear on the break between them and the first seven.
| - Shows read-only summary: "Viewing {count} animal(s): {subject1}, {subject2}, ..." | ||
| - Shows "Modify Search" button that switches to ID Search mode with current subjects pre-populated | ||
| - Reports filter by URL subjects without requiring ID resolution | ||
| - No ID limit applies (URL-provided subjects are assumed already validated/resolved) |
There was a problem hiding this comment.
If it's worth capping the limit in "normal" usage (for performance, I guess?), it seems like we shouldn't exempt URL-provided values. But I'm also not really clear on why this is a useful mode.
There was a problem hiding this comment.
This is the same UI as participant history which completely derives the filters from the URL. This is pretty much only used when clicking on links generated in the EHR, so we assume we don't need to validate the parameters. In the normal search we're just trying to scope the amount of Ids that can be filtered, but may need to revisit this as we get more testing done.
| }, [activeReport]); | ||
|
|
||
| const handleFilterChange = useCallback(( | ||
| newFilterType: 'idSearch' | 'all' | 'aliveAtCenter' | 'urlParams', |
There was a problem hiding this comment.
Seems like a lot of separate hard-codings of these values.
There was a problem hiding this comment.
These were moved to variables.
| 2. Likelihood: Medium | ||
| 3. Mitigation: | ||
| 1. Id resolution feedback for in UI. | ||
| 2. Implemented as experimental feature to be able to view old and new animal history results side-by-side. |
There was a problem hiding this comment.
What does this feature flag control? Being able to open the new and old versions without toggling an experimental flag seems like it would be more convenient. Or does the flag control which is the target URL by default for existing UI?
There was a problem hiding this comment.
I updated the spec. This is going to just be hidden with link in admin page for now.
| - Repeat with tab-separated and semicolon-separated lists | ||
|
|
||
| 4. **Multi-Animal Search (Mixed Separators)** | ||
| - Enter IDs using multiple separators in single input: "ID1, ID2\nID3;ID4\tID5" |
There was a problem hiding this comment.
Because IDs can contain special characters the test should try IDs with separator values (e.g. ID123,A).
| - Verify validation error appears: "Maximum of 100 animal IDs allowed. You entered 101 IDs." | ||
| - Verify "Update Report" button is disabled | ||
| - Remove one ID to get back to 100 | ||
| - Verify error clears and "Update Report" button re-enables |
There was a problem hiding this comment.
Are IDs stored as string values? Would another boundary case be an ID that is a very long string, or MAXINT + 1?
| 4. **Mixed Valid/Invalid IDs:** Resolve valid IDs; show invalid in "Not Found" section | ||
| 5. **All IDs Not Found:** Display "Not Found" section only; reports panel shows no data message | ||
| 6. **Alias Resolves to Same ID:** If multiple input values resolve to the same animal ID, show all in "Resolved" section but pass de-duplicated list to reports | ||
| 7. **Special Characters in IDs:** Support IDs with hyphens, underscores, and other special characters |
There was a problem hiding this comment.
It might be worthwhile to call out some explicit special characters to test. For example, any characters that could be interpreted as a comment(// ' #), string(" ') or something could have special meaning in a SQL query or a url (? @ :).
Also worthwhile to consider where in the ID the special character is. An ID that starts with @123, or '123 or an ID that looks like a link / email address (123@abc.def)
| - Close browser, reopen bookmark | ||
| - Verify exactly 50 animals display correctly | ||
| - Verify no ID limit validation (URL Params mode bypasses 100 limit) | ||
| - Verify URL hash length doesn't cause browser issues |
There was a problem hiding this comment.
Another boundary case could be 10 IDs that are 1K characters long (or some other ridiculous size) and bookmark that.
| private void clickFilterButton(String buttonText) | ||
| { | ||
| clickButton(buttonText); // "All Records", "Alive at Center", or "ID Search" | ||
| sleep(500); // Allow mode transition |
There was a problem hiding this comment.
I'm not opposed to using sleeps, if they work that's great. Sometimes waiting for an element to go stale, the button or some element to show up can make the test a little more reliable and have a quicker run time. Something like:
shortWait().until(ExpectedConditions.stalenessOf(button));
Not sure if that will be applicable, but just a thought.
There was a problem hiding this comment.
I need to work through a few of these.
Yeah I understand where you're coming from on that. I added it as a topic in our next front end meeting so we're consistent in that approach. It's a good time to formalize some of these standards across the team. |
There was a problem hiding this comment.
The code looks better, especially the new useReportTab hook which I think accomplishes a clearer component hierarchy, and is more idiomatic. I left a bunch of specific feedback, but I have some general feedback as well:
- There are a ton of comments in the code. In general I am a fan of code comments, and think that LabKey as a whole (myself included) do a poor job of commenting our code. That said, I think you should consider taking a pass to look at all of the comments across the files in this PR and trim the very obvious ones, and especially trim the ones that say something like "extracted from ", I don't think those are useful at all. I commented on a few of these, but there are a lot more that I didn't comment on.
- There are still a lot of tests that setup what seems like valid scenarios, but then fail to assert anything useful. If we cannot properly assert something, then I struggle to believe the test is useful.
Edit: some of my comments are on outdated code because you pushed changes while I was reviewing, but GitHub then hid those comments from me when it came to review submission time, so I couldn't remove them. I suspect some of those comments are already completely irrelevant.
| notFound: [], | ||
| }; | ||
|
|
||
| render(<IdResolutionFeedback isVisible={true} resolutionResult={resolutionResult} />); |
There was a problem hiding this comment.
Need to update these tests to no longer pass the flag now that it's been removed.
There was a problem hiding this comment.
This is interesting to me that this was not caught any where (other than IDE red squiggly). I've removed them but looking for better ways to catch this.
There was a problem hiding this comment.
Yeah this is just a problem with Typescript jest tests. This happens in all of our packages. Basically, because we're not using the webpack build when running tests, and we have our webpack build set to ignore tests, the type checks never happen on the jest files. It's mostly not a problem, because typically if you're altering the type you're also probably breaking the test in some way. We can maybe configure ts-jest to actually verify the types, but I haven't looked into that.
There was a problem hiding this comment.
I added a new npm target to test this. Can discuss in front end meeting.
There was a problem hiding this comment.
Yes and the way to do that within a react app is generally to use a ref and check if ref.current is not undefined. I'm sure this works, it's just not what I would call idiomatic react code.
| @@ -43,4 +43,8 @@ export const QueryReportWrapper: FC<{ report: ReportConfig; tab: any }> = memo(( | |||
| }, [tab, report, container]); | |||
|
|
|||
| return null; | |||
There was a problem hiding this comment.
Hard disagree that it is an established or clean pattern to create components that never render anything. I have never seen that before. I have asked other people, and they have never seen that before. Happy to discuss.
|
|
||
| const ReportTab: FC<{ children: (tab: any) => React.ReactNode; filters: any; report: ReportConfig }> = ({ | ||
| report, | ||
| const TabbedReportPanelComponent: FC<TabbedReportPanelProps> = ({ |
There was a problem hiding this comment.
Yes it is a style we have across many of our components, and the advantage is that you don't need to have code that destructures your props on multiple lines, you can condense it into a single line.
labkey-ui-ehr/src/ParticipantHistory/TabbedReportPanel/OtherReportWrapper.tsx
Outdated
Show resolved
Hide resolved
labkey-ui-ehr/src/ParticipantHistory/TabbedReportPanel/useReportTab.ts
Outdated
Show resolved
Hide resolved
| // The component should render the TabbedReportPanel with EHR.reports namespace | ||
| // This is verified indirectly by successful render | ||
| expect(screen.getByText('Loading reports...')).toBeVisible(); | ||
| await waitForReportsToLoad(); |
There was a problem hiding this comment.
Most of the tests in this file (34/55) only rely on await waitForReportsToLoad();, despite all of them having test names implying they're testing something specific, and some even having additional comments implying even more specific behavior is being tested. These two tests in particular (props passed to TabbedReportPanel) have the exact same test body, if you ignore the comments, I don't think that's particularly useful. We should be asserting something different for each test. The passes correct reportNamespace prop test in particular says the thing being tested is verified indirectly by successful render, but surely we could actually assert that the TabbedReportPanel is visible right?
Another example is the test case for readOnly mode, it's not asserting anything different than most of the tests in this file, but if you look at the implementation of the component we could easily assert that the SearchByIdPanel should not be visible, since we hide that in readOnly mode.
The tests in this file (and everywhere else) need to actually assert something. I don't think it is a useful test if the only thing being asserted is that the text "General" has appeared on the page somewhere. The coverage numbers don't matter if you aren't asserting anything in your tests.
If you can't actually assert something in the body of a test, then the test is probably worthless; maybe unit tests aren't appropriate for the particular test case.
There was a problem hiding this comment.
good call. I updated these tests.
| } | ||
|
|
||
| .update-report-button { | ||
| padding: 10px 20px; |
There was a problem hiding this comment.
Something to consider: these button styles don't match any other button styles within LabKey. I think they look good, but it's odd that we have completely different styles for these buttons. I would consider using the bootstrap styles that align with what you're doing e.g. btn btn-primary, btn btn-default, etc.
There was a problem hiding this comment.
Yeah I have to think about handling styling here. Some of that is linked to using @labkey/components or not.
There was a problem hiding this comment.
Well, because this isn't a full page app, you are getting the boostrap styles from @labkey/theme, so you should have full access to all of the normal bootstrap styles, but yes, you won't have access to the @labkey/components styles.
You can still override the styles in your own scss files here, but you will have to override them in a way that is compatible with the styles already being on the page (vs overriding them via the overrides.scss files), which means sometimes you will have to create really specific styles (this is why I recommend following BEM naming conventions), and in some rare cases you may have to do things like !important.
There was a problem hiding this comment.
These are all converted to BEM naming conventions.
|
@labkey-alan Alright tests, comments and css has been worked over. Let me know if you have any other feedback. Thanks. |
labkey-bpatel
left a comment
labkey-bpatel
left a comment
There was a problem hiding this comment.
Reviewed non-react files. Looks good
labkey-alan
left a comment
labkey-alan
left a comment
There was a problem hiding this comment.
There are still a lot of problems with the tests, especially the ParticipantReports tests
- Some tests are definitely not testing what they imply they're testing
- Some tests aren't testing anything useful
- Others are duplicates.
- Some assertions aren't thorough enough, leaving gaps for bugs that could be missed
- There are a bunch of mocks that don't seem to be necessary
- There are tests emitting react act errors
- I've probably missed some other issues, because there are so many tests
You need to be manually verifying every test that is generated. This is the third time in a row that there are obvious problems with the tests. Claude is pretty clearly not capable of writing tests perfectly, you're going to have to intervene.
I did leave some other feedback, but it was all mostly pretty minor.
labkey-ui-ehr/src/ParticipantHistory/ParticipantReports.test.tsx
Outdated
Show resolved
Hide resolved
labkey-ui-ehr/src/ParticipantHistory/ParticipantReports.test.tsx
Outdated
Show resolved
Hide resolved
labkey-ui-ehr/src/ParticipantHistory/ParticipantReports.test.tsx
Outdated
Show resolved
Hide resolved
labkey-ui-ehr/src/ParticipantHistory/ParticipantReports.test.tsx
Outdated
Show resolved
Hide resolved
labkey-ui-ehr/src/ParticipantHistory/TabbedReportPanel/OtherReportWrapper.test.tsx
Outdated
Show resolved
Hide resolved
labkey-ui-ehr/src/ParticipantHistory/SearchByIdPanel/SearchByIdPanel.tsx
Outdated
Show resolved
Hide resolved
labkey-ui-ehr/src/ParticipantHistory/TabbedReportPanel/useReportTab.ts
Outdated
Show resolved
Hide resolved
labkey-ui-ehr/src/ParticipantHistory/SearchByIdPanel/SearchByIdPanel.tsx
Outdated
Show resolved
Hide resolved
labkey-alan
left a comment
labkey-alan
left a comment
There was a problem hiding this comment.
The tests are in a much better spot, I did leave some minor feedback, most of which we already discussed on a call a little bit ago. Happy to see more usages of test.each, something we need to use more of in general.
| const { resolved, notFound } = resolutionResult; | ||
|
|
||
| // Separate direct matches from alias matches | ||
| const directMatches = resolved.filter(r => r.resolvedBy === 'direct'); |
There was a problem hiding this comment.
Still not seeing the change to use memo.
labkey-ui-ehr/src/ParticipantHistory/ParticipantReports.test.tsx
Outdated
Show resolved
Hide resolved
labkey-ui-ehr/src/ParticipantHistory/ParticipantReports.test.tsx
Outdated
Show resolved
Hide resolved
| @@ -812,8 +791,9 @@ describe('ParticipantReports', () => { | |||
| }); | |||
|
|
|||
| describe('dependency injection', () => { | |||
There was a problem hiding this comment.
As we discussed on the call a little bit ago, I think this whole dependency injection set of tests can just be removed, since mostly it's redundant tests, and the one test where we aren't passing the fetchReports prop is covered by all of the selenium tests. There may be a good reason to move the one case where there are multiple categories (~line 812) to a test above, but that's your call to make.
There was a problem hiding this comment.
Removed all but the multi-category test
6 participants
Rationale
Implements the first phase of React conversion of Animal History. This is currently a hidden view until we get to MVP. Link in the ehrAdmin page. Participant history is available if you have the experimental feature on, you'll get a link at the top of the old participant view which comes up when you click on an Id.
Spec is in this PR
Changes