Frontend/compact state config #822
Conversation
|
""" WalkthroughThis update introduces a comprehensive state self-serve configuration feature, including new Vue components, models, API endpoints, styles, and localization. It enables authorized users to view and edit compact and state-specific settings, such as fees, notifications, and privilege toggles, with robust form validation, confirmation modals, and permission-based navigation. Supporting unit tests and mock data are included. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant CompactSettingsPage
participant StateSettingsPage
participant CompactSettingsConfig
participant StateSettingsConfig
participant DataApi
User->>CompactSettingsPage: Navigate to /:compact/Settings
CompactSettingsPage->>CompactSettingsConfig: Render if isCompactAdmin
CompactSettingsPage->>StateSettingsPage: Route to /:compact/Settings/:state if state admin
User->>StateSettingsPage: Navigate to /:compact/Settings/:state
StateSettingsPage->>StateSettingsConfig: Render if state admin
CompactSettingsConfig->>DataApi: getCompactConfig(compact)
StateSettingsConfig->>DataApi: getCompactStateConfig(compact, state)
User->>CompactSettingsConfig: Edit form, submit
CompactSettingsConfig->>DataApi: updateCompactConfig(compact, config)
User->>StateSettingsConfig: Edit form, submit
StateSettingsConfig->>DataApi: updateCompactStateConfig(compact, state, config)
Assessment against linked issues
Assessment against linked issues: Out-of-scope changesNo out-of-scope changes detected related to the objectives in the linked issues. Possibly related PRs
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (1)
✨ Finishing Touches
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. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
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: 15
🧹 Nitpick comments (25)
webroot/src/components/PaymentProcessorConfig/PaymentProcessorConfig.less (1)
17-21: Typo in class name: “payment-processer” vs “processor”.The selector
.payment-processer-submitis spelled differently from the root.payment-processor(note the extra “e”). For clarity and consistency, rename it to.payment-processor-submit.Proposed diff:
-.payment-processer-submit { + .payment-processor-submit { :deep(.input-submit) { width: 100%; } }webroot/src/components/PaymentProcessorConfig/PaymentProcessorConfig.vue (1)
26-26: Align class name with CSS and correct spelling.The
class="payment-processer-submit"on<InputSubmit>should match the corrected CSS selector and use “processor” instead of “processer”.Proposed diff:
- <InputSubmit - :formInput="formData.submit" - :label="submitLabel" - :isEnabled="!isFormLoading" - class="payment-processer-submit" - /> + <InputSubmit + :formInput="formData.submit" + :label="submitLabel" + :isEnabled="!isFormLoading" + class="payment-processor-submit" + />webroot/src/styles.common/_inputs.less (1)
32-32: Consider responsive behavior of fixed percentage width.The fixed
flex-basis: 2%for icons might be too small on very narrow screens, potentially making icons difficult to see or interact with. Consider using a minimum width constraint or relative units.- flex-basis: 2%; + flex-basis: max(2%, 1.6rem);webroot/src/models/_formatters/currency.ts (3)
14-14: Simplify regex pattern for currency parsing.The regex
/\.(.*)/suses the 's' flag unnecessarily. For currency input, a simpler pattern would be more appropriate and clearer.- let [ dollars, cents ] = value.toString().split(/\.(.*)/s); + let [ dollars, cents ] = value.toString().split('.');
13-37: Consider edge case handling for currency input.The function doesn't handle negative currency values or extremely large numbers. While this might be intentional for the use case, consider documenting these limitations or adding validation.
Add JSDoc to clarify expected input ranges:
/** * Currency formatter for active input. + * Note: Does not handle negative values or numbers exceeding safe integer limits. * @param {string | number} value The active input value. * @return {string} */
49-51: Clarify optional input autofill behavior.The logic for
isOptionalautofill to '0' when value is empty might be unexpected. Consider documenting this behavior or making it more explicit.if (!value && isOptional) { - // Autofill if optional input is blank + // Autofill optional blank input to '0' for display consistency dollars = '0'; }webroot/src/components/CompactSettingsConfig/CompactSettingsConfig.spec.ts (1)
12-19: Consider expanding test coverage beyond basic mounting.While the basic mounting test is a good start, the AI summary indicates
CompactSettingsConfigis a complex component with form validation, API integration, and confirmation modals. Consider adding tests for:
- Form input validation scenarios
- API call handling (success/error states)
- Confirmation modal behavior
- User permission-based rendering
Would you like me to help generate additional test cases for the key functionality of this component?
webroot/src/pages/StateSettings/StateSettings.spec.ts (1)
12-19: Consider expanding test coverage for page-level functionality.The basic mounting test provides minimal coverage for a page component. Based on the AI summary,
StateSettingsincludes permission checks, routing logic, and Vuex integration. Consider adding tests for:
- Permission-based access control
- Route parameter handling
- State admin role validation
- Component conditional rendering
- Integration with StateSettingsConfig component
Would you like me to help generate test cases for the routing and permission logic mentioned in the AI summary?
webroot/src/components/Forms/InputText/InputText.vue (1)
46-46: Clarify the purpose of the separator span.The empty
<span class="separator"></span>element appears to be for styling purposes. Consider adding a comment to clarify its intent or ensure it's properly styled.Add a comment to clarify the purpose:
+ <!-- Visual separator for consistent spacing --> <span class="separator"></span>webroot/src/pages/StateSettings/StateSettings.less (1)
8-12: Consider adding min-height for viewport consistency.The container styling is well-structured, but it's missing the
min-heightproperty that's present inCompactSettings.less. For consistency across settings pages, consider adding:.state-settings-container { justify-content: center; max-width: 60rem; + min-height: ~"calc(100vh - @{appHeaderHeight} - @{appFooterHeight})"; margin: 0 auto; padding: 1.6rem;webroot/src/components/StateSettingsConfig/StateSettingsConfig.less (1)
111-137: Improve currency field layout for better maintainability.The currency field styling uses magic numbers and complex positioning that could be fragile. Consider using CSS Grid or Flexbox for more predictable layouts.
&.currency { - :deep(label) { - flex-basis: calc(100% - 12rem); - padding-right: 30px; - } - - :deep(input[type=text]) { - align-self: center; - width: 10rem; - height: 4rem; - margin-left: auto; - - & + .separator::before { - position: absolute; - top: 50%; - left: -11.6rem; - width: 30px; - font-weight: @fontWeightBold; - transform: translateY(-50%); - content: '$'; - } - } + display: grid; + grid-template-columns: 1fr auto auto; + gap: 1rem; + align-items: center; + + :deep(label) { + grid-column: 1; + } + + &::before { + content: '$'; + font-weight: @fontWeightBold; + grid-column: 2; + } + + :deep(input[type=text]) { + width: 10rem; + height: 4rem; + grid-column: 3; + }webroot/src/pages/CompactSettings/CompactSettings.vue (1)
13-33: Well-structured state list with proper accessibility.The state list implementation uses proper semantic structure and localization. However, consider adding ARIA labels for better accessibility.
<div v-if="isStateAdminMultiple" class="section state-list"> + <h2 class="sr-only">{{ $t('compact.stateAdministration') }}</h2> <div class="state-row header-row"> <div class="state-cell header-cell state">{{ $t('common.state') }}</div> <div class="state-cell header-cell actions">{{ $t('compact.configuration') }}</div> </div> <div v-for="(statePermission, index) in statePermissionsAdmin" :key="`state-row-${index + 1}`" class="state-row" + role="row" > - <div class="state-cell state">{{ statePermission.state.name() }}</div> - <div class="state-cell actions"> + <div class="state-cell state" role="cell">{{ statePermission.state.name() }}</div> + <div class="state-cell actions" role="cell"> <button class="transparent" + :aria-label="`${$t('common.edit')} ${statePermission.state.name()}`" @click="routeToStateConfig(statePermission.state.abbrev)" > {{ $t('common.edit')}} </button> </div> </div> </div>webroot/src/components/Forms/InputEmailList/InputEmailList.less (1)
14-14: Consider making the padding-right responsive or configurable.The fixed
padding-right: 80pxmight not work well across all screen sizes or if the help icon size changes.Consider using a LESS variable for this spacing:
- padding-right: 80px; + padding-right: @helpIconSpacing; // Define in variableswebroot/src/components/CompactSettingsConfig/CompactSettingsConfig.less (1)
112-137: Consider using CSS custom properties for currency formatting constants.The currency input styling has several hard-coded values (12rem, 30px, -11.6rem) that could be made more maintainable using CSS custom properties or LESS variables.
+ // Currency formatting constants + @currency-input-width: 10rem; + @currency-label-padding: 30px; + @currency-symbol-offset: -11.6rem; + &.currency { :deep(label) { flex-basis: calc(100% - 12rem); - padding-right: 30px; + padding-right: @currency-label-padding; } :deep(input[type=text]) { align-self: center; - width: 10rem; + width: @currency-input-width; height: 4rem; margin-left: auto; & + .separator::before { position: absolute; top: 50%; - left: -11.6rem; + left: @currency-symbol-offset; - width: 30px; + width: @currency-label-padding; font-weight: @fontWeightBold; transform: translateY(-50%); content: '$'; } }webroot/src/components/StateSettingsConfig/StateSettingsConfig.vue (2)
25-27: Consider extracting currency formatting logic into a composable.The currency formatting logic (
formatInputandformatBlur) is called inline in the template. Consider extracting this into a reusable composable for better testability and reuse across other currency input components.Example composable structure:
// useCurrencyFormatter.ts export function useCurrencyFormatter() { const formatInput = (formInput: FormInput) => { // formatting logic }; const formatBlur = (formInput: FormInput, isMilitary: boolean) => { // blur formatting logic }; return { formatInput, formatBlur }; }
37-41: Clarify the purpose of the hidden email list button.The button with class
btn-catch-email-listsappears to be a workaround for some form behavior. Consider adding a comment explaining its purpose or exploring if there's a cleaner solution.+ <!-- Hidden button to prevent form submission when adding emails --> <button class="btn-catch-email-lists" @click.stop.prevent="() => null" tabindex="-1" >+</button>webroot/src/components/Forms/InputEmailList/InputEmailList.ts (2)
35-51: Consider adding type safety for validation.The validation method doesn't handle potential type mismatches for
inputValueor validate the structure offormInput.Consider adding type guards:
validateInputValue(): void { const { formInput, inputValue } = this; + if (typeof inputValue !== 'string' || !formInput) { + return; + } + if (formInput.isTouched) { const emailValue = inputValue.toLowerCase();
57-75: Consider validating email uniqueness more robustly.The current implementation only checks if the email already exists in the array, but doesn't handle case sensitivity consistently throughout the component.
The
validateInputValuemethod converts to lowercase but the uniqueness check inaddalso converts to lowercase, which is good. However, consider normalizing the stored emails consistently:if (!formInput.errorMessage && Array.isArray(formInput.value)) { - if (!formInput.value.includes(emailValue)) { + if (!formInput.value.map(email => email.toLowerCase()).includes(emailValue)) { formInput.value.push(emailValue); }webroot/src/components/CompactSettingsConfig/CompactSettingsConfig.ts (2)
62-62: Consider using a more specific type instead ofany.The
initialCompactConfigproperty uses theanytype, which reduces type safety. Consider using theCompactConfiginterface or a partial version of it.-initialCompactConfig: any = {}; +initialCompactConfig: Partial<CompactConfig> = {};
268-270: Remove unnecessary property existence check.The
hasOwnPropertycheck forlicenseeRegistrationEnabledis unnecessary since this property is always set in the payload object at line 258.-if (Object.prototype.hasOwnProperty.call(payload, 'licenseeRegistrationEnabled')) { - this.isRegistrationEnabledInitialValue = (payload.licenseeRegistrationEnabled as boolean); -} +this.isRegistrationEnabledInitialValue = payload.licenseeRegistrationEnabled;webroot/src/pages/CompactSettings/CompactSettings.ts (1)
126-135: Consider preventing potential multiple redirects.Both watchers call
permissionRedirectCheck()which could trigger multiple navigation attempts ifcurrentCompactanduserchange in quick succession.Consider adding a debounce mechanism or a flag to prevent multiple concurrent redirects:
+private isRedirecting = false; + permissionRedirectCheck(): void { - if (this.currentCompact && this.user) { + if (this.currentCompact && this.user && !this.isRedirecting) { + this.isRedirecting = true; if (!this.isLoggedInAsStaff || (!this.isCompactAdmin && !this.isStateAdminAny)) { // Redirect user to home page this.$router.replace({ name: 'Home' }); } else if (!this.isCompactAdmin && this.isStateAdminExactlyOne) { // Redirect user to state config page const firstStatePermission = this.statePermissionsAdmin[0]; if (firstStatePermission?.state?.abbrev) { this.routeToStateConfig(firstStatePermission.state.abbrev, true); } } + // Reset flag after navigation completes + this.$nextTick(() => { + this.isRedirecting = false; + }); } }webroot/src/components/StateSettingsConfig/StateSettingsConfig.ts (4)
38-41: Consider type safety improvement for RadioOption interface.The
nameproperty allows bothstringandComputedRef<string>, which could lead to type checking issues when the value is used in templates or other contexts expecting a specific type.Consider using a more type-safe approach:
interface RadioOption { value: boolean; - name: string | ComputedRef<string>; + name: ComputedRef<string>; }This ensures consistent typing and better template compatibility.
391-393: Consider potential performance impact of compactType watcher.The watcher calls
init()which performs API calls and re-initializes the entire form. IfcompactTypechanges frequently, this could cause performance issues and unnecessary API calls.Consider adding debouncing or checking if the value actually changed:
@Watch('compactType') -fetchStateConfig() { - this.init(); +async fetchStateConfig(newVal: CompactType | null, oldVal: CompactType | null) { + if (newVal !== oldVal && newVal) { + await this.init(); + } }
396-399: Clean up export statements.There's a commented export statement that suggests uncertainty about the export approach. Choose one export method for consistency.
Remove the commented line:
export default toNative(StateSettingsConfig); -// export default StateSettingsConfig;The
toNativeexport is appropriate for vue-facing-decorator usage.
376-386: Ensure mockPopulate method is not accessible in production.While the component checks
isDevelopmentfor showing the MockPopulate component, themockPopulatemethod itself doesn't have any guards and could be called programmatically in production.Add a development guard to the method:
async mockPopulate(): Promise<void> { + if (!this.$envConfig.isDevelopment) { + return; + } + this.feeInputs.forEach((feeInput) => { this.populateFormInput(feeInput, 5); }); // ... rest of the method }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (44)
webroot/src/components/CompactSettingsConfig/CompactSettingsConfig.less(1 hunks)webroot/src/components/CompactSettingsConfig/CompactSettingsConfig.spec.ts(1 hunks)webroot/src/components/CompactSettingsConfig/CompactSettingsConfig.ts(1 hunks)webroot/src/components/CompactSettingsConfig/CompactSettingsConfig.vue(1 hunks)webroot/src/components/Forms/InputButton/InputButton.ts(1 hunks)webroot/src/components/Forms/InputButton/InputButton.vue(1 hunks)webroot/src/components/Forms/InputEmailList/InputEmailList.less(1 hunks)webroot/src/components/Forms/InputEmailList/InputEmailList.spec.ts(1 hunks)webroot/src/components/Forms/InputEmailList/InputEmailList.ts(1 hunks)webroot/src/components/Forms/InputEmailList/InputEmailList.vue(1 hunks)webroot/src/components/Forms/InputPassword/InputPassword.less(1 hunks)webroot/src/components/Forms/InputRadioGroup/InputRadioGroup.less(3 hunks)webroot/src/components/Forms/InputRadioGroup/InputRadioGroup.vue(2 hunks)webroot/src/components/Forms/InputText/InputText.vue(2 hunks)webroot/src/components/Forms/_mixins/form.mixin.ts(3 hunks)webroot/src/components/Forms/_mixins/mixins.spec.ts(1 hunks)webroot/src/components/Page/PageMainNav/PageMainNav.ts(1 hunks)webroot/src/components/PaymentProcessorConfig/PaymentProcessorConfig.less(1 hunks)webroot/src/components/PaymentProcessorConfig/PaymentProcessorConfig.vue(1 hunks)webroot/src/components/StateSettingsConfig/StateSettingsConfig.less(1 hunks)webroot/src/components/StateSettingsConfig/StateSettingsConfig.spec.ts(1 hunks)webroot/src/components/StateSettingsConfig/StateSettingsConfig.ts(1 hunks)webroot/src/components/StateSettingsConfig/StateSettingsConfig.vue(1 hunks)webroot/src/locales/en.json(6 hunks)webroot/src/locales/es.json(6 hunks)webroot/src/models/Compact/Compact.model.ts(1 hunks)webroot/src/models/FormInput/FormInput.model.spec.ts(3 hunks)webroot/src/models/FormInput/FormInput.model.ts(2 hunks)webroot/src/models/_formatters/currency.ts(1 hunks)webroot/src/models/_formatters/date.ts(1 hunks)webroot/src/models/_formatters/formatters.spec.ts(2 hunks)webroot/src/network/data.api.ts(2 hunks)webroot/src/network/mocks/mock.data.api.ts(3 hunks)webroot/src/network/mocks/mock.data.ts(1 hunks)webroot/src/network/stateApi/data.api.ts(2 hunks)webroot/src/pages/CompactSettings/CompactSettings.less(1 hunks)webroot/src/pages/CompactSettings/CompactSettings.ts(1 hunks)webroot/src/pages/CompactSettings/CompactSettings.vue(1 hunks)webroot/src/pages/StateSettings/StateSettings.less(1 hunks)webroot/src/pages/StateSettings/StateSettings.spec.ts(1 hunks)webroot/src/pages/StateSettings/StateSettings.ts(1 hunks)webroot/src/pages/StateSettings/StateSettings.vue(1 hunks)webroot/src/router/routes.ts(1 hunks)webroot/src/styles.common/_inputs.less(2 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (8)
webroot/src/components/CompactSettingsConfig/CompactSettingsConfig.spec.ts (1)
webroot/tests/helpers/setup.ts (1)
mountShallow(221-221)
webroot/src/components/Forms/InputEmailList/InputEmailList.spec.ts (1)
webroot/tests/helpers/setup.ts (1)
mountShallow(221-221)
webroot/src/pages/StateSettings/StateSettings.spec.ts (1)
webroot/tests/helpers/setup.ts (1)
mountShallow(221-221)
webroot/src/components/StateSettingsConfig/StateSettingsConfig.spec.ts (1)
webroot/tests/helpers/setup.ts (1)
mountShallow(221-221)
webroot/src/models/_formatters/formatters.spec.ts (1)
webroot/src/models/_formatters/currency.ts (2)
formatCurrencyInput(73-73)formatCurrencyBlur(74-74)
webroot/src/network/data.api.ts (2)
webroot/src/models/Compact/Compact.model.ts (3)
PaymentProcessorConfig(22-26)CompactConfig(33-51)CompactStateConfig(53-71)webroot/src/network/stateApi/data.api.ts (1)
stateDataApi(161-161)
webroot/src/network/stateApi/data.api.ts (1)
webroot/src/models/Compact/Compact.model.ts (2)
CompactConfig(33-51)CompactStateConfig(53-71)
webroot/src/pages/CompactSettings/CompactSettings.ts (3)
webroot/src/pages/StateSettings/StateSettings.ts (2)
Component(16-120)statePermission(75-77)webroot/src/models/Compact/Compact.model.ts (2)
Compact(84-120)abbrev(113-119)webroot/src/models/StaffUser/StaffUser.model.ts (2)
CompactPermission(27-33)StatePermission(19-25)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: CheckWebroot
🔇 Additional comments (77)
webroot/src/models/_formatters/date.ts (1)
2-2: Updated header comment to match filename
The file header has been corrected to referencedate.tsinstead of the previous filename. This aligns the comment with the actual file name.webroot/src/components/PaymentProcessorConfig/PaymentProcessorConfig.less (1)
9-11: Full-width form container styling looks good.The
.payment-processor-formrule correctly sets the form’s width to 100%, ensuring it spans its parent.webroot/src/styles.common/_inputs.less (4)
24-24: Good addition for responsive label layout.Adding
flex-wrap: wrapenables labels to wrap appropriately, which supports the new subtext functionality.
36-41: Well-implemented subtext styling.The new
.input-label-subtextclass provides good visual hierarchy with appropriate color, font-weight, and font-size choices. Theflex-basis: 100%ensures it spans the full width when labels wrap.
45-45: Minor spacing refinement approved.Reducing padding from 4px to 2px provides tighter spacing for the required indicator.
66-67: Critical CSS syntax fix.Excellent correction! The previous
:placeholderwas invalid CSS syntax. The proper::placeholderpseudo-element selector is now used, and the added padding provides consistent spacing.webroot/src/models/_formatters/currency.ts (1)
72-75: Well-structured exports.The named exports are clear and follow good TypeScript practices.
webroot/src/components/Forms/InputButton/InputButton.vue (1)
19-19: Good addition of warning state styling.The conditional class binding follows the established pattern and enables warning state styling for the button component.
webroot/src/components/Forms/InputPassword/InputPassword.less (2)
31-31: Good spacing improvement for eye icon.Adding padding-right provides better visual spacing between the toggle icon and the container edge.
36-36: Appropriate icon size adjustment.Reducing the SVG height from 80% to 60% creates better visual balance within the password input field.
webroot/src/components/Forms/InputButton/InputButton.ts (1)
23-23: LGTM! Clean prop addition for warning state support.The
isWarningprop is properly implemented with appropriate typing, default value, and naming convention. This follows the established pattern of other boolean props in the component.webroot/src/components/Forms/_mixins/mixins.spec.ts (1)
77-77:✅ Verification successful
Verify that the method signature change is consistent across the codebase.
The removal of the second argument from
populateFormInputcall aligns with the reported method signature changes. However, ensure all other usages of this method have been updated consistently.
🏁 Script executed:
#!/bin/bash # Description: Search for all usages of populateFormInput to verify consistent signature usage # Expected: All calls should use the new single-argument signature echo "Searching for populateFormInput method calls..." rg -A 2 -B 1 "populateFormInput\(" --type ts --type js --type vue echo -e "\nSearching for populateFormInput method definition..." rg -A 5 -B 2 "populateFormInput.*\(" --type ts --type jsLength of output: 11527
🏁 Script executed:
#!/bin/bash # Display the populateFormInput signature and any default parameter in the mixin rg -n -C 3 "populateFormInput" webroot/src/components/Forms/_mixins/form.mixin.tsLength of output: 381
populateFormInput signature is unchanged; test update is correct
The mixins.spec.ts change removed the explicit
nullargument so that the call relies on theundefined‐value fallback—this is intentional and covered by the first test. All other production calls still pass both parameters, and the method signature remains:populateFormInput(formInput: FormInput, value: any): voidNo inconsistencies detected; no further changes required.
webroot/src/components/Forms/InputText/InputText.vue (1)
21-30:❓ Verification inconclusive
Security concern with v-html directive.
The use of
v-htmlto renderformInput.labelSubtextcould introduce XSS vulnerabilities if the content isn't properly sanitized.Consider using safe HTML rendering or ensure that
labelSubtextcontent is sanitized before being passed to this component:<div v-if="formInput.labelSubtext" - v-html="formInput.labelSubtext" + v-text="formInput.labelSubtext" class="input-label-subtext" > </div>If HTML content is required, ensure the content is sanitized at the data source or use a safe HTML rendering library.
🏁 Script executed:
#!/bin/bash # Check if labelSubtext is sanitized in the FormInput model or validation ast-grep --pattern 'labelSubtext' -A 3 -B 3Length of output: 43
🏁 Script executed:
#!/bin/bash # Search for all occurrences of labelSubtext in the repo rg -n "labelSubtext" # Look for FormInput type or interface definitions rg -n "interface FormInput" rg -n "type FormInput" # Search for potential sanitization routines rg -n "sanitize" rg -n "Sanitizer"Length of output: 8053
Review v-html usage in labelSubtext for XSS risk
Our search did not find any sanitization applied to
FormInput.labelSubtextin the codebase (seewebroot/src/models/FormInput/FormInput.model.ts). Becausev-htmlis used to render this field in several components, you must ensure its content is always trusted:• Components affected:
webroot/src/components/Forms/InputText/InputText.vue(around line 26)webroot/src/components/Forms/InputRadioGroup/InputRadioGroup.vue(around line 25–26)webroot/src/components/Forms/InputEmailList/InputEmailList.vue(around line 26–27)• If
labelSubtextoriginates solely from developer‐managed sources (e.g., static translation files viathis.$t), no further action is needed.
• If you ever inject dynamic or user-provided HTML, integrate a sanitizer (for example, DOMPurify) at the data source or switch tov-textto prevent XSS.webroot/src/components/Page/PageMainNav/PageMainNav.ts (1)
204-204: LGTM! Navigation access properly expanded for state admins.The change from
isCompactAdmintoisAnyTypeOfAdmincorrectly allows both compact and state administrators to access the CompactSettings page, which aligns with the new state configuration features introduced in this PR.webroot/src/pages/CompactSettings/CompactSettings.less (3)
13-13: LGTM: Consistent padding addition.The padding addition follows consistent spacing patterns using rem units.
19-25: LGTM: Well-structured responsive section styling.The section styling follows good practices with responsive design using media queries and consistent margin spacing.
27-56: LGTM: Well-structured state list component styling.The flexbox-based table layout follows semantic CSS practices with:
- Clear hierarchy (
.state-list>.state-row>.state-cell)- Proper use of BEM-like naming conventions
- Consistent spacing and styling patterns
- Appropriate border styling for table rows
- Smart use of
margin-left: autofor right-aligned actionsThe implementation provides good accessibility and maintainability.
webroot/src/models/FormInput/FormInput.model.spec.ts (3)
25-25: LGTM: Proper test coverage for labelSubtext default value.The test correctly verifies the default value for the new
labelSubtextproperty, following the existing test pattern.
87-87: LGTM: Test data includes labelSubtext property.The test data object properly includes the new
labelSubtextproperty with a test value, ensuring comprehensive testing of the property assignment.
128-128: LGTM: Assertion verifies labelSubtext assignment.The assertion correctly verifies that the
labelSubtextproperty is properly assigned during FormInput instantiation.webroot/src/pages/StateSettings/StateSettings.less (1)
14-28: LGTM: Consistent styling patterns.The title, section, and config styling follows the same patterns established in
CompactSettings.less, ensuring UI consistency across settings pages.webroot/src/pages/StateSettings/StateSettings.vue (3)
10-17: LGTM: Well-implemented back button with accessibility.The back button implementation follows Vue best practices with:
- Proper internationalization using
$t()- Good accessibility with
aria-label- Consistent styling properties
- Clear event handling with
@click="goBack"
18-23: LGTM: Clean conditional rendering and component structure.The implementation properly:
- Uses dynamic page title binding
- Implements permission-based rendering with
v-if="isStateAdmin"- Passes required props to the StateSettingsConfig component
- Maintains clean template structure
The conditional rendering ensures only authorized users see the configuration interface.
27-28: LGTM: Proper separation of concerns.Using external files for script and styles with scoped styling follows Vue best practices for maintainability and organization.
webroot/src/components/StateSettingsConfig/StateSettingsConfig.less (1)
1-179: Well-structured stylesheet with comprehensive responsive design.The stylesheet is well-organized with logical grouping of styles and proper use of LESS features. The responsive design implementation using media queries and the accessibility considerations (like
.visually-hidden()) are commendable.webroot/src/components/Forms/InputRadioGroup/InputRadioGroup.less (2)
13-16: Good implementation of disabled state styling.The disabled state styling correctly changes the label color and provides clear visual feedback.
84-91:✅ Verification successful
Verify color contrast compliance for disabled state.
The disabled radio button colors should meet accessibility standards. Please verify that the contrast ratios comply with WCAG guidelines.
🌐 Web query:
What are the WCAG color contrast requirements for disabled form elements?💡 Result:
WCAG Color Contrast Requirements for Disabled Form Elements
Disabled form elements, such as disabled input fields or buttons, are components of web forms that users cannot interact with until they become enabled. The Web Content Accessibility Guidelines (WCAG) address color contrast for form elements, but disabled elements are treated differently from active ones.
WCAG Contrast Requirements Overview
Active Text and Interactive Elements:
- Regular text and interactive component states (focus, hover, active) must have a minimum contrast ratio of 4.5:1 (for normal text) or 3:1 (for user interface (UI) components and graphical objects) against adjacent colors, per WCAG 2.1 and 2.2, Success Criterion 1.4.3 and 1.4.11[2][5][7].
- UI component boundaries (such as input borders) must have a 3:1 contrast ratio with adjacent background colors[2][8].
Disabled Form Elements:
- No Minimum Contrast Requirement: WCAG 2.2 (the latest published version) specifies that "text or images of text that are part of an inactive user interface component have no contrast requirement"[10]. This generally applies to disabled buttons and fields—there is no explicit minimum contrast ratio required for disabled or inactive elements.
- The rationale: Disabled elements are not intended for interaction. Their lack of interactivity should be visually apparent, and it can be beneficial for accessibility that they stand out from active elements. However, they also should not be “completely unreadable” so that users can still identify them as disabled[10].
Practical Considerations
While WCAG allows disabled elements to be lower contrast, it is best practice that the text remains at least somewhat readable so that users can identify the form control as disabled[10]:
"Considering these are disabled buttons, they need to be visually distinct and less readable to be more easily distinguishable from enabled ones. On the other hand, we need to be careful they don’t reach the degree of 'unreadability' where users cannot tell they are buttons at all."[10]
Standards do not require disabled states to meet the 4.5:1 or 3:1 contrast ratios applied to active content[10].
Summary Table
Element Type WCAG Contrast Ratio Requirement Active text (normal size) 4.5:1 Active UI components 3:1 Disabled (inactive) components None (no minimum specified) Best Practice Recommendation
Even though no minimum contrast is required for disabled elements under WCAG, designers should:
- Ensure disabled form elements are still recognizable as controls (e.g., by not making them fully invisible or completely unreadable).
- Use other cues besides color (such as opacity, strikethrough, or icons) to indicate disabled status, especially for users with low vision or color blindness.
Key Points
- Disabled form elements are exempt from WCAG’s minimum color contrast requirements for text and user interface components[10].
- For accessibility, disabled controls should still be perceivable as form controls but clearly distinct from enabled ones.
For maximum clarity and inclusiveness, always supplement color differences with other visual cues to indicate disabled state.
Citations:
- 1: https://webaim.org/articles/contrast/
- 2: https://www.w3.org/WAI/WCAG22/Understanding/non-text-contrast.html
- 3: https://www.w3.org/WAI/WCAG21/Understanding/contrast-minimum
- 4: https://dequeuniversity.com/rules/axe/4.8/color-contrast
- 5: https://www.boia.org/blog/designing-for-color-contrast-guidelines-for-accessibility
- 6: https://reciteme.com/us/news/website-form-accessibility/
- 7: https://accessiblyapp.com/web-accessibility/color-contrast/
- 8: https://accessibleweb.com/question-answer/what-are-the-contrast-requirements-for-an-elements-focus-mouseover-select-states/
- 9: https://www.deque.com/blog/anatomy-of-accessible-forms-required-form-fields/
- 10: https://uxdesign.cc/what-should-be-the-contrast-level-of-inactive-buttons-e618424c1f57
No WCAG contrast requirement for disabled radio buttons
WCAG 2.2 explicitly exempts inactive UI components (including disabled form controls) from any minimum color-contrast ratio. You can remove the request to verify contrast for the disabled radio state. Just ensure the disabled styling remains perceivable (e.g. via opacity or icons) to distinguish it from the enabled state.• Location:
webroot/src/components/Forms/InputRadioGroup/InputRadioGroup.less(lines 84–91)webroot/src/components/Forms/InputRadioGroup/InputRadioGroup.vue (4)
11-14: Clean conditional class binding implementation.The disabled class binding follows Vue best practices and maintains readability.
20-29: Well-structured label with subtext support.The label restructuring maintains semantic meaning while adding subtext functionality. The use of
v-htmlfor subtext provides flexibility for rich content.
51-57: Proper disabled state implementation for radio inputs.The disabled attribute binding and label class application correctly implement the disabled state functionality.
22-22:❓ Verification inconclusive
Verify required indicator logic for edge cases.
The condition
isRequired && !formInput.isDisabledcorrectly hides the required indicator for disabled fields, but please verify this behavior aligns with your UX requirements.
🏁 Script executed:
#!/bin/bash # Search for similar required indicator patterns in other form components rg -A 3 -B 3 "required-indicator" --type vueLength of output: 72
#!/bin/bash # Re-run search for required-indicator across all .vue files using glob pattern rg -A 3 -B 3 "required-indicator" -g "*.vue"
Confirm required-indicator behavior
I didn’t find any other instances of.required-indicatorin the repo, and thev-if="isRequired && !formInput.isDisabled"check will indeed hide the asterisk when the field is disabled. Please double-check that this matches your UX/design requirements for all form controls.webroot/src/pages/CompactSettings/CompactSettings.vue (2)
10-12: Excellent role-based conditional rendering implementation.The conditional rendering based on
isCompactAdminproperly restricts access to sensitive configuration sections.
25-31:✅ Verification successful
Verify routeToStateConfig method implementation.
The button click handler assumes the
routeToStateConfigmethod exists in the component logic. Please ensure this method is properly implemented with error handling.
🏁 Script executed:
#!/bin/bash # Search for the routeToStateConfig method implementation ast-grep --pattern 'routeToStateConfig($$$) { $$$ }'Length of output: 56
🏁 Script executed:
#!/bin/bash # Locate CompactSettings.vue and display its top section to verify method definitions FILE=$(fd "CompactSettings.vue" -e vue | head -n1) echo "Inspecting file: $FILE" sed -n '1,200p' "$FILE" # Search the entire repository for any occurrence of routeToStateConfig echo "Searching for routeToStateConfig references across the repo" rg -n "routeToStateConfig" .Length of output: 2451
🏁 Script executed:
#!/bin/bash # Locate CompactSettings.ts and display lines around the routeToStateConfig method FILE=$(fd "CompactSettings.ts" -e ts | head -n1) echo "Inspecting file: $FILE" sed -n '90,150p' "$FILE"Length of output: 1592
routeToStateConfig method is implemented and wired correctly
I’ve confirmed that
routeToStateConfigis defined inwebroot/src/pages/CompactSettings/CompactSettings.ts(lines 105–110) and correctly constructsrouteConfigbefore calling eitherthis.$router.pushorthis.$router.replace. No missing implementation or obvious error-handling gaps remain.webroot/src/components/Forms/InputEmailList/InputEmailList.less (1)
8-51: LGTM! Well-structured stylesheet with good organization.The LESS stylesheet is well-organized with appropriate nesting and follows consistent naming conventions. The use of LESS variables for colors and border radius promotes maintainability.
webroot/src/models/_formatters/formatters.spec.ts (3)
22-22: Import statement looks correct.The import of currency formatting functions is properly structured.
88-149: Comprehensive test coverage for currency formatters.The test suite provides excellent coverage of various currency formatting scenarios including edge cases. The distinction between input and blur formatting behaviors is well-tested.
124-128: Verify the optional parameter behavior.The test expects
formatCurrencyBlur('', true)to return'0', but this might be confusing behavior. Ensure this aligns with the intended UX.Please verify that returning
'0'for empty optional currency fields is the intended behavior in the UI. This might be unexpected for users if they see a0appear when they blur an empty optional field.webroot/src/router/routes.ts (2)
92-94: Verify the webpack chunk name change rationale.The chunk name was changed from "licensing" to "upload". Ensure this grouping makes sense for code splitting and loading performance.
The webpack chunk name changed from "licensing" to "upload". Please verify:
- Does this grouping improve code splitting efficiency?
- Are there other settings-related components that should be in the same chunk?
- Will this affect initial loading performance?
95-100: New StateSettings route follows good patterns.The new route structure
/:compact/Settings/:stateis logical and follows established patterns. The meta requirements are appropriate for staff-only functionality.webroot/src/components/Forms/_mixins/form.mixin.ts (4)
61-74: Enhanced validation messages improve user experience.The addition of URI, number, and currency validation messages provides comprehensive coverage for form validation scenarios. The structure follows the existing pattern well.
99-99: Boolean validation message enhancement.The addition of
'boolean.base'validation message improves error handling for boolean inputs.
140-146: Improved logic for handling falsy values.The change from checking for falsy values to explicitly checking
value !== undefinedis more precise and correctly handles valid falsy values like0,false, or empty strings.This change ensures that:
0values are properly set instead of being treated as emptyfalseboolean values are handled correctly- Only truly undefined values trigger the empty value path
140-146: Verify file input handling remains correct.Ensure the file input special case handling still works properly with the updated value assignment logic.
Please verify that file inputs continue to work correctly with the updated
populateFormInputlogic, particularly whenvalueis a File object but evaluates to truthy.webroot/src/models/FormInput/FormInput.model.ts (1)
18-18: LGTM! Clean interface extension following existing patterns.The addition of
labelSubtextproperty and expansion of thevaluetype to includeArray<string>are well-implemented extensions that maintain backward compatibility while supporting the new email list functionality.Also applies to: 22-22, 58-58
webroot/src/components/CompactSettingsConfig/CompactSettingsConfig.less (1)
164-166: Good accessibility practice using visually-hidden utility.The visually-hidden class for the email list button is a good accessibility practice for maintaining proper focus behavior.
webroot/src/components/StateSettingsConfig/StateSettingsConfig.vue (1)
63-64:✅ Verification successful
Verify focus trap implementation for accessibility compliance.
The modal uses custom focus trapping with
@keydown.tab="focusTrapConfirmConfigModal($event)". Ensure the implementation properly handles:
- Forward and backward tabbing (Shift+Tab)
- First and last focusable elements
- Initial focus placement when modal opens
🏁 Script executed:
#!/bin/bash # Search for the focus trap implementation to verify proper accessibility handling ast-grep --pattern 'focusTrapConfirmConfigModal($_) { $$$ }'Length of output: 64
To locate the focus-trap logic, let’s search the entire component directory for its definition:
🏁 Script executed:
#!/bin/bash # Find where focusTrapConfirmConfigModal is declared/used rg "focusTrapConfirmConfigModal" -n .Length of output: 591
Let’s inspect the actual implementation to confirm it handles both forward/backward tabbing and focus cycling.
🏁 Script executed:
#!/bin/bash # Show the focusTrapConfirmConfigModal method with surrounding context rg -n -C5 'focusTrapConfirmConfigModal' webroot/src/components/StateSettingsConfig/StateSettingsConfig.tsLength of output: 606
Let’s pull in the full trap function and check the modal open logic:
🏁 Script executed:
#!/bin/bash # Show the full focusTrapConfirmConfigModal implementation rg -n -C10 'focusTrapConfirmConfigModal' webroot/src/components/StateSettingsConfig/StateSettingsConfig.ts # Find where the modal is opened to verify initial focus placement rg -n -C5 'isConfirmConfigModalDisplayed' webroot/src/components/StateSettingsConfig/StateSettingsConfig.tsLength of output: 2073
To verify the full tab‐cycling logic, let’s pull in more of the method body:
🏁 Script executed:
#!/bin/bash # Display the complete focusTrapConfirmConfigModal implementation with extra context rg -n -C20 'focusTrapConfirmConfigModal' webroot/src/components/StateSettingsConfig/StateSettingsConfig.tsLength of output: 1620
Focus-trap implementation is correct and accessible
The
focusTrapConfirmConfigModalmethod in StateSettingsConfig.vue properly:
- Wraps focus from the first (submit) to last (cancel) on Shift + Tab
- Wraps focus from the last (cancel) to first (submit) on Tab
- Sets initial focus to the cancel button when the modal opens
No further changes needed.
webroot/src/components/Forms/InputEmailList/InputEmailList.vue (1)
65-72: Good accessibility implementation for removable email tags.The CloseXIcon implementation properly includes accessibility attributes (
role="button",aria-label,tabindex="0") and keyboard interaction support. Well done!webroot/src/locales/en.json (4)
13-13: LGTM: Good addition of edit action label.The addition of the "edit" action label is consistent with existing common action labels and supports the new configuration UI features.
74-74: LGTM: Important warning message for irreversible actions.The "cannotBeUndone" warning message is crucial for the live status configuration features where actions are irreversible, providing clear user communication about action consequences.
396-427: LGTM: Comprehensive validation error messages for new input types.The additions of URI, number, currency, and numeric range validation messages provide proper localization support for the enhanced form validation in the new configuration components.
470-503: LGTM: Comprehensive compact configuration localization.The extensive additions to the compact section provide thorough localization support for:
- Configuration loading and error handling
- Privilege fees management
- Jurisprudence requirements
- Notification email management
- Live status toggles with appropriate warnings
- Confirmation dialogs and success messaging
All keys follow consistent naming conventions and provide clear, user-friendly text.
webroot/src/components/Forms/InputEmailList/InputEmailList.ts (1)
8-18: LGTM: Proper component structure and imports.The component imports and decorator setup follow Vue 3 composition patterns correctly with appropriate mixin extension.
webroot/src/components/CompactSettingsConfig/CompactSettingsConfig.vue (4)
9-14: LGTM: Proper conditional rendering for different states.The conditional rendering structure properly handles loading, error, and normal states with appropriate user feedback.
37-41: Clarify the purpose of the catch button.The button with class "btn-catch-email-lists" appears to be a workaround or hidden element, but its purpose isn't clear from the template.
Could you clarify the purpose of this button? If it's meant to catch focus or handle email list interactions, consider adding a comment or using a more semantic approach.
58-91: LGTM: Well-implemented accessible modal.The modal implementation includes excellent accessibility features:
- Focus trapping with tab key handling
- Escape key support for closing
- Proper ARIA structure with title and content slots
- Clear action buttons with appropriate styling
The modal follows accessibility best practices for keyboard navigation and user interaction.
20-31: Good currency formatting integration.The currency input fields properly integrate formatting on both input and blur events, with the blur event including a parameter (true) for the second field which likely indicates different formatting behavior.
webroot/src/locales/es.json (4)
15-15: LGTM: Proper Spanish translation for edit action.The translation "Editar" for "edit" is accurate and consistent with Spanish UI conventions.
74-74: LGTM: Clear warning message translation.The translation "Esta acción no se puede deshacer" clearly conveys the irreversible nature of actions, maintaining the important warning context.
395-426: LGTM: Comprehensive validation error translations.The Spanish translations for validation errors are accurate and maintain technical precision while being user-friendly. The parameterized messages with {min}, {max} placeholders are properly preserved.
469-502: LGTM: Thorough compact configuration translations.The Spanish translations comprehensively cover all compact configuration features with accurate technical terminology. The translations maintain context and clarity for complex concepts like jurisprudence requirements and irreversible live status changes.
webroot/src/components/CompactSettingsConfig/CompactSettingsConfig.ts (2)
187-187: Review the logic for disabling the registration toggle.The
isDisabledcomputed property disables the toggle whenisRegistrationEnabledInitialValueis true, but this seems counterintuitive. Based on the comment at line 223, the confirmation modal is shown when enabling registration if it was previously disabled, suggesting users should be able to enable registration but perhaps not disable it once enabled.Could you clarify the intended behavior? Should the toggle be disabled when registration is already enabled (preventing users from disabling it), or should it be disabled when registration is disabled (preventing users from enabling it)?
306-321: Excellent accessibility implementation for focus trapping.The
focusTrapConfirmConfigModalmethod properly implements keyboard navigation within the modal, ensuring users can't tab outside the modal content. This follows accessibility best practices for modal dialogs.webroot/src/network/mocks/mock.data.ts (2)
1772-1796: Well-structured mock data for compact configuration.The
compactConfigmock data provides comprehensive test data that aligns with theCompactConfiginterface, including fee structures, email arrays, and transaction configurations. The data values are realistic and suitable for development and testing.
1798-1828: Comprehensive mock data for state configuration.The
stateConfigmock data includes all required fields from theCompactStateConfiginterface, with realistic values for privilege fees, jurisprudence requirements, and notification emails. The mix of null and non-null values formilitaryRateproperly tests edge cases.webroot/src/pages/StateSettings/StateSettings.ts (3)
67-77: Robust permission checking logic.The
staffPermissionandstatePermissioncomputed properties correctly navigate the permission hierarchy to determine if the user has admin rights for the specific state and compact. The null checks and optional chaining prevent runtime errors.
97-104: Proper security implementation with permission-based redirects.The
permissionRedirectCheckmethod correctly enforces access control by redirecting unauthorized users to the home page. The checks for both staff authentication and state admin privileges ensure proper authorization.
113-119: Reactive permission checks ensure security.The watchers on
currentCompactanduserchanges ensure that permission checks are re-evaluated when the user context changes, preventing unauthorized access even if the user's permissions or compact selection changes after initial page load.webroot/src/network/data.api.ts (3)
12-12: Proper import of new model types.The import statement correctly includes the new
CompactConfigandCompactStateConfiginterfaces alongside the existingPaymentProcessorConfig, maintaining consistency with the existing import pattern.
59-76: Well-documented API methods for compact configuration.The
getCompactConfigandupdateCompactConfigmethods follow the established pattern in the codebase, with comprehensive JSDoc documentation and proper delegation to thestateDataApi. The method signatures correctly use theCompactConfigtype for type safety.
78-97: Consistent API methods for state configuration management.The
getCompactStateConfigandupdateCompactStateConfigmethods maintain consistency with other API methods in the class. The documentation clearly specifies parameter requirements and return types, and the methods properly handle both compact and state parameters.webroot/src/models/Compact/Compact.model.ts (2)
28-31: LGTM!The
FeeTypeenum is well-defined with clear, descriptive values following TypeScript naming conventions.
53-71: LGTM!The
CompactStateConfiginterface is well-structured with appropriate nullable types. The comment clarifying the server requirement formilitaryRateis helpful for maintainability.webroot/src/network/mocks/mock.data.api.ts (2)
24-28: LGTM!The import statement correctly includes the new mock data constants needed for the compact and state configuration methods.
86-120: LGTM!The new mock API methods follow the established patterns in the file, with consistent async simulation and appropriate return values for both GET and PUT operations.
webroot/src/network/stateApi/data.api.ts (1)
16-16: LGTM!The import correctly includes the new configuration interfaces needed for type safety in the API methods.
webroot/src/pages/CompactSettings/CompactSettings.ts (2)
8-22: LGTM!The imports and component registration are properly structured with all necessary dependencies included.
25-89: LGTM!The lifecycle hook and computed properties are well-structured with clear, hierarchical permission checks that progress from basic authentication to specific admin roles.
webroot/src/components/StateSettingsConfig/StateSettingsConfig.ts
Outdated
Show resolved
Hide resolved
webroot/src/components/StateSettingsConfig/StateSettingsConfig.ts
Outdated
Show resolved
Hide resolved
webroot/src/components/StateSettingsConfig/StateSettingsConfig.ts
Outdated
Show resolved
Hide resolved
webroot/src/components/StateSettingsConfig/StateSettingsConfig.ts
Outdated
Show resolved
Hide resolved
webroot/src/components/StateSettingsConfig/StateSettingsConfig.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
webroot/src/components/StateSettingsConfig/StateSettingsConfig.ts (1)
115-134:⚠️ Potential issueFix error handling to prevent undefined behavior.
This is the same critical error handling issue from previous reviews. When the API call fails,
stateConfigbecomes undefined, but the method continues executing and tries to process the undefined value.Apply this fix to properly handle errors:
async getStateConfig(): Promise<void> { const compact = this.compactType || ''; const licenseTypes = (this.$tm('licensing.licenseTypes') || []).filter((licenseType) => licenseType.compactKey === compact); - const stateConfig: any = await dataApi.getCompactStateConfig(compact, this.stateAbbrev).catch((err) => { - this.loadingErrorMessage = err?.message || this.$t('serverErrors.networkError'); - }); - - if (!this.loadingErrorMessage) { + + try { + const stateConfig: CompactStateConfig = await dataApi.getCompactStateConfig(compact, this.stateAbbrev); + stateConfig?.privilegeFees?.forEach((privilegeFee) => { const licenseType = licenseTypes.find((type) => type.abbrev === privilegeFee.licenseTypeAbbreviation); privilegeFee.name = licenseType?.name || privilegeFee.licenseTypeAbbreviation?.toUpperCase() || ''; }); this.initialStateConfig = stateConfig; this.isPurchaseEnabledInitialValue = this.initialStateConfig?.licenseeRegistrationEnabled || false; + } catch (err: any) { + this.loadingErrorMessage = err?.message || this.$t('serverErrors.networkError'); } this.isLoading = false; }webroot/src/pages/CompactSettings/CompactSettings.ts (1)
102-112: Array access is now safer but consider more explicit defensive programming.The optional chaining
?.was added to address the previous review concern, which improves safety. However, the past review comment suggested a more explicit approach for better robustness.While the optional chaining helps, consider implementing the more explicit defensive check suggested in the past review:
} else if (!this.isCompactAdmin && this.isStateAdminExactlyOne) { // Redirect user to state config page - this.routeToStateConfig(this.statePermissionsAdmin[0]?.state?.abbrev || '', true); + const firstStatePermission = this.statePermissionsAdmin[0]; + if (firstStatePermission?.state?.abbrev) { + this.routeToStateConfig(firstStatePermission.state.abbrev, true); + } else { + // Fallback: redirect to home if state data is invalid + this.$router.replace({ name: 'Home' }); + } }
🧹 Nitpick comments (3)
webroot/src/components/StateSettingsConfig/StateSettingsConfig.ts (1)
294-304: Improve type safety in fee mapping logic.The type assertions and object finding logic could be made more type-safe and cleaner.
Consider this improvement:
privilegeFees: feeInputsCore.map((feeInputCore) => { // Map indeterminate set of privilege fee inputs to their payload structure const [ licenseType ] = feeInputCore.id.split('-'); - const militaryInput = Object.values(this.formData).find((formInput) => - (formInput as unknown as FormInput).id === `${feeInputCore.id}-military`) as FormInput | undefined; + const militaryInputKey = `${licenseType}Military`; + const militaryInput = this.formData[militaryInputKey] as FormInput | undefined; return { licenseTypeAbbreviation: licenseType, amount: Number(feeInputCore.value), militaryRate: Number(militaryInput?.value) || null, }; }),webroot/src/pages/CompactSettings/CompactSettings.ts (2)
25-30: Consider adding error handling to the initialization.While the created lifecycle hook properly calls init(), consider adding error handling in case the permission check fails unexpectedly.
created(): void { - this.init(); + try { + this.init(); + } catch (error) { + console.error('Failed to initialize CompactSettings:', error); + this.$router.replace({ name: 'Home' }); + } }
133-139: Consider debouncing the watchers to prevent excessive permission checks.The watchers immediately trigger permission checks which could cause performance issues if the watched properties change frequently.
@Watch('currentCompact') currentCompactUpdate() { - this.permissionRedirectCheck(); + this.$nextTick(() => { + this.permissionRedirectCheck(); + }); } @Watch('user') userUpdate() { - this.permissionRedirectCheck(); + this.$nextTick(() => { + this.permissionRedirectCheck(); + }); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
webroot/src/components/Forms/InputEmailList/InputEmailList.vue(1 hunks)webroot/src/components/StateSettingsConfig/StateSettingsConfig.ts(1 hunks)webroot/src/models/Compact/Compact.model.ts(1 hunks)webroot/src/network/stateApi/data.api.ts(2 hunks)webroot/src/pages/CompactSettings/CompactSettings.ts(1 hunks)webroot/src/pages/CompactSettings/CompactSettings.vue(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- webroot/src/pages/CompactSettings/CompactSettings.vue
- webroot/src/components/Forms/InputEmailList/InputEmailList.vue
- webroot/src/models/Compact/Compact.model.ts
- webroot/src/network/stateApi/data.api.ts
🧰 Additional context used
🧠 Learnings (1)
webroot/src/components/StateSettingsConfig/StateSettingsConfig.ts (1)
Learnt from: jsandoval81
PR: csg-org/CompactConnect#822
File: webroot/src/components/StateSettingsConfig/StateSettingsConfig.ts:273-321
Timestamp: 2025-05-28T16:28:48.862Z
Learning: The development team has established length & complexity bounds for different types of methods, including submit-handler methods. Methods that fall within these bounds are considered acceptable even if they handle multiple responsibilities. Refactoring is considered when methods grow beyond these established bounds in the future.
🔇 Additional comments (7)
webroot/src/components/StateSettingsConfig/StateSettingsConfig.ts (4)
136-208: Form initialization method is well-structured.The
initFormInputsmethod effectively sets up the reactive form inputs with proper validation and computed labels. The separation of privilege fee initialization into a dedicated method (initPrivilegeFeeInputs) is a good design choice.
282-330: Submit handler method is within complexity bounds.Based on the team's established complexity bounds for submit-handler methods, this implementation is acceptable. The method handles data transformation, API calls, and success handling in a cohesive manner.
246-263: Currency formatting methods are well-implemented.The currency formatting logic properly handles input formatting and blur events with validation. The separation of concerns between input formatting and blur formatting is appropriate.
351-383: Modal handling provides excellent accessibility support.The focus management and keyboard trap implementation for the confirmation modal demonstrates good accessibility practices. The proper focus restoration after modal close is particularly well done.
webroot/src/pages/CompactSettings/CompactSettings.ts (3)
8-14: LGTM! Clean import organization.The imports are well-organized and include the necessary types and components for the enhanced permission-based functionality.
16-23: Component registration looks good.The CompactSettingsConfig component is properly registered and follows Vue component patterns.
63-69: 🛠️ Refactor suggestionAdd null safety for user permissions access.
The computed property accesses nested properties without sufficient null checking, which could cause runtime errors.
get staffPermission(): CompactPermission | null { - const currentPermissions = this.user?.permissions; + const currentPermissions = this.user?.permissions; + if (!currentPermissions || !this.currentCompact?.type) { + return null; + } const compactPermission = currentPermissions?.find((currentPermission) => currentPermission.compact.type === this.currentCompact?.type) || null; return compactPermission; }Likely an incorrect or invalid review comment.
|
Nice work! I have a few nitpicks but other than that this is looking great |
|
For the military fees we replace the value with a zero but ultimately send up null if this value wasn't filled in. When we refresh the page, its empty again. Ideally, what you see after sending the form matches what the form looks like when you refresh the page and populate the form with that same data. What does being empty signify?
Depending on the answer there I would suggest different fixes to the behavior. |
|
The atomized emails can overflow the screen, mostly only even a potential issue on mobile, we should maybe do an ellipsis for very long ones? |
If the military fee is never meant to be 0 (free), then the existing implementation may work. If OTTH the military fee can be free then there might be some additional work to un-conflate the number+null type here. We can check with the server devs offline about this. Side note: this is why dealing with types that are number+null is tricky - and we avoid where possible 🙂 |
|
@ChiefStief I just added a commit with 2 small updates:
|
|
@jlkravitz This is ready for your review. |
webroot/src/components/StateSettingsConfig/StateSettingsConfig.ts
Outdated
Show resolved
Hide resolved
|
@jlkravitz This is ready for your re-review. |
Requirements List
Description List
Testing List
yarn test:unit:allshould run without errors or warningsyarn serveshould run without errors or warningsyarn buildshould run without errors or warningsCloses #782
Summary by CodeRabbit
New Features
Improvements
Bug Fixes
Tests
Style