feat: Add custom leaves support for time off entries #2036
Conversation
- Add custom_leave_id to timeoff_entries table with safe migrations - Update TimeoffEntry model with custom_leave association and validation - Integrate custom leaves into leave balance calculation - Update TimeTrackingIndexService to return user's custom leaves - Update TimeoffEntriesController and policy to handle custom_leave_id - Update jbuilder views to serialize custom leaves with composite IDs - Update frontend components (LeaveBlock, TimeoffForm, TableRow) for custom leaves - Add custom leave color and icon to constants - Add comprehensive test coverage for custom leaves feature - Add product guide documentation for leave management
📝 WalkthroughWalkthroughAdds end-to-end support for custom leaves: DB migrations and FK, model associations and validations, controller/policy updates, service-layer balance calculations, API JSON changes, frontend UI/form handling, tests, and documentation updates. Changes
Sequence Diagram(s)sequenceDiagram
participant FE as Frontend (TimeoffForm / Leave UI)
participant API as Internal API Controller
participant SVC as TimeoffEntries::IndexService / TimeTrackingIndexService
participant DB as Database
FE->>API: POST /internal_api/v1/timeoff_entries (payload may include custom_leave_id or composite leave id)
activate API
API->>API: load_custom_leave! (when provided) / validate params
API->>DB: INSERT / UPDATE timeoff_entries with custom_leave_id
DB-->>API: OK (record)
API-->>FE: JSON timeoffEntry (type:"custom_leave", custom_leave_id)
deactivate API
FE->>API: GET /internal_api/v1/timeoff_entries?year=YYYY
activate API
API->>SVC: fetch entries + leave_types
activate SVC
SVC->>DB: Query timeoff_entries LEFT JOIN leave_type/holiday_info/custom_leave
DB-->>SVC: rows
SVC->>SVC: calculate_custom_leave_balance() -> aggregate allocations & durations
SVC-->>API: timeoffEntries + leave_balance (includes custom leaves)
API-->>FE: JSON response
deactivate API
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🤖 Fix all issues with AI agents
In @app/javascript/src/components/LeaveManagement/Container/LeaveBlock.tsx:
- Around line 19-27: The icon lookup for custom leaves is inconsistent: in
LeaveBlock.getLeaveIcon you pass the dynamic icon value to
generateLeaveIcon(icon) while TableRow passes the literal "custom"; unify them
by changing LeaveBlock.getLeaveIcon to call generateLeaveIcon("custom") ||
CUSTOM_LEAVE_ICON for type === "custom_leave" so both components use the same
lookup key (refer to getLeaveIcon, generateLeaveIcon, and CUSTOM_LEAVE_ICON).
In @app/javascript/src/components/LeaveManagement/Container/Table/TableRow.tsx:
- Around line 20-30: The getLeaveIcon function currently passes the hardcoded
string "custom" to generateLeaveIcon which doesn't match the leaveIcons set;
update getLeaveIcon to use the actual icon value from the custom leave object
(referencing customLeave and its icon property) so it mirrors LeaveBlock.tsx's
behavior (generateLeaveIcon(icon) || CUSTOM_LEAVE_ICON) and still falls back to
CUSTOM_LEAVE_ICON when generateLeaveIcon returns undefined.
In @app/views/internal_api/v1/time_tracking/index.json.jbuilder:
- Around line 51-69: For the custom branch in the leave_types jbuilder loop,
stop returning hardcoded values and use the CustomLeave model attributes
instead: keep json.id "custom_#{leave_type[:id]}" and json.custom_leave_id
leave_type[:id], but replace json.allocation_frequency with the CustomLeave enum
(e.g. leave_type[:allocation_period] or map allocation_period to the same shape
as LeaveType#allocation_frequency), replace json.carry_forward_days with the
CustomLeave numeric attribute (e.g. leave_type[:allocation_value] or the
appropriate field), and replace json.color and json.icon with the corresponding
CustomLeave attributes (e.g. leave_type[:color], leave_type[:icon]); if
allocation_frequency/carry_forward_days truly do not apply for CustomLeave then
remove those fields instead.
In @docs/docs/product-guide/05-time-tracking.md:
- Line 40: The heading "Adding Time in the Weekly Overview" is currently at
level ### directly under the top-level # which skips a level; change that
heading to level ## (i.e., use "## Adding Time in the Weekly Overview") so the
markdown hierarchy is correct and run the markdown linter to confirm there are
no other skipped heading levels in the document.
- Line 52: Change the heading "### Adding Time in the Monthly Overview" to use
level 2 (##) so it matches the surrounding hierarchy; locate the literal heading
string "Adding Time in the Monthly Overview" in the markdown and replace the
leading "###" with "##".
- Line 32: Update the relative markdown link in the sentence that currently
points to "./team/leave-management" so it references the actual file name;
change the link target to "./team/03-leave-management" (the link text "Leave
Management guide" stays the same) to correctly point to
docs/docs/product-guide/team/03-leave-management.md.
In @spec/services/timeoff_entries/index_service_spec.rb:
- Around line 330-389: The spec's context "with custom leaves" is defined after
the file's private helper methods block, which is unconventional; move the
entire context "with custom leaves" block (including its nested describe
"#process with custom leaves" and its before/it examples) so it appears before
the private section (i.e., alongside the other describe/context blocks) in
spec/services/timeoff_entries/index_service_spec.rb to restore normal RSpec
structure and readability while keeping the block contents unchanged.
🧹 Nitpick comments (10)
docs/docs/product-guide/team/03-leave-management.md (1)
102-116: Consider hyphenating "time-off" for consistency.Static analysis suggests hyphenating compound adjectives like "time-off entries" when used as a noun modifier. However, this is a minor style preference - "time off" without hyphen is also commonly accepted.
📝 Optional: Hyphenate "time-off" when used as a compound modifier
### Filtering Time Off Entries -On the Leave Management page, you can filter time off entries by leave type: +On the Leave Management page, you can filter time-off entries by leave type: 1. Navigate to **Leave Management** 2. Click on any leave type card (PTO, Sick Leave, Custom Leave, etc.) -3. The table below will filter to show only time off entries for that leave type +3. The table below will filter to show only time-off entries for that leave typeapp/javascript/src/components/LeaveManagement/Container/LeaveBlock.tsx (1)
12-14: Extract shared constants to avoid duplication.
CUSTOM_LEAVE_COLORandCUSTOM_LEAVE_ICONare duplicated in bothLeaveBlock.tsxandTableRow.tsx. Consider extracting them to a shared location likeconstants/leaveType.tsxfor consistency and easier maintenance.// In constants/leaveType.tsx or similar shared file export const CUSTOM_LEAVE_COLOR = { value: "#9B59B6", label: "custom" }; export const CUSTOM_LEAVE_ICON = { icon: null, value: "custom" };app/javascript/src/components/LeaveManagement/Container/Table/TableRow.tsx (1)
54-59: Consider null-safety forleaveColor?.value.If all lookups fail and no fallback is returned,
leaveColorcould be undefined, causingbackgroundColor: undefined. While the fallback constants should prevent this, TypeScript would benefit from a more explicit type guard or default value.<div className="mr-2 hidden h-6 w-6 items-center justify-center rounded-full text-white lg:flex" - style={{ backgroundColor: leaveColor?.value }} + style={{ backgroundColor: leaveColor?.value ?? "#9B59B6" }} >app/javascript/src/components/LeaveManagement/index.tsx (1)
112-123: Consider type-safe filtering to avoid potential runtime issues.The expression
timeoffEntries.length && timeoffEntries.filter(...)returns0(a number) whentimeoffEntriesis empty, rather than an empty array. While this matches the existing pattern at lines 125-129, it could cause subtle type issues.♻️ Suggested improvement for type safety
} else if (selectedLeaveType.type === "custom_leave") { // Custom leaves have composite IDs like "custom_1", extract the numeric ID const customLeaveId = typeof selectedLeaveType.id === "string" ? parseInt(selectedLeaveType.id.replace("custom_", ""), 10) : selectedLeaveType.id; - sortedTimeoffEntries = - timeoffEntries.length && - timeoffEntries.filter( - timeoffEntry => timeoffEntry.customLeave?.id === customLeaveId - ); + sortedTimeoffEntries = timeoffEntries.filter( + timeoffEntry => timeoffEntry.customLeave?.id === customLeaveId + ); } else {Note: The same pattern exists in the
elsebranch (lines 125-129) and could be similarly simplified.app/javascript/src/components/TimeoffEntries/TimeoffForm/index.tsx (1)
124-134: Minor: Redundant.toString()call.Since
typeof leaveTypeId === "string"already confirms it's a string, the subsequent.toString()calls are redundant.Suggested simplification
const isCustomLeaveEntry = () => typeof leaveTypeId === "string" && - leaveTypeId.toString().startsWith("custom_"); + leaveTypeId.startsWith("custom_"); const getCustomLeaveId = () => { if (isCustomLeaveEntry()) { - return Number(leaveTypeId.toString().replace("custom_", "")); + return Number((leaveTypeId as string).replace("custom_", "")); } return null; };spec/services/timeoff_entries/index_service_spec.rb (1)
374-387: Test creates timeoff_entry insideitblock - consider usinglet!for consistency.Creating the
timeoff_entryinside theitblock at line 375 is valid, but for consistency with the rest of the spec file, consider using a nested context withlet!to set up the data. This also allows for easier extension if more tests need the same setup.app/services/time_tracking_index_service.rb (1)
80-89: The custom leaves integration looks good, but consider adding.distinctfor robustness.The method properly combines regular leave types with user-specific custom leaves. Since the
custom_leave_userstable has no unique constraint on the (custom_leave_id, user_id) pair, adding.distinctto the custom leaves query prevents potential duplicates if a user is assigned to the same custom leave multiple times:Suggested change
user_custom_leaves = leave.custom_leaves.joins(:custom_leave_users) .where(custom_leave_users: { user_id: user.id }) .distinctspec/requests/internal_api/v1/timeoff_entries/index_spec.rb (1)
27-80: Consider using underscore prefix for unused variables.The
timeoff_entryvariables on lines 29, 41, 67, and 68 are assigned but never referenced. Since only the side effect of creation matters, consider using underscore prefix to indicate intentionally unused variables:- timeoff_entry = create(:timeoff_entry, user:, leave_type:, duration: 480) + _timeoff_entry = create(:timeoff_entry, user:, leave_type:, duration: 480)Or simply call
createwithout assignment.app/services/timeoff_entries/index_service.rb (2)
96-135: Potential N+1 query inside the loop.Line 106 executes a database query for each custom leave in the loop, which could cause performance issues if a user has many custom leaves assigned:
timeoff_entries_duration = custom_leave.timeoff_entries.kept.where(user_id:).sum(:duration)Consider preloading the timeoff entries or calculating the sums in a single query:
♻️ Suggested optimization
def calculate_custom_leave_balance leave = current_company.leaves&.kept&.find_by(year:) return unless leave # Get custom leaves assigned to this user user_custom_leaves = leave.custom_leaves.joins(:custom_leave_users) .where(custom_leave_users: { user_id: }) + # Precompute timeoff entry durations to avoid N+1 + timeoff_durations = TimeoffEntry.kept + .where(user_id:, custom_leave_id: user_custom_leaves.select(:id)) + .group(:custom_leave_id) + .sum(:duration) user_custom_leaves.each do |custom_leave| total_days = calculate_custom_leave_allocation(custom_leave) - timeoff_entries_duration = custom_leave.timeoff_entries.kept.where(user_id:).sum(:duration) + timeoff_entries_duration = timeoff_durations[custom_leave.id] || 0 # ... rest of the method
108-118: Consider extracting shared label calculation logic.The label calculation logic (lines 108-118) is duplicated from
calculate_leave_balance(lines 67-77). While acceptable for this PR, consider extracting a shared helper method for maintainability:def format_balance_label(net_hours, net_days, extra_hours) if net_hours.abs < @working_hours_per_day "#{net_hours} hours" else "#{net_days} days #{extra_hours} hours" end end
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (27)
app/controllers/internal_api/v1/timeoff_entries_controller.rbapp/javascript/src/components/LeaveManagement/Container/LeaveBlock.tsxapp/javascript/src/components/LeaveManagement/Container/Table/TableRow.tsxapp/javascript/src/components/LeaveManagement/index.tsxapp/javascript/src/components/TimeoffEntries/TimeoffForm/index.tsxapp/javascript/src/constants/leaveType.tsxapp/models/custom_leave.rbapp/models/timeoff_entry.rbapp/policies/timeoff_entry_policy.rbapp/services/time_tracking_index_service.rbapp/services/timeoff_entries/index_service.rbapp/views/internal_api/v1/time_tracking/index.json.jbuilderapp/views/internal_api/v1/timeoff_entries/index.json.jbuilderdb/migrate/20260112175140_add_custom_leave_id_to_timeoff_entries.rbdb/migrate/20260112175342_add_foreign_key_custom_leave_to_timeoff_entries.rbdb/migrate/20260112175355_validate_foreign_key_custom_leave_to_timeoff_entries.rbdb/schema.rbdocs/docs/product-guide/05-time-tracking.mddocs/docs/product-guide/team/03-leave-management.mddocs/docs/product-guide/team/_category_.jsonspec/factories/custom_leave_users.rbspec/factories/custom_leaves.rbspec/models/timeoff_entry_spec.rbspec/requests/internal_api/v1/time_tracking/index_spec.rbspec/requests/internal_api/v1/timeoff_entries/create_spec.rbspec/requests/internal_api/v1/timeoff_entries/index_spec.rbspec/services/timeoff_entries/index_service_spec.rb
🧰 Additional context used
🧬 Code graph analysis (14)
app/javascript/src/components/TimeoffEntries/TimeoffForm/index.tsx (1)
app/javascript/src/constants/leaveType.tsx (1)
leaveTypes(16-37)
spec/requests/internal_api/v1/timeoff_entries/create_spec.rb (4)
app/controllers/internal_api/v1/timeoff_entries_controller.rb (1)
create(25-30)spec/support/helpers/request_parser.rb (1)
send_request(3-16)spec/rails_helper.rb (1)
auth_headers(90-95)spec/support/helpers/request_helper.rb (1)
json_response(4-6)
app/javascript/src/components/LeaveManagement/Container/Table/TableRow.tsx (1)
app/javascript/src/components/Profile/Organization/Leaves/utils.ts (4)
generateLeaveIcon(11-12)generateHolidayIcon(17-18)generateLeaveColor(8-9)generateHolidayColor(14-15)
app/controllers/internal_api/v1/timeoff_entries_controller.rb (1)
app/controllers/concerns/current_company_concern.rb (1)
current_company(10-14)
spec/models/timeoff_entry_spec.rb (1)
app/models/timeoff_entry.rb (1)
include(33-125)
db/migrate/20260112175355_validate_foreign_key_custom_leave_to_timeoff_entries.rb (2)
db/migrate/20260112175140_add_custom_leave_id_to_timeoff_entries.rb (1)
change(6-8)db/migrate/20260112175342_add_foreign_key_custom_leave_to_timeoff_entries.rb (2)
change(3-7)change(4-6)
app/models/timeoff_entry.rb (1)
app/models/custom_leave.rb (1)
belongs_to(23-37)
spec/services/timeoff_entries/index_service_spec.rb (3)
app/services/timeoff_entries/index_service.rb (1)
process(22-31)app/services/timeoff_entries/calculate_total_duration_of_defined_leaves_service.rb (1)
process(33-64)app/services/timeoff_entries/calculate_optional_holiday_timeoff_entries_service.rb (1)
process(14-41)
app/javascript/src/components/LeaveManagement/Container/LeaveBlock.tsx (1)
app/javascript/src/components/Profile/Organization/Leaves/utils.ts (4)
generateLeaveIcon(11-12)generateHolidayIcon(17-18)generateLeaveColor(8-9)generateHolidayColor(14-15)
db/migrate/20260112175342_add_foreign_key_custom_leave_to_timeoff_entries.rb (2)
db/migrate/20260112175140_add_custom_leave_id_to_timeoff_entries.rb (1)
change(6-8)db/migrate/20260112175355_validate_foreign_key_custom_leave_to_timeoff_entries.rb (2)
change(3-7)change(4-6)
db/migrate/20260112175140_add_custom_leave_id_to_timeoff_entries.rb (2)
db/migrate/20260112175342_add_foreign_key_custom_leave_to_timeoff_entries.rb (2)
change(3-7)change(4-6)db/migrate/20260112175355_validate_foreign_key_custom_leave_to_timeoff_entries.rb (2)
change(3-7)change(4-6)
app/models/custom_leave.rb (1)
app/models/company.rb (1)
has_many(24-138)
app/services/timeoff_entries/index_service.rb (2)
app/controllers/concerns/current_company_concern.rb (1)
current_company(10-14)app/services/time_tracking_index_service.rb (1)
timeoff_entries(66-71)
app/javascript/src/constants/leaveType.tsx (1)
app/javascript/src/miruIcons/index.ts (1)
CalendarIcon(200-200)
🪛 LanguageTool
docs/docs/product-guide/team/03-leave-management.md
[grammar] ~104-~104: Use a hyphen to join words.
Context: ...ave Management page, you can filter time off entries by leave type: 1. Navigate ...
(QB_NEW_EN_HYPHEN)
[grammar] ~108-~108: Use a hyphen to join words.
Context: ...able below will filter to show only time off entries for that leave type 4. The d...
(QB_NEW_EN_HYPHEN)
[grammar] ~169-~169: Use a hyphen to join words.
Context: ...to see updated balance - Verify the time off entry was saved successfully - Check...
(QB_NEW_EN_HYPHEN)
[grammar] ~172-~172: Use a hyphen to join words.
Context: ...ype was selected ### Cannot edit a time off entry - Only the employee who create...
(QB_NEW_EN_HYPHEN)
docs/docs/product-guide/05-time-tracking.md
[grammar] ~22-~22: Use a hyphen to join words.
Context: ...acking work hours, you can also log time off (leave) entries: 1. Click to add a ...
(QB_NEW_EN_HYPHEN)
🪛 markdownlint-cli2 (0.18.1)
docs/docs/product-guide/team/03-leave-management.md
22-22: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
docs/docs/product-guide/05-time-tracking.md
40-40: Heading levels should only increment by one level at a time
Expected: h2; Actual: h3
(MD001, heading-increment)
52-52: Heading levels should only increment by one level at a time
Expected: h2; Actual: h3
(MD001, heading-increment)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Running tests
🔇 Additional comments (36)
app/javascript/src/constants/leaveType.tsx (2)
72-75: LGTM!The custom leave color entry is properly added. The purple color (
#9B59B6) aligns with the documentation stating custom leaves appear with a distinct purple color by default.
119-123: LGTM!The custom leave icon entry is properly added, reusing
CalendarIconas the default icon for custom leaves. Thevalueandlabelboth being "custom" is more descriptive than the existinglabelNpattern used by other entries.app/policies/timeoff_entry_policy.rb (1)
20-24: LGTM!The addition of
:custom_leave_idtopermitted_attributescorrectly enables mass assignment for custom leave entries. This aligns with the model's newbelongs_to :custom_leaveassociation and the validation requiring exactly one ofleave_type_id,holiday_info_id, orcustom_leave_id.db/migrate/20260112175342_add_foreign_key_custom_leave_to_timeoff_entries.rb (1)
3-7: LGTM! Safe migration pattern correctly implemented.This migration properly follows the Rails best practice for adding foreign keys to existing tables without locking:
- Prior migration adds the column with concurrent index
- This migration adds the FK constraint with
validate: false(avoids full table scan/lock)- Subsequent migration validates the FK separately
This ensures zero-downtime deployment for production databases.
docs/docs/product-guide/team/_category_.json (1)
1-9: LGTM!The updated description "Managing your team in Miru" better reflects the section's content, especially with the addition of the new leave management documentation page.
docs/docs/product-guide/team/03-leave-management.md (2)
1-9: LGTM!The frontmatter is properly configured with appropriate id, title, and sidebar position for Docusaurus.
140-158: LGTM! Comprehensive documentation.The Best Practices section and Permissions table are well-documented. The permissions accurately reflect the implementation in
TimeoffEntryPolicywhere owners, admins, and employees can all create/log time off, but only owners and admins can manage leave types and custom leaves.db/schema.rb (2)
629-630: LGTM!The schema correctly adds the nullable
custom_leave_idcolumn with an index, properly supporting the optionalbelongs_to :custom_leaveassociation in the model.
767-767: LGTM!Foreign key constraint properly references the
custom_leavestable, ensuring referential integrity.db/migrate/20260112175355_validate_foreign_key_custom_leave_to_timeoff_entries.rb (1)
1-7: LGTM! Well-structured safe migration pattern.This follows the recommended approach for adding foreign keys to existing tables in production:
- Add column with concurrent index
- Add FK constraint with
validate: false- Validate FK separately (this migration)
This avoids long-running locks on the table.
app/javascript/src/components/LeaveManagement/Container/LeaveBlock.tsx (1)
39-40: LGTM!Clean refactor to use helper functions for computing icon and color values.
app/javascript/src/components/LeaveManagement/Container/Table/TableRow.tsx (1)
44-46: LGTM!The priority chain for
leaveName(customLeave?.name || leaveType?.name || holidayInfo?.name) is clean and handles all cases appropriately with proper null-safe access.spec/factories/custom_leaves.rb (1)
1-10: LGTM!The factory is well-structured with proper associations and randomized test data. Using
rand()and.sampleprovides good test diversity while maintaining valid values.app/controllers/internal_api/v1/timeoff_entries_controller.rb (1)
62-68: LGTM!The
load_custom_leave!method correctly validates that the custom leave belongs to the current company by joining through theleavestable. The pattern is consistent with the existingload_leave_type!andload_holiday_info!methods.app/models/custom_leave.rb (1)
27-27: LGTM!The
has_many :timeoff_entries, dependent: :nullifyassociation is appropriate—it preserves timeoff entry records when a custom leave is deleted while nullifying the reference.spec/factories/custom_leave_users.rb (1)
1-8: LGTM!Simple and correct factory for the join model.
app/views/internal_api/v1/timeoff_entries/index.json.jbuilder (1)
13-20: LGTM!The three-way type determination with
custom_leavetaking precedence is logical, and the conditional serialization follows the existing patterns forleave_typeandholiday_info. Theoptional_timeoff_entriesandnational_timeoff_entriesblocks correctly omit custom leave handling since they are specifically for holiday entries.spec/requests/internal_api/v1/timeoff_entries/create_spec.rb (1)
34-45: No additional setup required for this test.The test correctly creates a timeoff entry with a custom leave without explicitly associating the user via
CustomLeaveUser. The controller validates only that the custom_leave belongs to the current company, not user membership. The test as written is valid and follows the actual business logic.spec/requests/internal_api/v1/time_tracking/index_spec.rb (1)
75-111: LGTM! Good test coverage for custom leaves integration.The test context properly sets up the required associations and verifies the API response includes both regular leave types and custom leaves with correct type fields and composite ID formats.
Consider adding an edge case test for when a user has no custom leaves assigned to ensure the response doesn't include any custom leave entries in that scenario.
app/views/internal_api/v1/time_tracking/index.json.jbuilder (1)
34-34: LGTM!Adding
custom_leave_idto the leave entry response aligns with the model changes and enables the frontend to correctly identify custom leave entries.db/migrate/20260112175140_add_custom_leave_id_to_timeoff_entries.rb (1)
1-9: LGTM! Well-structured safe migration pattern.The migration correctly uses
disable_ddl_transaction!withalgorithm: :concurrentlyto avoid locking the table during index creation. The three-step pattern (add reference, add FK without validation, validate FK separately) is a best practice for zero-downtime deployments on production databases.app/javascript/src/components/TimeoffEntries/TimeoffForm/index.tsx (2)
155-169: LGTM! Custom leave data filling is correctly implemented.The code properly handles custom leave entries by constructing the composite ID and looking up the corresponding leave type from the list.
204-234: LGTM! Payload construction handles all leave type scenarios correctly.The mutual exclusion logic properly ensures only one of
holiday_info_id,custom_leave_id, orleave_type_idis set while nullifying the others, maintaining data consistency.spec/models/timeoff_entry_spec.rb (2)
117-163: LGTM! Good validation test coverage for custom leaves.The tests cover the essential validation scenarios for the new
custom_leaveassociation. The mutual exclusion and presence validations are properly tested.Consider adding a test case for the
holiday_info_id+custom_leave_idcombination to ensure complete coverage of the mutual exclusion validation:it "is not valid with both holiday_info_id and custom_leave_id" do timeoff_entry = build( :timeoff_entry, user:, leave_type: nil, holiday_info: national_holiday, custom_leave:, duration: 480, leave_date: Date.current ) expect(timeoff_entry).not_to be_valid expect(timeoff_entry.errors[:base]).to include("Choose only one of leave type, holiday info, or custom leave") end
114-115: LGTM!The association test correctly verifies the optional
belongs_to :custom_leaverelationship.spec/requests/internal_api/v1/timeoff_entries/index_spec.rb (3)
1-18: LGTM! Well-structured test setup.The test setup properly establishes the factory hierarchy with company, user, leave, leave_type, and custom_leave associations. The
custom_leave_userassociation in thebeforeblock correctly links the user to the custom leave.
83-102: LGTM! Employee authorization properly tested.The test correctly verifies that employees can view their own timeoff entries with custom leaves. Same minor note about the unused
timeoff_entryvariable on line 92.
104-109: LGTM! Security test for unauthenticated access.Properly verifies that unauthenticated requests receive a 401 unauthorized response.
app/services/timeoff_entries/index_service.rb (3)
39-44: LGTM! Proper eager loading added.Adding
:custom_leaveto the includes prevents N+1 queries when accessing the custom_leave association on timeoff entries.
46-51: LGTM! Logical ordering of balance calculations.The custom leave balance calculation is appropriately positioned between standard leaves and holidays.
137-148: Document the 4-week month approximation or verify it aligns with business requirements.The
calculate_custom_leave_allocationmethod uses a 4-week approximation for monthly allocations (@working_days_per_week * 4). This same approximation is defined asWEEKS_PER_MONTH = 4inCalculateTotalDurationOfDefinedLeavesServiceand applied consistently across leave calculations. However, there is no documentation explaining why months are approximated as 4 weeks (28 days) rather than standard calendar months. Confirm this approximation is intentional and aligns with business requirements, then add a comment to the constant or method explaining the rationale.app/models/timeoff_entry.rb (5)
36-42: LGTM! Association correctly defined.The
belongs_to :custom_leave, optional: trueis properly configured, matching the inversehas_many :timeoff_entries, dependent: :nullifyin the CustomLeave model.
54-60: LGTM! Scope correctly updated for custom leaves.The
from_workspacescope properly handles the nested joincustom_leave: :leaveand uses the Rails-generated aliasleaves_custom_leavesin the WHERE clause. The logic correctly filters timeoff entries that belong to the company through any of the three paths (leave_type, holiday, or custom_leave).
62-64: LGTM! Company accessor correctly extended.The method properly handles the custom_leave path using safe navigation (
custom_leave&.leave&.company).
68-75: LGTM! Validation logic correctly ensures mutual exclusivity.The validation properly enforces that exactly one of
leave_type_id,holiday_info_id, orcustom_leave_idmust be present. The implementation usingcompact.countis clean and the error messages are descriptive.
47-52: LGTM! Validation declaration correctly updated.The validation name accurately describes the constraint being enforced.
app/javascript/src/components/LeaveManagement/Container/Table/TableRow.tsx
Show resolved
Hide resolved
| json.leave_types leave_types do |leave_type| | ||
| json.id leave_type[:id] | ||
| # Use composite id to avoid conflicts between LeaveType and CustomLeave | ||
| if leave_type.is_a?(LeaveType) | ||
| json.id leave_type[:id] | ||
| json.type "leave_type" | ||
| json.allocation_frequency leave_type[:allocation_frequency] | ||
| json.carry_forward_days leave_type[:carry_forward_days] | ||
| json.color leave_type[:color] | ||
| json.icon leave_type[:icon] | ||
| else | ||
| # CustomLeave - prefix id to distinguish from LeaveType | ||
| json.id "custom_#{leave_type[:id]}" | ||
| json.custom_leave_id leave_type[:id] | ||
| json.type "custom_leave" | ||
| json.allocation_frequency "per_year" | ||
| json.carry_forward_days 0 | ||
| json.color "custom" | ||
| json.icon "custom" | ||
| end |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Find CustomLeave model definition
find . -name "*.rb" -type f | xargs grep -l "class CustomLeave" | head -5Repository: saeloun/miru-web
Length of output: 162
🏁 Script executed:
# Check database schema for custom_leaves table
find . -name "schema.rb" -type f -exec grep -A 30 "create_table.*custom_leaves" {} \;Repository: saeloun/miru-web
Length of output: 1224
🏁 Script executed:
# Also search for migration files that might define custom_leaves table
find . -path "*/db/migrate/*" -name "*.rb" -type f | xargs grep -l "custom_leaves" | head -5Repository: saeloun/miru-web
Length of output: 274
🏁 Script executed:
cat -n ./app/models/custom_leave.rbRepository: saeloun/miru-web
Length of output: 1313
🏁 Script executed:
# Also check the migration to see the structure
cat -n ./db/migrate/20240426050940_create_custom_leaves.rbRepository: saeloun/miru-web
Length of output: 521
Replace hardcoded CustomLeave values with actual model attributes.
The CustomLeave model has allocation_value and allocation_period (enum: days/weeks/months), but the jbuilder template ignores these and returns fabricated hardcoded values (allocation_frequency: "per_year", carry_forward_days: 0, color: "custom", icon: "custom"). Use actual CustomLeave attributes instead of placeholders to ensure the API returns correct leave data. If allocation_frequency and carry_forward_days don't apply to CustomLeave, clarify what should be returned or remove these fields from the response.
🤖 Prompt for AI Agents
In @app/views/internal_api/v1/time_tracking/index.json.jbuilder around lines 51
- 69, For the custom branch in the leave_types jbuilder loop, stop returning
hardcoded values and use the CustomLeave model attributes instead: keep json.id
"custom_#{leave_type[:id]}" and json.custom_leave_id leave_type[:id], but
replace json.allocation_frequency with the CustomLeave enum (e.g.
leave_type[:allocation_period] or map allocation_period to the same shape as
LeaveType#allocation_frequency), replace json.carry_forward_days with the
CustomLeave numeric attribute (e.g. leave_type[:allocation_value] or the
appropriate field), and replace json.color and json.icon with the corresponding
CustomLeave attributes (e.g. leave_type[:color], leave_type[:icon]); if
allocation_frequency/carry_forward_days truly do not apply for CustomLeave then
remove those fields instead.
There was a problem hiding this comment.
This is actually the correct implementation. The CustomLeave model doesn't have icon or color columns in the database schema (see migration db/migrate/20240426050940_create_custom_leaves.rb). The table only contains name, allocation_value, allocation_period, and leave_id.
The hardcoded values icon: "custom" and color: "custom" are intentional type identifiers that the frontend uses to apply default styling via CUSTOM_LEAVE_COLOR and CUSTOM_LEAVE_ICON constants. Custom leaves don't have configurable icons/colors in the current design - they all use the same default purple color and icon.
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check how allocation_frequency and carry_forward_days are used in the frontend for custom leaves
echo "=== Searching for allocation_frequency usage in frontend ==="
rg -n "allocation_frequency" --type=ts --type=tsx -C2 app/javascript/
echo -e "\n=== Searching for carry_forward_days usage in frontend ==="
rg -n "carry_forward_days" --type=ts --type=tsx -C2 app/javascript/
echo -e "\n=== Checking TimeoffForm component specifically ==="
cat app/javascript/src/components/TimeoffEntries/TimeoffForm/index.tsx | grep -A5 -B5 "allocation"
echo -e "\n=== Checking LeaveBlock component for allocation usage ==="
cat app/javascript/src/components/LeaveManagement/Container/LeaveBlock.tsx | grep -A5 -B5 "allocation"
Length of output: 289
🏁 Script executed:
#!/bin/bash
# Search for allocation_frequency and carry_forward_days in JavaScript/TypeScript files
echo "=== Searching for allocation_frequency usage in frontend ==="
rg -n "allocation_frequency" -g "*.ts" -g "*.tsx" -g "*.js" -g "*.jsx" -C2
echo -e "\n=== Searching for carry_forward_days usage in frontend ==="
rg -n "carry_forward_days" -g "*.ts" -g "*.tsx" -g "*.js" -g "*.jsx" -C2
echo -e "\n=== Check CustomLeave type definition ==="
rg -n "type.*CustomLeave|interface.*CustomLeave" -g "*.ts" -g "*.tsx" -A10
echo -e "\n=== Check LeaveType type definition ==="
rg -n "type.*LeaveType|interface.*LeaveType" -g "*.ts" -g "*.tsx" -A10
Length of output: 4415
🏁 Script executed:
#!/bin/bash
# Check which components consume the time_tracking/index API
echo "=== Finding components that call time_tracking API ==="
rg -n "time_tracking|timeTracking" -g "*.ts" -g "*.tsx" -g "*.js" -g "*.jsx" -C3 | grep -A3 -B3 "fetch\|api\|get"
echo -e "\n=== Check if allocation_frequency or carry_forward_days are accessed on leave types in time tracking context ==="
rg -n "leaveType.*allocation_frequency|leave_type.*allocation_frequency|leaveTypes.*allocation_frequency" -g "*.ts" -g "*.tsx" -g "*.js" -g "*.jsx" -C2
echo -e "\n=== Check TimeTracking component files ==="
fd -e ts -e tsx -e js -e jsx . app/javascript/src/components/TimeTracking/
Length of output: 5166
@amolmjoshi93, Thank you for the clarification about icon and color! That makes sense—they're intentional type identifiers for the frontend to apply default styling via the constants.
However, I notice the diff also includes hardcoded values for allocation_frequency: "per_year" and carry_forward_days: 0 for custom leaves. My script results show these fields are only used in Profile/Organization/Leaves/utils.ts (the Settings page), not in the TimeTracking/TimesheetEntries components that actually consume this API endpoint.
Are allocation_frequency and carry_forward_days also intentional placeholder values to maintain API consistency between LeaveType and CustomLeave? Or should these fields be omitted from the custom leave response since they don't apply to the CustomLeave model?
/>/)
(>.<)
(" ")
✏️ Learnings added
Learnt from: amolmjoshi93
Repo: saeloun/miru-web PR: 2036
File: app/views/internal_api/v1/time_tracking/index.json.jbuilder:51-67
Timestamp: 2026-02-04T17:32:15.059Z
Learning: In the CustomLeave model (app/models/custom_leave.rb), the database schema does not include `icon` or `color` columns. Custom leaves use hardcoded values `icon: "custom"` and `color: "custom"` in API responses as type identifiers. The frontend applies default styling via `CUSTOM_LEAVE_COLOR` and `CUSTOM_LEAVE_ICON` constants defined in the TimeoffForm and LeaveBlock components.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
…-created-in-settings-leaves-are-not-integrated-with-the-time-off-system
…stencies - Unify custom leave icon lookup in LeaveBlock and TableRow components - LeaveBlock now uses hardcoded "custom" key like TableRow - TableRow updated to use actual customLeave.icon/color attributes - Remove non-existent allocation_frequency and carry_forward_days from CustomLeave jbuilder response (these only exist on LeaveType) - Fix markdown heading hierarchy in time-tracking.md (### to ##) - Correct relative link to leave-management doc (03-leave-management) - Move custom leaves spec context before private methods for proper RSpec structure
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@app/javascript/src/components/LeaveManagement/Container/LeaveBlock.tsx`:
- Around line 29-37: getLeaveColor can return undefined when
generateLeaveColor(color) finds no match, causing a runtime error when reading
leaveColor.value; fix by ensuring getLeaveColor always returns a valid fallback
color object (e.g., for type === "leave" return generateLeaveColor(color) ||
CUSTOM_LEAVE_COLOR, and for the holiday path return generateHolidayColor(color)
|| CUSTOM_LEAVE_COLOR or an appropriate CUSTOM_HOLIDAY_COLOR) so callers can
safely read leaveColor.value, or alternatively add optional chaining/default at
the usage site where leaveColor.value is accessed; update either getLeaveColor,
generateLeaveColor usage, or the access of leaveColor.value accordingly
(referencing getLeaveColor, generateLeaveColor, generateHolidayColor,
CUSTOM_LEAVE_COLOR, and leaveColor.value).
In `@app/services/timeoff_entries/index_service.rb`:
- Around line 116-127: The summary_object is hardcoding icon and color instead
of using the custom_leave attributes; update the hash in index_service.rb where
summary_object is built (referencing summary_object and custom_leave) to set
icon: custom_leave.icon and color: custom_leave.color so the frontend receives
the configured appearance values.
In `@docs/docs/product-guide/05-time-tracking.md`:
- Line 56: Replace the hyphenated phrase "team-members" in the sentence "If you
are an admin, you can select to view time entries for team-members using the
dropdown." with the spaced form "team members" so the sentence reads "If you are
an admin, you can select to view time entries for team members using the
dropdown."; update that exact string in the docs file (the occurrence of
"team-members") to remove the hyphen.
🧹 Nitpick comments (2)
spec/services/timeoff_entries/index_service_spec.rb (1)
302-360: Inconsistent indentation in the custom leaves context block.The
context "with custom leaves"block and its contents have extra indentation (6 spaces instead of 2). This doesn't break functionality but reduces readability and deviates from the rest of the file's style.♻️ Suggested fix (partial)
- context "with custom leaves" do - let(:custom_leave_company) { create(:company) } - let(:custom_leave_user_record) { create(:user, current_workspace_id: custom_leave_company.id) } + context "with custom leaves" do + let(:custom_leave_company) { create(:company) } + let(:custom_leave_user_record) { create(:user, current_workspace_id: custom_leave_company.id) } # ... continue fixing indentation for all linesapp/services/timeoff_entries/index_service.rb (1)
133-144: Month allocation uses 4-week approximation consistent with codebase.The "months" case multiplies by 4 weeks, yielding 20 working days per month for a 5-day week. The actual average is ~21.67 days (260 working days ÷ 12 months), so this may result in slight underallocation for monthly custom leaves. This same approximation is used in
calculate_months_per_year_leave_allocationin the relatedCalculateTotalDurationOfDefinedLeavesService.Verify whether this standardized approximation aligns with your business requirements.
- Add fallback to CUSTOM_LEAVE_COLOR in LeaveBlock.getLeaveColor for all branches (leave type and holiday) to prevent undefined errors when accessing leaveColor.value - Update index_service.rb to use custom_leave.icon and custom_leave.color instead of hardcoded "custom" strings in summary_object - Fix grammar in time-tracking.md: change "team-members" to "team members"
…-created-in-settings-leaves-are-not-integrated-with-the-time-off-system
2 participants
Closes #2035
Summary by CodeRabbit
New Features
Improvements
Documentation
Tests & Migrations
✏️ Tip: You can customize this high-level summary in your review settings.