Jayapraksh1234 patch 3 #13617
Open
Jayapraksh1234 patch 3 #13617
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Fix: Make environment samples toggle independent from case samples on Map Dashboard - Environment samples are now OFF by default. - Toggling the case samples checkbox does not affect environment samples. - Environment samples checkbox is fully independent.
WalkthroughRefactors SampleDashboardMapComponent: adds explicit state fields for human/environment sample visibility, introduces helper methods for criteria checks, restructures addComponents, loadMapData, layer options, legend generation, and marker counting, and updates UI bindings for sample type toggles. Behavior for mixed criteria remains unsupported. Adds JavaDoc and clarifies onMarkerClicked. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant UI as Dashboard UI
participant Comp as SampleDashboardMapComponent
participant Crit as Criteria Helpers
participant Svc as Sample Services
participant Map as Map/Marker Layer
User->>UI: Toggle sample types / open map
UI->>Comp: addComponents() / addLayerOptions()
Note right of Comp: Initialize displayedHumanSamples<br/>Initialize showEnvironmentalSamples (per permissions)
User->>UI: Apply filters (criteria)
UI->>Comp: loadMapData(criteria)
Comp->>Crit: Evaluate criteria (only human / only env / empty / other)
alt Only human or empty-with-disease
Comp->>Svc: Fetch human samples (CASE/CONTACT/EVENT_PARTICIPANT)
Svc-->>Comp: Human sample markers
Comp->>Map: Add human markers (icon set)
else Only environment and env toggle ON
Comp->>Svc: Fetch environmental samples
Svc-->>Comp: Environmental markers
Comp->>Map: Add environmental markers (icon set)
else Mixed/other
Note over Comp: Mixed criteria not supported<br/>(no markers added)
end
Comp->>Map: Update marker count (human [+ env if enabled])
Comp-->>UI: getLegendComponents() based on toggles
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Please see the documentation for more information. Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (4)
sormas-ui/src/main/java/de/symeda/sormas/ui/dashboard/sample/components/SampleDashboardMapComponent.java (4)
184-185: Fix preallocation bugCapacity uses environment size twice; should include human size.
- List<LeafletMarker> markers = new ArrayList<>(environmentSamples.size() + environmentSamples.size()); + List<LeafletMarker> markers = new ArrayList<>(humanSamples.size() + environmentSamples.size());
291-294: Show legend for environment‑only viewEarly return hides the legend when only environment is enabled.
- if (displayedHumanSamples.size() == 0) { - return Collections.emptyList(); - } + if (displayedHumanSamples.isEmpty() && !showEnvironmentalSamples) { + return Collections.emptyList(); + }
180-182: Mixed criteria silently returns no markersConsider supporting union (human + environment) or at least show an info message to avoid empty, unexplained results.
84-87: Heading info doesn’t update when toggling environment
appendHeadingInforuns only inaddComponents; toggling the checkbox won’t update the heading.
- Either recompute heading on toggle, or move environment info into a dynamic UI element that refreshes with
refreshMap.Also applies to: 276-280
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
sormas-ui/src/main/java/de/symeda/sormas/ui/dashboard/sample/components/SampleDashboardMapComponent.java(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
sormas-ui/src/main/java/de/symeda/sormas/ui/dashboard/sample/components/SampleDashboardMapComponent.java (4)
sormas-ui/src/main/java/de/symeda/sormas/ui/UiUtil.java (1)
UiUtil(17-146)sormas-api/src/main/java/de/symeda/sormas/api/i18n/I18nProperties.java (1)
I18nProperties(39-536)sormas-api/src/main/java/de/symeda/sormas/api/FacadeProvider.java (1)
FacadeProvider(127-594)sormas-ui/src/main/java/de/symeda/sormas/ui/utils/CssStyles.java (1)
CssStyles(26-535)
🔇 Additional comments (2)
sormas-ui/src/main/java/de/symeda/sormas/ui/dashboard/sample/components/SampleDashboardMapComponent.java (2)
186-201: LGTM: marker icon mapping for human samplesIcon selection per association type is clear and correct.
162-164: Verify Java version; List.of requires Java 9+If the project targets Java 8,
List.of()will not compile. UseCollections.emptyList()instead.Run to verify target Java version and spot usages:
If Java 8, apply:
- List<MapSampleDto> humanSamples = List.of(); - List<MapSampleDto> environmentSamples = List.of(); + List<MapSampleDto> humanSamples = Collections.emptyList(); + List<MapSampleDto> environmentSamples = Collections.emptyList();
Comment on lines
+76
to
+83
| displayedHumanSamples = | ||
| new HashSet<>(Arrays.asList(SampleAssociationType.CASE, SampleAssociationType.CONTACT, SampleAssociationType.EVENT_PARTICIPANT)); | ||
| if (UiUtil.permitted(FeatureType.ENVIRONMENT_MANAGEMENT, UserRight.ENVIRONMENT_SAMPLE_VIEW)) { | ||
| showEnvironmentalSamples = false; // Environment samples are OFF by default | ||
| } | ||
|
|
||
| super.addComponents(); | ||
|
|
There was a problem hiding this comment.
Prevent unauthorized data exposure: initialize human sample types based on user rights
Without filtering, unauthorized users can see CONTACT/EVENT_PARTICIPANT markers because they’re included by default and the toggle may not be shown to disable them. Initialize displayedHumanSamples with only permitted types.
- displayedHumanSamples =
- new HashSet<>(Arrays.asList(SampleAssociationType.CASE, SampleAssociationType.CONTACT, SampleAssociationType.EVENT_PARTICIPANT));
- if (UiUtil.permitted(FeatureType.ENVIRONMENT_MANAGEMENT, UserRight.ENVIRONMENT_SAMPLE_VIEW)) {
- showEnvironmentalSamples = false; // Environment samples are OFF by default
- }
+ final boolean canViewCase = UiUtil.permitted(UserRight.CASE_VIEW);
+ final boolean canViewContact = UiUtil.permitted(UserRight.CONTACT_VIEW);
+ final boolean canViewEpp = UiUtil.permitted(UserRight.EVENTPARTICIPANT_VIEW);
+ displayedHumanSamples = new HashSet<>();
+ if (canViewCase) {
+ displayedHumanSamples.add(SampleAssociationType.CASE);
+ }
+ if (canViewContact) {
+ displayedHumanSamples.add(SampleAssociationType.CONTACT);
+ }
+ if (canViewEpp) {
+ displayedHumanSamples.add(SampleAssociationType.EVENT_PARTICIPANT);
+ }
+ // Environment samples OFF by default; permission is enforced where used📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| displayedHumanSamples = | |
| new HashSet<>(Arrays.asList(SampleAssociationType.CASE, SampleAssociationType.CONTACT, SampleAssociationType.EVENT_PARTICIPANT)); | |
| if (UiUtil.permitted(FeatureType.ENVIRONMENT_MANAGEMENT, UserRight.ENVIRONMENT_SAMPLE_VIEW)) { | |
| showEnvironmentalSamples = false; // Environment samples are OFF by default | |
| } | |
| super.addComponents(); | |
| final boolean canViewCase = UiUtil.permitted(UserRight.CASE_VIEW); | |
| final boolean canViewContact = UiUtil.permitted(UserRight.CONTACT_VIEW); | |
| final boolean canViewEpp = UiUtil.permitted(UserRight.EVENTPARTICIPANT_VIEW); | |
| displayedHumanSamples = new HashSet<>(); | |
| if (canViewCase) { | |
| displayedHumanSamples.add(SampleAssociationType.CASE); | |
| } | |
| if (canViewContact) { | |
| displayedHumanSamples.add(SampleAssociationType.CONTACT); | |
| } | |
| if (canViewEpp) { | |
| displayedHumanSamples.add(SampleAssociationType.EVENT_PARTICIPANT); | |
| } | |
| // Environment samples OFF by default; permission is enforced where used | |
| super.addComponents(); |
🤖 Prompt for AI Agents
sormas-ui/src/main/java/de/symeda/sormas/ui/dashboard/sample/components/SampleDashboardMapComponent.java
around lines 76 to 83: currently displayedHumanSamples is populated with CASE,
CONTACT and EVENT_PARTICIPANT unconditionally which can expose markers to
unauthorized users; instead, initialize displayedHumanSamples by always adding
SampleAssociationType.CASE and only add CONTACT and EVENT_PARTICIPANT if the
current user has the appropriate viewing permission(s) (use
UiUtil.permitted(...) checks as used elsewhere), then set
showEnvironmentalSamples as before and call super.addComponents(); this ensures
the default set respects user rights before components (and toggles) are
created.
Comment on lines
+96
to
+103
| @Override | ||
| protected Long getMarkerCount(Date fromDate, Date toDate, int maxCount) { | ||
| return FacadeProvider.getSampleDashboardFacade() | ||
| .countSamplesForMap(dashboardDataProvider.buildDashboardCriteriaWithDates(), displayedHumanSamples) | ||
| + (showEnvironmentalSamples | ||
| ? FacadeProvider.getSampleDashboardFacade().countEnvironmentalSamplesForMap(dashboardDataProvider.buildDashboardCriteriaWithDates()) | ||
| : 0); | ||
| } |
There was a problem hiding this comment.
Gate environment count by permission, not only by toggle
Avoid counting environment samples when the user lacks the right/feature; prevents unauthorized queries and potential leakage.
- return FacadeProvider.getSampleDashboardFacade()
- .countSamplesForMap(dashboardDataProvider.buildDashboardCriteriaWithDates(), displayedHumanSamples)
- + (showEnvironmentalSamples
- ? FacadeProvider.getSampleDashboardFacade().countEnvironmentalSamplesForMap(dashboardDataProvider.buildDashboardCriteriaWithDates())
- : 0);
+ final boolean canShowEnv =
+ showEnvironmentalSamples && UiUtil.permitted(FeatureType.ENVIRONMENT_MANAGEMENT, UserRight.ENVIRONMENT_SAMPLE_VIEW);
+ return FacadeProvider.getSampleDashboardFacade()
+ .countSamplesForMap(dashboardDataProvider.buildDashboardCriteriaWithDates(), displayedHumanSamples)
+ + (canShowEnv
+ ? FacadeProvider.getSampleDashboardFacade().countEnvironmentalSamplesForMap(dashboardDataProvider.buildDashboardCriteriaWithDates())
+ : 0);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| @Override | |
| protected Long getMarkerCount(Date fromDate, Date toDate, int maxCount) { | |
| return FacadeProvider.getSampleDashboardFacade() | |
| .countSamplesForMap(dashboardDataProvider.buildDashboardCriteriaWithDates(), displayedHumanSamples) | |
| + (showEnvironmentalSamples | |
| ? FacadeProvider.getSampleDashboardFacade().countEnvironmentalSamplesForMap(dashboardDataProvider.buildDashboardCriteriaWithDates()) | |
| : 0); | |
| } | |
| @Override | |
| protected Long getMarkerCount(Date fromDate, Date toDate, int maxCount) { | |
| final boolean canShowEnv = | |
| showEnvironmentalSamples && UiUtil.permitted(FeatureType.ENVIRONMENT_MANAGEMENT, UserRight.ENVIRONMENT_SAMPLE_VIEW); | |
| return FacadeProvider.getSampleDashboardFacade() | |
| .countSamplesForMap(dashboardDataProvider.buildDashboardCriteriaWithDates(), displayedHumanSamples) | |
| (canShowEnv | |
| ? FacadeProvider.getSampleDashboardFacade().countEnvironmentalSamplesForMap(dashboardDataProvider.buildDashboardCriteriaWithDates()) | |
| : 0); | |
| } |
🤖 Prompt for AI Agents
In
sormas-ui/src/main/java/de/symeda/sormas/ui/dashboard/sample/components/SampleDashboardMapComponent.java
around lines 96-103, the code unconditionally counts environmental samples when
the UI toggle is set, which can trigger unauthorized queries; change the
condition to require both the showEnvironmentalSamples toggle AND the
user/feature permission for environmental samples before calling
countEnvironmentalSamplesForMap. Implement this by checking the appropriate
permission/feature flag (e.g. the existing authorization/feature helper used
elsewhere in the project or a SampleDashboardFacade/Session utility method) and
only invoking
FacadeProvider.getSampleDashboardFacade().countEnvironmentalSamplesForMap(...)
when that check passes; also avoid calling
dashboardDataProvider.buildDashboardCriteriaWithDates() twice by storing it in a
local variable and reuse it for both counts.
Comment on lines
+161
to
+173
| SampleDashboardCriteria criteria = dashboardDataProvider.buildDashboardCriteriaWithDates(); | ||
| List<MapSampleDto> humanSamples = List.of(); | ||
| List<MapSampleDto> environmentSamples = List.of(); | ||
| if (isSampleCriteriaForOther(criteria)) { | ||
| humanSamples = FacadeProvider.getSampleDashboardFacade().getSamplesForMap(criteria, displayedHumanSamples); | ||
| environmentSamples = | ||
| showEnvironmentalSamples ? FacadeProvider.getSampleDashboardFacade().getEnvironmentalSamplesForMap(criteria) : Collections.emptyList(); | ||
| } else if (isEmptySampleCriteriaWithoutDisease(criteria)) { | ||
| // If no sample material is selected, we want to show all samples | ||
| humanSamples = FacadeProvider.getSampleDashboardFacade().getSamplesForMap(criteria, displayedHumanSamples); | ||
| environmentSamples = | ||
| showEnvironmentalSamples ? FacadeProvider.getSampleDashboardFacade().getEnvironmentalSamplesForMap(criteria) : Collections.emptyList(); | ||
| } else if (isOnlyHumanSampleCriteria(criteria) || isEmptySampleCriteriaWithDisease(criteria)) { |
There was a problem hiding this comment.
Enforce permission when fetching environment samples
Use a permission-guarded flag to prevent unauthorized data fetches.
SampleDashboardCriteria criteria = dashboardDataProvider.buildDashboardCriteriaWithDates();
- List<MapSampleDto> humanSamples = List.of();
- List<MapSampleDto> environmentSamples = List.of();
+ final boolean canShowEnv =
+ showEnvironmentalSamples && UiUtil.permitted(FeatureType.ENVIRONMENT_MANAGEMENT, UserRight.ENVIRONMENT_SAMPLE_VIEW);
+ List<MapSampleDto> humanSamples = List.of();
+ List<MapSampleDto> environmentSamples = List.of();
if (isSampleCriteriaForOther(criteria)) {
humanSamples = FacadeProvider.getSampleDashboardFacade().getSamplesForMap(criteria, displayedHumanSamples);
- environmentSamples =
- showEnvironmentalSamples ? FacadeProvider.getSampleDashboardFacade().getEnvironmentalSamplesForMap(criteria) : Collections.emptyList();
+ environmentSamples =
+ canShowEnv ? FacadeProvider.getSampleDashboardFacade().getEnvironmentalSamplesForMap(criteria) : Collections.emptyList();
} else if (isEmptySampleCriteriaWithoutDisease(criteria)) {
// If no sample material is selected, we want to show all samples
humanSamples = FacadeProvider.getSampleDashboardFacade().getSamplesForMap(criteria, displayedHumanSamples);
- environmentSamples =
- showEnvironmentalSamples ? FacadeProvider.getSampleDashboardFacade().getEnvironmentalSamplesForMap(criteria) : Collections.emptyList();
+ environmentSamples =
+ canShowEnv ? FacadeProvider.getSampleDashboardFacade().getEnvironmentalSamplesForMap(criteria) : Collections.emptyList();
} else if (isOnlyHumanSampleCriteria(criteria) || isEmptySampleCriteriaWithDisease(criteria)) {
// If only human sample is selected, we want to show only human samples
humanSamples = FacadeProvider.getSampleDashboardFacade().getSamplesForMap(criteria, displayedHumanSamples);
} else if (isOnlyEnvironmentSampleCriteria(criteria)) {
// If only an environment sample is selected, we want to show only environment samples
- environmentSamples =
- showEnvironmentalSamples ? FacadeProvider.getSampleDashboardFacade().getEnvironmentalSamplesForMap(criteria) : Collections.emptyList();
+ environmentSamples =
+ canShowEnv ? FacadeProvider.getSampleDashboardFacade().getEnvironmentalSamplesForMap(criteria) : Collections.emptyList();Also applies to: 176-180
🤖 Prompt for AI Agents
sormas-ui/src/main/java/de/symeda/sormas/ui/dashboard/sample/components/SampleDashboardMapComponent.java
around lines 161-173 (and also apply same change at 176-180): the code
unconditionally uses showEnvironmentalSamples to decide whether to call
FacadeProvider.getSampleDashboardFacade().getEnvironmentalSamplesForMap(...),
which can fetch sensitive environment sample data without verifying the caller
has permission; change these checks to combine the existing
showEnvironmentalSamples flag with a permission check (e.g., Security/Permission
API or a hasPermissionForEnvironmentalSamples() method) so the
environmentSamples are fetched only when showEnvironmentalSamples is true AND
the current user has the required permission, and apply the same guarded check
at the other occurrence on lines 176-180.
Comment on lines
+272
to
+283
| if (UiUtil.permitted(UserRight.ENVIRONMENT_VIEW)) { | ||
| CheckBox showEnvironmentSamplesCheckBox = new CheckBox(); | ||
| showEnvironmentSamplesCheckBox.setId(Captions.sampleDashboardShowEnvironmentSamples); | ||
| showEnvironmentSamplesCheckBox.setCaption(I18nProperties.getCaption(Captions.sampleDashboardShowEnvironmentSamples)); | ||
| showEnvironmentSamplesCheckBox.setValue(showEnvironmentalSamples); // Use the correct state! | ||
| showEnvironmentSamplesCheckBox.addValueChangeListener(e -> { | ||
| showEnvironmentalSamples = (boolean) e.getProperty().getValue(); | ||
| refreshMap(true); | ||
| }); | ||
| layersLayout.addComponent(showEnvironmentSamplesCheckBox); | ||
| } | ||
| } |
There was a problem hiding this comment.
Align environment permission checks with sample rights
Use ENVIRONMENT_SAMPLE_VIEW (+ feature) consistently; ENVIRONMENT_VIEW may be too broad and inconsistent with other checks.
- if (UiUtil.permitted(UserRight.ENVIRONMENT_VIEW)) {
+ if (UiUtil.permitted(FeatureType.ENVIRONMENT_MANAGEMENT, UserRight.ENVIRONMENT_SAMPLE_VIEW)) {
CheckBox showEnvironmentSamplesCheckBox = new CheckBox();
showEnvironmentSamplesCheckBox.setId(Captions.sampleDashboardShowEnvironmentSamples);
showEnvironmentSamplesCheckBox.setCaption(I18nProperties.getCaption(Captions.sampleDashboardShowEnvironmentSamples));
showEnvironmentSamplesCheckBox.setValue(showEnvironmentalSamples); // Use the correct state!
showEnvironmentSamplesCheckBox.addValueChangeListener(e -> {
showEnvironmentalSamples = (boolean) e.getProperty().getValue();
refreshMap(true);
});
layersLayout.addComponent(showEnvironmentSamplesCheckBox);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if (UiUtil.permitted(UserRight.ENVIRONMENT_VIEW)) { | |
| CheckBox showEnvironmentSamplesCheckBox = new CheckBox(); | |
| showEnvironmentSamplesCheckBox.setId(Captions.sampleDashboardShowEnvironmentSamples); | |
| showEnvironmentSamplesCheckBox.setCaption(I18nProperties.getCaption(Captions.sampleDashboardShowEnvironmentSamples)); | |
| showEnvironmentSamplesCheckBox.setValue(showEnvironmentalSamples); // Use the correct state! | |
| showEnvironmentSamplesCheckBox.addValueChangeListener(e -> { | |
| showEnvironmentalSamples = (boolean) e.getProperty().getValue(); | |
| refreshMap(true); | |
| }); | |
| layersLayout.addComponent(showEnvironmentSamplesCheckBox); | |
| } | |
| } | |
| if (UiUtil.permitted(FeatureType.ENVIRONMENT_MANAGEMENT, UserRight.ENVIRONMENT_SAMPLE_VIEW)) { | |
| CheckBox showEnvironmentSamplesCheckBox = new CheckBox(); | |
| showEnvironmentSamplesCheckBox.setId(Captions.sampleDashboardShowEnvironmentSamples); | |
| showEnvironmentSamplesCheckBox.setCaption(I18nProperties.getCaption(Captions.sampleDashboardShowEnvironmentSamples)); | |
| showEnvironmentSamplesCheckBox.setValue(showEnvironmentalSamples); // Use the correct state! | |
| showEnvironmentSamplesCheckBox.addValueChangeListener(e -> { | |
| showEnvironmentalSamples = (boolean) e.getProperty().getValue(); | |
| refreshMap(true); | |
| }); | |
| layersLayout.addComponent(showEnvironmentSamplesCheckBox); | |
| } | |
| } |
🤖 Prompt for AI Agents
In
sormas-ui/src/main/java/de/symeda/sormas/ui/dashboard/sample/components/SampleDashboardMapComponent.java
around lines 272 to 283, the code is using
UiUtil.permitted(UserRight.ENVIRONMENT_VIEW) which is too broad; replace this
with the environment-sample specific right (UserRight.ENVIRONMENT_SAMPLE_VIEW)
and include the same feature flag/check used elsewhere for environment samples
(e.g., check the Feature or FeatureToggle method used in this project) so the
visibility logic matches other components; update the condition to use both the
specific permission and the feature check and keep the rest of the checkbox
setup identical.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes #
Summary by CodeRabbit
New Features
Bug Fixes