SED-4661 add-new-grid-layout-to-cross-execution-views - #674
SED-4661 add-new-grid-layout-to-cross-execution-views#674david-stephan wants to merge 6 commits into
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces support for distinguishing report layouts by type (SingleExecution vs. CrossExecution). It updates the layout accessor, plugin, and services to filter and configure layouts based on this new property, and adds a migration task to backfill legacy documents. The review feedback highlights two important improvements: first, collecting documents to a list in the migration task before updating them to avoid cursor instability during streaming; and second, adding defensive null checks for the report type in the accessor to prevent potential NullPointerExceptions.
cl-exense
left a comment
cl-exense
left a comment
There was a problem hiding this comment.
Approving, two really minor ("FYI") comments that don't necessitate any action.
| WebApplicationConfigurationManager configurationManager = context.require(WebApplicationConfigurationManager.class); | ||
| configurationManager.registerHook(s -> Map.of(DEFAULT_LAYOUT_ID_CONFIG_KEY, defaultLayoutId)); | ||
| configurationManager.registerHook(s -> { | ||
| Map<String, String> config = new HashMap<>(); |
There was a problem hiding this comment.
Minor, no need to change: you could also have used Map.of(K1,V1,K2,V2) I guess.
| public List<ReportLayout> getAllReportLayouts() { | ||
| return reportLayoutAccessor.getAccessibleReportLayoutsDefinitions(getSession().getUser().getUsername()); | ||
| public List<ReportLayout> getAllReportLayouts(@QueryParam("reportType") ReportLayout.ReportLayoutType reportType) { | ||
| ReportLayout.ReportLayoutType effectiveReportType = reportType != null ? reportType : ReportLayout.ReportLayoutType.SingleExecution; |
There was a problem hiding this comment.
Alternative:
public List<ReportLayout> getAllReportLayouts(@DefaultValue("SingleExecution") @QueryParam("reportType") ReportLayout.ReportLayoutType reportType) {
If the DefaultValue annotation could directly take the correct type this would be a no-brainer. However it can only take magic string values (which I don't like), so I'll let you choose. Both options are perfectly fine with me.
2 participants
No description provided.