Conversation
WalkthroughThis PR introduces user-scoped calendar filter persistence through a new Changes
Sequence Diagram(s)sequenceDiagram
participant UI as UI/Form
participant SB as SessionScopeBean
participant USB as UserSettingsBean
participant DAO as UserDaoImpl
participant DB as Database
UI->>USB: Load user settings
USB->>DAO: Load user from DB
DAO->>DB: Query User + CategoryPreset
DB-->>DAO: User + filters
DAO->>DAO: Convert to UserInVO<br/>(entityToValueCollection)
DAO-->>USB: UserInVO w/ filters
USB->>USB: Initialize filter sets<br/>from UserInVO
USB-->>UI: Display filters
UI->>USB: User modifies filters
USB->>USB: Update filter sets
UI->>USB: Save settings
USB->>DAO: sanitizeInVals()<br/>calls setUser*Filters
DAO->>DAO: Convert filters to<br/>CategoryPreset entities
DAO->>DB: Persist User + CategoryPreset
DB-->>DAO: ✓ Saved
DAO-->>USB: ✓ Complete
SB->>SB: Cache filters for<br/>schedule models
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Areas requiring extra attention:
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)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ast-grep (0.40.0)core/src/exec/java/org/phoenixctms/ctsms/executable/DemoDataProvider.javaTip 📝 Customizable high-level summaries are now available in beta!You can now customize how CodeRabbit generates the high-level summary in your pull requests — including its content, structure, tone, and formatting.
Example instruction:
Note: This feature is currently in beta for Pro-tier users, and pricing will be announced later. 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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
web/src/main/java/org/phoenixctms/ctsms/web/model/trial/DutyRosterLazyScheduleModel.java (1)
119-131: Guardcalendarsinitialization against missing session or filtersInitializing
calendars = new LinkedHashSet<String>(WebUtil.getSessionScopeBean().getDutyRosterCalendarFilters());will throw a
NullPointerExceptionif eitherWebUtil.getSessionScopeBean()returnsnull(e.g., no HTTP session / tests) or itsgetDutyRosterCalendarFilters()returnsnull.Safer initialization that still seeds from the session could be:
- StaffOutVO identity = WebUtil.getUserIdentity(); + StaffOutVO identity = WebUtil.getUserIdentity(); if (identity != null) { ... } - calendars = new LinkedHashSet<String>(WebUtil.getSessionScopeBean().getDutyRosterCalendarFilters()); + calendars = new LinkedHashSet<String>(); + Set<String> defaultCalendars = null; + if (WebUtil.getSessionScopeBean() != null) { + defaultCalendars = WebUtil.getSessionScopeBean().getDutyRosterCalendarFilters(); + } + if (defaultCalendars != null) { + calendars.addAll(defaultCalendars); + }This preserves behavior when defaults exist but avoids constructor‑time NPEs in edge cases.
🧹 Nitpick comments (12)
web/src/main/java/org/phoenixctms/ctsms/web/model/inventory/InventoryBookingLazyScheduleModel.java (1)
134-135: Guard against possible nullSessionScopeBeanwhen seeding calendars
WebUtil.getSessionScopeBean()can returnnull(see its overload takingHttpSession), soWebUtil.getSessionScopeBean().getInventoryBookingCalendarFilters()risks an NPE outside a fully initialized JSF session. A small guard keeps behavior but hardens the code.- calendars = new LinkedHashSet<String>(WebUtil.getSessionScopeBean().getInventoryBookingCalendarFilters()); + org.phoenixctms.ctsms.web.model.SessionScopeBean sessionBean = WebUtil.getSessionScopeBean(); + Collection<String> sessionCalendars = sessionBean != null + ? sessionBean.getInventoryBookingCalendarFilters() + : Collections.<String>emptyList(); + calendars = new LinkedHashSet<String>(sessionCalendars);core/db/schema-drop.sql (1)
17-22: CATEGORY_PRESET teardown is ordered correctly; ensure constraint names match createsDropping the two
CATEGORY_PRESET_*_USER_FKCconstraints beforedrop table CATEGORY_PRESET cascadefits the existing drop pattern and should avoid dependency issues.It’s worth confirming the constraint names here are exactly the ones used in
schema-create.sql/schema-up-master.sqlforCATEGORY_PRESET, to avoid surprises on teardown.Also applies to: 1028-1029
core/src/main/java/org/phoenixctms/ctsms/service/user/UserServiceImpl.java (1)
21-25: Calendar preset cleanup on user delete is consistent with existing patternsThe new
CategoryPresetimports and the delete‑time cleanup ofDutyRosterCalendarFiltersandInventoryBookingCalendarFiltersmatch how passwords, permission profiles, and table columns are handled and should prevent FK violations againstCATEGORY_PRESET.Two minor points to consider:
- This assumes the two filter collections are always non‑null. If the mapping doesn’t guarantee that, adding a simple null check before iterating would be safer.
- If more
CategoryPreset‑based associations are added later, extracting this repeated removal logic into a small helper (similar todeleteDataTableColumns) would keephandleDeleteUserfrom growing further.Also applies to: 273-287
web/src/main/java/org/phoenixctms/ctsms/web/model/user/UserBean.java (1)
1176-1200: Make calendar filter setters tolerant of null to avoid potential NPEs
setUserDutyRosterCalendarFiltersandsetUserInventoryBookingCalendarFiltersassume the incomingfilterscollection is non‑null. IfUserSettingsBeanBaseor UI binding ever passesnullto mean “no filters”,addAll(filters)will throw an NPE.A small guard keeps this safe without changing semantics (null just maps to an empty collection):
- @Override - protected void setUserDutyRosterCalendarFilters(Collection<String> filters) { - if (in != null) { - in.getDutyRosterCalendarFilters().clear(); - in.getDutyRosterCalendarFilters().addAll(filters); - } - } + @Override + protected void setUserDutyRosterCalendarFilters(Collection<String> filters) { + if (in != null) { + in.getDutyRosterCalendarFilters().clear(); + if (filters != null) { + in.getDutyRosterCalendarFilters().addAll(filters); + } + } + } @@ - @Override - protected void setUserInventoryBookingCalendarFilters(Collection<String> filters) { - if (in != null) { - in.getInventoryBookingCalendarFilters().clear(); - in.getInventoryBookingCalendarFilters().addAll(filters); - } - } + @Override + protected void setUserInventoryBookingCalendarFilters(Collection<String> filters) { + if (in != null) { + in.getInventoryBookingCalendarFilters().clear(); + if (filters != null) { + in.getInventoryBookingCalendarFilters().addAll(filters); + } + } + }core/src/main/java/org/phoenixctms/ctsms/vocycle/UserReflexionGraph.java (1)
196-210: Tighten generics and simplifyentityToValueCollectionThe helper works but mixes raw collections and generics and relies on runtime type checks:
public static ArrayList entityToValueCollection(Collection items)and rawArrayList resultwill generate unchecked warnings.items.size()will NPE ifitemsis evernull.- The manual
instanceofguard becomes unnecessary if the API is typed toCategoryPreset.A more type‑safe and compact version would be along these lines:
- public static ArrayList entityToValueCollection(Collection items) { - ArrayList result = new ArrayList<String>(items.size()); - items = new ArrayList(items); - Collections.sort((ArrayList) items, CATEGORY_PRESET_ID_COMPARATOR); - Iterator it = items.iterator(); - while (it.hasNext()) { - Object item = it.next(); - if (item instanceof CategoryPreset) { - result.add(((CategoryPreset) item).getCategory()); - } else { - throw new UnsupportedOperationException("cannot convert " + item.getClass()); - } - } - return result; - } + public static ArrayList<String> entityToValueCollection(Collection<? extends CategoryPreset> items) { + if (items == null || items.isEmpty()) { + return new ArrayList<String>(0); + } + ArrayList<CategoryPreset> sorted = new ArrayList<CategoryPreset>(items); + Collections.sort(sorted, CATEGORY_PRESET_ID_COMPARATOR); + ArrayList<String> result = new ArrayList<String>(sorted.size()); + for (CategoryPreset preset : sorted) { + result.add(preset.getCategory()); + } + return result; + }This keeps the ID‑based sorting behavior while removing raw types and hardening against nulls.
core/db/schema-create.sql (1)
124-131: CATEGORY_PRESET table and FKs are coherent; consider encoding semanticsThe table definition and foreign keys are syntactically correct and consistent with the rest of the schema (no cascades, nullable FKs referencing
users).If the intent is that a given preset belongs to exactly one “role” (duty‑roster calendar vs. inventory‑booking calendar), you might later consider a check constraint to enforce that at most one of
DUTY_ROSTER_CALENDAR_FILTER_USER_FK/INVENTORY_BOOKING_CALENDAR_FILTER_USER_FKis non‑null, or split into two tables. Right now the DB allows rows with both or neither set, leaving enforcement purely to application code.Also applies to: 2305-2313
web/src/main/resources/org/phoenixctms/ctsms/web/user/labels.properties (1)
170-177: Inventory preset label naming slightly inconsistent with duty roster labelThe English duty roster label uses “calendar filter preset” while the inventory one uses just “calendar preset”. Consider aligning wording (e.g., “Inventory booking calendar filter preset”) for consistency with the duty roster and the German texts.
core/src/main/java/org/phoenixctms/ctsms/domain/UserDaoImpl.java (1)
294-321: Repeated remove/recreate logic for CategoryPreset collections and possible null filtersThe three blocks converting VO filter strings back to
CategoryPresetentities all duplicate the same pattern (remove old, clear, recreate each time). That’s correct but a bit brittle and noisy.Two follow-ups to consider:
- Extract a small private helper like
updateCalendarFilterPresets(Collection<String> filters, Collection<CategoryPreset> target, Consumer<CategoryPreset> ownerSetter)to centralize this logic and avoid future drift between the three call sites.- Optionally guard against
nullfilter collections on the VO side (if (filters != null && !filters.isEmpty())) to avoid surprises if a caller ever passesnullinstead of an empty collection.Also applies to: 376-403, 448-475
web/src/main/java/org/phoenixctms/ctsms/web/model/user/UserSettingsBean.java (1)
252-276: Make calendar filter setters more defensive against null collectionsThe new protected setters mutate
in’s collections viaclear()+addAll(filters). This matches existing patterns but will NPE iffiltersis evernull.You could make them slightly more robust with an explicit null check:
- if (in != null) { - in.getDutyRosterCalendarFilters().clear(); - in.getDutyRosterCalendarFilters().addAll(filters); - } + if (in != null) { + in.getDutyRosterCalendarFilters().clear(); + if (filters != null) { + in.getDutyRosterCalendarFilters().addAll(filters); + } + }(and analogously for
inventoryBookingCalendarFilters).web/src/main/webapp/META-INF/includes/shared/userSettingsBaseInput.xhtml (1)
246-291: New calendar filter preset UI wiring looks correct; consider removing dead ajax handlersThe inherit checkboxes, tooltips, and
p:autoCompletecomponents fordutyRosterCalendarFiltersandinventoryBookingCalendarFiltersare wired consistently with existing settings (bindings,inheritedPropertiesMap, labels, and tooltips all line up).You’ve left commented-out
p:ajaxitemSelect/itemUnselecthandlers inside both autocompletes. If these callbacks are no longer needed, consider removing the commented blocks to keep the template easier to maintain.Also applies to: 293-339
web/src/main/java/org/phoenixctms/ctsms/web/model/shared/UserSettingsBeanBase.java (2)
297-298: Avoid rawCollectioncast in tooltip handling for inherited collectionsThe
Collectionbranch ingetInheritedPropertyTooltipcurrently uses a raw cast:} else if (inheritedValue instanceof Collection) { inheritedValue = String.join(", ", (Collection) inheritedValue); }This compiles with an unchecked warning and can throw a
ClassCastExceptionat runtime if the collection contains non‑CharSequenceelements. A safer approach is to work withCollection<?>and convert elements toStringexplicitly:- } else if (inheritedValue instanceof Collection) { - inheritedValue = String.join(", ", (Collection) inheritedValue); + } else if (inheritedValue instanceof Collection<?>) { + Collection<?> collection = (Collection<?>) inheritedValue; + ArrayList<String> values = new ArrayList<String>(collection.size()); + for (Object element : collection) { + if (element != null) { + values.add(String.valueOf(element)); + } + } + inheritedValue = String.join(", ", values);This removes the raw-type warning and is robust even if future inherited properties expose collections of non‑String elements.
391-395: Consider removing commented‑out handler stubsThe commented
handle*CalendarFilterSelect/Unselectmethods look like unused stubs. If there’s no immediate plan to wire them into the UI, consider deleting them to keep the base class lean; otherwise, adding a brief comment about planned usage would clarify why they’re retained.Also applies to: 427-431
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (29)
common/src/main/java/org/phoenixctms/ctsms/util/CommonUtil.java(1 hunks)core/db/schema-create.sql(2 hunks)core/db/schema-drop.sql(2 hunks)core/db/schema-set-version.sql(1 hunks)core/db/schema-up-master.sql(1 hunks)core/src/exec/java/org/phoenixctms/ctsms/executable/DemoDataProvider.java(1 hunks)core/src/main/java/org/phoenixctms/ctsms/domain/UserDaoImpl.java(8 hunks)core/src/main/java/org/phoenixctms/ctsms/service/user/UserServiceImpl.java(2 hunks)core/src/main/java/org/phoenixctms/ctsms/vocycle/UserReflexionGraph.java(4 hunks)mda/src/main/uml/UML_Standard_Profile.StandardProfileL3.profile.uml(1 hunks)mda/src/main/uml/andromda-common.andromda-service.profile.uml(6 hunks)mda/src/main/uml/andromda-presentation.profile.uml(5 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(5 hunks)web/src/main/java/org/phoenixctms/ctsms/web/model/SessionScopeBean.java(1 hunks)web/src/main/java/org/phoenixctms/ctsms/web/model/inventory/InventoryBookingLazyScheduleModel.java(1 hunks)web/src/main/java/org/phoenixctms/ctsms/web/model/shared/UserSettingsBeanBase.java(7 hunks)web/src/main/java/org/phoenixctms/ctsms/web/model/trial/DutyRosterLazyScheduleModel.java(1 hunks)web/src/main/java/org/phoenixctms/ctsms/web/model/user/UserBean.java(4 hunks)web/src/main/java/org/phoenixctms/ctsms/web/model/user/UserSettingsBean.java(2 hunks)web/src/main/java/org/phoenixctms/ctsms/web/util/DefaultSettings.java(2 hunks)web/src/main/java/org/phoenixctms/ctsms/web/util/SettingCodes.java(2 hunks)web/src/main/resources/org/phoenixctms/ctsms/web/settings.properties(2 hunks)web/src/main/resources/org/phoenixctms/ctsms/web/user/labels.properties(1 hunks)web/src/main/resources/org/phoenixctms/ctsms/web/user/labels_de.properties(1 hunks)web/src/main/webapp/META-INF/includes/shared/userSettingsBaseInput.xhtml(10 hunks)web/src/main/webapp/META-INF/includes/user/userMain.xhtml(1 hunks)web/src/main/webapp/user/changeSettings.xhtml(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (7)
web/src/main/java/org/phoenixctms/ctsms/web/model/trial/DutyRosterLazyScheduleModel.java (1)
web/src/main/java/org/phoenixctms/ctsms/web/util/WebUtil.java (1)
WebUtil(75-5208)
web/src/main/java/org/phoenixctms/ctsms/web/model/SessionScopeBean.java (2)
web/src/main/java/org/phoenixctms/ctsms/web/util/Settings.java (1)
Settings(34-372)web/src/main/java/org/phoenixctms/ctsms/web/util/DefaultSettings.java (1)
DefaultSettings(24-803)
core/src/main/java/org/phoenixctms/ctsms/domain/UserDaoImpl.java (2)
core/src/main/java/org/phoenixctms/ctsms/vocycle/UserReflexionGraph.java (1)
UserReflexionGraph(34-274)common/src/main/java/org/phoenixctms/ctsms/util/CommonUtil.java (1)
CommonUtil(113-2756)
web/src/main/java/org/phoenixctms/ctsms/web/model/inventory/InventoryBookingLazyScheduleModel.java (1)
web/src/main/java/org/phoenixctms/ctsms/web/util/WebUtil.java (1)
WebUtil(75-5208)
web/src/main/java/org/phoenixctms/ctsms/web/model/shared/UserSettingsBeanBase.java (1)
web/src/main/java/org/phoenixctms/ctsms/web/util/WebUtil.java (1)
WebUtil(75-5208)
web/src/main/java/org/phoenixctms/ctsms/web/model/user/UserBean.java (2)
web/src/main/java/org/phoenixctms/ctsms/web/util/Settings.java (1)
Settings(34-372)web/src/main/java/org/phoenixctms/ctsms/web/util/DefaultSettings.java (1)
DefaultSettings(24-803)
core/src/main/java/org/phoenixctms/ctsms/vocycle/UserReflexionGraph.java (1)
core/src/main/java/org/phoenixctms/ctsms/compare/UserComparator.java (1)
UserComparator(7-26)
⏰ 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 (40)
web/src/main/webapp/META-INF/includes/user/userMain.xhtml (1)
664-666: Consistent use of*-top-columnclasses for user settings gridSwitching
columnClasseshere to thectsms-*-top-columnvariants aligns the user settings layout with the modules grid above and should give consistent top-aligned checkbox/label/input/message rows. No functional impact, and the change looks correct as long as the corresponding CSS classes are defined (which they already appear to be in related usages).mda/src/main/uml/UML_Standard_Profile.StandardProfileL3.profile.uml (1)
3-11: XMI/eAnnotation ID refresh only; semantic identifiers preservedThe updated
xmi:idvalues oneAnnotations,contents, andStandard:ModelLibrarykeepmd_idandhreftargets unchanged, so the profile’s semantic identity and imports remain intact. No issues from a codegen/tooling perspective.Also applies to: 265-265
mda/src/main/uml/andromda-xml.profile.uml (3)
4-88: Ecore package/enum/class IDs and nsURI regenerated; structure intactThe
EPackage,EEnum, andEClassdefinitions are re‑emitted with newxmi:ids and an updatednsURI, but:
- All classifier names, literals, and
referencesto UML elements are preserved.- EAttributes/EReferences still point to the same UML metaclasses and enums.
Functionally the profile structure is unchanged. Please just confirm no external EMF tooling or serialized instances rely on the old
nsURIstring.
120-144: Extension memberEnd rewrites are consistent with existing stereotypesThe
uml:Extensionelements around the XmlSchema/NoXml/XmlAttribute stereotypes only adjustmemberEndordering and IDs; the extension ends still reference the same stereotype and base UML metaclasses. No semantic change detected.
259-265: Profile application and ModelLibrary IDs updated safelyThe new
eAnnotationsid onprofileApplicationand theStandard:ModelLibraryid simply refresh internal XMI identifiers while still targeting the same Standard profile and base package. Safe housekeeping.mda/src/main/uml/andromda-webservice.profile.uml (3)
4-337: Ecore meta updated; stereotype semantics and mappings preservedThe regenerated
EPackagecontent (enums likeWebServiceStyle,ProviderMediaType,HttpRequestType, and classes likeWebService,WebServiceOperation,WebServiceParam,WebServiceFeed,WSSecurity, etc.):
- Keeps all classifier names and enum literals unchanged.
- Maintains
eAnnotations referencesto the same UML enumerations/stereotypes.- Keeps all EReferences pointing at the expected UML metaclasses (
Class,Operation,Parameter, etc.).Notably,
andromda_REST_suspendinWebServiceOperationis explicitly typed asEIntwith default0, which better matches the UMLIntegerattribute and should improve generated typings.Overall this looks like tooling‑driven ID/typing normalization. Please re‑run your MDA/codegen pipeline to ensure no EMF client was depending on the previous (possibly looser) Ecore typing.
743-767: Extension memberEnd adjustments remain consistent with their stereotypesThe
uml:Extensionelements forWebServiceParam,WebServiceFeed,WebService,WebServiceOperation,WebFault,WSCustomHeader,WSSecurity,WSAddressing, andWSPolicyonly:
- Reorder
memberEndentries and- Make the extension end the
navigableOwnedEndexplicitly.Each extension end still targets the correct stereotype and base UML metaclass, so extension semantics are unchanged.
Also applies to: 894-953
955-961: Profile application metadata and ModelLibrary ID refreshed safelyThe new
eAnnotationsid onprofileApplicationand the updatedStandard:ModelLibraryid keep the applied Standard profile andbase_Packagereference identical. This is harmless metadata cleanup.mda/src/main/uml/andromda-process.profile.uml (4)
4-93: Process Ecore package regenerated; stereotype wiring still correctThe
EPackagenow defines EClasses forandromda_process_jbpm_task_blocking,BusinessProcess,NodeLeave,Timer, jbpm timer tags, signal hooks,Task, and assignment expressions, with:
- EAttributes using appropriate Ecore types (
EBoolean/EString) and defaults (e.g., blocking defaultfalse).- EReferences consistently targeting the intended UML metaclasses (
CallEvent,UseCase,ActivityPartition, etc.).Everything matches the corresponding UML stereotypes further down; this is safe metadata regeneration.
199-205: Extensions for Before Signal and jbpm assignment remain well-formedThe
uml:Extensionelements forBefore Signalandandromda_process_jbpm_assignment_expressionsimply restate the extension ends and memberEnd ordering:
- Extension ends still point to the right stereotypes.
- Associations still bind to the same base UML elements (
CallEvent,ActivityPartition).No behavioral impact expected.
253-263: Timer transition/repeat extensions correctly reference their stereotypesThe timer‑related
uml:Extensionelements continue to associate:
andromda_process_jbpm_timer_transitionandandromda_process_jbpm_timer_repeatwith
CallEventvia the same stereotype types and associations. Only internal memberEnd wiring changed; semantics are preserved.
272-278: Profile application and ModelLibrary identifiers updated onlyThe added
eAnnotationsid underprofileApplicationand the newStandard:ModelLibraryid maintain the same Standard profile reference andbase_Package. This is benign XMI bookkeeping.mda/src/main/uml/andromda-common.andromda-service.profile.uml (3)
4-92: Service-related EEnums re-emitted; literals and UML bindings unchangedThe regenerated Ecore enums (
SpringRemotingProtocol,EjbTransactionManagement,EjbServiceType,TernaryBoolean,EjbTransactionType,SpringTransactionType,EjbViewType,EjbMdb*,EjbPersistenceUnitType,EjbServiceFlushMode) keep:
- The same literal names and integer values.
eAnnotations referencespointing at the same UML enumerations.This keeps all tag semantics stable while normalizing internal IDs.
225-377: Service extensions: memberEnd/xmi:type clean-up with stable associationsThe block of
uml:Extensionelements (Service,Timeout,Listener,Interceptor,PostConstruct,PreDestroy,ResourceRef,RunAs,PrePassivate,DataSource,PostActivate,PersistenceContext,UserTransaction,MessageDriven,ServiceRef,SecuredElement,InterceptorOperation) has:
- Explicit
uml:ExtensionEndtyping on the extension endmemberEnd.- MemberEnd order adjusted but still pairing each stereotype in andromda-common with the correct base UML element.
No change in how these stereotypes apply; this is structural hygiene only.
379-385: Profile application metadata and ModelLibrary ID updated safelyThe new
eAnnotationsid andStandard:ModelLibraryid leave the applied Standard profile andbase_Packageuntouched. Pure metadata refresh.mda/src/main/uml/andromda-presentation.profile.uml (3)
4-203: Presentation Ecore meta regenerated; FrontEnd stereotypes still alignWithin the
contentsblock, enums (WebActionFormScope,WebFieldType,WebActionType,WebExportType,ViewType,TernaryBoolean) and EClasses (FrontEndRegistration/Exception/Table/Field/Action/Application/Controller/UseCase/View/SessionObject/Message) are re‑emitted with new ids, but:
- Classifier names and enum literals match the UML definitions further down.
- Every
eAnnotations referencespoints to the same UML elements.- EAttributes and EReferences correspond one‑for‑one to the tagged values and base types on the UML stereotypes.
This is consistent with tooling‑driven regeneration. Recommend rerunning your MDA/presentation generation to confirm no EMF client depended on the old internal ids.
541-601: FrontEnd extensions: memberEnd/navigableOwnedEnd normalization only*The
uml:Extensionelements forFrontEndAction,FrontEndRegistration,FrontEndApplication,FrontEndField(calendar),FrontEndTable(columns),FrontEndSessionObject,FrontEndException, andFrontEndUseCasenow:
- Use consistent
memberEndordering, and- Make the appropriate extension end the
navigableOwnedEnd.Each extension end still targets the same stereotype and base UML element, so extension semantics and codegen behavior remain unchanged.
642-648: Profile application IDs updated; Standard profile linkage intactThe new
eAnnotationsid inprofileApplicationand the updatedStandard:ModelLibraryid continue to reference the same Standard profile and presentation profile package. This is harmless XMI metadata cleanup.web/src/main/webapp/user/changeSettings.xhtml (1)
14-28: VerifyonkeypressEL resolves to the intended JavaScript snippetThe new
onkeypress="#{autoCompleteForceSelectionPreventFormSubmission}"relies on EL to produce the JavaScript handler. IfautoCompleteForceSelectionPreventFormSubmissionis not a request/session/application‑scoped bean property (or a bundle string) that returns a valid JS snippet, this will no‑op and the autocomplete form‑submission guard won’t work. In that case you likely want a literal handler, e.g.:<h:form id="changesettings_form" onkeypress="return autoCompleteForceSelectionPreventFormSubmission(event);">The updated
columnClasseslook fine and are layout‑only.Please double‑check how this is wired in other pages and that the expression actually yields the correct JS.
core/src/exec/java/org/phoenixctms/ctsms/executable/DemoDataProvider.java (1)
587-607: Verify null-safety of calendar filter getters in UserInVOCould not locate the
UserInVOclass definition in the repository to verify whethergetDutyRosterCalendarFilters()andgetInventoryBookingCalendarFilters()are guaranteed to return non-null collections. However, the identical pattern (calling.clear()without null-checks) appears consistently throughout the codebase inUserBean,UserSettingsBean,CommonUtil,UserServiceImpl, andUserDaoImpl, suggesting these getters always return initialized collections rather than null.Please confirm that
UserInVOinitializes these collections in its constructor or field declaration, and that the getters never return null. If they can return null, the flagged lines 605–606 should add null-guards as suggested in the original comment.common/src/main/java/org/phoenixctms/ctsms/util/CommonUtil.java (1)
585-620: Add null-safety when copying new calendar filter collections (lines 616–619)The method calls
getDutyRosterCalendarFilters()andgetInventoryBookingCalendarFilters()on bothoutandinobjects, then immediately invokes.clear()and.addAll()without verifying the getters return non-null. If either getter returns null, aNullPointerExceptionwill be thrown at runtime.While the codebase shows inconsistent patterns (line 567 has an unguarded
getSelectionValues().clear(), but lines 787–789 guard withif (outValues != null)), the safer pattern should be applied here. Verify thatUserOutVOandUserInheritedVOguarantee these getters always return non-null collections, or add null guards as suggested in the original review.core/db/schema-set-version.sql (1)
14-14: Schema version bump looks consistent with new CATEGORY_PRESET changesUpdating to
'010801105'is fine; just ensure the same version is used inschema-createandschema-up-masterso migrations remain coherent.Please double‑check other DDL/migration files reference
010801105as the current schema version.web/src/main/resources/org/phoenixctms/ctsms/web/settings.properties (1)
3-4: New calendar filter defaults/presets are correctly wiredThe four new keys (
default_*_calendar_filtersanduser_*_calendar_filters_preset) follow the existing settings naming pattern and align with theSettingCodes/DefaultSettingsconstants.Leaving them empty by default is reasonable; operators can later configure comma‑separated filter IDs as needed.
Also applies to: 366-367
web/src/main/java/org/phoenixctms/ctsms/web/model/SessionScopeBean.java (1)
844-866: Calendar filter getters correctly fall back to global defaults; confirm empty‑list semanticsThe new
getDutyRosterCalendarFilters/getInventoryBookingCalendarFiltersmethods are consistent with other settings accessors: they use the inherited user value when present and non‑empty, otherwise fall back toDEFAULT_*fromsettings.properties.One behavioral detail to confirm: an empty user list is currently treated as “no user override, use defaults”. If you ever need to distinguish “explicitly no filters” from “inherit defaults”, you’d need an additional flag or sentinel; otherwise this implementation looks solid.
web/src/main/java/org/phoenixctms/ctsms/web/model/user/UserBean.java (2)
98-101: Symmetric propagation of calendar filters between UserInVO and UserOutVOThe new
DutyRosterCalendarFilters/InventoryBookingCalendarFilterscopy blocks incopyUserOutToInandcreateUserOutFromIn(clear +addAll) are symmetric and match how inherited properties and permission groups are already handled. This should keep in/out VO state consistent across UI interactions.Also applies to: 156-159
207-210: User default calendar filters correctly sourced from preset settings
initUserDefaultValuesnow initializesUserInVO’s two calendar filter collections fromUSER_DUTY_ROSTER_CALENDAR_FILTERS_PRESETandUSER_INVENTORY_BOOKING_CALENDAR_FILTERS_PRESETviaSettings.getStringList(...). This is consistent with how other user presets (theme, tab orientation, visible tabs) are wired.core/src/main/java/org/phoenixctms/ctsms/vocycle/UserReflexionGraph.java (1)
192-194: Consider null‑safety for calendar filter collections
entityToValueCollectionassumessource.getDutyRosterCalendarFilters()/getInventoryBookingCalendarFilters()are non‑null; if any mapping path ever leaves these asnullyou’ll get an NPE here. Either guarantee non‑null collections at the entity level or defensively treatnullas an empty collection before calling the helper.web/src/main/java/org/phoenixctms/ctsms/web/util/SettingCodes.java (2)
7-8: New default calendar filter keys look consistent
DEFAULT_DUTY_ROSTER_CALENDAR_FILTERSandDEFAULT_INVENTORY_BOOKING_CALENDAR_FILTERSfollow the existing naming/value pattern for default settings and align with the property keys.
215-216: User preset keys for calendar filters are well‑placed
USER_DUTY_ROSTER_CALENDAR_FILTERS_PRESETandUSER_INVENTORY_BOOKING_CALENDAR_FILTERS_PRESETfit the existingUSER_*_PRESETconvention and keep symmetry with the new default keys.web/src/main/java/org/phoenixctms/ctsms/web/util/DefaultSettings.java (2)
26-29: Default calendar filter lists follow existing pattern
DEFAULT_DUTY_ROSTER_CALENDAR_FILTERSandDEFAULT_INVENTORY_BOOKING_CALENDAR_FILTERSmirror how other list‑type defaults are modeled (mutableArrayList<String>constants) and are fine as empty defaults.
338-340: User preset calendar filter defaults are wired appropriately
USER_DUTY_ROSTER_CALENDAR_FILTERS_PRESETandUSER_INVENTORY_BOOKING_CALENDAR_FILTERS_PRESETalign with the otherUSER_*_PRESETdefaults and give a natural home for per‑user calendar filter presets (empty by default).core/db/schema-up-master.sql (1)
508-530: Migration for CATEGORY_PRESET matches base schemaThe new
010801105step correctly createsCATEGORY_PRESETand its two FKs tousers, mirroringschema-create.sql, and updates the DB version. This keeps install and upgrade schemas aligned.core/src/main/java/org/phoenixctms/ctsms/domain/UserDaoImpl.java (2)
179-180: VO mappings for calendar filter presets look consistentThe new mappings from
UsertoUserInVO,UserSettingsInVO, andUserInheritedVO, plus the registration ofdutyRosterCalendarFiltersandinventoryBookingCalendarFiltersinsetInheritedProperty, are symmetric with the entity collections and reuseUserReflexionGraph.entityToValueCollectionconsistently. This should keep all three VO types aligned with the underlyingCategoryPresetcollections.Also applies to: 413-414, 524-525, 552-553
572-579: Collection handling insetInheritedPropertyassumes CategoryPreset-based collectionsThe new
Collectionbranch insetInheritedPropertycorrectly:
- Pulls the inherited
Collectionfrom the parentUser,- Converts it via
UserReflexionGraph.entityToValueCollection(...), and- Replaces the
UserInheritedVOcollection contents.This is appropriate for the new calendar filter presets, but it implicitly assumes that any property passed with
type == Collection.classcorresponds to aCollection<CategoryPreset>onUser. If you ever add another collection-typed inheritable property of a different element type, you’ll need a different conversion strategy (or an explicit discriminator) here.Would you confirm that currently only the
dutyRosterCalendarFiltersandinventoryBookingCalendarFiltersproperties useCollection.classwithCategoryPresetelements? If others are added later, this helper may need to be generalized.web/src/main/resources/org/phoenixctms/ctsms/web/user/labels_de.properties (1)
169-176: German calendar preset labels are consistent and aligned with EnglishThe new German labels/tooltips for both duty roster and inventory booking calendar filter presets look consistent with each other and correctly mirror the English texts.
web/src/main/java/org/phoenixctms/ctsms/web/model/user/UserSettingsBean.java (1)
43-46: Copying calendar filter presets fromouttoinis correct and symmetricClearing
in.getDutyRosterCalendarFilters()/getInventoryBookingCalendarFilters()and thenaddAllfromoutkeeps the settings-VO exactly in sync with the user-VO and matches how other mutable collections are handled.web/src/main/java/org/phoenixctms/ctsms/web/model/shared/UserSettingsBeanBase.java (4)
46-47: Calendar filter fields and initialization look appropriateUsing
Set<String>withLinkedHashSetinitialization gives uniqueness plus predictable iteration order, and keeps the in-memory representation non-null throughout the bean lifecycle. This is a good fit for user presets and aligns with later copying intoList<String>for exposure.Also applies to: 54-55
108-117: Loading and persisting calendar filters is consistent and null-safeThe pattern in
initSets(clear, then populate fromgetUser*if non-null) and insanitizeInVals(persisting the internal sets back viasetUser*) is coherent, avoids stale state, and never exposesnullto the abstract setters. This wiring should work well with the persistence layer.Also applies to: 167-168
216-222: Abstract accessors for user calendar filters fit existing designThe added abstract getters/setters for duty roster and inventory booking calendar filters are consistent with the other user-scoped settings abstractions in this base class, and the use of
Collection<String>gives implementations flexibility in storage.
386-402: Getter/setter implementations for calendar filter lists are fineThe new
get*CalendarFiltersmethods returning copies of the internal sets, and the corresponding setters that clear then repopulate the sets from incoming lists, correctly encapsulate the internalSetwhile still exposingList<String>to the UI layer. This avoids external mutation of internal state and naturally de‑duplicates values via the underlyingSet.Also applies to: 422-438
| public List<String> completeDutyRosterCalendar(String query) { | ||
| Collection<String> calendars = null; | ||
| try { | ||
| calendars = WebUtil.getServiceLocator().getTrialService().getCalendars(WebUtil.getAuthentication(), | ||
| null, null, null, query, null); // let permission argument override decide... | ||
| } catch (ServiceException | AuthorisationException | IllegalArgumentException e) { | ||
| } catch (AuthenticationException e) { | ||
| WebUtil.publishException(e); | ||
| } | ||
| if (calendars != null) { | ||
| try { | ||
| return ((List<String>) calendars); | ||
| } catch (ClassCastException e) { | ||
| } | ||
| } | ||
| return new ArrayList<String>(); | ||
| } |
There was a problem hiding this comment.
Fix fragile Collection→List cast in autocomplete methods
Both autocomplete helpers try to cast an arbitrary Collection<String> from the service layer directly to List<String> and silently swallow ClassCastException:
if (calendars != null) {
try {
return ((List<String>) calendars);
} catch (ClassCastException e) {
}
}
return new ArrayList<String>();This is brittle:
- It relies on
getCalendarsalways returning aList, even though callers elsewhere (e.g.WebUtil.getDutyRosterTurnFilterCalendars) treat it as a genericCollection<String>. - If the implementation ever returns a
Setor any non‑Listcollection, autocomplete will just return an empty list, with the exception fully swallowed and no signal.
A safer and simpler implementation is to always wrap the returned Collection in a new ArrayList:
- if (calendars != null) {
- try {
- return ((List<String>) calendars);
- } catch (ClassCastException e) {
- }
- }
- return new ArrayList<String>();
+ if (calendars != null) {
+ return new ArrayList<String>(calendars);
+ }
+ return new ArrayList<String>();Apply this pattern to both completeDutyRosterCalendar and completeInventoryBookingCalendar. This removes the unsafe cast, avoids silently dropping valid data, and matches how other code (e.g. WebUtil.getDutyRosterTurnFilterCalendars / getInventoryBookingFilterCalendars) handles the same service call.
Also applies to: 440-456
🤖 Prompt for AI Agents
In
web/src/main/java/org/phoenixctms/ctsms/web/model/shared/UserSettingsBeanBase.java
around lines 404-420 (and similarly lines 440-456), the code unsafely casts a
Collection<String> returned by getCalendars(...) to List<String> and swallows
ClassCastException, which can drop valid data; replace the cast with creating
and returning a new ArrayList<>(calendars) when calendars is non-null (and
remove the try/catch around ClassCastException), preserving returned items and
avoiding fragile casting and silent failures.
1 participant
closing #395
Summary by CodeRabbit
Release Notes
✏️ Tip: You can customize this high-level summary in your review settings.