feat: warn the user during domain export if an object is out of scope #1582
Conversation
WalkthroughThis pull request adds back-end and front-end functionality to verify and confirm the export of folder-related data. It introduces a new API method to check for out-of-scope objects in folders and updates the page load function to fetch this information. Additionally, the front-end now uses a modal confirmation for exports when out-of-scope objects are present. New localization strings in English and French support the UI messaging, and dedicated Svelte modal components are added for handling these export confirmations. Changes
Sequence Diagram(s)sequenceDiagram
participant U as User
participant S as Svelte Component
participant MS as ModalStore
participant Modal as ConfirmExportScopeModal
participant Server as Backend
U->>S: Initiates export (form submission)
S->>S: Calls confirmExport(event)
S->>S: Checks for out-of-scope objects in data
alt Out-of-scope objects exist
S->>MS: Trigger modal for export confirmation
MS->>Modal: Display warning and object details
Modal->>U: Await user decision
alt User confirms
Modal->>S: Confirm export
S->>Server: Submit export request
else User cancels
Modal->>S: Abort submission
end
else No out-of-scope objects
S->>Server: Direct submission of export form
end
sequenceDiagram
participant B as Browser
participant PS as +page.server.ts
participant BE as FolderViewSet
B->>PS: Request folder details
PS->>PS: Call loadDetail()
PS->>BE: Fetch out-of-scope objects via check_out_of_scope_objects
BE->>PS: Return out-of-scope objects data
PS->>B: Respond with combined folder details and scope data
Suggested reviewers
Poem
Tip ⚡💬 Agentic Chat (Pro Plan, General Availability)
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (3)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (2)
⏰ Context from checks skipped due to timeout of 90000ms (10)
🪧 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 (
|
…-an-object-is-out-of-scope
…-an-object-is-out-of-scope
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (7)
enterprise/frontend/src/routes/(app)/(internal)/folders/[id=uuid]/+page.server.ts (2)
15-20: Consider enhancing error handling.The current implementation only checks if the response is not OK but doesn't handle potential network errors or malformed JSON responses.
// Fetch has_out_of_scope_objects - const outOfScopeRes = await event.fetch(`${BASE_API_URL}/folders/${event.params.id}/check_out_of_scope_objects/`); - if (!outOfScopeRes.ok) { - throw new Error('Failed to check out-of-scope objects'); - } + try { + const outOfScopeRes = await event.fetch(`${BASE_API_URL}/folders/${event.params.id}/check_out_of_scope_objects/`); + if (!outOfScopeRes.ok) { + throw new Error(`Failed to check out-of-scope objects: ${outOfScopeRes.statusText}`); + }
20-25: Add type safety for API response data.The destructuring of the response JSON lacks type definitions, which could lead to runtime errors if the API response structure changes.
+ interface OutOfScopeResponse { + has_out_of_scope_objects: string[]; + out_of_scope_objects: Record<string, unknown>; + } + - const { has_out_of_scope_objects, out_of_scope_objects } = await outOfScopeRes.json(); + const { has_out_of_scope_objects, out_of_scope_objects } = await outOfScopeRes.json() as OutOfScopeResponse; return { ...detailData, has_out_of_scope_objects, out_of_scope_objects };enterprise/frontend/src/routes/(app)/(internal)/folders/[id=uuid]/+page.svelte (2)
23-26: Ensure has_out_of_scope_objects is always an array.The check for
data.has_out_of_scope_objects?.lengthassumes it's an array, but this should be validated to prevent runtime errors.function confirmExport(event: Event) { event.preventDefault(); - if (data.has_out_of_scope_objects?.length) { + if (Array.isArray(data.has_out_of_scope_objects) && data.has_out_of_scope_objects.length > 0) {
49-57: Handle all modal response scenarios.The current implementation handles only the confirmation case but doesn't account for scenarios where a user might dismiss the modal without explicitly responding.
response: (r: boolean) => { if (r) (event.target as HTMLFormElement).submit(); + // You might want to add feedback or logging for when a user cancels the export }Also, you might want to add type safety for the form element:
- if (r) (event.target as HTMLFormElement).submit(); + const form = event.target; + if (r && form instanceof HTMLFormElement) { + form.submit(); + }frontend/src/lib/components/Modals/ConfirmExportScopeModal.svelte (1)
6-6: Consider using a more specific type thananyfor the parent prop.Using
anytype reduces TypeScript's ability to catch errors. Consider creating an interface that specifies the required methods likeonCloseandonConfirmthat the parent must implement.- export let parent: any; + interface ModalParent { + onClose: () => void; + onConfirm: () => void; + buttonNeutral: string; + regionFooter: string; + } + export let parent: ModalParent;frontend/messages/fr.json (1)
1193-1195: Consider consistent naming conventions for translation keys.The keys
loadedlibrary,riskmatrix, andreferencecontroldon't follow the camelCase convention used in the rest of the file (compare withloadedLibrariesat line 373). Consider standardizing these keys.- "loadedlibrary": "Bibliothèques chargées", - "riskmatrix": "Matrice de risques", - "referencecontrol": "Mesures de référence", + "loadedLibrary": "Bibliothèques chargées", + "riskMatrix": "Matrice de risques", + "referenceControl": "Mesures de référence",frontend/messages/en.json (1)
1204-1207: Consider consistent naming conventions for translation keys.Similar to the French translations, the keys
loadedlibrary,riskmatrix, andreferencecontroldon't follow the camelCase convention used elsewhere in the file. Consider standardizing these keys.- "loadedlibrary": "Loaded libraries", - "riskmatrix": "Risk matrix", - "referencecontrol": "Reference control", + "loadedLibrary": "Loaded libraries", + "riskMatrix": "Risk matrix", + "referenceControl": "Reference control",
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
backend/core/views.py(1 hunks)enterprise/frontend/src/routes/(app)/(internal)/folders/[id=uuid]/+page.server.ts(1 hunks)enterprise/frontend/src/routes/(app)/(internal)/folders/[id=uuid]/+page.svelte(1 hunks)frontend/messages/en.json(2 hunks)frontend/messages/fr.json(2 hunks)frontend/src/lib/components/Modals/ConfirmExportScopeModal.svelte(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (9)
- GitHub Check: startup-functional-test (3.12)
- GitHub Check: startup-docker-compose-test
- GitHub Check: functional-tests (3.12, chromium)
- GitHub Check: enterprise-startup-functional-test (3.12)
- GitHub Check: enterprise-functional-tests (3.12, chromium)
- GitHub Check: test (3.12)
- GitHub Check: enterprise-startup-docker-compose-test
- GitHub Check: build (3.12)
- GitHub Check: Analyze (python)
🔇 Additional comments (14)
enterprise/frontend/src/routes/(app)/(internal)/folders/[id=uuid]/+page.server.ts (2)
3-6: API import style is consistent with project standards.The import statement cleanup and addition of BASE_API_URL import follows the project's coding style.
9-13: Good refactoring to store loadDetail result.Storing the result of
loadDetailin a variable before enriching it with additional data makes the code more maintainable and easier to understand.enterprise/frontend/src/routes/(app)/(internal)/folders/[id=uuid]/+page.svelte (4)
5-12: Imports are well-organized and appropriate.The new imports for modal functionality and translation utilities are properly organized and necessary for the added feature.
17-18: Modal store initialization is correctly placed.The modal store is properly initialized outside of any reactive blocks.
27-32: Props passing looks good.The modal component receives the necessary props for displaying out-of-scope objects.
61-73: Form implementation looks good.The export form is well-implemented with appropriate event handling and styling. The button includes an icon which improves usability.
backend/core/views.py (1)
2393-2443: Good implementation of out-of-scope object detection during exportThe new method
check_out_of_scope_objectssuccessfully implements the feature described in the PR objectives. It properly identifies objects that would be exported but exist outside the specified domain by:
- Building a set of valid folder IDs (the current folder and its subfolders)
- Checking each exportable object type to find instances outside these valid folders
- Collecting and returning details about these out-of-scope objects
This implementation will allow the frontend to warn users appropriately when they're about to export objects that are outside their intended domain scope, helping prevent unintended data exports.
frontend/src/lib/components/Modals/ConfirmExportScopeModal.svelte (5)
40-40: Be careful with debug mode in production.The debug property is set to false by default which is good, but ensure it remains disabled in production to avoid exposing sensitive information.
42-43: Good implementation of the outOfScopeObjects structure.The data structure chosen for outOfScopeObjects allows for well-organized grouping by object type, and the optional description property provides flexibility for displaying additional context.
52-76: Good UI implementation for showing out-of-scope objects.The accordion implementation provides a clean, organized way to display out-of-scope objects grouped by type. The badge showing the count for each type is a nice touch for user experience.
84-92: Form handling looks correct.The form implementation with hidden fields for URLModel and id looks correct. The button actions are properly connected to the parent component's handlers.
93-95: Appropriate debug option implementation.The conditional rendering of the SuperDebug component only when debug is true is a good approach. This ensures debugging information doesn't appear in production.
frontend/messages/fr.json (1)
1182-1184: New translations for export warning feature added correctly.The added translations for the confirmation modal warning feature match the English counterparts and are appropriately phrased in French.
frontend/messages/en.json (1)
472-473: New warning title added correctly.The addition of the warning title for the confirmation modal is appropriate and matches its purpose in the component.
enterprise/frontend/src/routes/(app)/(internal)/folders/[id=uuid]/+page.svelte
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
frontend/src/lib/components/Modals/ConfirmExportBody.svelte (1)
1-6: Code looks good, but TypeScript type could be added.The component setup is clean and straightforward, but consider adding a TypeScript type for the
typesprop to improve type safety.- export let types = []; + export let types: string[] = [];enterprise/frontend/src/routes/(app)/(internal)/folders/[id=uuid]/+page.svelte (1)
25-51: Consider adding error handling to the confirmExport function.The export confirmation logic is correctly implemented, checking for out-of-scope objects and showing a modal when needed. However, there's no error handling for potential issues during form submission or modal display.
Consider adding try/catch blocks to handle potential errors:
function confirmExport(event: Event) { event.preventDefault(); + + try { if (data.has_out_of_scope_objects?.length) { const modalComponent: ModalComponent = { ref: ConfirmExportScopeModal, props: { outOfScopeObjects: data.out_of_scope_objects || {}, bodyComponent: ConfirmExportBody, bodyProps: { types: data.has_out_of_scope_objects } } }; const modal: ModalSettings = { type: 'component', component: modalComponent, title: m.confirmModalTitleWarning(), response: (r: boolean) => { + try { if (r) (event.target as HTMLFormElement).submit(); + } catch (error) { + console.error('Error submitting the form:', error); + // Add appropriate error handling/user notification + } } }; modalStore.trigger(modal); } else { (event.target as HTMLFormElement).submit(); } + } catch (error) { + console.error('Error in export confirmation process:', error); + // Add appropriate error handling/user notification + } }frontend/src/lib/components/Modals/ConfirmExportScopeModal.svelte (1)
1-43: Improve type safety and organize imports.The component setup is good, but there are a few areas for improvement:
- The
parent: anytype could be more specific- Imports could be organized better for readability
- Consider adding JSDoc comments for complex props
<script lang="ts"> import type { urlModel } from '$lib/utils/types'; + import type { ComponentType } from 'svelte'; + import { getModalStore } from '@skeletonlabs/skeleton'; + import type { ModalStore } from '@skeletonlabs/skeleton'; + import { Accordion, AccordionItem } from '@skeletonlabs/skeleton'; + import { superForm } from 'sveltekit-superforms'; + import SuperForm from '$lib/components/Forms/Form.svelte'; + import SuperDebug from 'sveltekit-superforms'; + import * as m from '$paraglide/messages'; + import { safeTranslate } from '$lib/utils/i18n'; // Props /** Exposes parent props to this component. */ - export let parent: any; + export let parent: { + onClose: () => void; + onConfirm: () => void; + buttonNeutral: string; + regionFooter: string; + }; // Stores - import { getModalStore } from '@skeletonlabs/skeleton'; - import type { ModalStore } from '@skeletonlabs/skeleton'; - - import * as m from '$paraglide/messages'; - import { Accordion, AccordionItem } from '@skeletonlabs/skeleton'; - import { safeTranslate } from '$lib/utils/i18n'; const modalStore: ModalStore = getModalStore(); export let _form = {}; export let URLModel: urlModel | '' = ''; export let id: string = ''; export let formAction: string; + /** Component to use for the modal body */ export let bodyComponent: ComponentType | undefined; + /** Props to pass to the body component */ export let bodyProps: Record<string, unknown> = {}; - import { superForm } from 'sveltekit-superforms'; - - import SuperForm from '$lib/components/Forms/Form.svelte'; - const { form } = superForm(_form, { dataType: 'json', id: `confirm-modal-form-${crypto.randomUUID()}` }); // Base Classes const cBase = 'card p-4 w-modal shadow-xl space-y-4'; const cHeader = 'text-2xl font-bold'; const cForm = 'p-4 space-y-4 rounded-container-token'; - import SuperDebug from 'sveltekit-superforms'; - import type { ComponentType } from 'svelte'; + /** Enable debug mode to show form data */ export let debug = false; + /** Structure of out-of-scope objects to display in the modal */ export let outOfScopeObjects: Record<string, Array<{ name: string; description?: string }>> = {};
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
enterprise/frontend/src/routes/(app)/(internal)/folders/[id=uuid]/+page.svelte(1 hunks)frontend/src/lib/components/Modals/ConfirmExportBody.svelte(1 hunks)frontend/src/lib/components/Modals/ConfirmExportScopeModal.svelte(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (10)
- GitHub Check: startup-functional-test (3.12)
- GitHub Check: startup-docker-compose-test
- GitHub Check: enterprise-functional-tests (3.12, chromium)
- GitHub Check: enterprise-startup-functional-test (3.12)
- GitHub Check: enterprise-startup-docker-compose-test
- GitHub Check: test (3.12)
- GitHub Check: build (3.12)
- GitHub Check: functional-tests (3.12, chromium)
- GitHub Check: Analyze (python)
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (6)
frontend/src/lib/components/Modals/ConfirmExportBody.svelte (1)
8-17: Well-structured UI component with proper i18n implementation.The component correctly uses the internationalization system and safely translates type values. The UI structure with flex layout and proper spacing is appropriate for displaying a list of warning badges.
enterprise/frontend/src/routes/(app)/(internal)/folders/[id=uuid]/+page.svelte (3)
5-14: Necessary imports for modal functionality.The imports are appropriate for implementing the export confirmation modal functionality.
19-20: Correct setup of modal store.Modal store is correctly initialized using the Skeleton UI library.
55-67: Well-structured export form UI.The export form is correctly implemented with appropriate action, method, and event handling. The button styling with icon is consistent with the application's UI patterns.
frontend/src/lib/components/Modals/ConfirmExportScopeModal.svelte (2)
45-83: Well-structured UI with good component composition.The modal UI is well-organized with appropriate conditionals for optional content and good use of the accordion component for displaying out-of-scope objects. The translation handling is properly implemented.
85-97:❓ Verification inconclusive
Ensure debug mode is not enabled in production.
The form implementation looks good with proper action and data binding. The debug mode is appropriately conditional, but make sure it's not enabled in production environments.
🏁 Script executed:
#!/bin/bash # Check if there are any debug flags set to true in production code grep -r "export let debug = true" --include="*.svelte" .Length of output: 58
Production Debug Mode Safeguard
The debug section in the ConfirmExportScopeModal component is conditionally rendered as expected. The grep check forexport let debug = trueproduced no output, suggesting that there isn’t an explicit assignment enabling debug mode in these Svelte files. However, please verify that the overall configuration (such as environment-specific settings or imports from higher-level components) guarantees that thedebugflag is false in production.
- Confirm that the default or externally injected value for
debugis set to false in production.- Check your build and deployment configurations to ensure that debug mode is not inadvertently enabled elsewhere.
…-an-object-is-out-of-scope
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
backend/core/views.py (1)
2402-2452: Well-implemented out-of-scope object detection logic.The new
check_out_of_scope_objectsmethod effectively identifies objects that are outside the scope of a folder and its sub-folders, supporting the PR's objective to warn users during domain export.A couple of suggestions to consider:
- Adding error handling to gracefully handle exceptions
- Consider adding caching for repeated calls, as the operation might be expensive for large datasets
You could improve error handling with a try/except block:
@action(detail=True, methods=["get"]) def check_out_of_scope_objects(self, request, pk=None): """Action to check if there are out-of-scope objects for the folder.""" + try: instance = self.get_object() if instance.content_type == "GL": return Response( { "has_out_of_scope_objects": [], "out_of_scope_objects": {}, } ) valid_folder_ids = set( Folder.objects.filter( Q(id=instance.id) | Q(id__in=[f.id for f in instance.get_sub_folders()]) ).values_list("id", flat=True) ) objects = get_domain_export_objects(instance) out_of_scope_types = [] out_of_scope_objects = {} for model_name, queryset in objects.items(): model = queryset.model folder_filter = None if "folder" in [f.name for f in model._meta.get_fields()]: folder_filter = Q(folder__id__in=valid_folder_ids) if folder_filter is None: continue out_of_scope_qs = queryset.exclude(folder_filter).distinct() if out_of_scope_qs.exists(): out_of_scope_types.append(model_name) out_of_scope_objects[model_name] = [ { "id": obj.id, "name": getattr(obj, "name", None), "description": getattr(obj, "description", None), } for obj in out_of_scope_qs ] return Response( { "has_out_of_scope_objects": out_of_scope_types, "out_of_scope_objects": out_of_scope_objects, } ) + except Exception as e: + logger.error( + "Error checking out-of-scope objects", + folder_id=pk, + error=str(e), + exc_info=True + ) + return Response( + {"error": "Failed to check out-of-scope objects"}, + status=status.HTTP_500_INTERNAL_SERVER_ERROR + )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
backend/core/views.py(1 hunks)frontend/messages/en.json(2 hunks)frontend/messages/fr.json(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- frontend/messages/en.json
- frontend/messages/fr.json
⏰ Context from checks skipped due to timeout of 90000ms (11)
- GitHub Check: startup-docker-compose-test
- GitHub Check: enterprise-startup-functional-test (3.12)
- GitHub Check: enterprise-startup-docker-compose-test
- GitHub Check: startup-functional-test (3.12)
- GitHub Check: functional-tests (3.12, chromium)
- GitHub Check: test (3.12)
- GitHub Check: ruff (3.12)
- GitHub Check: enterprise-functional-tests (3.12, chromium)
- GitHub Check: build (3.12)
- GitHub Check: Analyze (python)
- GitHub Check: Analyze (javascript-typescript)
…-an-object-is-out-of-scope
During domain export, we sometimes need to take objects outside of the domain which is exported (Global, or sub-domains mostly). This PR warn the user that some objects out of this domain will also be exported but only when it’s the case.
Summary by CodeRabbit