APP-542 & APP-665: Add endpoints for admin report create and edit#3238
Conversation
| }); | ||
| assertThat(config.advancedFilter().reportFilterUid()).isEqualTo(6L); | ||
| } | ||
| @Nested |
There was a problem hiding this comment.
Ended up experimenting with @Nested test groups here, as a means to hopefully better distinguish between the things being tested, and to be able to use beforeEach meaningfully across those groups. Thoughts?
| * Necessary because `report_uid` is not marked as an IDENTITY column, nor does it have a | ||
| * corresponding sequence, so we have to use a custom query. | ||
| */ | ||
| @Query("SELECT (MAX(r.id.reportUid)+1) FROM Report r") |
There was a problem hiding this comment.
q, b: two questions:
- is there a possibility for a race condition if two people try to create a report at the same time?
- Will this work if there aren't any reports? I think
MAX()will returnnullin that case
The second question feels like a simple fix (maybe use COALESCE?) but the first may be trickier and a pretty unlikely edge case anyways as users aren't making reports that often. Should we even try to solve for it?
There was a problem hiding this comment.
@mcmcgrath13 pointed out there's already an existing utility for this in 7, so I just ended up using that!
That doesn't necessarily address those questions specifically, but imo it does make this less of a problem that we wanna solve for as part of this work.
There was a problem hiding this comment.
The Id Generation service exists (hopefully) to avoid these sorts of races. It dibs the ID for us and then we can use it knowing there won't be a clash
| @@ -0,0 +1,130 @@ | |||
| package gov.cdc.nbs.report; | |||
|
|
|||
| import gov.cdc.nbs.entity.odse.*; | |||
There was a problem hiding this comment.
sugg, b: list out the imports instead of the wildcard?
There was a problem hiding this comment.
Will give that a shot manually - this was either the result of IntelliJ trying to be smart, or our Spotless formatter, definitely not something I intentionally did myself
There was a problem hiding this comment.
Confirmed, it was an IntelliJ setting. Though that does make me wonder if we have a preference on a potential upper limit there? Like at what point do we prefer *, if ever?
There was a problem hiding this comment.
Fair question. I set my upper limit to 999 lol
But I can't see a good reason to use the wildcard. Maybe imposing a somewhat reasonable limit (like 20ish?) would be an indicator that too much is happening in a single file and that some refactoring in needed. What do you think?
There was a problem hiding this comment.
Works for me! Will bump mine to 20 - I suppose if you've got more than that many imports from a single location, maybe worth re-evaluating why that is 😁
| @Table(name = "Report", catalog = "NBS_ODSE") | ||
| public class Report { | ||
| @NonNull @EmbeddedId private ReportId id; | ||
| @EmbeddedId @NonNull ReportId id; |
There was a problem hiding this comment.
q, nb: should this be fully reverted?
There was a problem hiding this comment.
Yep, should be removed from the diff now!
brick-green
left a comment
There was a problem hiding this comment.
Good work here! Found a few more wildcards and had one question
| if (!reportSectionRepository.existsBySectionCd(request.sectionCode())) { | ||
| throw new IllegalArgumentException( | ||
| "No report section found for code " + request.sectionCode()); | ||
| } |
There was a problem hiding this comment.
q, nb: this validation feels a little out of place for the method fetchReportMetadata(). Are there other validations we could group together into a valiate() method?
There was a problem hiding this comment.
No other validations as of right now - I can change this fn from fetch to verify, if that's helpful?
|



Description
This PR adds new endpoints to the reports module for admin report creation and editing (including filter support as part of that).
Tickets
Checklist before requesting a review