include a teammember's trials when filtering by site#397
Conversation
IDENTITY_COURSE_LECTURER_ID_FILTER
applyIdentityLecturerCriterion()
WalkthroughRefactors DAO query construction to use explicit aliased joins and moves department/identity filters into new CriteriaUtil helper methods; adds two identity-filter override cases to the AuthorisationInterceptor; regenerates internal IDs in multiple UML profile files. (50 words) Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant DAO
participant Criteria as "Hibernate Criteria (aliased)"
participant CriteriaUtil
participant DB
Client->>DAO: call find...(params)
DAO->>Criteria: createCriteria(root, "alias")
alt department filter present
Criteria->>CriteriaUtil: applyIdentity*(aliasCriteria, orConstraint?)
CriteriaUtil->>Criteria: build DetachedCriteria exists(subQuery)
Criteria->>Criteria: add(existsCriterion)
else no identity
Criteria->>Criteria: add(orConstraint if present)
end
Criteria->>DB: execute query
DB-->>DAO: results
DAO-->>Client: return results
sequenceDiagram
participant Request
participant AuthInterceptor
participant IdentityStore
participant Params
Request->>AuthInterceptor: parameter override request (IDENTITY_*)
AuthInterceptor->>IdentityStore: fetch current identity
alt identity exists
AuthInterceptor->>Params: set write access & set parameterValues [pathRoot, id]
AuthInterceptor-->>Request: continue with override
else no identity
AuthInterceptor-->>Request: throw NOT_IDENTITY error
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
core/src/main/java/org/phoenixctms/ctsms/query/CriteriaUtil.java (1)
20-33: Identity-based Criteria helpers look correct; optional small refactor onlyThe new
applyIdentityTeamMemberCriterion/applyIdentityLecturerCriterionmethods correctly:
- Guard on a non-null
Staffidentity,- Tie
TeamMemberImpl.trial.id/LecturerImpl.course.idto the passed criteria alias viaeqProperty,- Fall back to the provided
orcriterion when identity is absent.If you want to reduce duplication later, you could extract a small private helper that takes the target class and association property (
"trial.id"/"course.id") as parameters, but it’s not necessary for correctness.Also applies to: 132-164
core/src/main/java/org/phoenixctms/ctsms/domain/InventoryBookingDaoImpl.java (1)
147-151: Redundant second join oncourse; consider reusing the existing criteriaIn
handleFindByCourseParticipantDepartmentCategoryInterval, you first create:Criteria courseCriteria = bookingCriteria.createCriteria("course", CriteriaSpecification.INNER_JOIN);Then, for the department filter, you create another criteria on
"course"with alias"inventoryCourse"just forapplyIdentityLecturerCriterion:CriteriaUtil.applyIdentityLecturerCriterion( bookingCriteria.createCriteria("course", "inventoryCourse", CriteriaSpecification.LEFT_JOIN), Restrictions.eq("department.id", courseDepartmentId.longValue()));This results in two joins on the same association, which is likely unnecessary and can complicate the generated SQL and impact performance.
You can probably reuse
courseCriteria(with or without an explicit alias) and avoid the extra LEFT_JOIN, for example:- Criteria courseCriteria = bookingCriteria.createCriteria("course", CriteriaSpecification.INNER_JOIN); + Criteria courseCriteria = bookingCriteria.createCriteria("course", CriteriaSpecification.INNER_JOIN); if (courseDepartmentId != null) { - CriteriaUtil.applyIdentityLecturerCriterion( - bookingCriteria.createCriteria("course", "inventoryCourse", CriteriaSpecification.LEFT_JOIN), - Restrictions.eq("department.id", courseDepartmentId.longValue())); + CriteriaUtil.applyIdentityLecturerCriterion( + courseCriteria, + Restrictions.eq("department.id", courseDepartmentId.longValue())); }(Adjust aliasing if other usages in this PR depend on a specific alias name.)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
core/src/main/java/org/phoenixctms/ctsms/domain/CourseDaoImpl.java(1 hunks)core/src/main/java/org/phoenixctms/ctsms/domain/DutyRosterTurnDaoImpl.java(3 hunks)core/src/main/java/org/phoenixctms/ctsms/domain/InventoryBookingDaoImpl.java(3 hunks)core/src/main/java/org/phoenixctms/ctsms/domain/MoneyTransferDaoImpl.java(4 hunks)core/src/main/java/org/phoenixctms/ctsms/domain/TimelineEventDaoImpl.java(3 hunks)core/src/main/java/org/phoenixctms/ctsms/domain/TrialDaoImpl.java(2 hunks)core/src/main/java/org/phoenixctms/ctsms/domain/VisitScheduleItemDaoImpl.java(2 hunks)core/src/main/java/org/phoenixctms/ctsms/intercept/AuthorisationInterceptor.java(1 hunks)core/src/main/java/org/phoenixctms/ctsms/query/CriteriaUtil.java(2 hunks)mda/src/main/uml/UML_Standard_Profile.StandardProfileL3.profile.uml(1 hunks)mda/src/main/uml/andromda-common.andromda-service.profile.uml(7 hunks)mda/src/main/uml/andromda-presentation.profile.uml(7 hunks)mda/src/main/uml/andromda-process.profile.uml(4 hunks)mda/src/main/uml/andromda-webservice.profile.uml(5 hunks)mda/src/main/uml/andromda-xml.profile.uml(4 hunks)
🧰 Additional context used
🧬 Code graph analysis (7)
core/src/main/java/org/phoenixctms/ctsms/domain/CourseDaoImpl.java (1)
core/src/main/java/org/phoenixctms/ctsms/query/CriteriaUtil.java (1)
CriteriaUtil(57-1148)
core/src/main/java/org/phoenixctms/ctsms/domain/TrialDaoImpl.java (1)
core/src/main/java/org/phoenixctms/ctsms/query/CriteriaUtil.java (1)
CriteriaUtil(57-1148)
core/src/main/java/org/phoenixctms/ctsms/domain/DutyRosterTurnDaoImpl.java (1)
core/src/main/java/org/phoenixctms/ctsms/query/CriteriaUtil.java (1)
CriteriaUtil(57-1148)
core/src/main/java/org/phoenixctms/ctsms/domain/VisitScheduleItemDaoImpl.java (1)
core/src/main/java/org/phoenixctms/ctsms/query/CriteriaUtil.java (1)
CriteriaUtil(57-1148)
core/src/main/java/org/phoenixctms/ctsms/intercept/AuthorisationInterceptor.java (1)
core/src/main/java/org/phoenixctms/ctsms/util/L10nUtil.java (1)
L10nUtil(48-1767)
core/src/main/java/org/phoenixctms/ctsms/domain/InventoryBookingDaoImpl.java (1)
core/src/main/java/org/phoenixctms/ctsms/query/CriteriaUtil.java (1)
CriteriaUtil(57-1148)
core/src/main/java/org/phoenixctms/ctsms/domain/MoneyTransferDaoImpl.java (1)
core/src/main/java/org/phoenixctms/ctsms/query/CriteriaUtil.java (1)
CriteriaUtil(57-1148)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Build and Test
🔇 Additional comments (25)
mda/src/main/uml/UML_Standard_Profile.StandardProfileL3.profile.uml (1)
3-11: Auto-generated internal ID changes detected.These changes consist solely of regenerated
xmi:idattributes in eAnnotations and metadata elements. The element names, structure, and external references remain unchanged. Such ID regeneration typically occurs when UML modeling tools (e.g., MagicDraw) re-serialize model files.If these changes are unintentional (i.e., side-effect of opening the model in an IDE), consider reverting to reduce diff noise. If intentional (e.g., model cleanup or tool upgrade), this is fine to proceed.
mda/src/main/uml/andromda-process.profile.uml (2)
4-93: Internal identifier regeneration throughout the profile.All changes in this file are internal
xmi:idattribute updates for eAnnotations, eClassifiers, and eStructuralFeatures. The stereotype names, attribute names, type references, and profile structure remain semantically identical.
199-278: Extension memberEnd order adjustments and metadata ID updates.The
memberEndattribute order swaps in several Extension elements (e.g., lines 199, 205, 211, 217, 235) and the profileApplication/ModelLibrary ID updates are cosmetic changes that don't affect the functional behavior of the UML profile extensions.mda/src/main/uml/andromda-xml.profile.uml (2)
4-86: Internal ID regeneration for XML profile classifiers.The eAnnotations, eEnum, and eClass elements for XML-related stereotypes (XmlNsForm, TernaryBoolean, XmlAttribute, XmlSchemaType, XmlElement, XmlSchema, NoXml) have regenerated
xmi:idvalues. All element names and type references are preserved.
108-265: Extension and metadata updates are cosmetic.The memberEnd order adjustments and profileApplication/ModelLibrary ID changes follow the same pattern as other profile files and don't affect functionality.
mda/src/main/uml/andromda-webservice.profile.uml (2)
4-337: Extensive internal ID regeneration for webservice profile elements.This file contains the most changes, but all are
xmi:idattribute updates for the webservice-related enumerations (WebServiceStyle, WebServiceUse, WebServiceProvider, TernaryBoolean, HttpRequestType, ProviderMediaType, WSSecurityType, ParamType, WebServiceParameterStyle) and stereotypes (WebService, WebServiceOperation, WebFault, WebServiceParam, WebServiceFeed, WSSecurity, WSCustomHeader, WSAddressing, WSPolicy). All element names, literals, and structural features are preserved.
743-961: Extension memberEnd reordering and metadata ID updates.The memberEnd order adjustments in Extension elements and profileApplication/ModelLibrary ID changes are consistent with the pattern observed across all profile files in this PR.
mda/src/main/uml/andromda-presentation.profile.uml (2)
4-202: Internal ID regeneration for presentation profile elements.All eAnnotations, eEnum, and eClass elements for presentation-related stereotypes (WebActionFormScope, WebFieldType, WebActionType, WebExportType, ViewType, TernaryBoolean, FrontEnd* stereotypes) have regenerated
xmi:idvalues while preserving all element names, attribute definitions, and type references.
541-648: Extension and metadata updates consistent with other profiles.The memberEnd order adjustments and profileApplication/ModelLibrary ID updates follow the same pattern seen across all UML profile files in this PR.
mda/src/main/uml/andromda-common.andromda-service.profile.uml (1)
1-385: Based on the execution results and additional verification, I can now provide the final review comment.
UML profile regeneration is intentional and consistent with the PR objective.
The profile regeneration is confirmed to be part of the feature implementation. The commit message "add IDENTITY_TRIAL_TEAM_MEMBER_ID_FILTER" directly matches the PR objective about including team member trials when filtering by site. Both the service and common profiles were regenerated together as part of this feature addition.
The changes consist entirely of internal identifier regeneration (xmi:id attributes and cross-references). No public element names, enumeration values, or semantics were altered. This regeneration is standard behavior when UML modeling tools save profile modifications.
core/src/main/java/org/phoenixctms/ctsms/domain/TimelineEventDaoImpl.java (3)
82-101: Identity-aware department filter for trial join looks correctExplicit alias
"timelineEventTrial"andCriteriaUtil.applyIdentityTeamMemberCriterionare used consistently here, so department filtering now also includes trials where the current identity is a team member, while preserving the existingtrialId/ member filters. This matches the pattern used elsewhere in the PR.
116-135: Consistent identity-based department constraint in status/type queryThe same aliased join plus
applyIdentityTeamMemberCriterionpattern is applied for the status/type query, keeping semantics aligned with the interval method while adding identity-based access.
143-171: SubCriteriaMap usage with identity filter is coherentUsing
criteriaMap.createCriteria("trial", "timelineEventTrial")as the target forapplyIdentityTeamMemberCriterionis consistent with the helper’s alias expectations and with surrounding criteria-map usage; no issues seen.core/src/main/java/org/phoenixctms/ctsms/domain/CourseDaoImpl.java (1)
261-274: Identity-based lecturer extension of department filter is implemented cleanlyCreating the criteria with alias
"course"and routing the department restriction throughCriteriaUtil.applyIdentityLecturerCriterionis consistent with the new identity pattern and should correctly broaden results to include courses where the current identity is a lecturer even outside the selected department.core/src/main/java/org/phoenixctms/ctsms/domain/VisitScheduleItemDaoImpl.java (2)
460-482: Trial join alias and identity team‑member criterion are wired correctlyUsing alias
"visitScheduleItemTrial"for the trial join and passingCriteriaUtil.applyOr(..., idCriterion)intoapplyIdentityTeamMemberCriterionkeeps the previous(department OR id)behaviour while extending it to include trials where the current identity is a team member. The reuse ofidCriterionin both the trialId and department clauses also preserves the “always include this id if specified” semantics.
488-511: Status/type interval now respects identity for department-filtered trialsThe aliased
"visitScheduleItemTrial"join plusapplyIdentityTeamMemberCriterion(trialCriteria, Restrictions.eq("department.id", ...))matches the helper’s expectations and the pattern used elsewhere in the PR; the added identity-based OR is the only behavioural change here.core/src/main/java/org/phoenixctms/ctsms/intercept/AuthorisationInterceptor.java (1)
371-441: New identity-based filter overrides follow the established patternThe
IDENTITY_TRIAL_TEAM_MEMBER_ID_FILTERandIDENTITY_COURSE_LECTURER_ID_FILTERbranches mirror the existing*_DEPARTMENT_ID_FILTERimplementations: they enforce presence of an identity, seed aPSFVOwhen needed, and inject the appropriate property path plus the identity id as a string. This looks consistent with how other parameter overrides are applied.Please double-check that:
- The
ServiceMethodParameterOverrideenum declares these exact constants.- Any permissions or service-method metadata that should use these overrides have been updated accordingly.
core/src/main/java/org/phoenixctms/ctsms/domain/TrialDaoImpl.java (2)
73-89: Department payoffs query correctly extended with identity-team-member semanticsSwitching to
createTrialCriteria("trial")and wrapping the department restriction inCriteriaUtil.applyIdentityTeamMemberCriterionis consistent with the helper’s design and with other DAOs, broadening results to “department OR identity is team member” without touching payoff logic.
102-118: ECRF-based trial lookup uses alias + identity helper coherentlyUsing alias
"trial0"for both the main criteria and the ECRF subquery, and applyingapplyIdentityTeamMemberCriterionon the department filter, keeps the subquery correlation intact while adding the intended identity-based trial inclusion.core/src/main/java/org/phoenixctms/ctsms/domain/DutyRosterTurnDaoImpl.java (3)
204-244: Duty-roster interval query: identity-aware trialDepartment filter is soundThe aliased
"dutyRosterTurnTrial"join plusapplyIdentityTeamMemberCriterionwrappingRestrictions.or(isNull("dutyRosterTurn.trial"), department match)preserves the old “include unassigned or matching department” behaviour and extends it to also include trials where the current identity is a team member. This looks correct.
291-319: Calendar autocomplete correctly reuses the identity-team-member patternFor calendar lookup, the same aliased join and OR-wrapped department condition are fed through
applyIdentityTeamMemberCriterion, giving consistent identity-based broadening of trialDepartment-filtered results.
321-350: Title autocomplete adopts the same identity-aware department filterThe title autocomplete query mirrors the calendar query’s use of
"dutyRosterTurnTrial"andapplyIdentityTeamMemberCriterion, so behaviour stays consistent across both endpoints.core/src/main/java/org/phoenixctms/ctsms/domain/MoneyTransferDaoImpl.java (2)
97-129: Cost-type helper now cleanly incorporates identity-team-member trialsChanging the trial join to
"moneyTransferTrial"and wrappingRestrictions.or(isNull("moneyTransfer.trial"), department match)withCriteriaUtil.applyIdentityTeamMemberCriterionpreserves the previous department/null-trial logic while additionally permitting trials where the current identity is in the team. Helper behaviour remains backwards-compatible aside from the intended identity broadening.
157-197: Finder by proband/trial/method/costType uses aliased trial join with identity helper correctlyIn
handleFindByProbandTrialMethodCostTypePaidPerson, the aliased"moneyTransferTrial"join viacriteriaMap.createCriteriaand subsequent call toapplyIdentityTeamMemberCriterionmirror the helper usage above and should behave identically, while leaving proband/“person” filtering and PSF application unchanged.core/src/main/java/org/phoenixctms/ctsms/domain/InventoryBookingDaoImpl.java (1)
183-185: Identity-based trial department filter is consistent with helper semanticsUsing
CriteriaUtil.applyIdentityTeamMemberCriterionhere cleanly encapsulates the new behavior:
- If
trialDepartmentIdis provided and the user has an identity, results match either the chosen department or trials where the user is a team member.- If there is no identity, behavior degrades to the original simple
department.id = trialDepartmentIdfilter.The alias
"inventoryTrial"is also compatible with the helper’seqProperty("trial.id", alias + ".id")logic.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
core/src/main/java/org/phoenixctms/ctsms/domain/InventoryBookingDaoImpl.java (1)
147-151: Avoid duplicatingcoursejoins when applying lecturer identity filterIn this method you now have:
- an inner join
courseCriteria = bookingCriteria.createCriteria("course", CriteriaSpecification.INNER_JOIN);- plus a second left join
bookingCriteria.createCriteria("course", "inventoryCourse", CriteriaSpecification.LEFT_JOIN)used only for the identity/dept restriction.This creates two joins to the same association where one would suffice and slightly complicates the generated SQL.
Consider reusing the existing
courseCriteria(possibly giving it an alias) when applying the identity criterion, e.g.:- Criteria courseCriteria = bookingCriteria.createCriteria("course", CriteriaSpecification.INNER_JOIN); - if (courseDepartmentId != null) { - CriteriaUtil.applyIdentityLecturerCriterion(bookingCriteria.createCriteria("course", "inventoryCourse", CriteriaSpecification.LEFT_JOIN), - Restrictions.eq("department.id", courseDepartmentId.longValue())); - } + Criteria courseCriteria = bookingCriteria.createCriteria("course", CriteriaSpecification.INNER_JOIN); + if (courseDepartmentId != null) { + CriteriaUtil.applyIdentityLecturerCriterion( + courseCriteria, + Restrictions.eq("department.id", courseDepartmentId.longValue())); + }This keeps behavior while avoiding a redundant join.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
core/src/main/java/org/phoenixctms/ctsms/domain/InventoryBookingDaoImpl.java(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
core/src/main/java/org/phoenixctms/ctsms/domain/InventoryBookingDaoImpl.java (1)
core/src/main/java/org/phoenixctms/ctsms/query/CriteriaUtil.java (1)
CriteriaUtil(57-1148)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Build and Test
🔇 Additional comments (2)
core/src/main/java/org/phoenixctms/ctsms/domain/InventoryBookingDaoImpl.java (2)
90-97: Identity-based department filters for course/trial look correctUsing
CriteriaUtil.applyIdentityLecturerCriterionandCriteriaUtil.applyIdentityTeamMemberCriterionwith an OR on(department IS NULL OR department = ?)matches the intent to include both site-based appointments and those linked to the current lecturer/teammember. The null-identity fallback keeps the legacy department-only behavior. This is consistent and safe.
183-184: Team-member–aware trial department filtering is consistent with helper semanticsSwitching the trial-department branch to
CriteriaUtil.applyIdentityTeamMemberCriterionwith adepartment.idrestriction cleanly extends the query to include both:
- trials at the given department, and
- trials where the current identity is a team member (when identity is present),
while preserving the original behavior when identity is absent. This aligns with the stated PR objective.
1 participant
Summary by CodeRabbit
Refactor
Chores
✏️ Tip: You can customize this high-level summary in your review settings.