Feat/history frontend v2#998
Conversation
WalkthroughReworks privilege history end-to-end: new role-specific history endpoints and mocks, model/serializer shape changes (effectiveDate/createDate/serverNote), removal of fabricated events/StatusTimeBlock, added encumbrance/homeJurisdictionChange/licenseDeactivation handling, store/pages updated to fetch and render server-provided history. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant Page as PrivilegeDetail / PublicPrivilegeDetail
participant Store as Vuex (license/user)
participant Api as DataApi (staff|public|licensee)
participant Server as Backend
User->>Page: open privilege detail
Page->>Store: dispatch getPrivilegeHistoryRequest(params)
Store->>Api: call role-specific getPrivilegeHistory...(params)
Api->>Server: GET /history (staff | public | licensee)
Server-->>Api: { privilegeId, events: [...] (effectiveDate, createDate, note) }
Api-->>Store: licenseHistoryData (events mapped via LicenseHistoryItemSerializer)
Store->>Store: commit REQUEST → SUCCESS (update matching privilege.history)
Store-->>Page: state updated
Page->>UI: render PrivilegeHistory using effectiveDateDisplay() and detailDisplay()
Estimated code review effort🎯 4 (Complex) | ⏱️ ~35 minutes Assessment against linked issues
Assessment against linked issues: Out-of-scope changes
Possibly related PRs
Suggested reviewers
Poem
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
🧹 Nitpick comments (35)
webroot/src/components/PrivilegeEventNode/PrivilegeEventNode.vue (1)
24-24: Render detail only when presentAvoid an empty “event-detail” element when no details.
- <div class="event-detail">{{detailDisplay}}</div> + <div v-if="detailDisplay" class="event-detail">{{ detailDisplay }}</div>Accessibility: consider including details in the aria-label for screen readers.
Outside this range, update line 12 as:
:aria-label="$t('licensing.eventNodeLabel', { eventNameDisplay, eventDate }) + (detailDisplay ? ', ' + detailDisplay : '')"webroot/src/components/PrivilegeEventNode/PrivilegeEventNode.less (1)
29-37: Default background-color on.nodemay be redundant and can mask state bugsSetting a default
background-color: @blackwhile also styling.inactive/.activeis okay, but it can visually hide missing state classes during regressions. Consider either:
- Removing the default and relying solely on explicit state classes, or
- Using a neutral token (e.g.,
@midGrey) to better indicate “unknown/neutral” state.webroot/src/components/PrivilegeHistory/PrivilegeHistory.vue (1)
11-11: Ensure the new timeline element renders correctly; anchor its absolute positioningSwapping in
<div class="timeline-line" />per item is fine, but it relies on.events { position: relative; }for correct absolute positioning. Please add that in the stylesheet (see PrivilegeHistory.less comment). Optional: prefer<div></div>for native elements for consistency.webroot/src/components/PrivilegeCard/PrivilegeCard.ts (1)
171-173: Expiration always shown — aligns with new history modelReturning the expire date unconditionally is consistent with the new deactivation semantics. Optional: to reduce UX ambiguity when a privilege is deactivated but not expired, consider appending a deactivation indicator near the date.
webroot/src/network/userApi/data.api.ts (1)
257-269: Fix JSDoc and tighten URL handling/return types for getPrivilegeHistoryLicensee
- The JSDoc says “A User model instance” and
Promise<>; update to the correct type.- Encode path segments to be safe.
- Optionally type the return as
Promise<LicenseHistory>.-/** - * GET Authenticated Licensee User privilege's history. - * @param {string} jurisdiction jurisdiction of privilege - * @param {string} licenseTypeAbbrev license type abbreviation of privilege - * @return {Promise<>} A User model instance. - */ -public async getPrivilegeHistoryLicensee(jurisdiction: string, licenseTypeAbbrev: string) { - const serverResponse: any = await this.api.get(`/v1/provider-users/me/jurisdiction/${jurisdiction.toLowerCase()}/licenseType/${licenseTypeAbbrev.toLowerCase()}/history`); - - const response = LicenseHistorySerializer.fromServer(serverResponse); - - return response; -} +/** + * GET authenticated licensee user's privilege history. + * @param {string} jurisdiction Privilege jurisdiction (e.g., 'ky'). + * @param {string} licenseTypeAbbrev License type abbreviation (e.g., 'SLP'). + * @return {Promise<LicenseHistory>} LicenseHistory model instance. + */ +public async getPrivilegeHistoryLicensee( + jurisdiction: string, + licenseTypeAbbrev: string + // : Promise<LicenseHistory> +) { + const j = encodeURIComponent(jurisdiction.toLowerCase()); + const l = encodeURIComponent(licenseTypeAbbrev.toLowerCase()); + const serverResponse: any = await this.api.get( + `/v1/provider-users/me/jurisdiction/${j}/licenseType/${l}/history` + ); + const response = LicenseHistorySerializer.fromServer(serverResponse); + return response; +}And at the import (optional type import if available):
-import { LicenseHistorySerializer } from '@/models/LicenseHistory/LicenseHistory.model'; +import { LicenseHistory, LicenseHistorySerializer } from '@/models/LicenseHistory/LicenseHistory.model';webroot/src/locales/es.json (1)
666-697: Spanish event names need updates to match new keys and natural phrasing
- “Privilegio comprado” no longer matches “issuance”.
- “Expirado” reads as a past participle; event list should use a noun (“Vencimiento”/“Caducidad”).
- “Inicio Cambio de Jurisdicción” is unnatural; prefer “Cambio de estado de residencia”.
- “Privilegio sin trabas” is unusual; “Levantamiento de gravamen” is standard.
- “Encumbrance” is left in English; translate to “Gravamen”.
"licenseEvents": [ { - "name": "Privilegio comprado", + "name": "Emisión de privilegio", "key": "issuance" }, { - "name": "Expirado", + "name": "Vencimiento", "key": "expiration" }, { "name": "Desactivación", "key": "deactivation" }, { - "name": "Desactivación de Licencia", + "name": "Desactivación de licencia", "key": "licenseDeactivation" }, { - "name": "Inicio Cambio de Jurisdicción", + "name": "Cambio de estado de residencia", "key": "homeJurisdictionChange" }, { - "name": "Privilegio sin trabas", + "name": "Levantamiento de gravamen", "key": "lifting_encumbrance" }, { - "name": "Encumbrance", + "name": "Gravamen", "key": "encumbrance" } ],webroot/src/store/user/user.spec.ts (2)
396-439: Privilege history mutation tests — solid coverage; consider a negative pathLGTM for loading/error flags and history assignment via constructed privilegeId. Suggest adding a test where no matching privilege is found to ensure the mutation is a no-op and doesn’t throw.
945-975: Minor test title typos and expectation tightening
- Titles have duplicates: “request request”, “start start”.
- Optionally assert the dispatched action name(s) to strengthen the contract.
-it('should successfully start privilege history request request', async () => { +it('should successfully start privilege history request', async () => { ... -it('should successfully start start privilege history success', () => { +it('should successfully start privilege history success', () => {webroot/src/components/PrivilegeEventNode/PrivilegeEventNode.ts (1)
27-45: Switch to effectiveDate and add detailDisplay — good; tighten typing for eventLengthBucket
- Using
effectiveDateDisplay()matches the updated model.detailDisplaysurfaces notes as intended.- Optional: constrain
eventLengthBucketto a union type to prevent invalid values.-@Prop({ default: 'short' }) eventLengthBucket?: string; +@Prop({ default: 'short' }) eventLengthBucket?: 'short' | 'long';webroot/src/models/LicenseHistory/LicenseHistory.model.spec.ts (3)
19-31: Avoid leaking global i18n/Vue into other testsSet up is fine, but this mutates globals without teardown. Add an after() to restore previous locale and remove window.Vue to prevent cross-test side effects.
Example:
describe('LicenseHistory model', () => { - before(() => { + let previousLocale: string | undefined; + before(() => { const { tm: $tm, t: $t } = i18n.global; + previousLocale = i18n.global.locale; (window as any).Vue = { config: { globalProperties: { $tm, $t } } }; i18n.global.locale = 'en'; - }); + }); + after(() => { + i18n.global.locale = previousLocale || 'en'; + delete (window as any).Vue; + });
73-81: Also assert item instance types for stronger guaranteesYou validate fields with matchPattern; add an instanceof check to ensure each event is a LicenseHistoryItem instance.
- expect(licenseHistory.events).to.matchPattern([{ + expect(licenseHistory.events).to.matchPattern([{ type: 'privilegeUpdate', updateType: 'renewal', dateOfUpdate: '2025-05-01T15:27:35+00:00', effectiveDate: '2025-05-01', createDate: '2025-05-01T15:27:35+00:00', serverNote: 'Note', '...': '', }]); + expect(licenseHistory.events[0]).to.be.an.instanceof(LicenseHistoryItem);
112-120: Serializer mapping LGTM; add instance checkMapping note→serverNote through LicenseHistorySerializer/LicenseHistoryItemSerializer looks correct. Consider asserting instance type too.
- expect(licenseHistory.events).to.matchPattern([{ + expect(licenseHistory.events).to.matchPattern([{ type: 'privilegeUpdate', updateType: 'renewal', dateOfUpdate: '2025-05-01T15:27:35+00:00', effectiveDate: '2025-05-01', createDate: '2025-05-01T15:27:35+00:00', serverNote: 'Note', '...': '', }]); + expect(licenseHistory.events[0]).to.be.an.instanceof(LicenseHistoryItem);webroot/src/network/licenseApi/data.api.ts (1)
421-428: Clarify JSDoc: this is public/unauthenticated, not “Authenticated”Small wording nit to avoid confusion.
- /** - * GET Authenticated Privilege History for an unauthenticated user. + /** + * GET Public Privilege History (unauthenticated).webroot/src/store/license/license.actions.ts (1)
93-123: Action flow LGTM; consider typing payload and basic param guardsThe request/success/failure wiring is clean. For maintainability, add a payload interface and guard for required params to fail-fast in dev.
Example:
- getPrivilegeHistoryRequest: async ({ commit, dispatch }, { - compact, - providerId, - jurisdiction, - licenseTypeAbbrev, - isPublic - }: any) => { + interface GetPrivilegeHistoryPayload { + compact: string; + providerId: string; + jurisdiction: string; + licenseTypeAbbrev: string; + isPublic?: boolean; + } + getPrivilegeHistoryRequest: async ({ commit, dispatch }, { + compact, providerId, jurisdiction, licenseTypeAbbrev, isPublic + }: GetPrivilegeHistoryPayload) => { commit(MutationTypes.GET_PRIVILEGE_HISTORY_REQUEST); + if (!compact || !providerId || !jurisdiction || !licenseTypeAbbrev) { + return dispatch('getPrivilegeHistoryFailure', new Error('Missing required parameters')); + }webroot/src/models/License/License.model.spec.ts (2)
560-591: Strengthen isDeactivated test to isolate history-driven behaviorCurrently status is preset to 'inactive', which could mask history logic. Set status to 'active' in the fixture to ensure the deactivation result is driven by the history event alone.
- status: 'inactive', + status: 'active',
593-624: homeJurisdictionChange case LGTM; consider adding licenseDeactivation caseThis test is good. For completeness with model support, consider a third test using updateType: 'licenseDeactivation'.
Example addition (outside current hunk):
it('should populate isDeactivated correctly given license history (licenseDeactivation)', () => { const data = { /* same as prior fixture but with status: 'active' */ }; const license = LicenseSerializer.fromServer({ ...data, status: 'active' }); license.history = [ new LicenseHistoryItem({ type: 'privilegeUpdate', updateType: 'licenseDeactivation', dateOfUpdate: '2025-05-01T15:27:35+00:00', effectiveDate: '2025-05-01', createDate: '2025-05-01T15:27:35+00:00', serverNote: 'Note' })]; expect(license.isDeactivated()).to.equal(true); });webroot/src/store/user/user.actions.ts (1)
128-134: Minor nit – duplicate wording in comment
// GET GET LICENSEE PRIVILEGE HISTORY SUCCESS / FAIL HANDLERS→ remove the extra “GET”.webroot/src/network/data.api.ts (2)
267-310: Type annotations & DRY opportunityAll three history methods return the same payload type (e.g.
Promise<LicenseHistory>).
- Add an explicit return type to aid consumers and IDEs.
- The two staff/public methods differ only by the delegated function – consider a private helper that accepts the delegate to avoid four-arg duplication.
495-503: JSDoc mismatchDocstring says “
@return {Promise<>}A User model instance” – but the method actually returns privilege-history, not a user. Update the description and generic placeholder.webroot/src/store/license/license.spec.ts (1)
438-479: Remove commented-out assertionLines 457-458 leave a disabled
expect(dispatch.calledOnce)– either assert correctly or delete to avoid stale code.webroot/src/models/License/License.model.ts (1)
123-135: Simplify & future-proof deactivation checkBuilding a hard-coded OR list makes maintenance error-prone.
- const isLastEventDeactivation = lastEvent.updateType === 'deactivation' - || lastEvent.updateType === 'licenseDeactivation' - || lastEvent.updateType === 'homeJurisdictionChange'; + const deactivationFlags = ['deactivation','licenseDeactivation','homeJurisdictionChange']; + const isLastEventDeactivation = deactivationFlags.includes(lastEvent.updateType || '');This is clearer and easier to extend.
webroot/src/store/license/license.mutations.ts (1)
21-23: Prefix looks wrong for a license-store mutationUsing
[User]in the label while the enum lives in the license store is confusing and breaks the existing naming convention ([License] …).
Rename the three new constants to keep the module prefix consistent.webroot/src/network/mocks/mock.data.api.ts (3)
290-314: Duplicate logic – extract a small helper
getPrivilegeHistoryPublicandgetPrivilegeHistoryStaffcontain identical bodies.
Factor the common code into a private helper to avoid three copies of the same validation / serializer dance.
316-340: Header comment says “Public” but function is “Staff”Copy-paste residue – update the leading comment so it matches the method name to prevent confusion.
481-500: JSDoc returns the wrong modelThe docstring claims the method returns “A User model instance”, but it actually resolves to
LicenseHistory.
Adjust the description to avoid misleading IntelliSense.webroot/src/pages/PublicPrivilegeDetail/PublicPrivilegeDetail.ts (1)
27-35: Method name / flag mismatch
fetchPrivilegeHistoryStaff()is invoked from a public page and dispatched withisPublic: true.
Rename the method (and the corresponding call site/component) tofetchPrivilegeHistoryPublic(or drop the suffix entirely) for semantic clarity.webroot/src/models/LicenseHistory/LicenseHistory.model.ts (2)
1-6: Filename in header comment is wrongThe banner still says
LicenseHistoryItem.ts, which will mislead future greps.
Update toLicenseHistory.model.ts.
48-55: Avoid repeated translation look-ups
licenseTypeAbbreviation()fetches the full translation array every call.
Since the data is static, cachelicenseTypesat module level or inject it once to reduce unnecessary look-ups inside hot paths (e.g. lists).webroot/src/pages/PrivilegeDetail/PrivilegeDetail.ts (2)
80-81:isLoadingmay miss base user loading flag
licenseStore?.isLoading || userStore?.isLoadingPrivilegeHistoryomits the genericuserStore.isLoadingused elsewhere for user fetches. IflicenseeRecordis fetched through the user store (licensee login), the spinner won’t show. Add that flag or create a shared getter.
167-181: Un-awaited history fetch may race with UIInside
populateDatathe history fetches are called withoutawait. This can leaveisLoadingfalse while the request is still in flight, allowing the page to render incomplete data. Await them (and surface errors) to keep spinner and watcher logic consistent.webroot/src/models/LicenseHistoryItem/LicenseHistoryItem.model.ts (5)
48-50:effectiveDateDisplayshould early-return on null
dateDisplay(null)often yields"--"but the helper may throw if it expects a string/Date. Guard first to avoid runtime after deserialization errors.- return dateDisplay(this.effectiveDate); + return this.effectiveDate ? dateDisplay(this.effectiveDate) : '';
52-56: Keep event lists in a single source
isActivatingEvent,isDeactivatingEvent, and UI components each replicate event keys. Extract to a frozen constant exported from this module to prevent drift between lists and translation files.
71-82:updateTypeDisplayignores missing translation fallbackWhen the translation array is empty,
eventNamefalls back to “Unknown”. For regressions in i18n keys this hides the underlying issue.
Log a warning or surface the rawupdateTypestring so QA can spot untranslated values.
84-95:noteDisplayduplicates switch logicBoth
noteDisplayandupdateTypeDisplaybranch on the same special-case updateTypes.
Consider unifying into a map to avoid forgetting one path when adding new update types.
101-113: Serializer retains unuseddateOfUpdate
dateOfUpdateis no longer referenced in the model but is still assigned. Remove to avoid dead data and reduce payload.- dateOfUpdate: json.dateOfUpdate,
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
webroot/yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (36)
webroot/src/components/PrivilegeCard/PrivilegeCard.ts(1 hunks)webroot/src/components/PrivilegeEventNode/PrivilegeEventNode.less(2 hunks)webroot/src/components/PrivilegeEventNode/PrivilegeEventNode.ts(1 hunks)webroot/src/components/PrivilegeEventNode/PrivilegeEventNode.vue(1 hunks)webroot/src/components/PrivilegeHistory/PrivilegeHistory.less(1 hunks)webroot/src/components/PrivilegeHistory/PrivilegeHistory.ts(2 hunks)webroot/src/components/PrivilegeHistory/PrivilegeHistory.vue(1 hunks)webroot/src/components/StatusTimeBlock/StatusTimeBlock.less(0 hunks)webroot/src/components/StatusTimeBlock/StatusTimeBlock.spec.ts(0 hunks)webroot/src/components/StatusTimeBlock/StatusTimeBlock.ts(0 hunks)webroot/src/components/StatusTimeBlock/StatusTimeBlock.vue(0 hunks)webroot/src/locales/en.json(3 hunks)webroot/src/locales/es.json(3 hunks)webroot/src/models/License/License.model.spec.ts(2 hunks)webroot/src/models/License/License.model.ts(2 hunks)webroot/src/models/LicenseHistory/LicenseHistory.model.spec.ts(1 hunks)webroot/src/models/LicenseHistory/LicenseHistory.model.ts(1 hunks)webroot/src/models/LicenseHistoryItem/LicenseHistoryItem.model.spec.ts(1 hunks)webroot/src/models/LicenseHistoryItem/LicenseHistoryItem.model.ts(4 hunks)webroot/src/network/data.api.ts(2 hunks)webroot/src/network/licenseApi/data.api.ts(2 hunks)webroot/src/network/mocks/mock.data.api.ts(4 hunks)webroot/src/network/mocks/mock.data.ts(1 hunks)webroot/src/network/userApi/data.api.ts(2 hunks)webroot/src/network/userApi/interceptors.ts(1 hunks)webroot/src/pages/PrivilegeDetail/PrivilegeDetail.ts(5 hunks)webroot/src/pages/PublicPrivilegeDetail/PublicPrivilegeDetail.ts(3 hunks)webroot/src/store/license/license.actions.ts(1 hunks)webroot/src/store/license/license.getters.ts(1 hunks)webroot/src/store/license/license.mutations.ts(3 hunks)webroot/src/store/license/license.spec.ts(2 hunks)webroot/src/store/user/user.actions.ts(1 hunks)webroot/src/store/user/user.mutations.ts(3 hunks)webroot/src/store/user/user.spec.ts(2 hunks)webroot/src/store/user/user.state.ts(2 hunks)webroot/src/styles.common/_fonts.less(1 hunks)
💤 Files with no reviewable changes (4)
- webroot/src/components/StatusTimeBlock/StatusTimeBlock.less
- webroot/src/components/StatusTimeBlock/StatusTimeBlock.vue
- webroot/src/components/StatusTimeBlock/StatusTimeBlock.spec.ts
- webroot/src/components/StatusTimeBlock/StatusTimeBlock.ts
🧰 Additional context used
🧠 Learnings (15)
📓 Common learnings
Learnt from: jsandoval81
PR: csg-org/CompactConnect#922
File: webroot/src/components/UserAccount/UserAccount.ts:0-0
Timestamp: 2025-07-10T19:50:56.745Z
Learning: In the UserAccount component (webroot/src/components/UserAccount/UserAccount.ts), the email field should be disabled for staff users (`isDisabled: this.isStaff`) to maintain existing restrictions, while licensees should be able to change their email address through the new verification process. This is the intended behavior per PR #922 requirements.
📚 Learning: 2025-05-30T13:48:25.375Z
Learnt from: landonshumway-ia
PR: csg-org/CompactConnect#824
File: backend/compact-connect/lambdas/python/provider-data-v1/handlers/encumbrance.py:116-201
Timestamp: 2025-05-30T13:48:25.375Z
Learning: In encumbrance handling code, prefer to keep privilege and license encumbrance lifting implementations decoupled rather than extracting shared logic, as requirements between these implementations are likely to change in the future.
Applied to files:
webroot/src/store/license/license.getters.ts
📚 Learning: 2025-07-10T19:50:56.745Z
Learnt from: jsandoval81
PR: csg-org/CompactConnect#922
File: webroot/src/components/UserAccount/UserAccount.ts:0-0
Timestamp: 2025-07-10T19:50:56.745Z
Learning: In the UserAccount component (webroot/src/components/UserAccount/UserAccount.ts), the email field should be disabled for staff users (`isDisabled: this.isStaff`) to maintain existing restrictions, while licensees should be able to change their email address through the new verification process. This is the intended behavior per PR #922 requirements.
Applied to files:
webroot/src/network/userApi/interceptors.tswebroot/src/store/user/user.state.tswebroot/src/network/licenseApi/data.api.tswebroot/src/network/userApi/data.api.tswebroot/src/components/PrivilegeCard/PrivilegeCard.tswebroot/src/store/user/user.spec.tswebroot/src/models/LicenseHistoryItem/LicenseHistoryItem.model.spec.tswebroot/src/network/mocks/mock.data.api.tswebroot/src/store/user/user.actions.tswebroot/src/store/user/user.mutations.tswebroot/src/pages/PrivilegeDetail/PrivilegeDetail.tswebroot/src/network/data.api.tswebroot/src/models/License/License.model.tswebroot/src/pages/PublicPrivilegeDetail/PublicPrivilegeDetail.tswebroot/src/models/License/License.model.spec.ts
📚 Learning: 2025-07-08T18:40:24.408Z
Learnt from: landonshumway-ia
PR: csg-org/CompactConnect#882
File: backend/compact-connect/lambdas/python/common/cc_common/data_model/compact_configuration_client.py:287-359
Timestamp: 2025-07-08T18:40:24.408Z
Learning: In the CompactConnect codebase, landonshumway-ia prefers to avoid extraneous unit tests when existing test coverage is already sufficient to catch bugs. For the get_privilege_purchase_options method's live-jurisdiction filtering logic, the existing tests in the purchases test suite provide adequate coverage without needing additional edge case tests.
Applied to files:
webroot/src/store/license/license.spec.tswebroot/src/store/user/user.spec.tswebroot/src/models/LicenseHistoryItem/LicenseHistoryItem.model.spec.tswebroot/src/network/mocks/mock.data.api.tswebroot/src/models/License/License.model.spec.ts
📚 Learning: 2025-06-11T18:49:14.626Z
Learnt from: jsandoval81
PR: csg-org/CompactConnect#847
File: webroot/src/components/PrivilegePurchaseAcceptUI/PrivilegePurchaseAcceptUI.ts:141-155
Timestamp: 2025-06-11T18:49:14.626Z
Learning: `PrivilegePurchaseAcceptUI` publishes payment results via `"success"` / `"error"` events; the visual feedback is rendered by the parent component (`PrivilegePurchaseFinalize.ts`), so the child intentionally does not set its own `errorMessage` / `successMessage`.
Applied to files:
webroot/src/store/user/user.state.tswebroot/src/components/PrivilegeCard/PrivilegeCard.tswebroot/src/components/PrivilegeHistory/PrivilegeHistory.tswebroot/src/store/user/user.mutations.ts
📚 Learning: 2025-06-24T00:02:39.944Z
Learnt from: jsandoval81
PR: csg-org/CompactConnect#873
File: webroot/src/components/LicenseCard/LicenseCard.ts:414-443
Timestamp: 2025-06-24T00:02:39.944Z
Learning: In LicenseCard component's clickUnencumberItem method (webroot/src/components/LicenseCard/LicenseCard.ts), complex event handling for checkbox interactions is intentionally designed to ensure consistent behavior across checkbox input, wrapper label, and outer selection parent elements for custom UI requirements. This complexity should be preserved rather than simplified.
Applied to files:
webroot/src/components/PrivilegeEventNode/PrivilegeEventNode.tswebroot/src/components/PrivilegeCard/PrivilegeCard.tswebroot/src/models/LicenseHistory/LicenseHistory.model.tswebroot/src/models/LicenseHistoryItem/LicenseHistoryItem.model.spec.tswebroot/src/components/PrivilegeHistory/PrivilegeHistory.tswebroot/src/pages/PrivilegeDetail/PrivilegeDetail.tswebroot/src/models/License/License.model.tswebroot/src/pages/PublicPrivilegeDetail/PublicPrivilegeDetail.tswebroot/src/models/LicenseHistoryItem/LicenseHistoryItem.model.tswebroot/src/models/License/License.model.spec.ts
📚 Learning: 2025-06-18T21:57:02.978Z
Learnt from: jusdino
PR: csg-org/CompactConnect#864
File: backend/compact-connect/tests/smoke/encumbrance_smoke_tests.py:256-262
Timestamp: 2025-06-18T21:57:02.978Z
Learning: The `licenseJurisdiction` field is a required field in the provider data API response from the `/v1/providers/users/me` endpoint, so it can be accessed directly without defensive programming checks.
Applied to files:
webroot/src/network/licenseApi/data.api.tswebroot/src/network/userApi/data.api.tswebroot/src/network/mocks/mock.data.api.tswebroot/src/network/data.api.tswebroot/src/models/License/License.model.spec.ts
📚 Learning: 2025-06-24T00:07:10.463Z
Learnt from: jsandoval81
PR: csg-org/CompactConnect#873
File: webroot/src/components/LicenseCard/LicenseCard.ts:509-528
Timestamp: 2025-06-24T00:07:10.463Z
Learning: In the CompactConnect frontend codebase, the project prefers to avoid early returns in frontend code when possible, as mentioned by jsandoval81 in webroot/src/components/LicenseCard/LicenseCard.ts.
Applied to files:
webroot/src/components/PrivilegeCard/PrivilegeCard.tswebroot/src/models/LicenseHistory/LicenseHistory.model.tswebroot/src/pages/PrivilegeDetail/PrivilegeDetail.ts
📚 Learning: 2025-04-29T02:52:40.532Z
Learnt from: landonshumway-ia
PR: csg-org/CompactConnect#769
File: backend/compact-connect/lambdas/python/provider-data-v1/tests/function/test_handlers/test_encumbrance.py:138-147
Timestamp: 2025-04-29T02:52:40.532Z
Learning: In CompactConnect tests, hardcoded values (like license type abbreviations 'slp') in test queries are sometimes used intentionally rather than using dynamic lookups. This is a deliberate design decision to make tests fail if default test data changes, requiring developers to consciously update related tests.
Applied to files:
webroot/src/models/LicenseHistory/LicenseHistory.model.spec.tswebroot/src/models/LicenseHistoryItem/LicenseHistoryItem.model.spec.tswebroot/src/models/License/License.model.spec.ts
📚 Learning: 2025-06-19T23:09:51.733Z
Learnt from: jsandoval81
PR: csg-org/CompactConnect#873
File: webroot/src/models/AdverseAction/AdverseAction.model.ts:102-118
Timestamp: 2025-06-19T23:09:51.733Z
Learning: The CompactConnect project uses a consistent pattern of Serializer classes with static fromServer methods for deserializing server data across all models, even when the serializer initially contains only one static method. This pattern is maintained for consistency and future extensibility.
Applied to files:
webroot/src/models/LicenseHistory/LicenseHistory.model.tswebroot/src/network/mocks/mock.data.api.ts
📚 Learning: 2025-07-02T21:10:06.663Z
Learnt from: jsandoval81
PR: csg-org/CompactConnect#904
File: webroot/src/components/StateSettingsList/StateSettingsList.ts:49-49
Timestamp: 2025-07-02T21:10:06.663Z
Learning: In the CompactConnect TypeScript codebase, the team generally avoids `any` types but makes an exception for server responses. They use `any` for server response data to prevent overly verbose TypeScript syntax and avoid double casting as `unknown` when updating values from the server. These server responses get reorganized as typed models elsewhere in the codebase, maintaining type safety where it matters most while keeping the code clean.
Applied to files:
webroot/src/models/LicenseHistory/LicenseHistory.model.ts
📚 Learning: 2025-07-03T15:35:57.893Z
Learnt from: rmolinares
PR: csg-org/CompactConnect#905
File: webroot/src/components/UpdateHomeJurisdiction/UpdateHomeJurisdiction.vue:32-41
Timestamp: 2025-07-03T15:35:57.893Z
Learning: In the CompactConnect frontend codebase, the team prefers to keep non-dynamic text directly in Vue templates rather than moving it to computed properties in TypeScript modules, as this approach prevents cluttering the TS files with template labels.
Applied to files:
webroot/src/models/LicenseHistory/LicenseHistory.model.tswebroot/src/pages/PrivilegeDetail/PrivilegeDetail.ts
📚 Learning: 2025-04-29T01:57:43.684Z
Learnt from: landonshumway-ia
PR: csg-org/CompactConnect#769
File: backend/compact-connect/lambdas/python/common/tests/resources/dynamo/provider.json:5-5
Timestamp: 2025-04-29T01:57:43.684Z
Learning: The provider.json test data contains both "providerDateOfUpdate" and "dateOfUpdate" fields which serve different purposes, and both should be maintained in the test files.
Applied to files:
webroot/src/models/LicenseHistoryItem/LicenseHistoryItem.model.spec.tswebroot/src/models/License/License.model.spec.tswebroot/src/network/mocks/mock.data.ts
📚 Learning: 2025-06-09T19:57:51.519Z
Learnt from: rmolinares
PR: csg-org/CompactConnect#851
File: webroot/src/pages/RegisterLicensee/RegisterLicensee.ts:0-0
Timestamp: 2025-06-09T19:57:51.519Z
Learning: In the RegisterLicensee component, when handling DOM element availability issues, the developer prefers using Vue Watchers over retry mechanisms with requestAnimationFrame to avoid infinite recursion risks and maintain Vue's reactive patterns.
Applied to files:
webroot/src/components/PrivilegeHistory/PrivilegeHistory.tswebroot/src/pages/PrivilegeDetail/PrivilegeDetail.tswebroot/src/store/license/license.mutations.tswebroot/src/pages/PublicPrivilegeDetail/PublicPrivilegeDetail.ts
📚 Learning: 2025-05-28T16:13:19.506Z
Learnt from: jsandoval81
PR: csg-org/CompactConnect#822
File: webroot/src/components/Forms/InputEmailList/InputEmailList.vue:26-30
Timestamp: 2025-05-28T16:13:19.506Z
Learning: In the InputEmailList component (webroot/src/components/Forms/InputEmailList/InputEmailList.vue), the formInput.labelSubtext property rendered with v-html contains only developer-controlled content, not user-controlled content, so XSS concerns do not apply.
Applied to files:
webroot/src/pages/PrivilegeDetail/PrivilegeDetail.ts
🧬 Code Graph Analysis (8)
webroot/src/store/license/license.spec.ts (1)
webroot/src/models/License/License.model.ts (1)
License(69-160)
webroot/src/models/LicenseHistory/LicenseHistory.model.ts (3)
webroot/src/models/LicenseHistoryItem/LicenseHistoryItem.model.ts (2)
LicenseHistoryItem(25-96)LicenseHistoryItemSerializer(101-117)webroot/src/models/_helpers/index.ts (1)
deleteUndefinedProperties(24-24)webroot/src/components/LicenseCard/LicenseCard.ts (1)
licenseTypeAbbrev(149-151)
webroot/src/store/user/user.spec.ts (2)
webroot/src/store/user/user.state.ts (1)
state(34-47)webroot/src/models/License/License.model.ts (1)
License(69-160)
webroot/src/models/LicenseHistoryItem/LicenseHistoryItem.model.spec.ts (1)
webroot/src/models/LicenseHistoryItem/LicenseHistoryItem.model.ts (2)
LicenseHistoryItem(25-96)LicenseHistoryItemSerializer(101-117)
webroot/src/network/mocks/mock.data.api.ts (2)
webroot/src/network/mocks/mock.data.ts (1)
mockPrivilegeHistoryReponse(3004-3089)webroot/src/models/LicenseHistory/LicenseHistory.model.ts (1)
LicenseHistorySerializer(61-82)
webroot/src/network/data.api.ts (6)
webroot/src/pages/PrivilegeDetail/PrivilegeDetail.ts (1)
compact(83-87)webroot/src/pages/PublicPrivilegeDetail/PublicPrivilegeDetail.ts (1)
compact(60-62)webroot/src/pages/LicensingDetail/LicensingDetail.ts (1)
compact(59-63)webroot/src/pages/PublicLicensingDetail/PublicLicensingDetail.ts (1)
compact(49-53)webroot/src/network/licenseApi/data.api.ts (1)
licenseDataApi(443-443)webroot/src/network/userApi/data.api.ts (1)
userDataApi(406-406)
webroot/src/models/License/License.model.ts (1)
webroot/src/models/LicenseHistoryItem/LicenseHistoryItem.model.ts (1)
LicenseHistoryItem(25-96)
webroot/src/models/LicenseHistoryItem/LicenseHistoryItem.model.ts (1)
webroot/src/components/PrivilegeHistory/PrivilegeHistory.ts (1)
events(38-40)
⏰ 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: CheckWebroot
🔇 Additional comments (20)
webroot/src/locales/en.json (2)
681-713: No outdated event keys detected; all mappings and enums updatedI ran searches for the old “purchased”/“expired” keys and verified that:
- No
"key": "purchased"or"key": "expired"remain anywhere in the code or locales.- The JSON in
webroot/src/locales/en.jsonandes.jsonnow lists only the new keys.- Enums in
cc_common/data_model/schema/common.pyincludeissuance,expiration,licenseDeactivation,homeJurisdictionChange,encumbrance, andlifting_encumbrance.- Models, serializers, UI components, and tests all reference only the new updateType values.
Everything appears consistent—no further action required.
797-799: Deactivation reason translations wired through noteDisplay → detailDisplayConfirmed that:
webroot/src/models/LicenseHistoryItem/LicenseHistoryItem.model.ts’s noteDisplay() correctly returns thehomeStateChangeNoteandlicenseDeactivationNotetranslations.webroot/src/components/PrivilegeEventNode/PrivilegeEventNode.vue/.tsuses detailDisplay →event.noteDisplay(), rendering these notes in the UI.No further changes required.
webroot/src/network/mocks/mock.data.ts (1)
3011-3089: Mock data sequence is valid for edge-case testingOur status-derivation logic (in LicenseHistorySerializer → LicenseHistory →
License.isDeactivated()) deterministically uses the final update event—so a “deactivation” followed by “renewal,” “expiration,” etc. will correctly surface an active license when the last event is a renewal. Including deactivation, homeJurisdictionChange, licenseDeactivation and subsequent renewals/expiration in the mock sequence ensures we exercise conflict-resolution in the UI and business logic.No changes required. Feel free to annotate specific events with
notefields for clarity, but the existing sequence should remain to cover these edge cases.webroot/src/store/license/license.getters.ts (1)
19-19: No functional changeWhitespace-only change. No issues.
webroot/src/components/PrivilegeEventNode/PrivilegeEventNode.less (1)
47-50: Confirmed@fontSizeSmallerDefinition
- Found in
webroot/src/styles.common/_fonts.less(line 33) as@fontSizeSmaller: 1.3rem;
No further changes required.webroot/src/store/user/user.state.ts (1)
25-41: Good addition; keep reset semantics in mind
isLoadingPrivilegeHistoryis correctly added to the interface and initial state. Ensure it’s reset in logout/clear mutations to avoid stale spinners after user switches.webroot/src/network/userApi/interceptors.ts (1)
33-39: Over-broadincludes('me') && includes('history')can misroute tokens; also guard undefinedurl
- Axios
requestConfig.urlcan be undefined or absolute;url.includes(...)can throw or mis-match.- The substring check is too broad and may send licensee tokens to non-licensee endpoints that coincidentally contain both words.
Proposed hardening:
- if (licenseeUserEndPoints.includes(url) || (url.includes('me') && url.includes('history'))) { + // Normalize to path and precisely match licensee “me history” routes + const rawUrl = url || ''; + const path = rawUrl.replace(/^https?:\/\/[^/]+/, ''); // strip origin if present + const isLicenseeHistory = + /^\/v1\/provider-users\/me(\/.*)?\/history(\/.*)?$/i.test(path) || + /^\/v1\/provider-users\/me\/privileges\/history(\/.*)?$/i.test(path); + if (licenseeUserEndPoints.includes(path) || isLicenseeHistory) { authToken = authStorage.getItem(tokens.licensee.ID_TOKEN); authTokenType = authStorage.getItem(tokens.licensee.AUTH_TOKEN_TYPE); } else { authToken = authStorage.getItem(tokens.staff.AUTH_TOKEN); authTokenType = authStorage.getItem(tokens.staff.AUTH_TOKEN_TYPE); }Optional: if either token or type is missing, avoid sending a malformed
Authorizationheader and let downstream 401 surface cleanly.To verify endpoint patterns and ensure the regex matches real calls:
webroot/src/locales/es.json (1)
780-782: New deactivation notes look goodTranslations for license deactivation and home state change notes are clear and consistent with the UI text elsewhere.
webroot/src/models/LicenseHistoryItem/LicenseHistoryItem.model.spec.ts (5)
41-50: Defaults and displays LGTMDefaults to null/empty and displays ('', 'Unknown', '') align with model. Good coverage on baseline behavior.
55-75: Renewal mapping and displays LGTMDirect construction with serverNote and expected effective date display is correct. Solid positive-path test.
78-99: Serializer mapping LGTMnote maps to serverNote and date fields deserialize correctly. Effective date display expectation matches US locale.
100-123: homeJurisdictionChange behavior verifiedUpdate type display coerces to Deactivation and note uses i18n key. This matches the model’s special-casing.
124-147: licenseDeactivation behavior verifiedCovers new deactivation type end-to-end. Nice addition.
webroot/src/network/licenseApi/data.api.ts (2)
408-419: Return shaping LGTMUsing interceptors to return response.data and deserializing via LicenseHistorySerializer is consistent with adjacent methods.
429-440: No change needed for public endpoint path
The public/historyroute inbackend/compact-connect/lambdas/python/provider-data-v1/handlers/privilege_history.py(and its corresponding tests) is defined without a/privilegessegment, so the client call inwebroot/src/network/licenseApi/data.api.tsalready matches the backend. You can ignore the suggestion to insert/privilegesinto the public path.Likely an incorrect or invalid review comment.
webroot/src/models/License/License.model.spec.ts (1)
557-557: Aligns with removal of fabricated eventsAsserting raw history.length === 0 matches the model change to stop fabricating purchase/expiration events. Good.
webroot/src/store/license/license.spec.ts (1)
226-233: Test duplicates existing pattern – looks goodThe mutation request test mirrors prior patterns and keeps coverage consistent. No issues found.
webroot/src/store/user/user.mutations.ts (1)
41-44: Ensure mutation types are mirrored in actions & testsMutation enums look correct and unique – confirmed.
webroot/src/pages/PublicPrivilegeDetail/PublicPrivilegeDetail.ts (1)
115-127: Possible future 404 – license type abbreviation vs full stringYou pass
licenseTypeAbbrev(e.g.OT) downstream, while the store currently reconstructs an id with the full license-type label (see previous comment in mutations).
Once the mutation is fixed, ensure both sides agree on abbreviation vs full string to avoid mismatched history look-ups.webroot/src/pages/PrivilegeDetail/PrivilegeDetail.ts (1)
186-190: Watcher may double-fire under fast staff pathFor licensee users you guard correctly, but staff path relies solely on
populateData.
IflicenseeRecordfetch is slow, the privilege getter initially returns an empty object, then gains anid, triggering the watcher which callsfetchPrivilegeHistoryProvidereven for staff auth type = false. Current guard prevents this but a future refactor could break it.
Consider adding@Watch('isPrivilegeLoaded', { immediate: false, flush: 'post' })and an explicitif (!this.isLoggedInAsStaff)to avoid surprises.
jsandoval81
left a comment
jsandoval81
left a comment
There was a problem hiding this comment.
Nice work here.
I did more of an initial pass here for pattern implementation & questions. Once we get that in a good spot then we can focus more on the UI testing.
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (4)
webroot/src/network/userApi/data.api.ts (1)
21-21: Import looks good; consider alias consistency within this file.Minor: This file mixes
@models/...and@/...aliases. Pick one convention and stick to it for readability.webroot/src/store/license/license.spec.ts (3)
243-273: Align test data with production types for events.You’re pushing a string into
history.events. Downstream UI expectsLicenseHistoryIteminstances. Using the real serializer objects here will reduce the chance of false positives.Apply:
- const history = { + const history = { providerId: '1', jurisdiction: '2', licenseType: '3', - events: ['1'] + events: [new (require('@/models/LicenseHistoryItem/LicenseHistoryItem.model').LicenseHistoryItem)({ + updateType: 'renewal', + effectiveDate: '2024-01-01' + })] };Note: If importing in spec top-level is preferred, move the require/import accordingly.
274-304: Unfound-privilege test is good; consider asserting immutability.Add an assertion that other privileges remain untouched to better guard regression (e.g., history not added to the wrong record).
Example follow-up assertion:
- expect(state.model[1].privileges[1].history.length).to.equal(0)
469-489: Resolve the commented-out dispatch assertion or document intent.There’s a commented
dispatchassertion. Either:
- assert the intended behavior (dispatch should/should not be called), or
- remove the comment to avoid confusion.
If the non-public path performs the API call directly and does not chain another action, keep
dispatchunchecked and delete the commented line.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
webroot/src/network/licenseApi/data.api.ts(2 hunks)webroot/src/network/mocks/mock.data.api.ts(4 hunks)webroot/src/network/userApi/data.api.ts(2 hunks)webroot/src/network/userApi/interceptors.ts(1 hunks)webroot/src/store/license/license.mutations.ts(2 hunks)webroot/src/store/license/license.spec.ts(2 hunks)webroot/src/store/user/user.mutations.ts(2 hunks)webroot/src/store/user/user.spec.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- webroot/src/store/license/license.mutations.ts
- webroot/src/store/user/user.mutations.ts
- webroot/src/store/user/user.spec.ts
- webroot/src/network/mocks/mock.data.api.ts
🧰 Additional context used
🧬 Code Graph Analysis (3)
webroot/src/network/userApi/interceptors.ts (1)
webroot/src/app.config.ts (2)
authStorage(40-40)tokens(41-56)
webroot/src/store/license/license.spec.ts (2)
webroot/src/store/license/license.state.ts (1)
state(19-32)webroot/src/models/License/License.model.ts (1)
License(69-160)
webroot/src/network/licenseApi/data.api.ts (1)
webroot/src/models/LicenseHistoryItem/LicenseHistoryItem.model.ts (2)
LicenseHistoryItem(25-96)LicenseHistoryItemSerializer(101-117)
⏰ 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: CheckWebroot
🔇 Additional comments (2)
webroot/src/store/license/license.spec.ts (1)
490-510: Public request action test LGTM.The test correctly asserts that a public request path dispatches another action. No issues.
webroot/src/network/licenseApi/data.api.ts (1)
19-19: Good: Reusing LicenseHistoryItem serializer.This aligns event shape across contexts and avoids fabricated client events.
There was a problem hiding this comment.
Actionable comments posted: 4
🔭 Outside diff range comments (1)
webroot/src/pages/PrivilegeDetail/PrivilegeDetail.ts (1)
136-140: Guard against missing licenseeId and catch request errorsPrevent invalid calls when licenseeId is absent and make failures observable.
Apply this diff:
- async fetchLicenseeData(): Promise<void> { - const { licenseeId } = this; - - await this.$store.dispatch('license/getLicenseeRequest', { compact: this.compact, licenseeId }); - } + async fetchLicenseeData(): Promise<void> { + const { licenseeId } = this; + if (!licenseeId) return; + try { + await this.$store.dispatch('license/getLicenseeRequest', { compact: this.compact, licenseeId }); + } catch (error) { + console.error('Failed to fetch licensee data', { licenseeId, error }); + // Optionally surface a UI notification + } + }
♻️ Duplicate comments (3)
webroot/src/pages/PrivilegeDetail/PrivilegeDetail.ts (3)
32-34: Await and handle errors in mounted()mounted() calls an async method without await; errors will be unhandled. Make mounted async and catch errors.
Apply this diff:
- mounted() { - this.populateData(); - } + async mounted() { + try { + await this.populateData(); + } catch (error) { + // TODO: route to a user-visible notification if appropriate + console.error('Failed to populate privilege detail data', error); + } + }
142-154: Add try/catch, guard providerId, and confirm action payload keysWrap the dispatch to avoid unhandled rejections, early-return when providerId missing, and verify the action expects providerId (vs licenseeId).
Apply this diff:
- async fetchPrivilegeHistoryStaff(): Promise<void> { - const issueStateAbbrev = this.privilege.issueState?.abbrev; - - if (issueStateAbbrev && this.privilegeLicenseTypeAbbrev) { - await this.$store.dispatch('license/getPrivilegeHistoryRequest', { - compact: this.compact, - providerId: this.licenseeId, - jurisdiction: issueStateAbbrev, - licenseTypeAbbrev: this.privilegeLicenseTypeAbbrev, - isPublic: false - }); - } - } + async fetchPrivilegeHistoryStaff(): Promise<void> { + const issueStateAbbrev = this.privilege.issueState?.abbrev; + const providerId = this.licenseeId; + if (!providerId) return; + if (issueStateAbbrev && this.privilegeLicenseTypeAbbrev) { + try { + await this.$store.dispatch('license/getPrivilegeHistoryRequest', { + compact: this.compact, + providerId, + jurisdiction: issueStateAbbrev, + licenseTypeAbbrev: this.privilegeLicenseTypeAbbrev, + isPublic: false + }); + } catch (error) { + console.error('Failed to fetch staff privilege history', { providerId, issueStateAbbrev, error }); + } + } + }Run this script to confirm the expected payload keys for the license action and whether “providerId” is correct:
#!/bin/bash # Inspect license actions/mutations for getPrivilegeHistoryRequest payload rg -n "getPrivilegeHistoryRequest" -A 20 webroot/src/store # Look for usage of 'providerId' vs 'licenseeId' in the license history flow rg -n "providerId|licenseeId" -A 5 webroot/src/store | sed -n '1,200p'
167-181: Make populateData resilient and clarify branchingAdd try/catch and use else-if to avoid double-path execution. Optionally ensure isPrivilegeLoaded before staff history fetch.
Apply this diff:
- async populateData(): Promise<void> { - if (this.isLoggedInAsStaff) { - if (!this.licenseeRecord) { - await this.fetchLicenseeData(); - } - - if (!this.isPrivilegeHistoryLoaded) { - await this.fetchPrivilegeHistoryStaff(); - } - } - - if (this.isLoggedInAsLicensee && this.isPrivilegeLoaded && !this.isPrivilegeHistoryLoaded) { - await this.fetchPrivilegeHistoryLicensee(); - } - } + async populateData(): Promise<void> { + try { + if (this.isLoggedInAsStaff) { + if (!this.licenseeRecord) { + await this.fetchLicenseeData(); + } + if (this.isPrivilegeLoaded && !this.isPrivilegeHistoryLoaded) { + await this.fetchPrivilegeHistoryStaff(); + } + } else if (this.isLoggedInAsLicensee && this.isPrivilegeLoaded && !this.isPrivilegeHistoryLoaded) { + await this.fetchPrivilegeHistoryLicensee(); + } + } catch (error) { + console.error('Failed during populateData()', error); + } + }
🧹 Nitpick comments (15)
webroot/src/pages/LicensingDetail/LicensingDetail.ts (3)
49-51: De-duplicate: reuse existinglicenseecomputed instead of introducinglicenseeRecord.
licenseeRecordduplicates the logic oflicenseeone-to-one. Prefer a single source of truth and usethis.licenseefor the guard inmounted().Apply this diff:
- if (!this.licenseeRecord) { + if (!this.licensee) { await this.fetchLicenseeData(); - } + }
49-55: Confirm 404 routing only on true not-found, not transient fetch failures.After fetching,
!this.licenseetriggers a 404 redirect. If the fetch fails due to a network/server error, this will also route to 404. Verify the store distinguishes 404/not-found from other failures and only redirect when appropriate.I can propose a small pattern to gate the redirect on a store-level notFound flag or HTTP 404 response if you share the store’s error shape.
203-212: Remove duplicate computed getterlicenseeRecord.This getter exactly duplicates
licensee. Keeping both increases maintenance burden and cognitive load without adding value.Apply this diff to remove the duplicate:
- get licenseeRecord(): Licensee | null { - const { licenseeId } = this; - let storeRecord: Licensee | null = null; - - if (licenseeId && this.licenseStore.model) { - storeRecord = this.$store.getters['license/licenseeById'](licenseeId); - } - - return storeRecord; - }webroot/src/store/user/user.spec.ts (4)
413-437: Strengthen test by using real event objects rather than stringsRight now, history.events is ['1'], which doesn't validate the expected shape or serialization of history events. Use objects matching LicenseHistoryItemSerializer input and assert type on the appended item to catch regressions earlier.
As an example, change the test payload to something like:
const history = { providerId: '1', jurisdiction: '2', licenseType: '3', events: [{ type: 'privilegeUpdate', updateType: 'renewal', effectiveDate: '2025-05-01' }] };And add an assertion:
expect(state.model.licensee.privileges[0].history[0]).to.be.an.instanceof(LicenseHistoryItem);If needed, add:
import { LicenseHistoryItem } from '@models/LicenseHistoryItem/LicenseHistoryItem.model';
439-462: Also strengthen the “privilege not found” case using realistic event objectsSame rationale as above; using proper event objects helps ensure the mutation ignores non-matching privileges without mutating types inadvertently.
968-979: Fix typo in test description (“request request”)Minor nit: duplicate word in the test name.
Apply this diff:
- it('should successfully start privilege history request request', async () => { + it('should successfully start privilege history request', async () => {
989-998: Fix typo in test description (“start start”)Minor nit: duplicate word in the test name.
Apply this diff:
- it('should successfully start start privilege history success', () => { + it('should successfully start privilege history success', () => {webroot/src/models/LicenseHistoryItem/LicenseHistoryItem.model.ts (1)
83-94: noteDisplay override is appropriate; consider encumbrance-specific note laterThe special-case notes for homeJurisdictionChange and licenseDeactivation are helpful. If product wants specific copy for encumbrance events, we can extend this similarly.
webroot/src/components/PrivilegeDetailBlock/PrivilegeDetailBlock.ts (1)
62-64: Avoid trailing colon when there is no expiry dateCurrently expiresContent returns ': ' even when there’s no date, resulting in “(Expires: )”. Only include the colon when a date exists.
Apply this diff:
- get expiresContent(): string { - return this.isAdminDeactivated ? '' : `: ${this.privilege?.expireDateDisplay() || ''}`; - } + get expiresContent(): string { + if (this.isAdminDeactivated) return ''; + const exp = this.privilege?.expireDateDisplay() || ''; + return exp ? `: ${exp}` : ''; + }webroot/src/models/License/License.model.spec.ts (2)
560-591: Rename test title to reflect API rename (isAdminDeactivated)The description still says “isDeactivated”. Update to “isAdminDeactivated” for clarity.
Apply this diff:
- it('should populate isDeactivated correctly given license history (deactivation)', () => { + it('should populate isAdminDeactivated correctly given license history (deactivation)', () => {
592-624: Rename test title to reflect API rename (isAdminDeactivated)Same as above: update the test name for consistency.
Apply this diff:
- it('should populate isDeactivated correctly given license history (homeJurisdictionChange)', () => { + it('should populate isAdminDeactivated correctly given license history (homeJurisdictionChange)', () => {webroot/src/pages/PrivilegeDetail/PrivilegeDetail.ts (4)
79-81: Include staff history loading in isLoading and normalize to booleanIf license store tracks a separate isLoadingPrivilegeHistory (staff path), include it. Also return a strict boolean.
Apply this diff:
- return this.licenseStore?.isLoading || this.userStore?.isLoadingPrivilegeHistory || false; + return Boolean( + this.licenseStore?.isLoading + || this.licenseStore?.isLoadingPrivilegeHistory + || this.userStore?.isLoadingPrivilegeHistory + );Please verify that license store actually exposes isLoadingPrivilegeHistory; if not, ignore that part of the change.
117-120: Avoid dangling hyphen when either part is missingSmall UX polish: build the title from available parts to prevent " - " when abbrev or state is missing.
Apply this diff:
- return `${this.privilegeLicenseTypeAbbrev} - ${this.privilege?.issueState?.name() || ''}`; + const abbrev = this.privilegeLicenseTypeAbbrev; + const stateName = this.privilege?.issueState?.name() || ''; + return [abbrev, stateName].filter(Boolean).join(' - ');
125-127: Simplify boolean conversionThe trailing “|| false” is redundant.
Apply this diff:
- return !!this.privilege.history?.length || false; + return Boolean(this.privilege.history?.length);
186-190: Guard watcher against concurrent/in-flight requestsAdd an isLoadingPrivilegeHistory guard to prevent redundant dispatches if the watcher fires during an in-flight request. This also aligns with the team’s preference for using watchers cleanly.
Apply this diff:
- @Watch('isPrivilegeLoaded') async loadPrivilegeHistory() { - if (this.isLoggedInAsLicensee && !this.isPrivilegeHistoryLoaded) { - await this.fetchPrivilegeHistoryLicensee(); - } - } + @Watch('isPrivilegeLoaded') + async loadPrivilegeHistory() { + if ( + this.isLoggedInAsLicensee + && !this.isPrivilegeHistoryLoaded + && !this.userStore?.isLoadingPrivilegeHistory + ) { + await this.fetchPrivilegeHistoryLicensee(); + } + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
webroot/src/components/PrivilegeDetailBlock/PrivilegeDetailBlock.ts(2 hunks)webroot/src/models/License/License.model.spec.ts(5 hunks)webroot/src/models/License/License.model.ts(2 hunks)webroot/src/models/LicenseHistoryItem/LicenseHistoryItem.model.ts(5 hunks)webroot/src/pages/LicensingDetail/LicensingDetail.ts(2 hunks)webroot/src/pages/PrivilegeDetail/PrivilegeDetail.ts(5 hunks)webroot/src/pages/PublicPrivilegeDetail/PublicPrivilegeDetail.ts(3 hunks)webroot/src/store/license/license.spec.ts(2 hunks)webroot/src/store/user/user.spec.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- webroot/src/pages/PublicPrivilegeDetail/PublicPrivilegeDetail.ts
- webroot/src/store/license/license.spec.ts
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-06-09T19:57:51.519Z
Learnt from: rmolinares
PR: csg-org/CompactConnect#851
File: webroot/src/pages/RegisterLicensee/RegisterLicensee.ts:0-0
Timestamp: 2025-06-09T19:57:51.519Z
Learning: In the RegisterLicensee component, when handling DOM element availability issues, the developer prefers using Vue Watchers over retry mechanisms with requestAnimationFrame to avoid infinite recursion risks and maintain Vue's reactive patterns.
Applied to files:
webroot/src/pages/LicensingDetail/LicensingDetail.tswebroot/src/pages/PrivilegeDetail/PrivilegeDetail.ts
🧬 Code Graph Analysis (4)
webroot/src/pages/LicensingDetail/LicensingDetail.ts (6)
webroot/src/models/Licensee/Licensee.model.ts (1)
Licensee(63-284)webroot/src/pages/PrivilegeDetail/PrivilegeDetail.ts (1)
licenseeId(75-77)webroot/src/pages/PublicPrivilegeDetail/PublicPrivilegeDetail.ts (1)
licenseeId(56-58)webroot/src/components/PrivilegeCard/PrivilegeCard.ts (1)
licenseeId(121-123)webroot/src/components/LicenseCard/LicenseCard.ts (1)
licenseeId(133-135)webroot/src/pages/PublicLicensingDetail/PublicLicensingDetail.ts (1)
licenseeId(55-57)
webroot/src/store/user/user.spec.ts (2)
webroot/src/store/user/user.state.ts (1)
state(34-47)webroot/src/models/License/License.model.ts (1)
License(69-156)
webroot/src/models/License/License.model.ts (1)
webroot/src/models/LicenseHistoryItem/LicenseHistoryItem.model.ts (1)
LicenseHistoryItem(25-95)
webroot/src/models/License/License.model.spec.ts (2)
webroot/src/models/LicenseHistoryItem/LicenseHistoryItem.model.ts (1)
LicenseHistoryItem(25-95)webroot/src/models/License/License.model.ts (1)
LicenseSerializer(161-199)
⏰ 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: CheckWebroot
🔇 Additional comments (8)
webroot/src/store/user/user.spec.ts (1)
396-403: LGTM: mutation request case coveredThe test correctly asserts loading and error reset for GET_PRIVILEGE_HISTORY_REQUEST.
webroot/src/models/LicenseHistoryItem/LicenseHistoryItem.model.ts (3)
17-19: Fields align with backend shape and PR goalsAdding createDate, effectiveDate, and serverNote aligns with the “use effectiveDate for timeline events” objective and enables richer UI.
76-79: Override for shared “Deactivation” label looks goodMapping both homeJurisdictionChange and licenseDeactivation to a common deactivation translation keeps the UI consistent.
108-112: Serializer mapping matches new fieldsMapping createDate/effectiveDate and note→serverNote is correct; removal of previous/updated value maps aligns with the simplified model.
webroot/src/models/License/License.model.spec.ts (1)
552-558: LGTM: fabricated history removed and contract updatedAsserting history.length === 0 after serialization correctly reflects the new flow (history fetched separately).
webroot/src/pages/PrivilegeDetail/PrivilegeDetail.ts (3)
8-12: LGTM on bringing in the Watch decoratorThe Watch import is used correctly below and aligns with the team’s preference for Vue watchers (per prior learnings).
113-116: LGTM: dedicated abbreviation getter improves readabilityCentralizing the abbrev lookup is clean and avoids repeating calls to licenseTypeAbbreviation().
121-124: LGTM: simple and clear loaded checkBoolean coercion on privilege.id is appropriate.
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (1)
webroot/src/network/mocks/mock.data.ts (1)
3009-3009: Fix privilegeId prefix to match licenseType (OTA vs OT).licenseType is “occupational therapy assistant” (OTA) but privilegeId starts with “OT-…”. Align the prefix to avoid inconsistent rendering and lookups.
- privilegeId: 'OT-NE-26', + privilegeId: 'OTA-NE-26',
🧹 Nitpick comments (3)
webroot/src/network/mocks/mock.data.ts (1)
3026-3031: Consider treating “lifting_encumbrance” as an activating event.Currently only “renewal” is activating. If your UI uses color/status semantics, lifting an encumbrance typically restores privilege activity and should be rendered as activating.
Outside this file (LicenseHistoryItem.model.ts):
- public isActivatingEvent(): boolean { - const activatingEvents = ['renewal']; + public isActivatingEvent(): boolean { + const activatingEvents = ['renewal', 'lifting_encumbrance']; return activatingEvents.some((event) => (this.updateType && event === this.updateType)); }Also applies to: 3127-3132, 3172-3177
webroot/src/network/mocks/mock.data.api.ts (2)
537-543: Fix JSDoc: return type and description are incorrect.The method returns license history data, not a User model instance. Update the JSDoc to reflect actual return shape.
- * @return {Promise<>} A User model instance. + * @return {Promise<{ + * compact: string; + * jurisdiction: string; + * licenseType: string; + * privilegeId: string; + * providerId: string; + * events: LicenseHistoryItem[]; + * }>} License history data for the authenticated licensee user.
294-345: Optional: factor out duplicate serialization logic.Three methods duplicate the licenseHistoryData construction. Extract to a private helper for maintainability.
Example (outside the selected ranges):
private buildLicenseHistoryData(serverResponse: any): { compact: string; jurisdiction: string; licenseType: string; privilegeId: string; providerId: string; events: LicenseHistoryItem[]; } { return { compact: serverResponse.compact, jurisdiction: serverResponse.jurisdiction, licenseType: serverResponse.licenseType, privilegeId: serverResponse.privilegeId, providerId: serverResponse.providerId, events: Array.isArray(serverResponse.events) ? serverResponse.events.map((e) => LicenseHistoryItemSerializer.fromServer(e)) : [], }; }Then call this helper in each endpoint.
Also applies to: 346-396, 543-581
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
webroot/src/network/mocks/mock.data.api.ts(4 hunks)webroot/src/network/mocks/mock.data.ts(1 hunks)webroot/src/store/user/user.mutations.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- webroot/src/store/user/user.mutations.ts
🧰 Additional context used
🧬 Code Graph Analysis (1)
webroot/src/network/mocks/mock.data.api.ts (7)
webroot/src/pages/PrivilegeDetail/PrivilegeDetail.ts (1)
compact(83-87)webroot/src/pages/PublicPrivilegeDetail/PublicPrivilegeDetail.ts (1)
compact(60-62)webroot/src/pages/LicensingDetail/LicensingDetail.ts (1)
compact(61-65)webroot/src/pages/PublicLicensingDetail/PublicLicensingDetail.ts (1)
compact(49-53)webroot/src/components/LicenseCard/LicenseCard.ts (1)
licenseTypeAbbrev(149-151)webroot/src/network/mocks/mock.data.ts (4)
mockPrivilegeHistoryResponse1(3004-3089)mockPrivilegeHistoryResponse2(3091-3134)mockPrivilegeHistoryResponse3(3136-3179)mockPrivilegeHistoryResponse4(3181-3210)webroot/src/models/LicenseHistoryItem/LicenseHistoryItem.model.ts (2)
LicenseHistoryItem(25-95)LicenseHistoryItemSerializer(100-116)
⏰ 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: CheckWebroot
🔇 Additional comments (2)
webroot/src/network/mocks/mock.data.api.ts (2)
10-10: LGTM: Correct import for LicenseHistoryItem and serializer.
28-33: LGTM: Mock privilege history imports are correctly added.
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
webroot/src/components/PrivilegeEventNode/PrivilegeEventNode.vue(1 hunks)webroot/src/components/PrivilegeHistory/PrivilegeHistory.vue(1 hunks)webroot/src/locales/en.json(3 hunks)webroot/src/locales/es.json(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- webroot/src/components/PrivilegeEventNode/PrivilegeEventNode.vue
- webroot/src/components/PrivilegeHistory/PrivilegeHistory.vue
⏰ 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: CheckWebroot
🔇 Additional comments (6)
webroot/src/locales/es.json (2)
637-637: LGTM: new "deactivation" labelAddition is consistent with UI changes introducing deactivation notes.
665-665: LGTM: added "events" labelMatches the new timeline/taxonomy UI.
webroot/src/locales/en.json (4)
653-653: LGTM: new "deactivation" labelConsistent with the added deactivation note UI.
681-681: LGTM: added "events" labelMatches the new timeline grouping.
798-799: LGTM: added explanatory notesThese keys align with new history behavior and mirror the Spanish locale.
683-715: Consider updating the issuance display label
All event-mapping code and tests correctly recognize the newissuancekey, but the user-facing label still reads “Privilege purchased.” If you’d like to align the phrasing with the key’s semantics, you can change it in your English locale:• File: webroot/src/locales/en.json
"licenseEvents": [ - { - "name": "Privilege purchased", - "key": "issuance" - }, + { + "name": "Issued", + "key": "issuance" + }, { "name": "Renewal", "key": "renewal" },If you prefer to retain “Privilege purchased” for familiarity, no additional code changes are necessary.
jsandoval81
left a comment
jsandoval81
left a comment
There was a problem hiding this comment.
Pattern and UI are looking good 👍
Just some mostly minor items remaining.
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
webroot/src/pages/PublicPrivilegeDetail/PublicPrivilegeDetail.ts (1)
75-84: Type mismatch:foundPrivilegeshould beLicense | null, notLicensee | nullThis will cause a TypeScript error and can break downstream typings.
- get privilege(): License | null { - let foundPrivilege: Licensee | null = null; + get privilege(): License | null { + let foundPrivilege: License | null = null; if (this.licenseeRecord) { foundPrivilege = this.licenseeRecord.privileges?.find((privilege: License) => (privilege.id === this.privilegeId)) || null; } return foundPrivilege; }
🧹 Nitpick comments (4)
webroot/src/pages/PublicLicensingDetail/PublicLicensingDetail.ts (1)
32-40: Guard on licensee presence (not privileges length), type the hook, and avoid polluting history on 404
- Checking
licenseePrivileges.lengthcan trigger unnecessary fetches when a loaded licensee legitimately has no privileges. Guard onlicenseeinstead.- Add an explicit return type to the lifecycle hook.
- Prefer
router.replacefor 404 to avoid adding an extra history entry.- async created() { - if (!this.licenseePrivileges.length) { + async created(): Promise<void> { + if (!this.licensee) { await this.fetchLicenseeData(); } - if (!this.licensee) { - this.$router.push({ name: '404' }); + if (!this.licensee) { + this.$router.replace({ name: '404' }); + return; } }webroot/src/pages/LicensingDetail/LicensingDetail.ts (1)
48-52: Use the existinglicenseegetter and add an explicit return type
licenseeRecordduplicateslicensee; use the existinglicenseegetter here to avoid divergence.- Add an explicit return type for consistency.
- async created() { - if (!this.licenseeRecord) { + async created(): Promise<void> { + if (!this.licensee) { await this.fetchLicenseeData(); }webroot/src/pages/PublicPrivilegeDetail/PublicPrivilegeDetail.ts (2)
27-34: Move looks good; add explicit return type to the hookThe transition to
createdis appropriate here. Add an explicit return type for consistency across pages.- async created() { + async created(): Promise<void> { if (!this.licenseeRecord) { await this.fetchLicenseePublicData(); } if (!this.isPrivilegeHistoryLoaded) { await this.fetchPrivilegeHistory(); } }
90-97: Optional: prevent duplicate fetches when history is legitimately empty
isPrivilegeHistoryLoadedreturns false for an empty history, so a remount may re-fetch unnecessarily. Consider guarding with a store-level flag (e.g.,isLoadingPrivilegeHistoryorhasFetchedPrivilegeHistoryper-privilege) to avoid redundant calls when history is empty but already fetched.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these settings in your CodeRabbit configuration.
📒 Files selected for processing (13)
webroot/src/components/PrivilegeEventNode/PrivilegeEventNode.vue(2 hunks)webroot/src/components/PrivilegeHistory/PrivilegeHistory.vue(1 hunks)webroot/src/models/License/License.model.ts(2 hunks)webroot/src/network/data.api.ts(2 hunks)webroot/src/network/licenseApi/data.api.ts(2 hunks)webroot/src/network/mocks/mock.data.api.ts(4 hunks)webroot/src/network/mocks/mock.data.ts(1 hunks)webroot/src/pages/LicensingDetail/LicensingDetail.ts(2 hunks)webroot/src/pages/PrivilegeDetail/PrivilegeDetail.ts(5 hunks)webroot/src/pages/PublicLicensingDetail/PublicLicensingDetail.ts(1 hunks)webroot/src/pages/PublicPrivilegeDetail/PublicPrivilegeDetail.ts(3 hunks)webroot/src/store/license/license.mutations.ts(2 hunks)webroot/src/store/user/user.actions.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (10)
- webroot/src/store/license/license.mutations.ts
- webroot/src/components/PrivilegeHistory/PrivilegeHistory.vue
- webroot/src/components/PrivilegeEventNode/PrivilegeEventNode.vue
- webroot/src/network/data.api.ts
- webroot/src/pages/PrivilegeDetail/PrivilegeDetail.ts
- webroot/src/network/licenseApi/data.api.ts
- webroot/src/store/user/user.actions.ts
- webroot/src/models/License/License.model.ts
- webroot/src/network/mocks/mock.data.api.ts
- webroot/src/network/mocks/mock.data.ts
🧰 Additional context used
🧬 Code Graph Analysis (1)
webroot/src/pages/LicensingDetail/LicensingDetail.ts (6)
webroot/src/models/Licensee/Licensee.model.ts (1)
Licensee(63-284)webroot/src/pages/PrivilegeDetail/PrivilegeDetail.ts (1)
licenseeId(75-77)webroot/src/pages/PublicLicensingDetail/PublicLicensingDetail.ts (1)
licenseeId(55-57)webroot/src/pages/PublicPrivilegeDetail/PublicPrivilegeDetail.ts (1)
licenseeId(56-58)webroot/src/components/PrivilegeCard/PrivilegeCard.ts (1)
licenseeId(121-123)webroot/src/components/LicenseCard/LicenseCard.ts (1)
licenseeId(133-135)
⏰ 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: CheckWebroot
🔇 Additional comments (1)
webroot/src/pages/PublicPrivilegeDetail/PublicPrivilegeDetail.ts (1)
115-127: Confirmed — use providerId (no change required)The store action, network API, mocks, and mutations all expect/providerId; dispatching providerId: this.licenseeId is correct.
- webroot/src/store/license/license.actions.ts — getPrivilegeHistoryRequest signature accepts providerId and forwards it to dataApi.getPrivilegeHistoryPublic/getPrivilegeHistoryStaff
- webroot/src/network/licenseApi/data.api.ts — getPrivilegeHistoryPublic/getPrivilegeHistoryStaff use providerId in the URL/response
- webroot/src/store/license/license.mutations.ts — GET_PRIVILEGE_HISTORY_SUCCESS uses history.providerId to locate the licensee
- webroot/src/pages/PublicPrivilegeDetail/PublicPrivilegeDetail.ts — dispatch uses providerId: this.licenseeId (matches expected payload)
- Note: webroot/src/store/user/user.actions.ts contains getPrivilegeHistoryRequestLicensee (different action for the current licensee)
|
@jlkravitz This is ready for your review. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (7)
webroot/src/pages/PrivilegeDetail/PrivilegeDetail.vue (3)
13-15: Prevent empty render gap while history loadsWith the current gating, there’s a possible “empty viewport” window if
isLoadingis false whileisPrivilegeLoadedorisPrivilegeHistoryLoadedare still false (spinner hidden and content hidden). Keep the spinner visible until both are loaded.Apply this change to the spinner (outside the changed hunk) to tie its visibility to the composite load state:
<!-- line 11 --> <LoadingSpinner class="place-holder" v-show="isLoading || !isPrivilegeLoaded || !isPrivilegeHistoryLoaded"></LoadingSpinner>Optionally, simplify the outer container condition to rely only on the loaded flags:
- <div v-if="!isLoading && isPrivilegeHistoryLoaded && isPrivilegeLoaded"> + <div v-if="isPrivilegeHistoryLoaded && isPrivilegeLoaded">
14-22: Handle unauthorized/unauthenticated branch explicitlyIf neither
isLoggedInAsLicenseenorlicenseeRecordis truthy, the page renders nothing (spinner hidden and content gated). Consider rendering an authorization notice or redirecting, to avoid a blank state.Would you like a small “No access” placeholder with a Back button, or should this be handled by a route guard?
23-25: Use semantic heading for the titleImprove a11y by using a heading element for the page title.
Apply:
- <div class="privilege-detail-title"> - {{ privilegeTitle }} - </div> + <h1 class="privilege-detail-title"> + {{ privilegeTitle }} + </h1>webroot/src/store/license/license.spec.ts (4)
243-271: Strengthen success assertions by checking assigned eventsThe test currently validates length only. Assert the events array was assigned as-is to the target privilege’s history to catch shape regressions.
Apply:
mutations[MutationTypes.GET_PRIVILEGE_HISTORY_SUCCESS](state, { history }); expect(state.isLoading).to.equal(false); expect(state.error).to.equal(null); - expect(state.model[1].privileges[0].history.length).to.equal(1); + expect(state.model[1].privileges[0].history.length).to.equal(1); + expect(state.model[1].privileges[0].history).to.deep.equal(history.events);
465-484: Assert no dispatch on non-public flowFor the non-public request case, ensure we don’t dispatch the public variant (or any follow-up) by asserting dispatch wasn’t called.
Apply:
await actions.getPrivilegeHistoryRequest({ commit, dispatch }, { compact, providerId, jurisdiction, licenseTypeAbbrev, isPublic }); expect(commit.calledOnce).to.equal(true); expect(commit.firstCall.args).to.matchPattern([MutationTypes.GET_PRIVILEGE_HISTORY_REQUEST]); + expect(dispatch.notCalled).to.equal(true);
485-505: Public request flow assertion is adequateAsserting a single dispatch is a reasonable baseline. If feasible, consider also asserting the dispatched action name and payload to lock in the branch behavior.
I can add an assertion on
dispatch.firstCall.argsif you can share the action name expected for the public flow.
515-524: Fix test name typo (“start start”)Minor wording tweak for readability.
Apply:
- it('should successfully start start privilege history success', () => { + it('should successfully start privilege history success', () => {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
webroot/src/pages/LicensingDetail/LicensingDetail.ts(1 hunks)webroot/src/pages/PrivilegeDetail/PrivilegeDetail.vue(1 hunks)webroot/src/store/license/license.spec.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- webroot/src/pages/LicensingDetail/LicensingDetail.ts
🧰 Additional context used
🧬 Code Graph Analysis (1)
webroot/src/store/license/license.spec.ts (2)
webroot/src/store/license/license.state.ts (1)
state(19-32)webroot/src/models/License/License.model.ts (1)
License(69-156)
🔇 Additional comments (6)
webroot/src/pages/PrivilegeDetail/PrivilegeDetail.vue (1)
26-28: Consolidated PrivilegeDetailBlock is correctRendering a single PrivilegeDetailBlock inside the loaded/authorized container removes duplicate renders and avoids double-fetch side-effects. Looks good.
webroot/src/store/license/license.spec.ts (5)
226-233: Mutation request test looks goodThe request mutation assertions follow the existing pattern in this spec and are consistent with the store’s state shape.
234-242: Mutation failure test looks goodCorrectly verifies loading reset and error set.
272-300: Unfound privilege path test is clearGood coverage for the “privilege not located” scenario; validates non-destructive behavior.
506-514: Failure action test looks goodCorrectly checks the committed mutation and payload.
243-271: Confirm ID composition vs. matching keys to avoid brittle testsThe success-path mutation test assumes privilege IDs follow
providerId-jurisdiction-licenseType(e.g., '1-2-3'), while action tests passlicenseTypeAbbrevfor the request. Ensure the mutation’s matching logic uses the same key scheme as the privilege IDs in state (abbrev vs. full vs. numeric/string code), otherwise a future change in ID composition could silently desync the test.If the store uses
licenseTypeAbbrevfor matching in production, consider updating this test to build thehistoryfixture accordingly (or to assert against a computed lookup key rather than a hard-coded concatenated ID).Also applies to: 272-300, 465-505
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (6)
webroot/src/network/mocks/mock.data.ts (3)
3042-3042: Confirm all imports switched to the new constant name.This file exports mockPrivilegeHistoryResponses (plural). Past code and comments referenced mockPrivilegeHistoryReponse/Response (singular). Ensure all mock endpoints now import the pluralized name.
Run:
#!/bin/bash # Find remaining references to old names and usages of the new one rg -n -C2 -S "mockPrivilegeHistoryReponse|mockPrivilegeHistoryResponse|mockPrivilegeHistoryResponses" # Sanity-check the mock API to ensure it consumes an array and selects by provider/privilege context rg -n -C3 -S "mock\.data\.api\.ts|mock.data.api.ts"
3086-3091: “expiration” vs “expired” string mismatch likely breaks deactivation styling/logic.Your model logic and translation keys in the app typically use “expired” (not “expiration”). Using “expiration” here means these events won’t be treated as deactivating where expected.
Option A (preferred): support both in the model; Option B: normalize mocks to “expired”. If you choose B, apply:
- updateType: 'expiration', + updateType: 'expired',Occurrences to update within this hunk:
- Lines 3086-3091
- Lines 3144-3149
- Lines 3188-3193
- Lines 3232-3237
Also applies to: 3144-3149, 3188-3193, 3232-3237
3047-3049: PrivilegeId prefix does not match licenseType (OTA vs OT).licenseType is “occupational therapy assistant” (OTA) but privilegeId starts with “OT-…”. This will produce inconsistent labels and can break filtering/serialization that derive license type from the ID.
Apply this diff to fix the prefix:
- privilegeId: 'OT-NE-26', + privilegeId: 'OTA-NE-26',webroot/src/models/License/License.model.spec.ts (3)
129-129: Same here—rename usage is consistent
212-212: Same here—rename usage is consistent
557-557: Same here—rename usage is consistent
🧹 Nitpick comments (6)
webroot/src/network/mocks/mock.data.ts (1)
3246-3247: Optional: tighten types withas constto catch typos at compile-time.Freezing this structure helps TypeScript infer literal unions for updateType strings and reduces regressions from future typos.
-]; +] as const;webroot/src/locales/en.json (2)
799-801: Rename translation key for consistencyTo align with the
homeJurisdictionChangeevent type and improve discoverability, consider renaming the note key:• In webroot/src/locales/en.json
- Change
"homeStateChangeNote": "Deactivated due to home state change"
to
"homeJurisdictionChangeNote": "Deactivated due to home state change"• In webroot/src/locales/es.json
- Change
"homeStateChangeNote": "Desactivado debido al cambio de estado de origen"
to
"homeJurisdictionChangeNote": "Desactivado debido al cambio de estado de origen"• In webroot/src/models/LicenseHistoryItem/LicenseHistoryItem.model.ts
- Update the translation lookup:
this.$t('licensing.homeStateChangeNote')→this.$t('licensing.homeJurisdictionChangeNote')After renaming, verify there are no remaining references to
homeStateChangeNote.
684-715: Minor display name consistency—use Title Case for “Privilege Purchased”
- All event keys match backend and should remain as-is (including the snake_case
lifting_encumbrance).- Only the display name for
issuanceis in sentence case; update it to Title Case to match the others.webroot/src/locales/en.json (at the
issuanceentry):{ - "name": "Privilege purchased", + "name": "Privilege Purchased", "key": "issuance" },webroot/src/models/License/License.model.spec.ts (3)
562-562: Serializer ignoring server-provided history is intentional; assertion is correctGiven history is fetched via dedicated endpoints (per PR objectives), asserting
license.history.length === 0afterfromServeris the right expectation.Optionally, consider adding a short comment in the test (or serializer) clarifying that history is intentionally omitted by
LicenseSerializer.fromServerand retrieved via separate store actions to avoid confusion for future readers.
566-597: Rename test title to reflect the new API: use isAdminDeactivatedThe test description still references the old method name which can confuse future maintainers.
Apply this change:
-it('should populate isDeactivated correctly given license history (deactivation)', () => { +it('should set isAdminDeactivated true given license history (deactivation)', () => {Also consider adding coverage for the new event types mentioned in the PR (e.g., 'licenseDeactivation', 'encumbrance') to prevent regressions. I can draft those additional tests if helpful.
598-631: Same nit: update test title to isAdminDeactivatedThis case is correct functionally; let's align the title with the API rename.
-it('should populate isDeactivated correctly given license history (homeJurisdictionChange)', () => { +it('should set isAdminDeactivated true given license history (homeJurisdictionChange)', () => {Optional: add a companion negative test ensuring 'lifting-encumbrance' does not set
isAdminDeactivated()to true, to capture the intended distinctions between event types.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (5)
webroot/src/locales/en.json(3 hunks)webroot/src/locales/es.json(3 hunks)webroot/src/models/License/License.model.spec.ts(5 hunks)webroot/src/models/License/License.model.ts(2 hunks)webroot/src/network/mocks/mock.data.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- webroot/src/locales/es.json
- webroot/src/models/License/License.model.ts
🧰 Additional context used
🧬 Code Graph Analysis (1)
webroot/src/models/License/License.model.spec.ts (2)
webroot/src/models/LicenseHistoryItem/LicenseHistoryItem.model.ts (1)
LicenseHistoryItem(25-95)webroot/src/models/License/License.model.ts (1)
LicenseSerializer(184-222)
🔇 Additional comments (5)
webroot/src/network/mocks/mock.data.ts (2)
3051-3056: Good: events include effectiveDate and createDate.This aligns with the objective “Use effectiveDate for timeline events” and supports server-driven fabrication. Keep it consistent across all event types.
3065-3070: No changes needed forupdateTypecasingVerified that across the frontend models, serializers, locale keys, and backend schemas/tests, the canonical event-type strings are:
lifting_encumbrance(snake_case)homeJurisdictionChange(camelCase)No occurrences of kebab-case (
lifting-encumbranceorhome-jurisdiction-change) were found in the codebase or tests. The mock entries already match the expected values.webroot/src/locales/en.json (2)
682-682: LGTM: New section label "Events"The addition of "events": "Events" aligns with the new timeline UI.
654-656: No redundant “Deactivation” label in the modalOur search shows that the
licensing.deactivationkey is only used in
- webroot/src/models/LicenseHistoryItem/LicenseHistoryItem.model.ts:77
to render the event name in the license-history timeline.The Deactivate Privilege modal itself uses the
deactivateNotesTitle/deactivateNotesPlaceholderkeys, notdeactivation, so there is no duplicate heading shown to users in close proximity.No changes required.
webroot/src/models/License/License.model.spec.ts (1)
72-72: ✅ Migration complete: no lingeringisDeactivatedorhistoryWithFabricatedEventsusagesRan a repo-wide search and found no remaining calls to the old
isDeactivatedAPI or references tohistoryWithFabricatedEvents. The test expectation—expect(license.isAdminDeactivated()).to.equal(false);—now correctly reflects the updated API.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
webroot/src/models/License/License.model.ts (1)
123-131: Note: Using Array.at(-1) requires runtime support or a polyfillThis method relies on Array.prototype.at; the ambient typing is provided in shims, but ensure a runtime polyfill exists for targeted browsers that lack native support. See my comment in shims-tsx.d.ts for verification steps.
🧹 Nitpick comments (3)
webroot/src/shims-tsx.d.ts (1)
20-22: If keeping the shim, mirror ReadonlyArray typings for parity with lib.es2022Readonly arrays also support at; add its signature to avoid friction in readonly contexts.
Apply this diff:
declare global { namespace JSX { @@ - interface Array<T> { - at(index: number): T | undefined; - } + interface Array<T> { + at(index: number): T | undefined; + } + interface ReadonlyArray<T> { + at(index: number): T | undefined; + } }webroot/src/models/License/License.model.ts (2)
123-131: Derive “latest” event by date, not array order; avoid constructing an empty modelUsing history?.at(-1) assumes the array is chronologically ordered; if API/store ever returns unsorted events, this can yield incorrect results. Also, constructing a new LicenseHistoryItem for a missing last event is unnecessary.
Refactor to:
- Early-return if status isn’t INACTIVE.
- Pick the latest event by effectiveDate (fallback to dateOfUpdate, then createDate).
- Narrow types instead of instantiating a dummy model.
Apply this diff:
- public isAdminDeactivated(): boolean { - // NOTE: History is needed to determine this status; and history may be fetched in a separate API call and not always available on the License / Privilege list fetch - const adminDeactivateList = ['deactivation', 'licenseDeactivation', 'homeJurisdictionChange']; - const isInactive = this.status === LicenseStatus.INACTIVE; - const lastEvent: LicenseHistoryItem = this.history?.at(-1) || new LicenseHistoryItem(); - const lastEventType = lastEvent?.updateType || ''; - - return isInactive && adminDeactivateList.includes(lastEventType); - } + public isAdminDeactivated(): boolean { + // NOTE: History is needed to determine this status; and history may be fetched in a separate API call and not always available on the License / Privilege list fetch + const adminDeactivateList = ['deactivation', 'licenseDeactivation', 'homeJurisdictionChange']; + if (this.status !== LicenseStatus.INACTIVE) return false; + + const events: Array<LicenseHistoryItem> = Array.isArray(this.history) ? this.history : []; + if (!events.length) return false; + + const toMillis = (h: LicenseHistoryItem): number => { + const d = h?.effectiveDate || h?.dateOfUpdate || h?.createDate || ''; + const m = moment(d, [serverDateFormat, moment.ISO_8601], true); + return m.isValid() ? m.valueOf() : -Infinity; + }; + + const latest = events.reduce((a, b) => (toMillis(a) >= toMillis(b) ? a : b)); + const lastEventType = latest?.updateType; + + return typeof lastEventType === 'string' && adminDeactivateList.includes(lastEventType); + }Follow-up:
- If the store/serializer guarantees history is always sorted by effectiveDate ascending, keeping history?.at(-1) is okay; please confirm that invariant. Otherwise, the above keeps this helper robust regardless of upstream ordering.
123-131: Hoist adminDeactivateList to a module-level constant to avoid per-call allocationsMinor nit: define the list once outside the method.
Example (outside the class):
const ADMIN_DEACTIVATE_EVENTS = ['deactivation', 'licenseDeactivation', 'homeJurisdictionChange'] as const;Then use ADMIN_DEACTIVATE_EVENTS inside the method.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
webroot/src/models/License/License.model.ts(2 hunks)webroot/src/shims-tsx.d.ts(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
webroot/src/models/License/License.model.ts (1)
webroot/src/models/LicenseHistoryItem/LicenseHistoryItem.model.ts (1)
LicenseHistoryItem(25-95)
🔇 Additional comments (2)
webroot/src/shims-tsx.d.ts (1)
20-22: Manually verify TS lib and runtime support for Array.atIt appears we couldn’t reliably read your webroot/tsconfig.json (parse error) or detect any
Array.prototype.atcallsites or polyfills. Please manually confirm:
In
webroot/tsconfig.json, undercompilerOptions.lib, include"ES2022"(or"ESNext") so you get the officialArray.prototype.attypings.If your browserslist targets environments without native
Array.prototype.at, add a runtime polyfill (e.g. viacore-js/features/array/at).If you decide to keep the ambient shim, also add the
ReadonlyArrayoverload to match the official lib:interface ReadonlyArray<T> { at(index: number): T | undefined; }webroot/src/models/License/License.model.ts (1)
13-13: Import looks correctThis import is required since the method instantiates LicenseHistoryItem. No issues.
|
@jlkravitz Ok, ready for another look. Thanks! |
jlkravitz
left a comment
jlkravitz
left a comment
There was a problem hiding this comment.
@isabeleliassen good to merge!
4 participants
Requirements List
yarn install --ignore-enginesDescription List
Testing List
yarn test:unit:allshould run without errors or warningsyarn serveshould run without errors or warningsyarn buildshould run without errors or warningsCloses #742
Summary by CodeRabbit
New Features
Enhancements
Tests