[messages] add work hours support#390
Open
capcom6 wants to merge 1 commit into
Open
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (6)
✅ Files skipped from review due to trivial changes (2)
🚧 Files skipped from review as they are similar to previous changes (4)
WalkthroughAdds configurable work-hours scheduling for SMS delivery. ChangesWork Hours SMS Scheduling
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant MessagesService
participant MessagesSettings
participant SendMessagesWorker
MessagesService->>MessagesSettings: nextWorkHoursStart()
MessagesSettings-->>MessagesService: future start time
MessagesService->>SendMessagesWorker: start(context, true, startTime)
MessagesService->>MessagesService: delay until next work-hours start
Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@app/src/main/java/me/capcom/smsgateway/modules/messages/MessagesService.kt`:
- Around line 124-125: The SendMessagesWorker.start() method is being called
unconditionally with the replace flag set to true in the else block, which
overwrites any previously queued work and delays pending messages. Instead of
always replacing the work, add a condition to check whether work is already
scheduled before deciding to replace it. Only pass true to the replace parameter
if there is no existing scheduled work, otherwise pass false to preserve the
already queued earlier run.
In `@app/src/main/java/me/capcom/smsgateway/modules/messages/MessagesSettings.kt`:
- Around line 78-82: The isInWorkHours() method does not handle the edge case
where startMinutes equals endMinutes, causing it to always return false and
leading to indefinite deferral of non-expedited messages. Add an explicit check
at the beginning of the return statement to handle the case where startMinutes
equals endMinutes (treat this as always being in work hours), then proceed with
the existing logic for cases where startMinutes is less than or greater than
endMinutes. Apply the same fix to the nextWorkHoursStart() method (referenced in
the "Also applies to" comment) to ensure consistent behavior across both
methods.
- Around line 267-283: The WORK_HOURS_START and WORK_HOURS_END cases in the
import() method accept arbitrary string values without validating their time
format, allowing invalid values to be persisted and later silently coerced to
zero minutes by parseTimeMinutes(). Before calling storage.set(key, newValue) in
both the WORK_HOURS_START and WORK_HOURS_END cases, validate that the newValue
string matches the expected time format (HH:MM) using a regex pattern or parsing
attempt, and either reject invalid values by returning false or using a more
informative default that indicates validation failed. This same validation
should also be applied to the other work-hours related cases mentioned at lines
113-117.
In
`@app/src/main/java/me/capcom/smsgateway/ui/settings/MessagesSettingsFragment.kt`:
- Line 60: The String.format call formatting the time display (with pattern
"%02d:%02d" for hours and minutes) uses the default locale, which can produce
non-ASCII numerals on some devices and break downstream parsing. Fix this by
specifying Locale.US as the first parameter to String.format before the format
string to ensure consistent ASCII numerals are always used for the persisted
HH:mm time format.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 1a347e29-2d38-4d66-b8d8-e10526418c0f
📒 Files selected for processing (5)
app/src/main/java/me/capcom/smsgateway/modules/messages/MessagesService.ktapp/src/main/java/me/capcom/smsgateway/modules/messages/MessagesSettings.ktapp/src/main/java/me/capcom/smsgateway/ui/settings/MessagesSettingsFragment.ktapp/src/main/res/values/strings.xmlapp/src/main/res/xml/messages_preferences.xml
capcom6
force-pushed
the
messages/work-hours-support
branch
2 times, most recently
from
9dd438c to
82266a8
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@app/src/main/java/me/capcom/smsgateway/modules/messages/MessagesSettings.kt`:
- Line 152: The VERSION_CODE constant in MessagesSettings has been incorrectly
decremented to 1, which violates the requirement for monotonic increasing
migration versions. This will cause already-migrated installations with higher
version codes to be treated as if they need older schema migrations, breaking
the migration sequence. Revert VERSION_CODE to its previous value (which was
higher than 1). Migration version codes must only increase or remain unchanged,
never decrease.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 6a9fd13c-aa8a-4e75-8b6b-a11dae090c77
📒 Files selected for processing (1)
app/src/main/java/me/capcom/smsgateway/modules/messages/MessagesSettings.kt
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@app/src/main/java/me/capcom/smsgateway/modules/messages/MessagesSettings.kt`:
- Line 152: The VERSION_CODE constant in MessagesSettings has been incorrectly
decremented to 1, which violates the requirement for monotonic increasing
migration versions. This will cause already-migrated installations with higher
version codes to be treated as if they need older schema migrations, breaking
the migration sequence. Revert VERSION_CODE to its previous value (which was
higher than 1). Migration version codes must only increase or remain unchanged,
never decrease.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 6a9fd13c-aa8a-4e75-8b6b-a11dae090c77
📒 Files selected for processing (1)
app/src/main/java/me/capcom/smsgateway/modules/messages/MessagesSettings.kt
🛑 Comments failed to post (1)
app/src/main/java/me/capcom/smsgateway/modules/messages/MessagesSettings.kt (1)
152-152: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Do not decrement settings migration version code.
Line 152 regresses
VERSION_CODEto1. That can rewrite already-migrated installs (e.g., stored version2) back to an older schema version and break future migration sequencing. Keep migration versions monotonic increasing.Suggested fix
- private const val VERSION_CODE = 1 + private const val VERSION_CODE = 2📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.private const val VERSION_CODE = 2🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/src/main/java/me/capcom/smsgateway/modules/messages/MessagesSettings.kt` at line 152, The VERSION_CODE constant in MessagesSettings has been incorrectly decremented to 1, which violates the requirement for monotonic increasing migration versions. This will cause already-migrated installations with higher version codes to be treated as if they need older schema migrations, breaking the migration sequence. Revert VERSION_CODE to its previous value (which was higher than 1). Migration version codes must only increase or remain unchanged, never decrease.
capcom6
force-pushed
the
messages/work-hours-support
branch
from
698f35b to
f2f4d4d
Compare
Contributor
🤖 Pull request artifacts
|
capcom6
force-pushed
the
messages/work-hours-support
branch
from
f2f4d4d to
36c0cca
Compare
capcom6
force-pushed
the
messages/work-hours-support
branch
from
36c0cca to
d23aed7
Compare
capcom6
force-pushed
the
messages/work-hours-support
branch
from
d23aed7 to
53dcdba
Compare
capcom6
force-pushed
the
messages/work-hours-support
branch
from
53dcdba to
559794b
Compare
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
Summary by CodeRabbit