feat: business impact analysis (bia) #1940
Conversation
WalkthroughThese changes introduce a new "resilience" module across both backend and frontend. The backend adds Django models, migrations, serializers, viewsets, permissions, and URLs for Business Impact Analysis (BIA), Asset Assessment, and Escalation Thresholds. The frontend integrates forms, list/table views, navigation, localization, schemas, and multiple Svelte pages for viewing, editing, and visualizing resilience-related data. Navigation and permissions are updated accordingly. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Frontend
participant Backend
participant DB
User->>Frontend: Navigates to BIA/Asset Assessment/Threshold page
Frontend->>Backend: API request for list/detail/metrics
Backend->>DB: Query BIA/AssetAssessment/EscalationThreshold models
DB-->>Backend: Return data
Backend-->>Frontend: Respond with serialized data
Frontend-->>User: Render forms, tables, visualizations
User->>Frontend: Submits form (create/edit BIA/Asset Assessment/Threshold)
Frontend->>Backend: POST/PUT API request
Backend->>DB: Save/update model instance
DB-->>Backend: Confirm save
Backend-->>Frontend: Respond with result
Frontend-->>User: Show confirmation/UI update
Suggested reviewers
Poem
Tip ⚡️ Faster reviews with caching
Enjoy the performance boost—your workflow just got faster. 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
✅ Files skipped from review due to trivial changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (8)
✨ 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: 29
🔭 Outside diff range comments (1)
backend/resilience/tests.py (1)
1-4: 🛠️ Refactor suggestionTests missing for new resilience module
This file has been created as a placeholder for tests, but no actual tests have been implemented for the new resilience models (BusinessImpactAnalysis, AssetAssessment, EscalationThreshold) and their API endpoints. Since this PR introduces a new module with significant functionality, test coverage is important to ensure everything works correctly.
Additionally, the
TestCaseimport is currently unused since no test classes are defined.Would you like me to generate some sample test cases for the resilience models? This could include basic CRUD operations, permission tests, and validation tests.
🧰 Tools
🪛 Ruff (0.8.2)
1-1:
django.test.TestCaseimported but unusedRemove unused import:
django.test.TestCase(F401)
🧹 Nitpick comments (26)
backend/resilience/admin.py (1)
1-4: Admin model registration missingThis file has been created as a placeholder, but the new models (BusinessImpactAnalysis, AssetAssessment, EscalationThreshold) haven't been registered with the admin interface. Registering these models would provide administrators with an interface to manage them directly.
Additionally, the
adminimport is currently unused since no models are registered.Consider registering the resilience models with the admin site:
from django.contrib import admin from resilience.models import BusinessImpactAnalysis, AssetAssessment, EscalationThreshold admin.site.register(BusinessImpactAnalysis) admin.site.register(AssetAssessment) admin.site.register(EscalationThreshold)🧰 Tools
🪛 Ruff (0.8.2)
1-1:
django.contrib.adminimported but unusedRemove unused import:
django.contrib.admin(F401)
frontend/src/routes/(app)/(internal)/business-impact-analysis/[id=uuid]/visual/+page.server.ts (1)
1-11: Add TypeScript interface for the returned data structureThe component doesn't define the expected structure of the returned data, which makes it harder to understand what the API returns and how it's used in the UI.
Consider defining an interface for the data structure:
import { BASE_API_URL } from '$lib/utils/constants'; import type { PageServerLoad } from './$types'; +interface ImpactTableData { + // Define the expected structure based on the API response + // For example: + // id: string; + // impact: { name: string; hexcolor: string; }; + // pit: string; + // [other properties...] +} export const load = (async ({ fetch, params }) => { const endpoint = `${BASE_API_URL}/resilience/business-impact-analysis/${params.id}/build-table/`; const res = await fetch(endpoint); - const data = await res.json(); + const data: ImpactTableData[] = await res.json(); return { data }; }) satisfies PageServerLoad;frontend/src/lib/components/DataViz/LineHeatmap.svelte (1)
1-13: Add documentation for the component and the 'pit' fieldThe component lacks documentation explaining its purpose and the meaning of domain-specific terms like 'pit'.
Add JSDoc comments to explain the component and its properties:
<script lang="ts"> + /** + * LineHeatmap Component + * + * Visualizes impact data as a horizontal heatmap with color-coded segments + * @component + */ + /** + * Array of impact entries to visualize + * @property {Object} impact - Impact data with name and color + * @property {string} pit - Point in time value (e.g., recovery time or similar metric) + */ export let data; import { safeTranslate } from '$lib/utils/i18n'; </script>frontend/src/routes/(app)/(internal)/asset-assessments/[id=uuid]/action-plan/+page.server.ts (1)
1-12: Add TypeScript interface for the API responseSimilar to the previous file, there's no type definition for the API response data structure.
Add a type definition for the findings assessment data:
import { BASE_API_URL } from '$lib/utils/constants'; import type { PageServerLoad } from './$types'; +interface FindingsAssessment { + // Define the expected structure based on the API response + id: string; + // Add other relevant properties +} export const load = (async ({ fetch, params }) => { const URLModel = 'findings-assessments'; const endpoint = `${BASE_API_URL}/${URLModel}/${params.id}/`; const res = await fetch(endpoint); - const findings_assessment = await res.json(); + const findings_assessment: FindingsAssessment = await res.json(); return { URLModel, findings_assessment }; }) satisfies PageServerLoad;frontend/src/routes/(app)/(internal)/business-impact-analysis/[id=uuid]/action-plan/+page.server.ts (1)
10-11: Add type annotation for the returned data.The
findings_assessmentvariable lacks type information, which would improve code maintainability and IDE support.const res = await fetch(endpoint); - const findings_assessment = await res.json(); + const findings_assessment: FindingsAssessment = await res.json(); return { URLModel, findings_assessment };Consider adding an interface for
FindingsAssessmentin your types file.frontend/messages/en.json (1)
376-399: Comprehensive set of localization entries for the new resilience module.The added localization keys provide good coverage for the business impact analysis and asset assessment features. The keys follow the existing naming conventions and are well organized.
There's a potential consistency issue to consider: some keys use camelCase (like "dataWizard") while others use snake_case (like "recovery_documented"). It might be worth standardizing this in a future update.
frontend/src/routes/(app)/(internal)/asset-assessments/[id=uuid]/+page.svelte (1)
9-37: Good UI implementation for asset assessment details.The layout with disaster recovery and security objectives in a two-column grid works well, and the inclusion of the LineHeatmap visualization provides good data insights. The use of FontAwesome icons enhances the visual appeal.
Consider adding additional error handling for cases where the data might not be available:
-{#each data.asset.disaster_recovery_objectives as objective} +{#if data.asset.disaster_recovery_objectives?.length} + {#each data.asset.disaster_recovery_objectives as objective} <div class="uppercase">{objective.str}</div> -{/each} + {/each} +{:else} + <div class="text-gray-500">No disaster recovery objectives defined</div> +{/if}frontend/src/routes/(app)/(internal)/business-impact-analysis/[id=uuid]/+page.server.ts (2)
27-32: Remove commented guidance text.The comment "Keep your existing loadDetail logic" seems to be from a template or guidance for implementation. This should be removed or replaced with more specific documentation about what the code is doing.
29-29: Remove console.log statement.Debug logging should be removed before production deployment.
- console.log('delete');frontend/src/routes/(app)/(internal)/business-impact-analysis/[id=uuid]/action-plan/+page.svelte (1)
42-61: Consider using dynamic routing helpers for navigation links.The hardcoded paths in the navigation links could be more maintainable using routing helpers or constants, especially if route patterns change in the future.
- href="/perimeters/{data.findings_assessment.perimeter.id}/" + href={`/perimeters/${data.findings_assessment.perimeter.id}/`}And:
- href="/findings-assessments/{data.findings_assessment.id}/" + href={`/findings-assessments/${data.findings_assessment.id}/`}frontend/src/routes/(app)/(internal)/asset-assessments/[id=uuid]/+page.server.ts (2)
35-35: Remove console.log statement.Debug logging should be removed before production deployment.
- console.log('delete');
10-10: Remove commented guidance text.The comment "Keep your existing loadDetail logic" seems to be from a template or guidance for implementation. This should be removed or replaced with more specific documentation about what the code is doing.
frontend/src/lib/utils/table.ts (1)
775-802: Table configurations for new resilience models.The list view field configurations for the resilience models are appropriately defined. However, there are a couple of observations:
- The
business-impact-analysisconfiguration is minimal compared to other models, and doesn't include any filters despite having filter definitions in the URL_MODEL_MAP.- The
escalation-thresholdsmodel usesget_human_pitin the body array, which suggests a backend method that generates a human-readable string. This is fine, but it's a unique pattern not commonly seen in other models.Consider adding filters to the business-impact-analysis configuration:
'business-impact-analysis': { head: ['name', 'perimeter', 'status'], body: ['name', 'perimeter', 'status'], + filters: { + perimeter: PERIMETER_FILTER, + status: RISK_ASSESSMENT_STATUS_FILTER + } },backend/resilience/serializers.py (2)
30-31: Remove commented-out code.There's a commented-out line that should be removed as it doesn't serve any purpose and could create confusion.
class AssetAssessmentReadSerializer(BaseModelSerializer): str = serializers.CharField(source="__str__") - # name = serializers.CharField(source="__str__")
15-15: Consider replacingexclude = []withfields = "__all__"for clarity.Using
exclude = []is functionally equivalent to including all fields, but usingfields = "__all__"would be more explicit and consistent with the write serializers.class Meta: model = BusinessImpactAnalysis - exclude = [] + fields = "__all__"Also applies to: 43-43, 61-61
frontend/src/lib/components/Forms/ModelForm/AssetAssessmentForm.svelte (1)
18-19: Remove commented-out code.There are commented-out export declarations that should be removed to keep the codebase clean.
export let initialData: Record<string, any> = {}; export let object: Record<string, any> = {}; -// export let context: string = 'default'; -// export let updated_fields: Set<string> = new Set();frontend/src/routes/(app)/(internal)/business-impact-analysis/[id=uuid]/visual/+page.svelte (2)
15-28: Consider handling edge cases informatTimePoint.The time formatting function doesn't handle negative values and might not render seconds. Consider adding validation or handling for these cases.
function formatTimePoint(seconds: number) { + if (typeof seconds !== 'number' || isNaN(seconds)) return '--'; if (seconds === 0) return '0'; + if (seconds < 0) return `${seconds}s`; const days = Math.floor(seconds / 86400); const hours = Math.floor((seconds % 86400) / 3600); const minutes = Math.floor((seconds % 3600) / 60); + const remainingSeconds = Math.floor(seconds % 60); const parts = []; if (days) parts.push(`${days}d`); if (hours) parts.push(`${hours}h`); if (minutes) parts.push(`${minutes}m`); + if (remainingSeconds && parts.length === 0) parts.push(`${remainingSeconds}s`); return parts.join(' '); }
106-119: Optimize CSS selectors for impact transitions.The current CSS approach with attribute selectors and pseudo-elements is complex. Consider using classes instead for better readability and performance.
/* Add visual cues for impact transitions */ -td[style*='border-left: none'] { +.continuous-impact { position: relative; } -td[style*='border-left: none']::before { +.continuous-impact::before { content: ''; position: absolute; left: 0; top: 0; bottom: 0; width: 1px; background: rgba(0, 0, 0, 0.05); opacity: 0.3; }Then update the cell to use this class:
<td class="px-4 py-2 text-center" + class:continuous-impact={!isImpactChange(entry, i)} style:backgroundColor={entry.data[point].hexcolor || '#f9fafb'} - style:borderLeft={!isImpactChange(entry, i) ? 'none' : undefined} >frontend/src/lib/components/Forms/ModelForm/EscalationThresholdForm.svelte (2)
3-4: Remove unused imports.The component imports TextField and NumberField but doesn't use them. Remove these imports to keep the codebase clean.
<script lang="ts"> import AutocompleteSelect from '../AutocompleteSelect.svelte'; - import TextField from '$lib/components/Forms/TextField.svelte'; import Duration from '../Duration.svelte'; import TextArea from '$lib/components/Forms/TextArea.svelte'; import Select from '../Select.svelte'; - import NumberField from '$lib/components/Forms/NumberField.svelte'; import type { SuperValidated } from 'sveltekit-superforms';Also applies to: 7-7
19-20: Remove commented-out code.There are commented-out export declarations that should be removed to keep the codebase clean.
export let initialData: Record<string, any> = {}; export let object: Record<string, any> = {}; -// export let context: string = 'default'; -// export let updated_fields: Set<string> = new Set();frontend/src/lib/components/Forms/ModelForm/BusinessImpactAnalysisForm.svelte (2)
17-19: Consider removing commented code.These commented export statements suggest unfinished or abandoned functionality. Either implement them or remove them to keep the codebase clean.
- // export let context: string = 'default'; - // export let updated_fields: Set<string> = new Set();
78-86: Enhance date field with min/max constraints.The due date field could benefit from min/max date constraints to prevent invalid dates (e.g., past dates or dates too far in the future).
<TextField type="date" {form} field="due_date" label={m.dueDate()} helpText={m.dueDateHelpText()} + min={new Date().toISOString().split('T')[0]} cacheLock={cacheLocks['due_date']} bind:cachedValue={formDataCache['due_date']} />frontend/src/lib/utils/schemas.ts (2)
4-4: Remove unused import.This import doesn't appear to be used in the file and may be accidentally left from development.
-import type { evidences } from '$paraglide/messages/hi';
452-462: Consider adding validation for recovery fields interrelationships.The recovery fields (documented, tested, targets_met) likely have business rules governing their interdependencies. For example, if recovery_targets_met is true, then recovery_tested should also be true.
Consider enhancing the schema with Zod's refine method to enforce these logical constraints:
export const AssetAssessmentSchema = z.object({ // existing fields... recovery_documented: z.boolean().default(false), recovery_tested: z.boolean().default(false), recovery_targets_met: z.boolean().default(false), }).refine(data => { // Can't have tested recovery without documentation if (data.recovery_tested && !data.recovery_documented) return false; // Can't meet targets without testing if (data.recovery_targets_met && !data.recovery_tested) return false; return true; }, { message: "Recovery validation failed: cannot have testing without documentation or meet targets without testing", path: ["recovery_targets_met"] });backend/resilience/migrations/0001_initial.py (1)
55-64: Inconsistent help_text in version field.The help text for the version field mistakenly references a "compliance assessment" instead of a "business impact analysis".
( "version", models.CharField( blank=True, default="1.0", - help_text="Version of the compliance assessment (eg. 1.0, 2.0, etc.)", + help_text="Version of the business impact analysis (eg. 1.0, 2.0, etc.)", max_length=100, null=True, verbose_name="Version", ), ),backend/resilience/views.py (1)
9-11: Clarify time units in cache TTL comments.The abbreviation "mn" in comments is ambiguous. It should be clearly specified as "minutes" for better readability.
-SHORT_CACHE_TTL = 2 # mn -MED_CACHE_TTL = 5 # mn -LONG_CACHE_TTL = 60 # mn +SHORT_CACHE_TTL = 2 # minutes +MED_CACHE_TTL = 5 # minutes +LONG_CACHE_TTL = 60 # minutes
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (38)
backend/ciso_assistant/settings.py(1 hunks)backend/core/startup.py(1 hunks)backend/core/urls.py(1 hunks)backend/resilience/admin.py(1 hunks)backend/resilience/apps.py(1 hunks)backend/resilience/migrations/0001_initial.py(1 hunks)backend/resilience/models.py(1 hunks)backend/resilience/serializers.py(1 hunks)backend/resilience/tests.py(1 hunks)backend/resilience/urls.py(1 hunks)backend/resilience/views.py(1 hunks)enterprise/backend/enterprise_core/settings.py(1 hunks)enterprise/frontend/src/lib/components/SideBar/navData.ts(1 hunks)frontend/messages/en.json(1 hunks)frontend/src/lib/components/DataViz/ActivityTracker.svelte(2 hunks)frontend/src/lib/components/DataViz/LineHeatmap.svelte(1 hunks)frontend/src/lib/components/Forms/Duration.svelte(2 hunks)frontend/src/lib/components/Forms/ModelForm.svelte(2 hunks)frontend/src/lib/components/Forms/ModelForm/AssetAssessmentForm.svelte(1 hunks)frontend/src/lib/components/Forms/ModelForm/BusinessImpactAnalysisForm.svelte(1 hunks)frontend/src/lib/components/Forms/ModelForm/EscalationThresholdForm.svelte(1 hunks)frontend/src/lib/components/SideBar/navData.ts(2 hunks)frontend/src/lib/utils/crud.ts(2 hunks)frontend/src/lib/utils/load.ts(1 hunks)frontend/src/lib/utils/schemas.ts(3 hunks)frontend/src/lib/utils/table.ts(1 hunks)frontend/src/lib/utils/types.ts(1 hunks)frontend/src/routes/(app)/(internal)/asset-assessments/[id=uuid]/+page.server.ts(1 hunks)frontend/src/routes/(app)/(internal)/asset-assessments/[id=uuid]/+page.svelte(1 hunks)frontend/src/routes/(app)/(internal)/asset-assessments/[id=uuid]/action-plan/+page.server.ts(1 hunks)frontend/src/routes/(app)/(internal)/asset-assessments/[id=uuid]/action-plan/+page.svelte(1 hunks)frontend/src/routes/(app)/(internal)/business-impact-analysis/[id=uuid]/+page.server.ts(1 hunks)frontend/src/routes/(app)/(internal)/business-impact-analysis/[id=uuid]/+page.svelte(1 hunks)frontend/src/routes/(app)/(internal)/business-impact-analysis/[id=uuid]/action-plan/+page.server.ts(1 hunks)frontend/src/routes/(app)/(internal)/business-impact-analysis/[id=uuid]/action-plan/+page.svelte(1 hunks)frontend/src/routes/(app)/(internal)/business-impact-analysis/[id=uuid]/visual/+page.server.ts(1 hunks)frontend/src/routes/(app)/(internal)/business-impact-analysis/[id=uuid]/visual/+page.svelte(1 hunks)frontend/src/routes/(app)/(internal)/my-assignments/+page.svelte(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (6)
frontend/src/routes/(app)/(internal)/business-impact-analysis/[id=uuid]/visual/+page.server.ts (1)
frontend/src/lib/utils/constants.ts (1)
BASE_API_URL(3-7)
backend/resilience/urls.py (1)
backend/resilience/views.py (3)
BusinessImpactAnalysisViewSet(24-48)AssetAssessmentViewSet(51-78)EscalationThresholdViewSet(81-108)
frontend/src/routes/(app)/(internal)/asset-assessments/[id=uuid]/action-plan/+page.server.ts (3)
frontend/src/routes/(app)/(internal)/asset-assessments/[id=uuid]/+page.server.ts (1)
load(8-32)frontend/src/routes/(app)/(internal)/business-impact-analysis/[id=uuid]/action-plan/+page.server.ts (1)
load(5-12)frontend/src/lib/utils/constants.ts (1)
BASE_API_URL(3-7)
frontend/src/routes/(app)/(internal)/business-impact-analysis/[id=uuid]/action-plan/+page.server.ts (4)
frontend/src/routes/(app)/(internal)/asset-assessments/[id=uuid]/+page.server.ts (1)
load(8-32)frontend/src/routes/(app)/(internal)/asset-assessments/[id=uuid]/action-plan/+page.server.ts (1)
load(5-12)frontend/src/routes/(app)/(internal)/business-impact-analysis/[id=uuid]/+page.server.ts (1)
load(8-26)frontend/src/lib/utils/constants.ts (1)
BASE_API_URL(3-7)
frontend/src/lib/utils/schemas.ts (1)
backend/core/base_models.py (1)
NameDescriptionMixin(111-124)
backend/resilience/migrations/0001_initial.py (1)
backend/iam/models.py (2)
Folder(58-229)get_root_folder_id(70-75)
🪛 Ruff (0.8.2)
backend/resilience/admin.py
1-1: django.contrib.admin imported but unused
Remove unused import: django.contrib.admin
(F401)
backend/resilience/tests.py
1-1: django.test.TestCase imported but unused
Remove unused import: django.test.TestCase
(F401)
backend/resilience/models.py
19-19: icecream.ic imported but unused
Remove unused import: icecream.ic
(F401)
20-20: iam.models.User imported but unused
Remove unused import: iam.models.User
(F401)
backend/resilience/views.py
1-1: os.name imported but unused
Remove unused import: os.name
(F401)
⏰ 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: functional-tests (3.12, chromium)
- GitHub Check: enterprise-startup-functional-test (3.12)
- GitHub Check: build (3.12)
- GitHub Check: enterprise-functional-tests (3.12, chromium)
- GitHub Check: test (3.12)
- GitHub Check: enterprise-startup-docker-compose-test
🔇 Additional comments (34)
frontend/src/routes/(app)/(internal)/my-assignments/+page.svelte (1)
9-9: LGTM: Import path updated for shared componentThe import path for ActivityTracker has been updated to use the shared component from the library path, which is a good practice for component reuse across the application.
backend/ciso_assistant/settings.py (1)
204-204: LGTM: Resilience app correctly added to INSTALLED_APPSThe addition of the "resilience" app to the INSTALLED_APPS setting is required for Django to recognize the new module and include its models, URLs, and migrations.
enterprise/backend/enterprise_core/settings.py (1)
202-202: LGTM: Resilience app correctly added to enterprise INSTALLED_APPSAdding the "resilience" app to the enterprise INSTALLED_APPS ensures consistency between the community and enterprise versions of the application, making the new Business Impact Analysis features available in both.
backend/core/urls.py (1)
108-108: Good integration of the resilience module!The path for including the resilience URLs is properly integrated alongside other modules in the application.
backend/resilience/apps.py (1)
1-6: Standard Django app configuration looks good.This follows the Django convention for app configuration with appropriate settings for the resilience module.
backend/core/startup.py (1)
583-595: Comprehensive permission setup for resilience models.The permissions for the new resilience models (BusinessImpactAnalysis, EscalationThreshold, and AssetAssessment) are properly defined with full CRUD operations and follow the established pattern in the codebase.
enterprise/frontend/src/lib/components/SideBar/navData.ts (1)
146-161: Well-structured navigation for asset management features.The new 'assetsManagement' section logically groups the assets and business impact analysis functionality together. The navigation item for business impact analysis correctly requires the 'view_businessimpactanalysis' permission.
Have you considered adding entries for the other resilience models (AssetAssessment, EscalationThreshold) that were added to the backend? These don't appear to have corresponding navigation items yet.
frontend/src/lib/components/SideBar/navData.ts (2)
104-119: Good reorganization of the assets sectionThe assets management has been appropriately moved to its own top-level section, which better organizes the navigation structure. The added business impact analysis item follows the established pattern and correctly requires the corresponding permission.
318-323: DataWizard addition matches experimental feature patternThis new navigation item follows the established pattern for experimental features by requiring the same permissions as other experimental items and using appropriate icons.
frontend/src/lib/utils/types.ts (1)
92-95: LGTM: Appropriate URL model additions for the new resilience module.The new URL model entries for 'business-impact-analysis', 'escalation-thresholds', and 'asset-assessments' are properly added to the URL_MODEL constant array with appropriate categorization comments, following the existing pattern in the file.
frontend/src/routes/(app)/(internal)/business-impact-analysis/[id=uuid]/action-plan/+page.server.ts (1)
6-7: Verify that 'findings-assessments' is the correct URLModel for business impact analysis action plans.The route path is for business-impact-analysis, but the endpoint uses 'findings-assessments'. While this appears consistent with similar implementations in the codebase, please confirm this relationship is intentional.
I see from similar files that 'findings-assessments' is used as the data source for action plans. Please verify this is the correct data source for business impact analysis action plans too.
frontend/src/lib/utils/load.ts (1)
100-103: LGTM: Improved URL construction for nested select fields.The enhanced URL construction logic correctly handles nested endpoint paths for select fields when
formNestedFieldanddetailflags are set to true. This change appropriately supports the new resilience models that require nested API routes.backend/resilience/urls.py (1)
1-27: LGTM: Well-structured URL routing for resilience endpoints.The URL configuration follows Django REST Framework best practices by:
- Using a DefaultRouter for standardized RESTful endpoints
- Properly registering each viewset with appropriate basename
- Including the router URLs in the urlpatterns list
The URL patterns match the segments added to URL_MODEL in the frontend, ensuring consistency across the stack.
frontend/src/lib/components/Forms/Duration.svelte (3)
23-24: Good enhancement to make time units configurable.The addition of an
enabledUnitsprop allows greater flexibility by letting parent components specify which time units to display, rather than using hardcoded values.
36-43: Well-structured time units array with all possible options.The comprehensive list of time units with their conversion to seconds is clear and maintainable. Each unit includes all necessary properties correctly initialized.
45-49: Good implementation of dynamic unit filtering.The approach to filter time units based on the
enabledUnitsprop is clean and efficient, using a map operation to preserve all properties while only toggling theenabledflag.frontend/src/lib/components/DataViz/ActivityTracker.svelte (4)
12-36: Improved flexibility with dynamic gauge data generation.The dynamic creation of gauge data from
metrics.progressis a significant improvement that allows the component to handle any number of metrics rather than using hardcoded values. The vertical positioning calculations are well thought out.
38-40: Well-implemented dynamic color generation.The approach to generate colors based on the number of entries ensures visual consistency regardless of how many metrics are displayed.
104-104: Minor formatting improvement.The cleanup of the container div's class attribute improves code readability.
4-4:✅ Verification successful
Rename of 'act_tracker' to 'metrics_tracker' might impact existing references.
The default value change to this exported parameter could potentially break existing code that relies on the previous default name.
Let's verify if there are any existing references to the old name:
🏁 Script executed:
#!/bin/bash # Search for references to 'act_tracker' which might be affected by the rename rg "'act_tracker'" --type svelteLength of output: 71
Let's broaden the search to all relevant file types:
🏁 Script executed:
#!/bin/bash # Search for references to 'act_tracker' across .svelte, .js, and .ts files rg "act_tracker" -g "*.svelte" -g "*.js" -g "*.ts" || trueLength of output: 57
No references to ‘act_tracker’ found – rename is safe
I searched the entire codebase for any occurrences of the old default value and found none. Updating the exported
nameprop from"act_tracker"to"metrics_tracker"won’t break existing code.frontend/src/routes/(app)/(internal)/asset-assessments/[id=uuid]/+page.svelte (2)
1-7: Well-structured imports for the new asset assessment detail page.The imports are clean and focused, bringing in only what's needed for this view.
34-34:✅ Verification successful
Verify the data structure for LineHeatmap component.
Make sure the
aaMetricsproperty exists and is in the correct format for the LineHeatmap component. This would help prevent runtime errors if the data structure changes.Run the following script to check how the LineHeatmap component expects data and if
aaMetricsmatches that format:
🏁 Script executed:
#!/bin/bash # Check the LineHeatmap component props rg -A 10 "export let" frontend/src/lib/components/DataViz/LineHeatmap.svelte # Check if aaMetrics is used in other places rg "aaMetrics" --type tsLength of output: 544
LineHeatmap data prop verified
The
aaMetricsvalue is defined in+page.server.tsand passed as thedataprop toLineHeatmap. The component’s implementation (export let data;) iterates over entries expecting each to have:
impact.hexcolorimpact.namepitNo mismatches were found.
Suggestion:
- Define a TypeScript interface for the shape of
metricsData(i.e. the items inaaMetrics) to enforce these fields and catch future changes at compile time.frontend/src/routes/(app)/(internal)/business-impact-analysis/[id=uuid]/+page.svelte (1)
1-28: Solid implementation of the Business Impact Analysis detail view.The component is well-structured with a clean separation of concerns. It properly utilizes the DetailView component with action and widget slots. The ActivityTracker integration for displaying recovery insights is a nice addition.
frontend/src/lib/utils/crud.ts (1)
115-116: Good enhancement to the SelectField interface.The addition of optional
endpointUrlandformNestedFieldproperties extends the interface to support more complex use cases, particularly for nested form data and custom endpoints. This change aligns well with the needs of the new resilience module.frontend/src/lib/components/Forms/ModelForm.svelte (2)
48-50: Correctly imported new form components.The imports for the new form components (BusinessImpactAnalysisForm, AssetAssessmentForm, and EscalationThresholdForm) are properly added.
327-342: New form component conditionals added properly.The conditional rendering for the new form components follows the established pattern in the file. All necessary props are passed to each component, ensuring proper integration with the form handling mechanism.
frontend/src/routes/(app)/(internal)/asset-assessments/[id=uuid]/action-plan/+page.svelte (1)
1-40: Well-structured table configuration for action plan.The script section correctly sets up the necessary variables and table configuration for displaying applied controls related to an asset assessment. The table columns are comprehensive and well-organized.
frontend/src/lib/components/Forms/ModelForm/BusinessImpactAnalysisForm.svelte (1)
47-57: Good practice: Disabling risk matrix changes for existing objects.Preventing changes to the risk matrix for existing objects is a good practice as it ensures consistent risk evaluation throughout the BIA lifecycle.
frontend/src/lib/utils/schemas.ts (1)
439-450: Good schema implementation for BusinessImpactAnalysis.The BusinessImpactAnalysisSchema properly inherits from NameDescriptionMixin and includes all necessary fields with appropriate validation rules. The schema aligns well with the form component and expected backend model.
backend/resilience/migrations/0001_initial.py (3)
197-198: Good design: Enforcing unique BIA-asset pairs.The unique_together constraint ensures that an asset can only be assessed once within a specific BIA, preventing duplicate assessments and data inconsistency.
261-262: Good design: Ensuring unique point-in-time thresholds.The unique_together constraint for (asset_assessment, point_in_time) prevents duplicate threshold definitions at the same time point, maintaining data integrity.
134-199: 💡 Verification agent🧩 Analysis chain
Consider adding name/description fields to AssetAssessment.
Unlike BusinessImpactAnalysis, the AssetAssessment model doesn't inherit from NameDescriptionMixin or include name/description fields. While the model is identified by its BIA and asset relationship, adding these fields would provide better human-readable identification and align with frontend expectations.
🏁 Script executed:
#!/bin/bash # Check if AssetAssessment models have name fields in similar models rg "class.+Assessment" backend/ | grep -v "AssetAssessment"Length of output: 3659
🏁 Script executed:
#!/bin/bash # Locate the AssetAssessment model definition rg "class AssetAssessment" -n -A15 backend/resilience/models.pyLength of output: 815
🏁 Script executed:
#!/bin/bash # Inspect the BusinessImpactAnalysis model definition rg "class BusinessImpactAnalysis" -n -A15 backend/resilience/models.pyLength of output: 666
🏁 Script executed:
#!/bin/bash # Locate AbstractBaseModel definition rg "class AbstractBaseModel" -n -A20 backendLength of output: 1439
🏁 Script executed:
#!/bin/bash # Inspect NameDescriptionMixin implementation for name/description fields rg "class NameDescriptionMixin" -n -A15 backend/core/models.pyLength of output: 64
🏁 Script executed:
#!/bin/bash # Search for NameDescriptionMixin definition across the repo rg "class NameDescriptionMixin" -n -A15 . # Locate AssetAssessment serializers to see which fields are exposed rg "AssetAssessment" -n -A5 backend/resilience/serializers.pyLength of output: 1730
Add Name/Description fields to AssetAssessment
AssetAssessment currently doesn’t include the human-readable
name/descriptionfields provided byNameDescriptionMixin, unlike other assessment models. To align with the rest of the codebase and frontend expectations:• In backend/resilience/models.py:
- Import the mixin:
from core.base_models import NameDescriptionMixin- Update the class signature:
- class AssetAssessment(AbstractBaseModel, FolderMixin): + class AssetAssessment(NameDescriptionMixin, AbstractBaseModel, FolderMixin):• In backend/resilience/serializers.py:
- Expose the new fields in the read serializer:
class AssetAssessmentReadSerializer(BaseModelSerializer): str = serializers.CharField(source="__str__") + name = serializers.CharField() + description = serializers.CharField(allow_blank=True, allow_null=True) bia = FieldsRelatedField() asset = FieldsRelatedField() class Meta: model = AssetAssessment exclude = []
- Ensure the write serializer includes these fields (it already uses
fields="__all__").
• Optionally remove or adjust the existing__str__to default to the newnamefield.Likely an incorrect or invalid review comment.
backend/resilience/views.py (1)
35-38: Good practice: Caching status choices.Using the cache_page decorator on the status endpoint is appropriate for data that rarely changes, improving API performance.
backend/resilience/models.py (1)
90-98: Default entry only covers t=0 – later gaps show as “--”, not the last known impactIf an asset has thresholds
[t=60 → MED], the range[1 … 59]will be back-filled with “--”.
Typical heat-map expectations are that the first threshold applies retro-actively untilt=0.Either:
- insert the first threshold’s impact for
t=0, or- explicitly document the current behaviour so consumers can handle the “unknown before first threshold” case.
| export const load = (async ({ fetch, params }) => { | ||
| const endpoint = `${BASE_API_URL}/resilience/business-impact-analysis/${params.id}/build-table/`; | ||
| const res = await fetch(endpoint); | ||
| const data = await res.json(); | ||
|
|
||
| return { data }; | ||
| }) satisfies PageServerLoad; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling for the fetch request
The current implementation doesn't handle potential API request failures. If the endpoint returns an error status or the network request fails, this could result in uncaught exceptions.
Consider implementing proper error handling:
export const load = (async ({ fetch, params }) => {
const endpoint = `${BASE_API_URL}/resilience/business-impact-analysis/${params.id}/build-table/`;
- const res = await fetch(endpoint);
- const data = await res.json();
+ try {
+ const res = await fetch(endpoint);
+ if (!res.ok) {
+ throw new Error(`API request failed with status ${res.status}`);
+ }
+ const data = await res.json();
+ return { data };
+ } catch (error) {
+ console.error('Error fetching business impact analysis data:', error);
+ return { data: null, error: 'Failed to load data' };
+ }
- return { data };
}) satisfies PageServerLoad;📝 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.
| export const load = (async ({ fetch, params }) => { | |
| const endpoint = `${BASE_API_URL}/resilience/business-impact-analysis/${params.id}/build-table/`; | |
| const res = await fetch(endpoint); | |
| const data = await res.json(); | |
| return { data }; | |
| }) satisfies PageServerLoad; | |
| export const load = (async ({ fetch, params }) => { | |
| const endpoint = `${BASE_API_URL}/resilience/business-impact-analysis/${params.id}/build-table/`; | |
| try { | |
| const res = await fetch(endpoint); | |
| if (!res.ok) { | |
| throw new Error(`API request failed with status ${res.status}`); | |
| } | |
| const data = await res.json(); | |
| return { data }; | |
| } catch (error) { | |
| console.error('Error fetching business impact analysis data:', error); | |
| return { data: null, error: 'Failed to load data' }; | |
| } | |
| }) satisfies PageServerLoad; |
| <script lang="ts"> | ||
| export let data; | ||
| import { safeTranslate } from '$lib/utils/i18n'; | ||
| </script> |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add TypeScript interface for the data prop
The component accepts a data prop without type information, which makes it difficult to understand what structure is expected.
Define an interface for the expected data structure:
<script lang="ts">
- export let data;
+ interface ImpactEntry {
+ impact: {
+ name: string;
+ hexcolor: string;
+ };
+ pit: string | number;
+ }
+
+ export let data: ImpactEntry[];
import { safeTranslate } from '$lib/utils/i18n';
</script>📝 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.
| <script lang="ts"> | |
| export let data; | |
| import { safeTranslate } from '$lib/utils/i18n'; | |
| </script> | |
| <script lang="ts"> | |
| interface ImpactEntry { | |
| impact: { | |
| name: string; | |
| hexcolor: string; | |
| }; | |
| pit: string | number; | |
| } | |
| export let data: ImpactEntry[]; | |
| import { safeTranslate } from '$lib/utils/i18n'; | |
| </script> |
| export const load = (async ({ fetch, params }) => { | ||
| const URLModel = 'findings-assessments'; | ||
| const endpoint = `${BASE_API_URL}/${URLModel}/${params.id}/`; | ||
|
|
||
| const res = await fetch(endpoint); | ||
| const findings_assessment = await res.json(); | ||
| return { URLModel, findings_assessment }; | ||
| }) satisfies PageServerLoad; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling for the fetch request
Similar to the business impact analysis page, this code doesn't handle potential API failures.
Implement error handling for the fetch operation:
export const load = (async ({ fetch, params }) => {
const URLModel = 'findings-assessments';
const endpoint = `${BASE_API_URL}/${URLModel}/${params.id}/`;
+ try {
const res = await fetch(endpoint);
+ if (!res.ok) {
+ throw new Error(`API request failed with status ${res.status}`);
+ }
const findings_assessment = await res.json();
return { URLModel, findings_assessment };
+ } catch (error) {
+ console.error('Error fetching findings assessment data:', error);
+ return { URLModel, findings_assessment: null, error: 'Failed to load data' };
+ }
}) satisfies PageServerLoad;📝 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.
| export const load = (async ({ fetch, params }) => { | |
| const URLModel = 'findings-assessments'; | |
| const endpoint = `${BASE_API_URL}/${URLModel}/${params.id}/`; | |
| const res = await fetch(endpoint); | |
| const findings_assessment = await res.json(); | |
| return { URLModel, findings_assessment }; | |
| }) satisfies PageServerLoad; | |
| export const load = (async ({ fetch, params }) => { | |
| const URLModel = 'findings-assessments'; | |
| const endpoint = `${BASE_API_URL}/${URLModel}/${params.id}/`; | |
| try { | |
| const res = await fetch(endpoint); | |
| if (!res.ok) { | |
| throw new Error(`API request failed with status ${res.status}`); | |
| } | |
| const findings_assessment = await res.json(); | |
| return { URLModel, findings_assessment }; | |
| } catch (error) { | |
| console.error('Error fetching findings assessment data:', error); | |
| return { URLModel, findings_assessment: null, error: 'Failed to load data' }; | |
| } | |
| }) satisfies PageServerLoad; |
| const URLModel = 'findings-assessments'; | ||
| const endpoint = `${BASE_API_URL}/${URLModel}/${params.id}/`; |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Clarify the use of 'findings-assessments' in the asset-assessments route
There's a potential mismatch between the route path (asset-assessments) and the API endpoint (findings-assessments). This might be confusing for developers maintaining the code.
🏁 Script executed:
#!/bin/bash
# Description: Check the relationship between asset-assessments and findings-assessments
# Look for API definitions or routes that might clarify the relationship
rg -A 3 "findings-assessments" --type ts
# Check if there are any comments or documentation about this relationship
rg -A 3 "asset.*assessment.*findings" --type tsLength of output: 4572
🏁 Script executed:
#!/bin/bash
# Inspect the asset-assessments action-plan loader to understand context
cat 'frontend/src/routes/(app)/(internal)/asset-assessments/[id=uuid]/action-plan/+page.server.ts' | sed -n '1,200p'Length of output: 532
Document ‘findings-assessments’ mapping in asset-assessments route
The asset-assessments action-plan loader intentionally reuses the generic findings-assessments API model (the same endpoint used in business-impact-analysis and the core findings-assessments routes). To avoid confusion, please add a brief comment or refactor the naming for clarity.
• File: frontend/src/routes/(app)/(internal)/asset-assessments/[id=uuid]/action-plan/+page.server.ts
– Add a comment explaining why URLModel is set to 'findings-assessments'.
Suggested diff:
-export const load = (async ({ fetch, params }) => {
- const URLModel = 'findings-assessments';
+export const load = (async ({ fetch, params }) => {
+ // Asset assessments are backed by the shared “findings-assessments” API model
+ const URLModel = 'findings-assessments';📝 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.
| const URLModel = 'findings-assessments'; | |
| const endpoint = `${BASE_API_URL}/${URLModel}/${params.id}/`; | |
| export const load = (async ({ fetch, params }) => { | |
| // Asset assessments are backed by the shared “findings-assessments” API model | |
| const URLModel = 'findings-assessments'; | |
| const endpoint = `${BASE_API_URL}/${URLModel}/${params.id}/`; | |
| // …rest of loader logic… | |
| }); |
| def format_impact(impact: int, parsed_matrix: dict): | ||
| if impact < 0: | ||
| return { | ||
| "abbreviation": "--", | ||
| "name": "--", | ||
| "description": "not rated", | ||
| "value": -1, | ||
| "hexcolor": "#f9fafb", | ||
| } | ||
| risk_matrix = parsed_matrix | ||
| if not risk_matrix["impact"][impact].get("hexcolor"): | ||
| risk_matrix["impact"][impact]["hexcolor"] = "#f9fafb" | ||
| return { | ||
| **risk_matrix["impact"][impact], | ||
| "value": impact, | ||
| } | ||
|
|
There was a problem hiding this comment.
Guard against out-of-range indices in format_impact
risk_matrix["impact"][impact] will raise IndexError / KeyError if impact exceeds the configured scale.
Consider a safer fetch:
try:
impact_meta = risk_matrix["impact"][impact]
except (IndexError, KeyError):
return EscalationThreshold.format_impact(-1, risk_matrix)Without this, a malformed value could break the entire API response.
backend/resilience/models.py
Outdated
| def build_table(self): | ||
| table = list() | ||
| xAxis = set() | ||
| xAxis.add(0) | ||
| asset_assessments = AssetAssessment.objects.filter(bia=self).prefetch_related() | ||
|
|
||
| # First pass: collect all threshold points | ||
| for aa in asset_assessments: | ||
| thresholds = EscalationThreshold.objects.filter( | ||
| asset_assessment=aa | ||
| ).order_by("point_in_time") | ||
| for th in thresholds: | ||
| xAxis.add(th.point_in_time) | ||
|
|
||
| # Sort the x-axis values for consistent timeline | ||
| xAxis = sorted(xAxis) | ||
|
|
||
| # Second pass: build the table with proper padding | ||
| for aa in asset_assessments: | ||
| thresholds = list( | ||
| EscalationThreshold.objects.filter(asset_assessment=aa).order_by( | ||
| "point_in_time" | ||
| ) | ||
| ) | ||
|
|
||
| data_dict = {} | ||
|
|
||
| # Default for time 0 if no threshold exists | ||
| if not thresholds or thresholds[0].point_in_time > 0: | ||
| data_dict[0] = {"value": -1, "name": "--", "hexcolor": "#f9fafb"} | ||
|
|
||
| # For each threshold, extend its impact to the next threshold | ||
| for i, threshold in enumerate(thresholds): | ||
| current_impact = threshold.get_impact_compact_display | ||
| current_time = threshold.point_in_time | ||
|
|
||
| # Find the next time point for this asset | ||
| next_time = None | ||
| if i < len(thresholds) - 1: | ||
| next_time = thresholds[i + 1].point_in_time | ||
|
|
||
| # Apply the current impact to all points between current and next | ||
| for point in xAxis: | ||
| if point == current_time or ( | ||
| point > current_time | ||
| and (next_time is None or point < next_time) | ||
| ): | ||
| data_dict[point] = current_impact | ||
|
|
||
| # Add entry to table | ||
| table.append({"asset": aa.asset.name, "data": data_dict}) | ||
|
|
||
| for entry in table: | ||
| for point in xAxis: | ||
| if point not in entry["data"]: | ||
| entry["data"][point] = { | ||
| "value": -1, | ||
| "name": "--", | ||
| "hexcolor": "#f9fafb", | ||
| } | ||
|
|
||
| return table |
There was a problem hiding this comment.
🛠️ Refactor suggestion
build_table suffers from N+1 queries and quadratic loops
- For each asset you run an extra
SELECTfor thresholds (Nadditional hits). - Inside the loop you iterate over the full
xAxison every threshold, producing O(A * T * X) complexity (can explode for large data sets).
Consider:
- Prefetch thresholds in one go:
asset_assessments = (
AssetAssessment.objects.filter(bia=self)
.prefetch_related(
models.Prefetch(
"escalationthreshold_set",
queryset=EscalationThreshold.objects.order_by("point_in_time"),
to_attr="prefetched_thresholds",
)
)
)- Build
xAxisand the table in a single pass using thoseprefetched_thresholds.
These changes collapse DB access to 2 queries and bring the algorithm down to roughly O(A + X).
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
backend/resilience/models.py (2)
210-227: Guard against out-of-range indices informat_impact
risk_matrix["impact"][impact]will raiseIndexError/KeyErrorifimpactexceeds the configured scale.
Consider a safer fetch:@staticmethod def format_impact(impact: int, parsed_matrix: dict): if impact < 0: return { "abbreviation": "--", "name": "--", "description": "not rated", "value": -1, "hexcolor": "#f9fafb", } risk_matrix = parsed_matrix + try: if not risk_matrix["impact"][impact].get("hexcolor"): risk_matrix["impact"][impact]["hexcolor"] = "#f9fafb" return { **risk_matrix["impact"][impact], "value": impact, } + except (IndexError, KeyError): + return { + "abbreviation": "ERR", + "name": "Error", + "description": "invalid impact value", + "value": -1, + "hexcolor": "#ff0000", + }
70-95: 🛠️ Refactor suggestionPrefetched thresholds aren't being used correctly
You're using
prefetch_relatedto load thresholds in advance (lines 70-76), but then making new database queries in lines 80-82 and 91-95 instead of using the prefetched data.Replace the queries with the prefetched data to avoid N+1 query issues:
# First pass: collect all threshold points for aa in asset_assessments: - thresholds = EscalationThreshold.objects.filter( - asset_assessment=aa - ).order_by("point_in_time") + thresholds = aa.prefetched_thresholds for th in thresholds: xAxis.add(th.point_in_time) # Second pass: build the table with proper padding for aa in asset_assessments: - thresholds = list( - EscalationThreshold.objects.filter(asset_assessment=aa).order_by( - "point_in_time" - ) - ) + thresholds = aa.prefetched_thresholds
🧹 Nitpick comments (1)
backend/resilience/models.py (1)
163-177: Consider using Django's duration fieldFor the point_in_time field that represents a time duration in seconds, Django provides a specialized DurationField which might be more appropriate.
-point_in_time = IntegerField() # seconds and manage the display and units on front +from datetime import timedelta +point_in_time = models.DurationField(help_text="Duration since the incident started")This would allow more natural handling of durations in queries and would automatically handle conversion to/from Python's timedelta objects.
If you keep the current implementation, consider adding a help_text attribute to clarify the unit of measurement.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
backend/resilience/models.py(1 hunks)frontend/messages/en.json(2 hunks)frontend/messages/fr.json(1 hunks)frontend/src/lib/components/DataViz/LineHeatmap.svelte(1 hunks)frontend/src/routes/(app)/(internal)/business-impact-analysis/[id=uuid]/visual/+page.svelte(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- frontend/messages/fr.json
🚧 Files skipped from review as they are similar to previous changes (3)
- frontend/src/lib/components/DataViz/LineHeatmap.svelte
- frontend/messages/en.json
- frontend/src/routes/(app)/(internal)/business-impact-analysis/[id=uuid]/visual/+page.svelte
🧰 Additional context used
🧬 Code Graph Analysis (1)
backend/resilience/models.py (2)
backend/core/models.py (5)
AppliedControl(1990-2227)Asset(1495-1798)Assessment(2328-2380)Evidence(1801-1862)parse_json_translated(945-946)backend/iam/models.py (1)
FolderMixin(232-245)
⏰ 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-startup-functional-test (3.12)
- GitHub Check: functional-tests (3.12, chromium)
- GitHub Check: test (3.12)
- GitHub Check: enterprise-startup-docker-compose-test
- GitHub Check: enterprise-functional-tests (3.12, chromium)
- GitHub Check: build (3.12)
- GitHub Check: Analyze (python)
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (3)
backend/resilience/models.py (3)
34-64: Well optimized metrics aggregationNice work on the metrics method! Using Django's aggregate with Q objects to calculate all counts in a single database query is an efficient approach.
30-32: Good use of cached_propertyUsing cached_property for parsed_matrix is a good optimization that avoids repeated parsing of the risk matrix JSON during the request lifecycle.
193-208: Good human-readable time formattingThe get_human_pit property effectively converts seconds into a well-formatted, human-readable string with appropriate pluralization.
| def metrics(self): | ||
| thresholds = EscalationThreshold.objects.filter(asset_assessment=self) | ||
| res = [ | ||
| {"pit": et.get_human_pit, "impact": et.get_impact_display} | ||
| for et in thresholds | ||
| ] | ||
| return res |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid N+1 query pattern in AssetAssessment.metrics()
This method queries thresholds for each asset assessment individually, which leads to a performance issue when dealing with many records.
Use prefetch_related to efficiently load all thresholds in a single query:
def metrics(self):
- thresholds = EscalationThreshold.objects.filter(asset_assessment=self)
- res = [
- {"pit": et.get_human_pit, "impact": et.get_impact_display}
- for et in thresholds
- ]
+ # If calling this method on a queryset, ensure objects are prefetched:
+ # AssetAssessment.objects.prefetch_related('escalationthreshold_set')
+ res = [
+ {"pit": et.get_human_pit, "impact": et.get_impact_display}
+ for et in self.escalationthreshold_set.all().order_by('point_in_time')
+ ]
return res📝 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.
| def metrics(self): | |
| thresholds = EscalationThreshold.objects.filter(asset_assessment=self) | |
| res = [ | |
| {"pit": et.get_human_pit, "impact": et.get_impact_display} | |
| for et in thresholds | |
| ] | |
| return res | |
| def metrics(self): | |
| # If calling this method on a queryset, ensure objects are prefetched: | |
| # AssetAssessment.objects.prefetch_related('escalationthreshold_set') | |
| res = [ | |
| {"pit": et.get_human_pit, "impact": et.get_impact_display} | |
| for et in self.escalationthreshold_set.all().order_by('point_in_time') | |
| ] | |
| return res |
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
frontend/messages/en.json(2 hunks)frontend/src/lib/utils/crud.ts(2 hunks)frontend/src/lib/utils/table.ts(1 hunks)frontend/tests/functional/user-route.test.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- frontend/src/lib/utils/table.ts
- frontend/src/lib/utils/crud.ts
🧰 Additional context used
🪛 GitHub Actions: Frontend Linters
frontend/messages/en.json
[error] 1516-1516: Prettier formatting check failed due to JSON syntax error: Unexpected token, expected "," at line 1516.
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: test (3.12)
- GitHub Check: build (3.12)
- GitHub Check: Analyze (python)
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (1)
frontend/tests/functional/user-route.test.ts (1)
68-68: Navigation label correctly updated to reflect UI restructuringThe sidebar navigation label has been appropriately updated from "Organization" to "Assets management" when navigating to the assets page. This change aligns with the navigation restructuring described in the AI summary, where assets were moved from the "organization" section to a new top-level "assetsManagement" section that includes Business Impact Analysis.
The change correctly maintains the existing URL and title assertions, ensuring the test continues to work with the updated navigation structure.
frontend/messages/en.json
Outdated
| "dataWizard": "Data Wizard", | ||
| "bia": "BIA", | ||
| "businessImpactAnalysis": "Business Impact Analysis", | ||
| "escalationThreshold": "Escalation Threshold", | ||
| "assetAssessment": "Asset Assessment", | ||
| "assetAssessments": "BIA assessments", | ||
| "associatedAssetAssessments": "Assets in scope", | ||
| "addAssetAssessment": "Include asset", | ||
| "pointInTime": "Point In time", | ||
| "quantifiedImpact": "Quantified Impact", | ||
| "qualiImpact": "Qualitative Impact", | ||
| "quantUnit": "Unit", | ||
| "rationale": "Rationale", | ||
| "escalationThresholds": "BIA Thresholds", | ||
| "associatedEscalationThresholds": "Steps", | ||
| "addEscalationThreshold": "Add a step", | ||
| "recoveryDocumented": "Recovery documented", | ||
| "recoveryTested": "Recovery tested", | ||
| "recoveryTargetsMet": "Recovery targets met", | ||
| "recoveryInsights": "Recovery insights", | ||
| "impactOverTime": "Impact over time", | ||
| "assetsManagement": "Assets management", | ||
| "time": "time", |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Ensure consistent naming, casing, and phrasing for new BIA localization keys
Several of the newly added entries deviate from established conventions in this file:
- Key
qualiImpactshould be fully spelled out asqualitativeImpactto mirror the existingquantifiedImpact. - Values use title case elsewhere; please update
"Point In time"→"Point in time"and"time"→"Time". - The mapping
"assetAssessments": "BIA assessments"is ambiguous—either use"Asset assessments"or rename the key to something likebiaAssetAssessmentsif it specifically refers to BIA. - For thresholds, keep terminology consistent:
escalationThresholdscurrently maps to"BIA Thresholds". Consider renaming the key tobiaThresholdsor the value to"Escalation thresholds".
Aligning keys and values with existing patterns will improve maintainability and user-facing consistency.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
backend/resilience/models.py (3)
152-158: Avoid N+1 query pattern in AssetAssessment.metrics()This method queries thresholds for each asset assessment individually, which leads to a performance issue when dealing with many records.
Use prefetch_related to efficiently load all thresholds in a single query:
def metrics(self): - thresholds = EscalationThreshold.objects.filter(asset_assessment=self) - res = [ - {"pit": et.get_human_pit, "impact": et.get_impact_display} - for et in thresholds - ] + # If calling this method on a queryset, ensure objects are prefetched: + # AssetAssessment.objects.prefetch_related('escalationthreshold_set') + res = [ + {"pit": et.get_human_pit, "impact": et.get_impact_display} + for et in self.escalationthreshold_set.all().order_by('point_in_time') + ] return res
215-232: Guard against out-of-range indices informat_impact
risk_matrix["impact"][impact]will raiseIndexError/KeyErrorifimpactexceeds the configured scale.
Consider a safer fetch:try: impact_meta = risk_matrix["impact"][impact] except (IndexError, KeyError): return EscalationThreshold.format_impact(-1, risk_matrix)Without this, a malformed value could break the entire API response.
67-96:⚠️ Potential issueN+1 query issue in
build_table()methodLines 81-83 execute a separate database query inside a loop, negating the benefits of the prefetch_related you've already implemented in lines 71-77. You should use the prefetched data instead.
# First pass: collect all threshold points for aa in asset_assessments: - thresholds = EscalationThreshold.objects.filter( - asset_assessment=aa - ).order_by("point_in_time") + thresholds = aa.prefetched_thresholds for th in thresholds: xAxis.add(th.point_in_time)And similarly for the second pass:
# Second pass: build the table with proper padding for aa in asset_assessments: - thresholds = list( - EscalationThreshold.objects.filter(asset_assessment=aa).order_by( - "point_in_time" - ) - ) + thresholds = aa.prefetched_thresholds
🧹 Nitpick comments (5)
backend/resilience/models.py (5)
204-213: Improve handling of zero time components inget_human_pitWhen all time components (days, hours, minutes) are zero, the current logic will return "0 Minutes". Consider making this logic more explicit to improve readability.
parts = [] if days: parts.append(f"{days} {'Day' if days == 1 else 'Days'}") if hours: parts.append(f"{hours} {'Hour' if hours == 1 else 'Hours'}") if minutes: parts.append(f"{minutes} {'Minute' if minutes == 1 else 'Minutes'}") -return ", ".join(parts) or "0 Minutes" +# More explicit handling of the zero case +if parts: + return ", ".join(parts) +else: + return "0 Minutes"
198-213: Consider adding a method to convert human-readable time back to secondsThe model currently provides a method to convert seconds to a human-readable format, but lacks a corresponding method to parse a human-readable string back to seconds. This would be useful for API interactions.
@staticmethod def parse_human_pit(human_pit_str): """ Convert a human-readable point in time string back to seconds. Example: "2 Days, 3 Hours, 30 Minutes" -> 189000 """ total_seconds = 0 parts = human_pit_str.split(", ") for part in parts: value, unit = part.split(" ", 1) value = int(value) if unit.startswith("Day"): total_seconds += value * 86400 elif unit.startswith("Hour"): total_seconds += value * 3600 elif unit.startswith("Minute"): total_seconds += value * 60 return total_seconds
47-55: Add validation or safety check for percentage calculationsWhile you correctly handle division by zero, it's a good practice to validate that your percentages aren't exceeding 100% due to data inconsistencies.
doc_percentage = ( - round((documented_count / total_assets) * 100) if total_assets else 0 + min(100, round((documented_count / total_assets) * 100)) if total_assets else 0 ) test_percentage = ( - round((tested_count / total_assets) * 100) if total_assets else 0 + min(100, round((tested_count / total_assets) * 100)) if total_assets else 0 ) obj_percentage = ( - round((objectives_met_count / total_assets) * 100) if total_assets else 0 + min(100, round((objectives_met_count / total_assets) * 100)) if total_assets else 0 )
25-34: Consider adding string representation for BusinessImpactAnalysisWhile
AssetAssessmenthas a__str__method,BusinessImpactAnalysisdoesn't, which might make it harder to identify objects in the admin interface or when debugging.class BusinessImpactAnalysis(Assessment): risk_matrix = models.ForeignKey( RiskMatrix, on_delete=models.PROTECT, ) + def __str__(self) -> str: + return f"BIA: {self.name}" + @cached_property def parsed_matrix(self): return self.risk_matrix.parse_json_translated()
138-148: Userelated_nameconsistently across all relationshipsYou've used
related_namefordependenciesbut not for other relationships, which can make reverse lookups inconsistent and confusing.class AssetAssessment(AbstractBaseModel, FolderMixin): - asset = ForeignKey(Asset, on_delete=models.CASCADE) + asset = ForeignKey(Asset, on_delete=models.CASCADE, related_name='asset_assessments') dependencies = ManyToManyField(Asset, related_name="dependencies", blank=True) - associated_controls = ManyToManyField(AppliedControl, blank=True) + associated_controls = ManyToManyField(AppliedControl, related_name='asset_assessments', blank=True) recovery_documented = BooleanField(default=False) recovery_tested = BooleanField(default=False) recovery_targets_met = BooleanField(default=False) - evidences = ManyToManyField(Evidence, blank=True) + evidences = ManyToManyField(Evidence, related_name='asset_assessments', blank=True) observation = TextField(null=True, blank=True) - bia = ForeignKey(BusinessImpactAnalysis, on_delete=models.CASCADE) + bia = ForeignKey(BusinessImpactAnalysis, on_delete=models.CASCADE, related_name='asset_assessments')
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
backend/resilience/migrations/0001_initial.py(1 hunks)backend/resilience/models.py(1 hunks)frontend/tests/functional/user-route.test.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- frontend/tests/functional/user-route.test.ts
- backend/resilience/migrations/0001_initial.py
⏰ Context from checks skipped due to timeout of 90000ms (10)
- GitHub Check: startup-functional-test (3.12)
- GitHub Check: enterprise-startup-functional-test (3.12)
- GitHub Check: functional-tests (3.12, chromium)
- GitHub Check: startup-docker-compose-test
- GitHub Check: enterprise-startup-docker-compose-test
- GitHub Check: build (3.12)
- GitHub Check: test (3.12)
- GitHub Check: enterprise-functional-tests (3.12, chromium)
- GitHub Check: Analyze (python)
- GitHub Check: Analyze (javascript-typescript)
Summary by CodeRabbit
New Features
Chores
Tests