feat: allow non-ASCII characters in schedule comments - #15
Merged
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds a ScheduleFieldDeserializer, its ZCML adapter registration, tests, and a changelog entry to allow JSON (including non‑ASCII) input for Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Client
participant REST_API as REST API
participant Deserializer as ScheduleFieldDeserializer
participant Field as ISchedule_Field
participant Content as Dexterity_Content
Client->>REST_API: PATCH /contact {"schedule": ...}
REST_API->>Deserializer: IFieldDeserializer.__call__(value)
Deserializer->>Deserializer: if string -> json.loads(value)
Deserializer->>Field: field.validate(parsed_value)
Field-->>Deserializer: validated_value
Deserializer-->>REST_API: return validated_value
REST_API->>Content: set schedule = validated_value
REST_API-->>Client: 204 No Content
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 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 docstrings
🧪 Generate unit tests (beta)
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: 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 `@src/imio/directory/core/contents/contact/deserializer.py`:
- Around line 23-25: The deserializer currently calls json.loads(value) without
handling malformed JSON, which causes a 500 instead of a 400; update the block
around json.loads in the deserializer (the code that precedes
self.field.validate(value)) to catch json.JSONDecodeError and re-raise a
ValueError (or raise a new ValueError with a clear message) so the Dexterity
deserializer treats it as a field validation error and returns HTTP 400.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 3e78daaa-ceb6-4912-890d-d49949709060
📒 Files selected for processing (3)
CHANGES.rstsrc/imio/directory/core/contents/contact/configure.zcmlsrc/imio/directory/core/contents/contact/deserializer.py
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/imio/directory/core/tests/test_contact.py (1)
334-345: Consider adding an assertion to verify the resulting schedule state.The test confirms the endpoint accepts an empty schedule payload with a 204 response, but doesn't verify the resulting state. Adding a GET and assertion would clarify the expected behavior (whether the schedule is cleared, left unchanged, or set to an empty dict).
💡 Suggested improvement
def test_schedule_patch_with_empty_value(self): contact = api.content.create( container=self.entity, type="imio.directory.Contact", title="contact", ) transaction.commit() response = self.api_session.patch( contact.absolute_url(), json={"schedule": {}}, ) self.assertEqual(response.status_code, 204) + result = self.api_session.get(contact.absolute_url()).json() + self.assertEqual(result["schedule"], {}) # or expected default state🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/imio/directory/core/tests/test_contact.py` around lines 334 - 345, Update the test_schedule_patch_with_empty_value test to verify the contact's schedule after the PATCH: after calling self.api_session.patch(contact.absolute_url(), json={"schedule": {}}) perform a GET (e.g., self.api_session.get(contact.absolute_url())) and assert the schedule field in the returned JSON matches the expected behavior (cleared, unchanged, or empty dict) so the test both checks the 204 response and the resulting schedule state.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/imio/directory/core/tests/test_contact.py`:
- Around line 334-345: Update the test_schedule_patch_with_empty_value test to
verify the contact's schedule after the PATCH: after calling
self.api_session.patch(contact.absolute_url(), json={"schedule": {}}) perform a
GET (e.g., self.api_session.get(contact.absolute_url())) and assert the schedule
field in the returned JSON matches the expected behavior (cleared, unchanged, or
empty dict) so the test both checks the 204 response and the resulting schedule
state.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 6dcc3c5d-dc60-4e34-ad72-b8b67e2b320a
📒 Files selected for processing (2)
src/imio/directory/core/contents/contact/deserializer.pysrc/imio/directory/core/tests/test_contact.py
🚧 Files skipped from review as they are similar to previous changes (1)
- src/imio/directory/core/contents/contact/deserializer.py
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/imio/directory/core/tests/test_contact.py (1)
342-353: Please assert the persisted contract for empty payloads.Line 353 checks only HTTP status. Add a GET assertion so behavior for
{"schedule": {}}is explicitly pinned (stored as{}, normalized value, or cleared), preventing silent regressions.Suggested test hardening
response = self.api_session.patch( contact.absolute_url(), json={"schedule": {}}, ) self.assertEqual(response.status_code, 204) + result = self.api_session.get(contact.absolute_url()).json() + self.assertIn("schedule", result) + # Assert the expected normalization contract explicitly (adapt value as intended): + # self.assertEqual(result["schedule"], {})🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/imio/directory/core/tests/test_contact.py` around lines 342 - 353, The test test_schedule_patch_with_empty_value only asserts the PATCH returned 204 but does not verify the persisted state; after the api_session.patch call (and existing response.status_code assertion) perform a GET using api_session.get(contact.absolute_url()) and assert the returned JSON body contains the expected representation of the schedule (e.g. schedule equals {} or normalized/cleared value your model uses) to lock down behavior; reference the contact object, api_session.patch and api_session.get to locate where to add the additional assertion and ensure transaction.commit() or any necessary refresh is done before the GET if needed.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/imio/directory/core/tests/test_contact.py`:
- Around line 342-353: The test test_schedule_patch_with_empty_value only
asserts the PATCH returned 204 but does not verify the persisted state; after
the api_session.patch call (and existing response.status_code assertion) perform
a GET using api_session.get(contact.absolute_url()) and assert the returned JSON
body contains the expected representation of the schedule (e.g. schedule equals
{} or normalized/cleared value your model uses) to lock down behavior; reference
the contact object, api_session.patch and api_session.get to locate where to add
the additional assertion and ensure transaction.commit() or any necessary
refresh is done before the GET if needed.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: f53f0141-7d83-4496-9151-9e19b15a8e6f
📒 Files selected for processing (1)
src/imio/directory/core/tests/test_contact.py
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
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.
CITIBDC-568
Summary by CodeRabbit
New Features
Bug Fixes
Tests