Testcases and validation message fix for resource category api#3619
Open
nandkishorr wants to merge 3 commits intodevelopfrom
Open
Testcases and validation message fix for resource category api#3619nandkishorr wants to merge 3 commits intodevelopfrom
nandkishorr wants to merge 3 commits intodevelopfrom
Conversation
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
Your free trial has ended. If you'd like to continue receiving code reviews, you can add a payment method here.
Contributor
📝 WalkthroughWalkthroughError messages in Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
care/emr/api/viewsets/resource_category.py (1)
110-137: Consider extracting the repeated access-denied message constant.The same literal is repeated in three places; centralizing it avoids future drift the next time wording changes.
♻️ Proposed refactor
class ResourceCategoryViewSet( @@ ): + ACCESS_DENIED_MSG = "Access denied to resource category" @@ def authorize_create(self, instance): @@ - raise PermissionDenied("Access denied to resource category") + raise PermissionDenied(self.ACCESS_DENIED_MSG) @@ def authorize_update(self, request_obj, model_instance): @@ - raise PermissionDenied("Access denied to resource category") + raise PermissionDenied(self.ACCESS_DENIED_MSG) @@ def get_queryset(self): @@ - raise PermissionDenied("Access denied to resource category") + raise PermissionDenied(self.ACCESS_DENIED_MSG)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@care/emr/api/viewsets/resource_category.py` around lines 110 - 137, The repeated "Access denied to resource category" literal appears in authorize_create, authorize_update, and get_queryset; extract it into a single constant (e.g., RESOURCE_CATEGORY_ACCESS_DENIED) and replace the three inline strings with that constant to centralize the message; update authorize_create, authorize_update, and get_queryset to raise PermissionDenied(RESOURCE_CATEGORY_ACCESS_DENIED) and ensure the constant is defined at module scope near the top of resource_category.py.care/emr/tests/test_resource_category_api.py (3)
20-26: Tiny naming cleanup:premissions→permissions.It works, but this typo makes the setup harder to scan than it needs to.
✏️ Proposed rename
- self.premissions = [ + self.permissions = [ ResourceCategoryPermissions.can_write_resource_category.name, ResourceCategoryPermissions.can_read_resource_category.name, ] self.role = self.create_role_with_permissions( - permissions=self.premissions, + permissions=self.permissions, )As per coding guidelines
**/*.py: Use descriptive variable and function names; adhere to naming conventions (e.g., lowercase with underscores for functions and variables).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@care/emr/tests/test_resource_category_api.py` around lines 20 - 26, Rename the misspelled variable self.premissions to self.permissions in the test setup to follow naming conventions and improve readability; update the assignment where ResourceCategoryPermissions values are collected and any subsequent uses (e.g., the call to create_role_with_permissions) to reference self.permissions instead of self.premissions so all references to the role permissions are consistent.
60-62: Prefer model slug utility over hardcoded slug format in tests.Hardcoding
"f-{facility.external_id}-{slug_value}"makes tests brittle if slug generation rules evolve.🧪 Proposed helper update
def calculate_slug(self, slug_value, facility): - return f"f-{facility.external_id}-{slug_value}" + return ResourceCategory.calculate_slug_from_facility( + facility.external_id, slug_value + )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@care/emr/tests/test_resource_category_api.py` around lines 60 - 62, The test helper calculate_slug is hardcoding the slug format; change it to delegate to the model's slug generation utility (e.g., call ResourceCategory.generate_slug or the actual slug helper on the ResourceCategory model) so tests follow the canonical slug rules. Update calculate_slug to import and call that model method with the facility and slug_value and return its result, ensuring tests remain correct if slug logic changes.
47-58: Remove redundant kwargs merge in test payload helper.
**kwargsis already merged intodataon Line 55, sodata.update(kwargs)repeats the same work.♻️ Proposed simplification
def generate_resource_category_data(self, **kwargs): data = { @@ "facility": kwargs.get("facility", self.facility.external_id), **kwargs, } - data.update(kwargs) return data🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@care/emr/tests/test_resource_category_api.py` around lines 47 - 58, The helper function generate_resource_category_data is redundantly merging kwargs twice (via the dictionary literal using **kwargs and then calling data.update(kwargs)); remove the second merge by deleting the data.update(kwargs) call so the function only merges kwargs once and returns the constructed data object (look for generate_resource_category_data and the data.update(kwargs) line to change).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@care/emr/tests/test_resource_category_api.py`:
- Around line 75-164: Add a test that creates a resource category then attempts
to create another with the same slug_value using generate_resource_category_data
and self.client.post(self.url, ...); assert the second response.status_code is
400 and assert response.data["errors"][0]["msg"] equals the exact duplicate-slug
validation message defined in the viewset/serializer (locate the message in the
ResourceCategoryViewSet or its serializer) to cover the regression for duplicate
slug create.
---
Nitpick comments:
In `@care/emr/api/viewsets/resource_category.py`:
- Around line 110-137: The repeated "Access denied to resource category" literal
appears in authorize_create, authorize_update, and get_queryset; extract it into
a single constant (e.g., RESOURCE_CATEGORY_ACCESS_DENIED) and replace the three
inline strings with that constant to centralize the message; update
authorize_create, authorize_update, and get_queryset to raise
PermissionDenied(RESOURCE_CATEGORY_ACCESS_DENIED) and ensure the constant is
defined at module scope near the top of resource_category.py.
In `@care/emr/tests/test_resource_category_api.py`:
- Around line 20-26: Rename the misspelled variable self.premissions to
self.permissions in the test setup to follow naming conventions and improve
readability; update the assignment where ResourceCategoryPermissions values are
collected and any subsequent uses (e.g., the call to
create_role_with_permissions) to reference self.permissions instead of
self.premissions so all references to the role permissions are consistent.
- Around line 60-62: The test helper calculate_slug is hardcoding the slug
format; change it to delegate to the model's slug generation utility (e.g., call
ResourceCategory.generate_slug or the actual slug helper on the ResourceCategory
model) so tests follow the canonical slug rules. Update calculate_slug to import
and call that model method with the facility and slug_value and return its
result, ensuring tests remain correct if slug logic changes.
- Around line 47-58: The helper function generate_resource_category_data is
redundantly merging kwargs twice (via the dictionary literal using **kwargs and
then calling data.update(kwargs)); remove the second merge by deleting the
data.update(kwargs) call so the function only merges kwargs once and returns the
constructed data object (look for generate_resource_category_data and the
data.update(kwargs) line to change).
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 9e1cf525-1c87-453b-b796-e83e627fb685
📒 Files selected for processing (2)
care/emr/api/viewsets/resource_category.pycare/emr/tests/test_resource_category_api.py
Comment on lines
+75
to
+164
| # Test cases for create resource category api | ||
|
|
||
| def test_create_resource_category_as_super_user(self): | ||
| self.client.force_authenticate(user=self.super_user) | ||
| data = self.generate_resource_category_data() | ||
| response = self.client.post(self.url, data, format="json") | ||
| self.assertEqual(response.status_code, 200) | ||
|
|
||
| def test_create_resource_category_as_regular_user(self): | ||
| self.client.force_authenticate(user=self.user) | ||
| data = self.generate_resource_category_data() | ||
| response = self.client.post(self.url, data, format="json") | ||
| self.assertEqual(response.status_code, 403) | ||
| self.assertEqual(response.data["detail"], "Access denied to resource category") | ||
|
|
||
| def test_create_resource_category_with_parent(self): | ||
| self.client.force_authenticate(user=self.super_user) | ||
| parent_category = self.create_resource_category( | ||
| slug_value="parent-category", | ||
| facility=self.facility, | ||
| title="Parent Category", | ||
| ) | ||
| data = self.generate_resource_category_data( | ||
| title="Child Category", | ||
| slug_value="child-category", | ||
| parent=parent_category.slug, | ||
| ) | ||
| response = self.client.post(self.url, data, format="json") | ||
| self.assertEqual(response.status_code, 200) | ||
| self.assertEqual( | ||
| response.data["parent"]["id"], str(parent_category.external_id) | ||
| ) | ||
|
|
||
| def test_create_resource_category_with_parent_as_regular_user(self): | ||
| self.client.force_authenticate(user=self.user) | ||
| parent_category = self.create_resource_category( | ||
| slug_value="parent-category", | ||
| facility=self.facility, | ||
| title="Parent Category", | ||
| ) | ||
| data = self.generate_resource_category_data( | ||
| title="Child Category", | ||
| slug_value="child-category", | ||
| parent=parent_category.slug, | ||
| ) | ||
| response = self.client.post(self.url, data, format="json") | ||
| self.assertEqual(response.status_code, 403) | ||
| self.assertEqual(response.data["detail"], "Access denied to resource category") | ||
|
|
||
| def test_create_resource_category_with_parent_of_different_resource_type(self): | ||
| self.client.force_authenticate(user=self.super_user) | ||
| parent_category = self.create_resource_category( | ||
| slug_value="parent-category", | ||
| facility=self.facility, | ||
| title="Parent Category", | ||
| resource_type="activity_definition", | ||
| ) | ||
| data = self.generate_resource_category_data( | ||
| title="Child Category", | ||
| slug_value="child-category", | ||
| parent=parent_category.slug, | ||
| resource_type="product_knowledge", | ||
| ) | ||
| response = self.client.post(self.url, data, format="json") | ||
| self.assertEqual(response.status_code, 400) | ||
| self.assertEqual( | ||
| response.data["errors"][0]["msg"], | ||
| "Parent category does not belong to same resource type", | ||
| ) | ||
|
|
||
| def test_create_resource_category_with_parent_as_child_category(self): | ||
| self.client.force_authenticate(user=self.super_user) | ||
| parent_category = self.create_resource_category( | ||
| slug_value="parent-category", | ||
| facility=self.facility, | ||
| title="Parent Category", | ||
| is_child=True, | ||
| ) | ||
| data = self.generate_resource_category_data( | ||
| title="Child Category", | ||
| slug_value="child-category", | ||
| parent=parent_category.slug, | ||
| ) | ||
| response = self.client.post(self.url, data, format="json") | ||
| self.assertEqual(response.status_code, 400) | ||
| self.assertEqual( | ||
| response.data["errors"][0]["msg"], | ||
| "Parent category is a child", | ||
| ) | ||
|
|
Contributor
There was a problem hiding this comment.
One important regression case is still missing: duplicate slug create.
Given the validation-message update in the viewset, there should be a test that attempts a second create with the same slug_value and asserts the exact error message.
✅ Suggested test addition
+ def test_create_resource_category_with_duplicate_slug_rejected(self):
+ self.client.force_authenticate(user=self.super_user)
+ self.create_resource_category(
+ slug_value="duplicate-category",
+ facility=self.facility,
+ title="Existing Category",
+ )
+ data = self.generate_resource_category_data(
+ title="Duplicate Category",
+ slug_value="duplicate-category",
+ )
+ response = self.client.post(self.url, data, format="json")
+ self.assertEqual(response.status_code, 400)
+ self.assertEqual(
+ response.data["errors"][0]["msg"],
+ "Resource category with this slug already exists.",
+ )📝 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.
Suggested change
| # Test cases for create resource category api | |
| def test_create_resource_category_as_super_user(self): | |
| self.client.force_authenticate(user=self.super_user) | |
| data = self.generate_resource_category_data() | |
| response = self.client.post(self.url, data, format="json") | |
| self.assertEqual(response.status_code, 200) | |
| def test_create_resource_category_as_regular_user(self): | |
| self.client.force_authenticate(user=self.user) | |
| data = self.generate_resource_category_data() | |
| response = self.client.post(self.url, data, format="json") | |
| self.assertEqual(response.status_code, 403) | |
| self.assertEqual(response.data["detail"], "Access denied to resource category") | |
| def test_create_resource_category_with_parent(self): | |
| self.client.force_authenticate(user=self.super_user) | |
| parent_category = self.create_resource_category( | |
| slug_value="parent-category", | |
| facility=self.facility, | |
| title="Parent Category", | |
| ) | |
| data = self.generate_resource_category_data( | |
| title="Child Category", | |
| slug_value="child-category", | |
| parent=parent_category.slug, | |
| ) | |
| response = self.client.post(self.url, data, format="json") | |
| self.assertEqual(response.status_code, 200) | |
| self.assertEqual( | |
| response.data["parent"]["id"], str(parent_category.external_id) | |
| ) | |
| def test_create_resource_category_with_parent_as_regular_user(self): | |
| self.client.force_authenticate(user=self.user) | |
| parent_category = self.create_resource_category( | |
| slug_value="parent-category", | |
| facility=self.facility, | |
| title="Parent Category", | |
| ) | |
| data = self.generate_resource_category_data( | |
| title="Child Category", | |
| slug_value="child-category", | |
| parent=parent_category.slug, | |
| ) | |
| response = self.client.post(self.url, data, format="json") | |
| self.assertEqual(response.status_code, 403) | |
| self.assertEqual(response.data["detail"], "Access denied to resource category") | |
| def test_create_resource_category_with_parent_of_different_resource_type(self): | |
| self.client.force_authenticate(user=self.super_user) | |
| parent_category = self.create_resource_category( | |
| slug_value="parent-category", | |
| facility=self.facility, | |
| title="Parent Category", | |
| resource_type="activity_definition", | |
| ) | |
| data = self.generate_resource_category_data( | |
| title="Child Category", | |
| slug_value="child-category", | |
| parent=parent_category.slug, | |
| resource_type="product_knowledge", | |
| ) | |
| response = self.client.post(self.url, data, format="json") | |
| self.assertEqual(response.status_code, 400) | |
| self.assertEqual( | |
| response.data["errors"][0]["msg"], | |
| "Parent category does not belong to same resource type", | |
| ) | |
| def test_create_resource_category_with_parent_as_child_category(self): | |
| self.client.force_authenticate(user=self.super_user) | |
| parent_category = self.create_resource_category( | |
| slug_value="parent-category", | |
| facility=self.facility, | |
| title="Parent Category", | |
| is_child=True, | |
| ) | |
| data = self.generate_resource_category_data( | |
| title="Child Category", | |
| slug_value="child-category", | |
| parent=parent_category.slug, | |
| ) | |
| response = self.client.post(self.url, data, format="json") | |
| self.assertEqual(response.status_code, 400) | |
| self.assertEqual( | |
| response.data["errors"][0]["msg"], | |
| "Parent category is a child", | |
| ) | |
| def test_create_resource_category_with_duplicate_slug_rejected(self): | |
| self.client.force_authenticate(user=self.super_user) | |
| self.create_resource_category( | |
| slug_value="duplicate-category", | |
| facility=self.facility, | |
| title="Existing Category", | |
| ) | |
| data = self.generate_resource_category_data( | |
| title="Duplicate Category", | |
| slug_value="duplicate-category", | |
| ) | |
| response = self.client.post(self.url, data, format="json") | |
| self.assertEqual(response.status_code, 400) | |
| self.assertEqual( | |
| response.data["errors"][0]["msg"], | |
| "Resource category with this slug already exists.", | |
| ) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@care/emr/tests/test_resource_category_api.py` around lines 75 - 164, Add a
test that creates a resource category then attempts to create another with the
same slug_value using generate_resource_category_data and
self.client.post(self.url, ...); assert the second response.status_code is 400
and assert response.data["errors"][0]["msg"] equals the exact duplicate-slug
validation message defined in the viewset/serializer (locate the message in the
ResourceCategoryViewSet or its serializer) to cover the regression for duplicate
slug create.
nandkishorr
changed the title
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.
Proposed Changes
ohcnetwork/roadmap#254
Merge Checklist
/docsOnly PR's with test cases included and passing lint and test pipelines will be reviewed
@ohcnetwork/care-backend-maintainers @ohcnetwork/care-backend-admins
Summary by CodeRabbit