-
Notifications
You must be signed in to change notification settings - Fork 287
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: extra settings to enable mail notifications and matrix aggregation #1434
Conversation
WalkthroughThis pull request introduces new configuration settings for notifications and interface preferences across the application. The changes span multiple files in the backend and frontend, adding two new keys to the general settings: Changes
Sequence DiagramsequenceDiagram
participant User
participant Frontend
participant Backend
participant GlobalSettings
User->>Frontend: Access Settings Page
Frontend->>Backend: Fetch General Settings
Backend->>GlobalSettings: Retrieve Settings
GlobalSettings-->>Backend: Return Settings
Backend-->>Frontend: Provide Settings
Frontend->>User: Display Settings with Checkboxes
User->>Frontend: Toggle Notification/Interface Settings
Frontend->>Backend: Update Settings
Backend->>GlobalSettings: Save New Settings
GlobalSettings-->>Backend: Confirm Update
Backend-->>Frontend: Confirm Changes
Frontend->>User: Show Updated Settings
Possibly related PRs
Suggested reviewers
Poem
✨ Finishing Touches
🪧 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 (
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (1)
frontend/src/routes/(app)/(internal)/risk-assessments/[id=uuid]/+layout.server.ts (1)
23-25
: Add error handling for the interface settings fetch.While the fetch follows the existing pattern, consider adding error handling to gracefully handle API failures.
- const interface_settings = await fetch(`${BASE_API_URL}/settings/general/object`).then((res) => - res.json() - ); + let interface_settings = { interface_agg_scenario_matrix: false }; + try { + const response = await fetch(`${BASE_API_URL}/settings/general/object`); + if (response.ok) { + interface_settings = await response.json(); + } else { + console.error('Failed to fetch interface settings:', response.statusText); + } + } catch (error) { + console.error('Error fetching interface settings:', error); + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
backend/global_settings/serializers.py
(1 hunks)backend/global_settings/views.py
(2 hunks)frontend/messages/en.json
(1 hunks)frontend/messages/fr.json
(1 hunks)frontend/src/lib/components/Forms/ModelForm/GeneralSettingForm.svelte
(3 hunks)frontend/src/lib/utils/schemas.ts
(1 hunks)frontend/src/routes/(app)/(internal)/risk-assessments/[id=uuid]/+layout.server.ts
(2 hunks)frontend/src/routes/(app)/(internal)/risk-assessments/[id=uuid]/+page.svelte
(1 hunks)
👮 Files not reviewed due to content moderation or server errors (4)
- frontend/src/routes/(app)/(internal)/risk-assessments/[id=uuid]/+page.svelte
- frontend/src/lib/utils/schemas.ts
- frontend/messages/en.json
- frontend/messages/fr.json
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: functional-tests (3.12, chromium)
- GitHub Check: test (3.12)
- GitHub Check: enterprise-functional-tests (3.12, chromium)
🔇 Additional comments (6)
backend/global_settings/serializers.py (1)
11-12
: LGTM! The new settings keys are well-structured.The new keys follow the existing naming convention and their names clearly indicate their purpose.
backend/global_settings/views.py (2)
68-69
: LGTM! Conservative defaults are security-conscious.The new settings default to
False
, which is a secure default as it requires explicit opt-in for both mail notifications and matrix aggregation.
107-124
: LGTM! Well-structured action methods.The new action methods follow the existing pattern and are properly decorated with
@action
. They return specific subsets of settings which helps minimize data exposure.frontend/src/lib/components/Forms/ModelForm/GeneralSettingForm.svelte (2)
17-29
: LGTM! Well-structured notifications section.The notifications section is logically organized with appropriate icon and checkbox binding.
30-43
: LGTM! Well-structured interface settings section.The interface settings section follows the same pattern and maintains consistency in the UI.
frontend/src/routes/(app)/(internal)/risk-assessments/[id=uuid]/+layout.server.ts (1)
153-154
: LGTM! Clear mapping of interface settings.The mapping from
interface_agg_scenario_matrix
touseBubbles
is clear and maintains the semantic meaning.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Summary by CodeRabbit
New Features
Localization
User Interface