Fix: extra filter dropdown on tickler manager page#2409
Conversation
…c filter dropdown to show unconditionally
📝 WalkthroughWalkthroughThe Tickler Manager page's "Filter" dropdown is now conditionally rendered based on the Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
|
@SourceryAI review |
Reviewer's guide (collapsed on small PRs)Reviewer's GuideReintroduces a JSP conditional so the tickler filter dropdown only renders on patient-specific Tickler Manager views, preventing a duplicate/non-functional filter on general views. Flow diagram for conditional rendering of ticklerview filter in JSPflowchart TD
A[Request ticklerMain.jsp] --> B{param.demoview is present and not empty}
B -- No --> C[Render page without ticklerview dropdown]
B -- Yes --> D[Render ticklerview dropdown
label Filter
options Active Completed Deleted]
D --> E[Render rest of ticklerMain.jsp]
C --> E
File-Level Changes
Assessment against linked issues
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.Snapshot WarningsEnsure that dependencies are being submitted on PR branches and consider enabling retry-on-snapshot-warnings. See the documentation for more information and troubleshooting advice. Scanned FilesNone |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Pull request overview
Fixes a UI regression in OpenO EMR’s Tickler Manager page where an extra (non-functional) “Filter” dropdown was appearing in the non-patient (main) tickler view by restoring the patient-context-only conditional rendering.
Changes:
- Wrap the secondary “Filter” dropdown (
ticklerview) in a${not empty param.demoview}conditional so it only renders on patient-specific tickler pages.
There was a problem hiding this comment.
Code Review
This pull request modifies 'ticklerMain.jsp' to conditionally display the tickler filter dropdown based on the presence of the 'demoview' parameter. The review feedback identifies redundant bundle declarations and code duplication within the filter options, recommending a more efficient and readable implementation.
Review Summary by QodoFix duplicate filter dropdown on tickler manager page
WalkthroughsDescription• Re-adds missing conditional check for patient-specific tickler manager
• Wraps filter dropdown with <c:if test="${not empty param.demoview}"> condition
• Prevents duplicate non-functional filter from appearing on non-patient pages
• Fixes regression from PR #2268 that removed the conditional logic
Diagramflowchart LR
A["Tickler Manager Page"] --> B{"Patient-specific<br/>demoview param?"}
B -->|Yes| C["Show Filter Dropdown"]
B -->|No| D["Hide Filter Dropdown"]
C --> E["Single functional filter"]
D --> E
File Changes1. src/main/webapp/tickler/ticklerMain.jsp
|
Code Review by Qodo
|
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- The
param.demoviewcheck is a bit of a magic condition here; consider centralizing the notion of "patient-specific tickler manager page" (e.g., via a helper or a clearly named flag) so this logic is easier to understand and reuse across the JSP.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The `param.demoview` check is a bit of a magic condition here; consider centralizing the notion of "patient-specific tickler manager page" (e.g., via a helper or a clearly named flag) so this logic is easier to understand and reuse across the JSP.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
…down-tickler-manager Fix: extra filter dropdown on tickler manager page
Summary
A regression was found relating to a missing tickler manager change in this PR: #2268, where an added conditional around the patient-specific tickler manager UI filter dropdown was removed, resulting in a duplicate filter dropdown that should only show in patient-specific tickler manager pages.
Fixes #2392
Problem
A conditional check for a patient-specific tickler manager dropdown has been removed, causing the dropdown to always show on the page, this is an issue as there is already a filter option for non-patients AND this duplicate filter doesn't work in that context, causing potential user confusion as to why one of the filter dropdowns is broken.
Solution
Re-add the missing conditional that conditionally shows the filter dropdown on patient-specific tickler manager pages
Manual testing steps
Summary by Sourcery
Bug Fixes:
Summary by cubic
Fixes a regression that showed a duplicate filter dropdown on the Tickler Manager. Re-wraps the "ticklerview" select in
ticklerMain.jspwith<c:if test="${not empty param.demoview}">so it only renders on patient-specific pages.Written for commit 3739743. Summary will update on new commits.
Summary by Sourcery
Bug Fixes:
Summary by CodeRabbit