Skip to content

Comments

Change copy alternative logic#442

Merged
kaospr merged 1 commit intomainfrom
feature/update-copy-alternatives
Jan 19, 2026
Merged

Change copy alternative logic#442
kaospr merged 1 commit intomainfrom
feature/update-copy-alternatives

Conversation

@kaospr
Copy link
Collaborator

@kaospr kaospr commented Jan 19, 2026

Change "Copy to all of week" to "Copy through rest of week"
image

@kaospr kaospr merged commit e4d95b4 into main Jan 19, 2026
4 checks passed
@kaospr kaospr deleted the feature/update-copy-alternatives branch January 19, 2026 16:26
@claude
Copy link

claude bot commented Jan 19, 2026

Pull Request Review

This PR changes the Copy to all of week functionality to Copy through rest of week, implementing a new rest_of_weekdays helper method.

Code Quality - Strengths

  • Clean implementation with clear intent
  • Good naming convention following existing patterns
  • Locale strings updated in both English and Norwegian

Testing Concerns - CRITICAL

No tests added for the new rest_of_weekdays helper method. Tests should be added to test/helpers/time_regs_helper_test.rb for Monday, Friday, Saturday, and Wednesday edge cases.

Potential Bug

The controllers parse_target_dates method at app/controllers/time_reg/copies_controller.rb:29 expects strings but rest_of_weekdays returns Date objects. The line params[:dates].map { |d| Date.parse(d) } will fail because dates are already Date objects, not strings. Rails may serialize Date objects as strings in URL parameters, but this needs verification via integration test.

Other Findings

  • Performance: No concerns, range operation is efficient
  • Security: No issues, authorization handled correctly

Recommendations

Must fix before merge:

  • Add unit tests for rest_of_weekdays helper method
  • Verify or add integration test for the full copy flow

Nice to have:

  • Add inline comment explaining weekday logic
  • Add system test for complete user flow

The implementation looks solid but needs tests before merging.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant