2027 special chars not allowed in holiday list v2 #2045
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…sh message - Update BulkHolidayService to collect field-level validation errors instead of raising exceptions, returning success/failure boolean - Modify HolidaysController#update to return field_errors in response - Add error state management in Holidays component to track and display field-level errors for both public and optional holidays - Update EditHolidays component to show error messages near name inputs with red border styling - Skip generic toast in axios interceptor when field_errors are present - Update HolidayInfo name validation to allow hyphens and apostrophes (e.g., "New Year's Day", "Labor-Day") with descriptive error message - Fix net_days calculation in TimeoffEntries::IndexService to preserve negative values for leave balance display - Update related specs for BulkHolidayService and HolidayInfo model
- Replace save! with save in BulkHolidayService to collect database errors gracefully - Fix occurrence-aware error mapping in Holidays component for duplicate names - Enhance leave balance labels to show "Overdrawn by X" for negative balances - Add test coverage for zero and negative leave balance scenarios This ensures consistent error collection patterns, clearer user feedback for overdrawn leave balances, and addresses security vulnerabilities.
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📝 WalkthroughWalkthroughController now surfaces per-item holiday validation errors from BulkHolidayService to the frontend; BulkHolidayService collects per-index field errors and aborts on validation failures; frontend shows inline per-item errors and suppresses generic toasts when field_errors are returned; name validation and timeoff labeling adjusted; tests and Wise test helpers updated. Changes
Sequence Diagram(s)sequenceDiagram
participant User as User
participant Frontend as Frontend
participant API as Server(API)
participant Service as BulkHolidayService
participant DB as Database
User->>Frontend: Submit edited holidays
Frontend->>API: PATCH /internal_api/v1/holidays (payload)
API->>Service: BulkHolidayService.new(...).process
Service->>DB: validate/build/save add/update HolidayInfo
alt validations pass
Service-->>API: returns true
API-->>Frontend: 200 OK
Frontend->>User: show success, exit edit mode
else validation errors
Service-->>Service: collect per-index errors into errors hash
Service-->>API: returns false
API-->>Frontend: 422 { field_errors: {...} }
Frontend->>Frontend: map field_errors -> holidayErrors / optionalHolidayErrors
Frontend->>User: render ErrorSpan and red-bordered inputs
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 3❌ Failed checks (2 warnings, 1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches🧪 Generate unit tests (beta)
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: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/javascript/src/apis/api.ts (1)
36-59: Potential duplicate toast on 401 errors.The 401 handler (lines 37-41) shows a toast and schedules a redirect but doesn't return early. Execution then falls through to lines 44-53, which may show another generic toast if
field_errorsis absent. This could result in duplicate error toasts for authentication failures.🐛 Suggested fix
(error: any) => { if (error.response?.status === 401) { clearCredentialsFromLocalStorage(); Toastr.error(error.response?.data?.error); setTimeout(() => (window.location.href = "/"), 500); + return Promise.reject(error); } // Skip generic toast if field_errors are present (handled by the component)
🤖 Fix all issues with AI agents
In `@app/javascript/src/components/Profile/Organization/Holidays/index.tsx`:
- Around line 287-403: The error maps created in saveUpdatedHolidayDetails use
list indices, which can drift when rows are deleted; update handleDeleteHoliday
to reindex or clear those maps after removing an item by remapping errors based
on stable identifiers (id or name) or by rebuilding
newHolidayErrors/newOptionalHolidayErrors to match the post-delete holidayList
and optionalHolidaysList order, then call setHolidayErrors and
setOptionalHolidayErrors with the rebuilt maps so messages stay attached to the
correct rows.
- Line 471: The file exporting the Holidays component (export default Holidays)
is missing a final newline; add a single trailing newline character at the end
of the file so the EOF ends with a newline to satisfy Prettier and POSIX
requirements.
🧹 Nitpick comments (2)
app/services/timeoff_entries/index_service.rb (2)
71-86: Consider pluralization for singular values.Labels like
"1 hours"or"1 days"are grammatically incorrect. Consider using Rails'pluralizehelper or conditional logic to handle singular/plural forms properly (e.g.,"1 hour"vs"2 hours").♻️ Suggested improvement
+ label = if net_hours.zero? + "0 hours" elsif net_hours.negative? total_overdrawn_hours = net_hours.abs if total_overdrawn_hours < `@working_hours_per_day` - "Overdrawn by #{total_overdrawn_hours} hours" + "Overdrawn by #{total_overdrawn_hours} #{'hour'.pluralize(total_overdrawn_hours)}" else overdrawn_days = total_overdrawn_hours / `@working_hours_per_day` overdrawn_extra_hours = total_overdrawn_hours % `@working_hours_per_day` - "Overdrawn by #{overdrawn_days} days #{overdrawn_extra_hours} hours" + "Overdrawn by #{overdrawn_days} #{'day'.pluralize(overdrawn_days)} #{overdrawn_extra_hours} #{'hour'.pluralize(overdrawn_extra_hours)}" end elsif net_hours < `@working_hours_per_day` - "#{net_hours} hours" + "#{net_hours} #{'hour'.pluralize(net_hours)}" else - "#{net_days} days #{extra_hours} hours" + "#{net_days} #{'day'.pluralize(net_days)} #{extra_hours} #{'hour'.pluralize(extra_hours)}" end
84-86: Consider omitting "0 hours" suffix when extra_hours is zero.When the balance is exactly divisible by working hours per day (e.g.,
net_hours == 8), the label will be"1 days 0 hours". Hiding the hours portion whenextra_hours.zero?would produce cleaner output like"1 day".
app/javascript/src/components/Profile/Organization/Holidays/index.tsx
Outdated
Show resolved
Hide resolved
- Add Wise API environment variable stubs for test environment - Update VCR cassette profile ID to match test configuration (16455649) - Fix error message misalignment when deleting holiday entries by reindexing error maps - Add missing newline at end of Holidays component file Fixes VCR::Errors::UnhandledHTTPRequestError in Wise API tests and prevents error messages from appearing on wrong holiday rows after deletions.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@app/javascript/src/components/Profile/Organization/Holidays/index.tsx`:
- Around line 317-433: The catch in saveUpdatedHolidayDetails currently only
processes error.response.data.field_errors and otherwise swallows the error;
update the catch in saveUpdatedHolidayDetails to handle non-field errors by
either re-throwing the caught error (so the global axios interceptor handles it)
or invoking the app's toast/error notifier with a generic message (e.g., "Failed
to save holidays") when field_errors is absent; place this logic inside the
existing catch after determining fieldErrors so that
update_holiday_infos/add_holiday_infos handling remains unchanged.
🧹 Nitpick comments (2)
spec/support/wise_setup.rb (1)
3-10: Consider scoping this hook to Wise-related tests only.This
beforehook runs before every test in the suite, which is broader than necessary and may cause unintended side effects. Consider using RSpec metadata to limit the scope:RSpec.configure do |config| - config.before do + config.before(:each, :wise) do # Stub Wise environment variables for tests allow(ENV).to receive(:[]).and_call_original allow(ENV).to receive(:[]).with("WISE_API_URL").and_return("https://api.sandbox.transferwise.tech") allow(ENV).to receive(:[]).with("WISE_ACCESS_TOKEN").and_return("access-token-12345") allow(ENV).to receive(:[]).with("WISE_PROFILE_ID").and_return("16455649") end endThen tag relevant specs with
:wisemetadata (e.g.,describe "Wise API", :wise do).app/javascript/src/components/Profile/Organization/Holidays/index.tsx (1)
436-446: Consider clearing error states when canceling edits.When the user cancels,
holidayListandoptionalHolidaysListare reset viaupdateHolidaysList, butholidayErrorsandoptionalHolidayErrorsretain stale values. If the user re-enters edit mode, old errors could briefly appear until the next save attempt clears them.♻️ Suggested fix
const handleCancelAction = () => { if (isDesktop) { if (holidays.length) { updateHolidaysList(holidays); } setIsDetailUpdated(false); setIsEditable(false); + setHolidayErrors({}); + setOptionalHolidayErrors({}); } else { navigate("/settings/profile"); } };
- Import Toastr component for error notifications - Add fallback error handling in saveUpdatedHolidayDetails catch block - Show "Failed to save holidays" toast for non-validation errors - Prevents silent failures for network/server errors while preserving existing field error handling Ensures users are always notified when holiday save operations fail, improving UX by eliminating silent error states.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@app/javascript/src/components/Profile/Organization/Holidays/index.tsx`:
- Around line 238-243: The current logic deletes the entire holidayErrors[index]
object when a single field changes; instead, update the handlers so they only
remove the specific field error: in the name-change handler, if
holidayErrors[index]?.name exists create a shallow copy (const newErrors = {
...holidayErrors }) and remove only newErrors[index].name before calling
setHolidayErrors(newErrors); likewise in the date-change handler remove only
newErrors[index].date (and if that leaves newErrors[index] empty delete
newErrors[index]). Use the same holidayErrors and setHolidayErrors symbols in
both places (also apply to the other handler referenced at lines 248-253) so
other field errors (e.g., date or category) are preserved.
🧹 Nitpick comments (1)
app/javascript/src/components/Profile/Organization/Holidays/index.tsx (1)
192-210: Avoid mutating state arrays before error remap.Cloning before
splicekeeps React state updates predictable and avoids side effects while you reindex error maps.♻️ Suggested tweak
- const updatedHolidayList = holidayList; + const updatedHolidayList = [...holidayList]; updatedHolidayList.splice(index, 1); setHolidayList([...updatedHolidayList]); @@ - const updatedHolidayList = optionalHolidaysList; + const updatedHolidayList = [...optionalHolidaysList]; updatedHolidayList.splice(index, 1); setOptionalHolidaysList([...updatedHolidayList]);Also applies to: 215-229
app/javascript/src/components/Profile/Organization/Holidays/index.tsx
Outdated
Show resolved
Hide resolved
- Scope Wise environment variable stubs to :wise tagged tests only
- Updated spec/support/wise_setup.rb to use metadata filtering
- Tagged all Wise-related test files with :wise metadata
- Improves test performance by avoiding unnecessary setup overhead
- Clear error states when canceling holiday edits
- Added setHolidayErrors({}) and setOptionalHolidayErrors({}) to handleCancelAction
- Prevents stale error messages from appearing when re-entering edit mode
…ions in Holidays component - Implement granular error clearing for holiday form fields - Update error state structure to support field-specific errors (name, date, etc.) - Modify handleHolidateNameChange to only clear name field errors - Modify handleDatePicker to only clear date field errors - Preserve other field validation errors when individual fields are corrected - Fix state mutation issues in handleDeleteHoliday - Clone arrays before splice operations to avoid direct state mutation - Remove unnecessary double spreading for better performance - Ensure predictable React state updates and re-renders - Update error processing to preserve API field-specific error structure - Change error types from Record<number, string> to Record<number, Record<string, string[]>> - Update all error handling functions to work with new structure - Maintain backward compatibility with existing error display logic Improves UX by preserving validation context when users fix individual fields and follows React best practices for immutable state updates.
…-not-allowed-in-holiday-list-v2
…sive controller tests - Fix api.ts to return early after handling 401 errors to prevent duplicate toasts - Add comprehensive controller tests for holidays endpoint covering: - Valid and invalid holiday creation - Field-level error validation - Special characters support (hyphens and apostrophes) - Holiday update scenarios with validation - Proper error response structure (422 with field_errors) All tests passing.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Separate Commit is done to cover https://github.com/saeloun/miru-web/pull/2028/commits
Resolves #2028
Closes #2027
Summary by CodeRabbit
New Features
Improvements
Bug Fixes
Tests
✏️ Tip: You can customize this high-level summary in your review settings.