APP-422: Update non-sql fields based on report exec request#3150
APP-422: Update non-sql fields based on report exec request#3150JordanGuinn merged 38 commits intomainfrom
Conversation
JordanGuinn
requested review from
emyl3 and
krista-skylight
and removed request for
a team
JordanGuinn
changed the title
mcmcgrath13
left a comment
mcmcgrath13
left a comment
There was a problem hiding this comment.
The version field is not for the library, but for the spec itself so we can evolve the contents of it if need be, it can stay hardcoded to 1 at this point
| List<Long> columnUids, | ||
| List<Filter> filters) {} | ||
| List<Filter> filters, | ||
| TimeRange timeRange) { |
There was a problem hiding this comment.
time range won't directly come from the report execution request, nor will the title. The report title is metadata from the report configuration and the time range is derived from the basic filters somehow
| boolean isExport = reportExecRequest.isExport(); | ||
|
|
||
| String reportTitle = | ||
| reportExecRequest.reportTitle() == null || reportExecRequest.reportTitle().isEmpty() |
There was a problem hiding this comment.
this should come from the report config's report metadata
| String libraryName = reportConfig.reportLibrary().libraryName(); | ||
| Library reportLibrary = reportConfig.reportLibrary(); | ||
|
|
||
| int version = reportLibrary.version(); |
There was a problem hiding this comment.
| int version = reportLibrary.version(); | |
| int version = 1; |
|
|
||
| int version = reportLibrary.version(); | ||
| boolean isBuiltin = reportLibrary.isBuiltin(); | ||
| String libraryName = reportLibrary.libraryName(); |
There was a problem hiding this comment.
shouldn't need the library name
|
The time range need is under discussion from a product perspective, so hold on doing anything with that piece for now |
| this( | ||
| dbLibrary.getRunner(), | ||
| dbLibrary.getLibraryName(), | ||
| dbLibrary.getIsBuiltinIndex().toString().equalsIgnoreCase("Y"), |
There was a problem hiding this comment.
(q, nb) I don't have a lot of context on this code base so take my feedback with a grain of salt but could this conversion logic be built into the ReportLibrary object to make it more reusable?
There was a problem hiding this comment.
Definitely a fan - updated!
|
|
||
| if (start.isAfter(end)) { | ||
| throw new IllegalArgumentException("Start date cannot be after end date"); | ||
| } |
There was a problem hiding this comment.
(q, nb) Any way to not repeat this non null enforcement for TimeRange start and end from ReportExecutionRequest.java? Put it in a utility?
There was a problem hiding this comment.
Ended up removing this property from ReportExecutionRequest.java entirely based on initial feedback from Mary, but agreed in principle!
krista-skylight
left a comment
krista-skylight
left a comment
There was a problem hiding this comment.
Left just a couple questions!
| import java.util.Map; | ||
|
|
||
| public record ReportSpec( | ||
| @JsonProperty(value = "version", required = true) int version, |
There was a problem hiding this comment.
thanks! in this PR or a quick follow, could you remove this field from the report execution service as well?
|
3 participants
Description
This PR updates the
ReportSpecBuilderto set all non-SQL fields on the generatedReportSpecto non-default values, but rather the corresponding values coming from the ReportLibrary and the corresponding request.Tickets
Checklist before requesting a review