feat: add extra confirmation when deleting domain or importing a backup #1391
Conversation
WalkthroughThe pull request introduces updates to the frontend localization files and modal confirmation system. New localization keys are added to the English and French JSON files for error messages related to backup versions and user confirmations. A new Changes
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
⏰ Context from checks skipped due to timeout of 90000ms (8)
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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: 0
🧹 Nitpick comments (2)
backend/app_tests/api/test_api_user_groups.py (1)
19-21: Simplify the dictionary key lookup.While the formatting improvement looks good, we can further optimize the code by following Python's best practices.
Apply this diff to simplify the dictionary key lookup:
- assert ( - perm in user_permissions.keys() - ), f"Permission {perm} not found in user permissions (group: {test.user_group})" + assert perm in user_permissions, f"Permission {perm} not found in user permissions (group: {test.user_group})"This change:
- Uses the more idiomatic
key in dictinstead ofkey in dict.keys()- Maintains the same functionality while being more efficient
- Aligns with the formatting improvements in other files
🧰 Tools
🪛 Ruff (0.8.2)
20-20: Use
key in dictinstead ofkey in dict.keys()Remove
.keys()(SIM118)
🪛 GitHub Actions: Backend Linters
[warning] File requires formatting using ruff format
frontend/src/lib/components/Modals/PromptConfirmModal.svelte (1)
64-85: Consider validating the form action before submission.While the form handling is good, consider adding validation for the formAction prop to prevent potential issues with invalid form submissions.
export let formAction: string = ''; +function validateFormAction(action: string): boolean { + return action.length > 0 && action.startsWith('/'); +}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
backend/app_tests/api/test_api_libraries.py(3 hunks)backend/app_tests/api/test_api_user_groups.py(1 hunks)backend/app_tests/api/test_api_users.py(2 hunks)backend/app_tests/api/test_utils.py(21 hunks)frontend/messages/en.json(1 hunks)frontend/messages/fr.json(1 hunks)frontend/src/lib/components/Modals/PromptConfirmModal.svelte(1 hunks)frontend/src/routes/(app)/(internal)/backup-restore/+page.server.ts(2 hunks)frontend/src/routes/(app)/(internal)/backup-restore/+page.svelte(2 hunks)
✅ Files skipped from review due to trivial changes (3)
- backend/app_tests/api/test_api_users.py
- backend/app_tests/api/test_api_libraries.py
- frontend/messages/fr.json
🧰 Additional context used
🪛 GitHub Actions: Backend Linters
backend/app_tests/api/test_utils.py
[warning] File requires formatting using ruff format
backend/app_tests/api/test_api_user_groups.py
[warning] File requires formatting using ruff format
🪛 Ruff (0.8.2)
backend/app_tests/api/test_api_user_groups.py
20-20: Use key in dict instead of key in dict.keys()
Remove .keys()
(SIM118)
⏰ Context from checks skipped due to timeout of 90000ms (11)
- GitHub Check: enterprise-startup-docker-compose-test
- GitHub Check: startup-functional-test (3.12)
- GitHub Check: functional-tests (3.12, chromium)
- GitHub Check: startup-docker-compose-test
- GitHub Check: enterprise-functional-tests (3.12, chromium)
- GitHub Check: enterprise-startup-functional-test (3.12)
- GitHub Check: build (3.12)
- GitHub Check: migrations-check (3.12)
- GitHub Check: test (3.12)
- GitHub Check: Analyze (python)
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (12)
backend/app_tests/api/test_utils.py (3)
141-143: LGTM! The assertion formatting looks good.The reformatting improves readability while maintaining the same functionality.
🧰 Tools
🪛 GitHub Actions: Backend Linters
[warning] File requires formatting using ruff format
173-175: Similar formatting improvement as before.🧰 Tools
🪛 GitHub Actions: Backend Linters
[warning] File requires formatting using ruff format
Line range hint
141-1061: All assertion formatting changes look good.The changes consistently improve readability by:
- Removing unnecessary parentheses around assertion conditions
- Moving error messages to the same line
- Maintaining the same logical checks
These changes align with Python's style guidelines and make the code more maintainable.
🧰 Tools
🪛 GitHub Actions: Backend Linters
[warning] File requires formatting using ruff format
frontend/src/routes/(app)/(internal)/backup-restore/+page.server.ts (3)
Line range hint
16-21: LGTM! Improved null safety in file validation.The use of optional chaining (
?.) provides better null safety when checking the file name.
35-49: Well-structured error handling for backup version compatibility!The switch statement provides clear differentiation between various backup version scenarios:
- Invalid version
- Greater version
- Lower version
- Default case for other errors
Each case sets an appropriate localized error message.
Line range hint
51-55: LGTM! Clean response structure.The response properly includes both the status code and stringified body.
frontend/src/routes/(app)/(internal)/backup-restore/+page.svelte (2)
9-35: Well-implemented modal confirmation flow!The modal configuration:
- Uses proper typing for modal store and components
- Implements a clean confirmation flow
- Correctly handles form submission on confirmation
73-73: LGTM! Proper event binding.The button's click event is correctly bound to the new modalConfirm function.
frontend/src/lib/components/Modals/PromptConfirmModal.svelte (3)
1-41: Well-structured component with proper TypeScript usage!The component:
- Has well-defined and typed props with sensible defaults
- Uses proper type imports
- Implements proper store typing
51-57: LGTM! Clear and user-friendly input handling.The confirmation input:
- Has clear placeholder text
- Uses proper two-way binding
- Implements consistent styling
72-79: LGTM! Robust button state handling.The submit buttons:
- Have proper disabled state logic
- Implement case-insensitive confirmation check
- Use consistent styling
Also applies to: 91-98
frontend/messages/en.json (1)
779-780: LGTM! Clear and user-friendly localization messages.The new entries:
- Provide clear error messages for backup version issues
- Have a clear confirmation prompt
- Maintain consistent message style
ab-smith
changed the title
Mohamed-Hacene
changed the title
…s action.ts delete hidden input on forms
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (8)
frontend/src/lib/components/Modals/PromptConfirmModal.svelte (4)
6-6: Consider typing the parent prop.The
parentprop is typed asany, which bypasses TypeScript's type checking. Consider creating an interface to define the expected shape of the parent component.-export let parent: any; +interface ModalParent { + onClose: () => void; + onConfirm: () => void; + buttonNeutral: string; + regionFooter: string; +} +export let parent: ModalParent;
42-42: Enhance user input validation.The current implementation only checks if the input matches "yes" (case-insensitive). Consider:
- Trimming whitespace during binding
- Adding aria-label for accessibility
- Using a form element for the input to handle Enter key submission
-let userInput = ''; +let userInput = ''; +$: normalizedInput = userInput.trim().toLowerCase(); -<input +<input + aria-label={m.confirmYesPlaceHolder()} type="text" - bind:value={userInput} + bind:value={userInput} placeholder={m.confirmYesPlaceHolder()} class="w-full mt-2 p-2 border border-gray-300 rounded" + on:keydown={(e) => { + if (e.key === 'Enter' && normalizedInput === m.yes()) { + e.preventDefault(); + parent.onConfirm(); + } + }} />Also applies to: 50-56
74-75: Avoid duplicate validation logic.The validation logic for the submit button is duplicated between form and non-form modes.
Extract the validation into a reactive statement:
+$: isConfirmEnabled = userInput && userInput.trim().toLowerCase() === m.yes(); -disabled={!userInput || userInput.trim().toLowerCase() !== m.yes()} +disabled={!isConfirmEnabled}Also applies to: 93-94
82-84: Enhance debug mode implementation.The debug mode could be more comprehensive by including the userInput state and validation results.
{#if debug === true} + <div class="space-y-2"> <SuperDebug data={$form} /> + <pre>User Input: {JSON.stringify({ + raw: userInput, + normalized: userInput.trim().toLowerCase(), + isValid: userInput.trim().toLowerCase() === m.yes() + }, null, 2)}</pre> + </div> {/if}frontend/src/lib/components/TableRowActions/TableRowActions.svelte (2)
64-94: Reduce code duplication in modal confirmation functions.The
promptModalConfirmDeletefunction largely duplicatesmodalConfirmDelete. Consider extracting common logic.+function getModalConfig(id: string, row: { [key: string]: string | number | boolean | null }) { + const name = + URLModel === 'users' && row.first_name + ? `${row.first_name} ${row.last_name} (${row.email})` + : (row.name ?? Object.values(row)[0]); + const body = + URLModel === 'users' + ? m.deleteUserMessage({ name: name }) + : m.deleteModalMessage({ name: name }); + return { name, body }; +} function promptModalConfirmDelete( id: string, row: { [key: string]: string | number | boolean | null } ): void { + const { body } = getModalConfig(id, row); const modalComponent: ModalComponent = { ref: PromptConfirmModal, props: { _form: deleteForm, id: id, debug: false, URLModel: URLModel, formAction: '?/delete' } }; - const name = - URLModel === 'users' && row.first_name - ? `${row.first_name} ${row.last_name} (${row.email})` - : (row.name ?? Object.values(row)[0]); - const body = - URLModel === 'users' - ? m.deleteUserMessage({ name: name }) - : m.deleteModalMessage({ name: name }); const modal: ModalSettings = { type: 'component', component: modalComponent, title: m.deleteModalTitle(), body: body }; modalStore.trigger(modal); }
134-154: Enhance keyboard accessibility in delete buttons.The keydown handlers don't specify which key should trigger the action.
-on:keydown={() => promptModalConfirmDelete(row.meta.id, row)} +on:keydown={(e) => { + if (e.key === 'Enter' || e.key === ' ') { + e.preventDefault(); + promptModalConfirmDelete(row.meta.id, row); + } +}} -on:keydown={() => modalConfirmDelete(row.meta.id, row)} +on:keydown={(e) => { + if (e.key === 'Enter' || e.key === ' ') { + e.preventDefault(); + modalConfirmDelete(row.meta.id, row); + } +}}frontend/src/routes/(app)/(internal)/[model=urlmodel]/[id=uuid]/+page.server.ts (1)
80-80: Remove debugging console.log statement.The console.log statement appears to be debugging code and should be removed before merging to production.
-console.log('delete');frontend/src/lib/utils/actions.ts (1)
219-242: Consider adding request timeout handling.The delete request could benefit from a timeout to handle cases where the server is unresponsive.
const requestInitOptions: RequestInit = { method: 'DELETE', + signal: AbortSignal.timeout(5000) // 5 second timeout }; const res = await event.fetch(endpoint, requestInitOptions); +} catch (error) { + if (error.name === 'AbortError') { + setFlash({ type: 'error', message: m.requestTimeout() }, event); + return fail(408, { form: deleteForm }); + } + throw error; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
frontend/messages/en.json(1 hunks)frontend/messages/fr.json(1 hunks)frontend/src/lib/components/Modals/DeleteConfirmModal.svelte(0 hunks)frontend/src/lib/components/Modals/PromptConfirmModal.svelte(1 hunks)frontend/src/lib/components/TableRowActions/TableRowActions.svelte(3 hunks)frontend/src/lib/utils/actions.ts(1 hunks)frontend/src/routes/(app)/(internal)/[model=urlmodel]/[id=uuid]/+page.server.ts(1 hunks)
💤 Files with no reviewable changes (1)
- frontend/src/lib/components/Modals/DeleteConfirmModal.svelte
🚧 Files skipped from review as they are similar to previous changes (2)
- frontend/messages/fr.json
- frontend/messages/en.json
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: functional-tests (3.12, chromium)
- GitHub Check: enterprise-functional-tests (3.12, chromium)
…omain-import-a-backup
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
frontend/messages/en.json(1 hunks)frontend/messages/fr.json(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- frontend/messages/fr.json
🧰 Additional context used
🪛 Biome (1.9.4)
frontend/messages/en.json
[error] 782-782: The key yes was already declared.
This where a duplicated key was declared again.
If a key is defined multiple times, only the last definition takes effect. Previous definitions are ignored.
(lint/suspicious/noDuplicateObjectKeys)
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: enterprise-startup-functional-test (3.12)
- GitHub Check: enterprise-startup-docker-compose-test
- GitHub Check: startup-functional-test (3.12)
- GitHub Check: startup-docker-compose-test
- GitHub Check: functional-tests (3.12, chromium)
- GitHub Check: enterprise-functional-tests (3.12, chromium)
🔇 Additional comments (1)
frontend/messages/en.json (1)
779-781: LGTM! New localization keys for backup version error and confirmation prompts.The new keys align well with the PR objectives of adding extra confirmation for critical actions.
frontend/messages/en.json
Outdated
| "backupVersionError": "Can't load the backup, the version of the backup is invalid", | ||
| "confirmYes": "Confirm by typing 'yes' below:", | ||
| "confirmYesPlaceHolder": "Type 'yes' to confirm", | ||
| "yes": "yes", |
There was a problem hiding this comment.
Fix duplicate key "yes".
The key "yes" is defined twice in the file. The second occurrence at line 782 will override the first one.
Remove the duplicate key at line 782 since it's already defined later in the file:
- "yes": "yes",🧰 Tools
🪛 Biome (1.9.4)
[error] 782-782: The key yes was already declared.
This where a duplicated key was declared again.
If a key is defined multiple times, only the last definition takes effect. Previous definitions are ignored.
(lint/suspicious/noDuplicateObjectKeys)
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
frontend/src/lib/components/Modals/PromptConfirmModal.svelte (2)
75-75: Refactor duplicated confirmation logic.The confirmation logic is duplicated between form and non-form sections. Consider extracting it into a reactive statement.
+$: isConfirmed = userInput && userInput.trim().toLowerCase() === m.yes().toLowerCase(); -disabled={!userInput || userInput.trim().toLowerCase() !== m.yes().toLowerCase()} +disabled={!isConfirmed}Also applies to: 94-94
51-56: Enhance input field accessibility.The confirmation input field should have an associated label and proper ARIA attributes.
+<label for="confirm-input" class="text-red-500 font-bold">{m.confirmYes()}</label> <input + id="confirm-input" type="text" bind:value={userInput} placeholder={m.confirmYesPlaceHolder()} class="w-full mt-2 p-2 border border-gray-300 rounded" + aria-required="true" + aria-invalid={userInput && userInput.trim().toLowerCase() !== m.yes().toLowerCase()} />
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
frontend/messages/en.json(1 hunks)frontend/src/lib/components/Modals/PromptConfirmModal.svelte(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- frontend/messages/en.json
⏰ Context from checks skipped due to timeout of 90000ms (8)
- GitHub Check: startup-functional-test (3.12)
- GitHub Check: startup-docker-compose-test
- GitHub Check: enterprise-startup-functional-test (3.12)
- GitHub Check: functional-tests (3.12, chromium)
- GitHub Check: enterprise-functional-tests (3.12, chromium)
- GitHub Check: enterprise-startup-docker-compose-test
- GitHub Check: Analyze (python)
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (1)
frontend/src/lib/components/Modals/PromptConfirmModal.svelte (1)
28-28: Consider browser compatibility for crypto.randomUUID().The
crypto.randomUUID()method might not be supported in all browsers. Consider using a fallback mechanism.
| {#if debug === true} | ||
| <SuperDebug data={$form} /> | ||
| {/if} |
There was a problem hiding this comment.
Security: Restrict debug mode in production.
The debug mode exposes form data which could leak sensitive information. Ensure this is disabled in production.
-{#if debug === true}
+{#if debug === true && import.meta.env.DEV}
<SuperDebug data={$form} />
{/if}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| {#if debug === true} | |
| <SuperDebug data={$form} /> | |
| {/if} | |
| {#if debug === true && import.meta.env.DEV} | |
| <SuperDebug data={$form} /> | |
| {/if} |
|
|
||
| // Props | ||
| /** Exposes parent props to this component. */ | ||
| export let parent: any; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid using any type for better type safety.
The parent prop is typed as any, which bypasses TypeScript's type checking. Consider creating a proper interface for the parent component's props.
-export let parent: any;
+interface ParentProps {
+ onClose: () => void;
+ onConfirm: () => void;
+ buttonNeutral: string;
+ regionFooter: string;
+}
+export let parent: ParentProps;Committable suggestion skipped: line range outside the PR's diff.
| {#if $modalStore[0]} | ||
| <div class="modal-example-form {cBase}"> | ||
| <header class={cHeader}>{$modalStore[0].title ?? '(title missing)'}</header> | ||
| <article>{$modalStore[0].body ?? '(body missing)'}</article> | ||
|
|
||
| <p class="text-red-500 font-bold">{m.confirmYes()}</p> | ||
| <input | ||
| type="text" | ||
| bind:value={userInput} | ||
| placeholder={m.confirmYesPlaceHolder()} | ||
| class="w-full mt-2 p-2 border border-gray-300 rounded" | ||
| /> | ||
|
|
||
| {#if bodyComponent} | ||
| <div class="max-h-96 overflow-y-scroll scroll card"> | ||
| <svelte:component this={bodyComponent} {...bodyProps} /> | ||
| </div> | ||
| {/if} | ||
| {#if _form && Object.keys(_form).length > 0} | ||
| <form method="POST" action={formAction} use:enhance class="modal-form {cForm}"> | ||
| <footer class="modal-footer {parent.regionFooter}"> | ||
| <button type="button" class="btn {parent.buttonNeutral}" on:click={parent.onClose} | ||
| >{m.cancel()}</button | ||
| > | ||
| <input type="hidden" name="urlmodel" value={URLModel} /> | ||
| <input type="hidden" name="id" value={id} /> | ||
| <button | ||
| class="btn variant-filled-error" | ||
| type="submit" | ||
| on:click={parent.onConfirm} | ||
| disabled={!userInput || userInput.trim().toLowerCase() !== m.yes().toLowerCase()} | ||
| > | ||
| {m.submit()} | ||
| </button> | ||
| </footer> | ||
| </form> | ||
|
|
||
| {#if debug === true} | ||
| <SuperDebug data={$form} /> | ||
| {/if} | ||
| {:else} | ||
| <footer class="modal-footer {parent.regionFooter}"> | ||
| <button type="button" class="btn {parent.buttonNeutral}" on:click={parent.onClose} | ||
| >{m.cancel()}</button | ||
| > | ||
| <button | ||
| class="btn variant-filled-error" | ||
| type="button" | ||
| on:click={parent.onConfirm} | ||
| disabled={!userInput || userInput.trim().toLowerCase() !== m.yes().toLowerCase()} | ||
| > | ||
| {m.submit()} | ||
| </button> | ||
| </footer> | ||
| {/if} | ||
| </div> | ||
| {/if} |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add ARIA attributes for better accessibility.
The modal lacks proper ARIA attributes for accessibility. Consider adding role, aria-labelledby, and aria-describedby attributes.
-<div class="modal-example-form {cBase}">
+<div
+ class="modal-example-form {cBase}"
+ role="dialog"
+ aria-modal="true"
+ aria-labelledby="modal-title"
+ aria-describedby="modal-description"
+>
- <header class={cHeader}>{$modalStore[0].title ?? '(title missing)'}</header>
- <article>{$modalStore[0].body ?? '(body missing)'}</article>
+ <header class={cHeader}>
+ <h2 id="modal-title">{$modalStore[0].title ?? '(title missing)'}</h2>
+ </header>
+ <article id="modal-description">{$modalStore[0].body ?? '(body missing)'}</article>📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| {#if $modalStore[0]} | |
| <div class="modal-example-form {cBase}"> | |
| <header class={cHeader}>{$modalStore[0].title ?? '(title missing)'}</header> | |
| <article>{$modalStore[0].body ?? '(body missing)'}</article> | |
| <p class="text-red-500 font-bold">{m.confirmYes()}</p> | |
| <input | |
| type="text" | |
| bind:value={userInput} | |
| placeholder={m.confirmYesPlaceHolder()} | |
| class="w-full mt-2 p-2 border border-gray-300 rounded" | |
| /> | |
| {#if bodyComponent} | |
| <div class="max-h-96 overflow-y-scroll scroll card"> | |
| <svelte:component this={bodyComponent} {...bodyProps} /> | |
| </div> | |
| {/if} | |
| {#if _form && Object.keys(_form).length > 0} | |
| <form method="POST" action={formAction} use:enhance class="modal-form {cForm}"> | |
| <footer class="modal-footer {parent.regionFooter}"> | |
| <button type="button" class="btn {parent.buttonNeutral}" on:click={parent.onClose} | |
| >{m.cancel()}</button | |
| > | |
| <input type="hidden" name="urlmodel" value={URLModel} /> | |
| <input type="hidden" name="id" value={id} /> | |
| <button | |
| class="btn variant-filled-error" | |
| type="submit" | |
| on:click={parent.onConfirm} | |
| disabled={!userInput || userInput.trim().toLowerCase() !== m.yes().toLowerCase()} | |
| > | |
| {m.submit()} | |
| </button> | |
| </footer> | |
| </form> | |
| {#if debug === true} | |
| <SuperDebug data={$form} /> | |
| {/if} | |
| {:else} | |
| <footer class="modal-footer {parent.regionFooter}"> | |
| <button type="button" class="btn {parent.buttonNeutral}" on:click={parent.onClose} | |
| >{m.cancel()}</button | |
| > | |
| <button | |
| class="btn variant-filled-error" | |
| type="button" | |
| on:click={parent.onConfirm} | |
| disabled={!userInput || userInput.trim().toLowerCase() !== m.yes().toLowerCase()} | |
| > | |
| {m.submit()} | |
| </button> | |
| </footer> | |
| {/if} | |
| </div> | |
| {/if} | |
| {#if $modalStore[0]} | |
| <div | |
| class="modal-example-form {cBase}" | |
| role="dialog" | |
| aria-modal="true" | |
| aria-labelledby="modal-title" | |
| aria-describedby="modal-description" | |
| > | |
| <header class={cHeader}> | |
| <h2 id="modal-title">{$modalStore[0].title ?? '(title missing)'}</h2> | |
| </header> | |
| <article id="modal-description">{$modalStore[0].body ?? '(body missing)'}</article> | |
| <p class="text-red-500 font-bold">{m.confirmYes()}</p> | |
| <input | |
| type="text" | |
| bind:value={userInput} | |
| placeholder={m.confirmYesPlaceHolder()} | |
| class="w-full mt-2 p-2 border border-gray-300 rounded" | |
| /> | |
| {#if bodyComponent} | |
| <div class="max-h-96 overflow-y-scroll scroll card"> | |
| <svelte:component this={bodyComponent} {...bodyProps} /> | |
| </div> | |
| {/if} | |
| {#if _form && Object.keys(_form).length > 0} | |
| <form method="POST" action={formAction} use:enhance class="modal-form {cForm}"> | |
| <footer class="modal-footer {parent.regionFooter}"> | |
| <button type="button" class="btn {parent.buttonNeutral}" on:click={parent.onClose} | |
| >{m.cancel()}</button | |
| > | |
| <input type="hidden" name="urlmodel" value={URLModel} /> | |
| <input type="hidden" name="id" value={id} /> | |
| <button | |
| class="btn variant-filled-error" | |
| type="submit" | |
| on:click={parent.onConfirm} | |
| disabled={!userInput || userInput.trim().toLowerCase() !== m.yes().toLowerCase()} | |
| > | |
| {m.submit()} | |
| </button> | |
| </footer> | |
| </form> | |
| {#if debug === true} | |
| <SuperDebug data={$form} /> | |
| {/if} | |
| {:else} | |
| <footer class="modal-footer {parent.regionFooter}"> | |
| <button type="button" class="btn {parent.buttonNeutral}" on:click={parent.onClose} | |
| >{m.cancel()}</button | |
| > | |
| <button | |
| class="btn variant-filled-error" | |
| type="button" | |
| on:click={parent.onConfirm} | |
| disabled={!userInput || userInput.trim().toLowerCase() !== m.yes().toLowerCase()} | |
| > | |
| {m.submit()} | |
| </button> | |
| </footer> | |
| {/if} | |
| </div> | |
| {/if} |
|
@melinoix tests need adjustments as well to deal with this extra step |
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
frontend/tests/functional/user-route.test.ts (1)
Line range hint
266-267: Update user deletion to use the new confirmation pattern.The user deletion still uses the old confirmation pattern while folder deletion uses the new one. This inconsistency should be addressed.
Update the user deletion to match the new pattern:
- await usersPage.deleteModalConfirmButton.click(); + await expect(usersPage.deletePromptConfirmTextField()).toBeVisible(); + await usersPage.deletePromptConfirmTextField().fill(m.yes()); + await usersPage.deletePromptConfirmButton().click();
🧹 Nitpick comments (2)
frontend/tests/utils/page-content.ts (1)
143-149: Consider consolidating the prompt confirmation locators.The methods are correctly implemented, but there's a minor inconsistency between property and method naming patterns.
Consider consolidating the locator access pattern by either:
- Converting all locator properties to methods, or
- Converting these methods to properties to match the pattern used in lines 15-16
- deletePromptConfirmTextField() { - return this.page.getByTestId('delete-prompt-confirm-textfield'); - } - - deletePromptConfirmButton() { - return this.page.getByTestId('delete-prompt-confirm-button'); - } + readonly deleteModalPromptConfirmTextField: Locator; + readonly deleteModalPromptConfirmButton: Locator; + + // In constructor: + this.deleteModalPromptConfirmTextField = this.page.getByTestId('delete-prompt-confirm-textfield'); + this.deleteModalPromptConfirmButton = this.page.getByTestId('delete-prompt-confirm-button');frontend/src/lib/components/Modals/PromptConfirmModal.svelte (1)
28-29: Add fallback for browsers not supporting crypto.randomUUID().Some browsers might not support
crypto.randomUUID(). Consider adding a fallback.-id: `confirm-modal-form-${crypto.randomUUID()}` +id: `confirm-modal-form-${typeof crypto.randomUUID === 'function' ? crypto.randomUUID() : Math.random().toString(36).substring(2)}`
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
frontend/messages/en.json(1 hunks)frontend/messages/fr.json(1 hunks)frontend/src/lib/components/Modals/PromptConfirmModal.svelte(1 hunks)frontend/src/lib/utils/actions.ts(1 hunks)frontend/tests/functional/detailed/compliance-assessments.test.ts(2 hunks)frontend/tests/functional/user-permissions.test.ts(2 hunks)frontend/tests/functional/user-route.test.ts(2 hunks)frontend/tests/utils/page-content.ts(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- frontend/src/lib/utils/actions.ts
- frontend/messages/en.json
🔇 Additional comments (11)
frontend/tests/utils/page-content.ts (1)
15-16: LGTM! Properties are well-named and follow the existing naming convention.The new properties for delete modal prompt confirmation are consistent with the existing codebase structure.
frontend/tests/functional/detailed/compliance-assessments.test.ts (2)
4-4: LGTM! Correctly imports messages for localization.The import is properly added to support the new confirmation flow.
213-215: LGTM! Cleanup logic properly implements the new confirmation flow.The sequence of actions follows the correct pattern:
- Wait for confirmation field visibility
- Fill confirmation text
- Click confirm button
frontend/tests/functional/user-permissions.test.ts (2)
3-4: LGTM! Correctly imports messages for localization.The import is properly added and includes a newline for better readability.
226-228: LGTM! Cleanup logic properly implements the new confirmation flow.The implementation follows the same pattern as other test files, maintaining consistency across the test suite.
frontend/tests/functional/user-route.test.ts (2)
3-3: LGTM! Correctly imports messages for localization.The import is properly added to support the new confirmation flow.
259-261: LGTM! Folder deletion properly implements the new confirmation flow.The implementation follows the same pattern as other test files.
frontend/src/lib/components/Modals/PromptConfirmModal.svelte (3)
84-86:⚠️ Potential issueRestrict debug mode in production.
The debug mode exposes form data which could leak sensitive information.
-{#if debug === true} +{#if debug === true && import.meta.env.DEV} <SuperDebug data={$form} /> {/if}Likely invalid or redundant comment.
6-6: 🛠️ Refactor suggestionDefine proper interface for parent prop instead of using
any.Using
anytype bypasses TypeScript's type checking. Consider creating a proper interface for the parent component's props.-export let parent: any; +interface ParentProps { + onClose: () => void; + onConfirm: () => void; + buttonNeutral: string; + regionFooter: string; +} +export let parent: ParentProps;Likely invalid or redundant comment.
46-48: 🛠️ Refactor suggestionAdd ARIA attributes for better accessibility.
The modal lacks proper ARIA attributes for accessibility.
-<div class="modal-example-form {cBase}"> +<div + class="modal-example-form {cBase}" + role="dialog" + aria-modal="true" + aria-labelledby="modal-title" + aria-describedby="modal-description" +> - <header class={cHeader}>{$modalStore[0].title ?? '(title missing)'}</header> - <article>{$modalStore[0].body ?? '(body missing)'}</article> + <header class={cHeader}> + <h2 id="modal-title">{$modalStore[0].title ?? '(title missing)'}</h2> + </header> + <article id="modal-description">{$modalStore[0].body ?? '(body missing)'}</article>Likely invalid or redundant comment.
frontend/messages/fr.json (1)
1129-1131: LGTM! The new localization keys are well-structured.The added translations for backup version error and confirmation prompts are clear and consistent with the English versions.
…omain-import-a-backup
ab-smith
deleted the
CA-822-add-word-confirmation-when-deleting-domain-import-a-backup
branch
-> Created a new modal 'promptconfirmmodal' for really important choices
-> Fixed boolean confirmation on the modal
-> Fixed flash error messages
Summary by CodeRabbit
Localization
New Features
PromptConfirmModalcomponent for enhanced user confirmations.Bug Fixes
Refactor
Tests