Introduce sowing_period, refactor period parsing, update admin/bot texts and add migrations#18
Merged
Merged
Conversation
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
WalkthroughThe changes add a new Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ 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.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
backend/bot/bot.py (1)
147-173:⚠️ Potential issue | 🟡 MinorHeader wording does not match readiness logic
The message says plants are “ready for transplanting”, but current filtering still includes future windows (when
start_date > now). Either tighten the filter to “ready now” or soften the header to “upcoming transplanting periods”.Suggested fix (ready-now behavior)
- answer = _("Your plants ready for transplanting:") + "\n" + answer = _("Your plants ready for transplanting:") + "\n" @@ - if end_date < now: + if now < start_date or end_date < now: continue🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/bot/bot.py` around lines 147 - 173, The header "Your plants ready for transplanting" conflicts with the loop which includes future windows; update the logic in bot.py so the header and filtering match: either (A) tighten the filter to only include plants whose parsed_planting_period contains today (i.e. parsed_planting_period -> start_date and end_date and require start_date <= now <= end_date before setting plants_found and appending to answer) or (B) change the header text in the answer variable to something like "Your upcoming transplanting periods" to reflect that future windows are listed; locate symbols parsed_planting_period, start_date, end_date, now, the plants iteration and the answer string to apply the change.
🧹 Nitpick comments (1)
backend/diary/models.py (1)
152-186: Typo:romanian_monthsshould beroman_months.The dictionary maps Roman numerals (I, II, III, etc.) to integers. "Romanian" refers to the country/language, while "Roman" refers to the numeral system.
✏️ Suggested fix
- romanian_months = { + roman_months = { "I": 1, "II": 2, "III": 3, "IV": 4, "V": 5, "VI": 6, "VII": 7, "VIII": 8, "IX": 9, "X": 10, "XI": 11, "XII": 12, } try: start_str, end_str = period.split("-") start_day, start_month = start_str.split(".") end_day, end_month = end_str.split(".") today = date.today() - start_date = date(today.year, romanian_months[start_month], int(start_day)) - end_date = date(today.year, romanian_months[end_month], int(end_day)) + start_date = date(today.year, roman_months[start_month], int(start_day)) + end_date = date(today.year, roman_months[end_month], int(end_day))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/diary/models.py` around lines 152 - 186, Rename the incorrect variable name romanian_months to roman_months inside the _parse_period static method and update any references within that function (e.g., where romanian_months[start_month] and romanian_months[end_month] are used) so the dictionary name matches the Roman numeral mapping; ensure no other semantics change and run tests to confirm no remaining references to the old name.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@backend/bot/bot.py`:
- Around line 147-173: The header "Your plants ready for transplanting"
conflicts with the loop which includes future windows; update the logic in
bot.py so the header and filtering match: either (A) tighten the filter to only
include plants whose parsed_planting_period contains today (i.e.
parsed_planting_period -> start_date and end_date and require start_date <= now
<= end_date before setting plants_found and appending to answer) or (B) change
the header text in the answer variable to something like "Your upcoming
transplanting periods" to reflect that future windows are listed; locate symbols
parsed_planting_period, start_date, end_date, now, the plants iteration and the
answer string to apply the change.
---
Nitpick comments:
In `@backend/diary/models.py`:
- Around line 152-186: Rename the incorrect variable name romanian_months to
roman_months inside the _parse_period static method and update any references
within that function (e.g., where romanian_months[start_month] and
romanian_months[end_month] are used) so the dictionary name matches the Roman
numeral mapping; ensure no other semantics change and run tests to confirm no
remaining references to the old name.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
AGENTS.mdbackend/bot/bot.pybackend/catalogs/admin.pybackend/catalogs/migrations/0005_alter_planting_period_labels.pybackend/catalogs/migrations/0006_add_sowing_period_and_restore_planting_period.pybackend/catalogs/models.pybackend/diary/admin.pybackend/diary/models.py
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
backend/diary/models.py (1)
183-183: Make parse-failure logs more diagnosable.The warning currently omits the invalid
periodvalue and exception details, which makes production troubleshooting harder.🛠️ Proposed improvement
- except (ValueError, KeyError): - logger.warning("Failed to parse %s for plant %s", period_name, plant_id) + except (ValueError, KeyError) as exc: + logger.warning( + "Failed to parse %s for plant %s: value=%r error=%s", + period_name, + plant_id, + period, + exc, + ) return None🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/diary/models.py` at line 183, The log in the parse failure (logger.warning("Failed to parse %s for plant %s", period_name, plant_id)) omits the invalid period value and exception details; update the error logging in the parse path that references period_name and plant_id to capture the exception (e.g., as e) and include both the offending period value and the exception info in the log message (or use logger.exception / exc_info=True) so the log shows period, plant_id, and exception traceback for diagnosability.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@backend/diary/models.py`:
- Around line 178-181: The start_date and end_date are being built using the
same year, which fails for ranges that cross year boundaries (e.g., start_date
from November and end_date in February); after computing start_date and end_date
(using roman_months, start_month, end_month, start_day, end_day), check if
end_date < start_date and if so adjust end_date to the next calendar year
(increment its year by 1) so the returned tuple is always a forward-facing
range.
---
Nitpick comments:
In `@backend/diary/models.py`:
- Line 183: The log in the parse failure (logger.warning("Failed to parse %s for
plant %s", period_name, plant_id)) omits the invalid period value and exception
details; update the error logging in the parse path that references period_name
and plant_id to capture the exception (e.g., as e) and include both the
offending period value and the exception info in the log message (or use
logger.exception / exc_info=True) so the log shows period, plant_id, and
exception traceback for diagnosability.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
backend/bot/bot.pybackend/diary/models.py
🚧 Files skipped from review as they are similar to previous changes (1)
- backend/bot/bot.py
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@backend/diary/models.py`:
- Around line 183-187: The start_date is always built with today.year which
mis-anchors wrapped ranges spanning year boundaries; change the logic so when
the range wraps (i.e., (end_month_i, end_day_i) < (start_month_i, start_day_i))
you set end_year = today.year and start_year = end_year - 1, otherwise set
start_year = today.year; then construct start_date = date(start_year,
start_month_i, start_day_i) and end_date = date(end_year, end_month_i,
end_day_i). This updates the variables referenced (start_date, end_date,
end_year, start_month_i, start_day_i, end_month_i, end_day_i) to correctly
anchor cross-year periods.
capcom6
force-pushed
the
codex/add-planting-period-for-crops
branch
from
0543834 to
0624b83
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@backend/bot/bot.py`:
- Around line 167-175: The fallback message is misleading because the date
filter (now < start_date or end_date < now) can make plants_found False even
when the user has plants; update the fallback logic that builds answer (uses
variables plants_found, now, start_date, end_date and appends plant.name and
planting_period) to return a message indicating no plants are currently within
the transplanting window (e.g., "You don't have any plants ready for
transplanting right now.") and ensure you use the translation function _() for
the new message so localization remains consistent.
In `@backend/catalogs/models.py`:
- Line 6: The PLANTING_PERIOD_REGEX only enforces format but allows impossible
combinations (e.g., 31.II); add a parser-backed validator that runs after the
regex: implement a function (e.g., validate_planting_period) that parses the two
dates from the string using the same Roman-month tokens as in
PLANTING_PERIOD_REGEX, map Roman numerals to month numbers, validate day bounds
against each month's actual max days (account for February 28/29 if needed or
assume non-leap for schedule domain), ensure the start date is <= end date, and
raise ValidationError when invalid; wire this validator into the model/field
that uses PLANTING_PERIOD_REGEX so impossible month/day combos are rejected
before runtime parsing.
In `@backend/diary/models.py`:
- Around line 177-196: Replace non-timezone-aware date.today() calls with
Django's timezone.localdate(): add "from django.utils import timezone" import
and change the two usages shown — the earlier date.today().strftime(...) and the
later today = date.today() used to compute start_year/end_year and
start_date/end_date — to use timezone.localdate() (keep the same .strftime and
tuple comparisons logic); ensure roman_months, start_date and end_date logic
remains unchanged.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
backend/bot/bot.pybackend/catalogs/admin.pybackend/catalogs/migrations/0005_planttype_sowing_period_plantvariety_sowing_period.pybackend/catalogs/models.pybackend/diary/admin.pybackend/diary/models.py
capcom6
force-pushed
the
codex/add-planting-period-for-crops
branch
3 times, most recently
from
30fd70f to
6996294
Compare
capcom6
force-pushed
the
codex/add-planting-period-for-crops
branch
from
6996294 to
fd1b660
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.
Motivation
sowing_periodalongside existingplanting_periodto model separate semantics for sowing vs transplanting.Description
AGENTS.mdwith contributor notes about project structure, domain semantics, and developer checks.sowing_periodCharField onPlantTypeandPlantVarietywith the same regex validation asplanting_period, and extracted the regex intoPLANTING_PERIOD_REGEXinbackend/catalogs/models.py.backend/diary/models.pyby adding a static helper_parse_periodand separatesowing_period/parsed_sowing_periodproperties while preservingplanting_periodandparsed_planting_period, and improved logging to includeperiod_nameandplant_id.PlantTypeAdminandPlantVarietyAdminnow includesowing_periodinfieldsandlist_display, andPlantAdminshowssowing_periodandplanting_periodcolumns with display helpers.backend/bot/bot.pyfor the/plantingcommand to clarify transplanting readiness and changed the label from "Planned planting period" to "Planned transplanting period".backend/catalogs/migrations/0005_alter_planting_period_labels.pyand0006_add_sowing_period_and_restore_planting_period.pyto alter verbose names/validators and addsowing_periodfields.Testing
python -m compileallwhich completed without syntax errors.python backend/manage.py makemigrations --checkto confirm migrations are in place and it reported no additional model changes.python backend/manage.py testand the test suite passed.Codex Task
Summary by CodeRabbit
New Features
Bug Fixes
Chores