feat(definitions): add first day of the week field #1561
Conversation
Signed-off-by: Max Makaluk <[email protected]>
WalkthroughA new optional property, Changes
✨ 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:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/v3/definition/congregation.ts (1)
164-164: Consider clarifying the relationship with existingweek_start_sundayfield.The addition of
first_day_of_the_week?: string;follows the proper naming conventions and is correctly implemented as an optional field. However, there's already a similar fieldweek_start_sunday?: string;at line 159 that appears to serve a related purpose.Consider:
- Adding documentation to clarify the purpose and allowed values for this new field
- Explaining how it differs from or relates to the existing
week_start_sundayfield- Using a more specific type (like string literal union) if there are predetermined valid values
/** * Specifies the first day of the week (e.g., "monday", "sunday"). * This field provides more flexibility compared to the boolean `week_start_sunday` field. */ first_day_of_the_week?: "monday" | "tuesday" | "wednesday" | "thursday" | "friday" | "saturday" | "sunday";
|
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/v3/definition/congregation.ts(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Code QL
| @@ -161,6 +161,7 @@ export type CongSettingsType = { | |||
| responsabilities?: string; | |||
| last_backup: string; | |||
| group_publishers_sort?: string; | |||
| first_day_of_the_week?: string; | |||
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Potential redundancy with existing week start property.
The addition of first_day_of_the_week?: string appears to overlap with the existing week_start_sunday?: string property at line 159. Consider:
- Clarifying the distinction between these properties
- Using a more specific type (enum or union type) rather than string for better type safety
- Adding documentation to explain the expected values and format
+ /**
+ * Defines the first day of the week for the congregation.
+ * Possible values: "sunday", "monday", etc.
+ */
first_day_of_the_week?: string;🏁 Script executed:
#!/bin/bash
# Check for usage of the existing week_start_sunday property
echo "Checking for usage of week_start_sunday property..."
rg "week_start_sunday" -A 2 -B 2Length of output: 1095
Consolidate week‐start properties
There’s already a week_start_sunday?: string in:
src/v3/definition/congregation.ts(line 159)src/v3/classes/Congregation.tssrc/v3/services/firebase/congregations.ts
Introducing first_day_of_the_week?: string overlaps with that. To avoid confusion:
• Define a strict union (or enum) for weekdays:
export type Weekday =
| 'sunday'
| 'monday'
| 'tuesday'
| 'wednesday'
| 'thursday'
| 'friday'
| 'saturday';• Replace both props with a single field:
- week_start_sunday?: string;
- first_day_of_the_week?: string;
+ /**
+ * First day of the week for the congregation.
+ * Allowed values: 'sunday', 'monday', …, 'saturday'.
+ */
+ first_day_of_the_week?: Weekday;• Update all references in:
src/v3/definition/congregation.tssrc/v3/classes/Congregation.tssrc/v3/services/firebase/congregations.ts
This will improve type safety and clarify intent.
Type of change
Checklist: