[Forge][P1] Allow editing custom Post Types after creation#246
Conversation
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughAdded a dedicated custom post type edit page with a form supporting name, prompt guidance, and logo updates. Includes explicit logo removal functionality and form-level validation to prevent simultaneous uploads and removals. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
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 unit tests (beta)
📝 Coding Plan
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 |
Greptile SummaryThis PR replaces the previous inline-edit forms on the custom post types list page with a dedicated Key changes and findings:
Confidence Score: 2/5
Important Files Changed
Sequence DiagramsequenceDiagram
participant Browser
participant ListPage as project_custom_post_types (GET/POST)
participant EditView as ProjectCustomPostTypeEditView /edit/
participant UpdateView as ProjectCustomPostTypeUpdateView /update/ (legacy)
participant Form as ProjectCustomPostTypeForm
participant DB as Database / Storage
Browser->>ListPage: GET /project/:pk/posts/custom/
ListPage-->>Browser: list + create form
Browser->>EditView: GET /project/:pk/posts/custom/:id/edit/
Note over EditView: dispatch() — get_object_or_404<br/>(runs BEFORE LoginRequiredMixin auth check ⚠️)
EditView->>Form: ProjectCustomPostTypeForm(instance=cpt)
EditView-->>Browser: edit page with pre-filled form
Browser->>EditView: POST /edit/ (name, prompt, logo?, remove_logo?)
EditView->>Form: form.is_valid()
alt valid
Form->>DB: form.save() — update fields, delete logo if remove_logo
EditView-->>Browser: 302 → project_custom_post_types
else invalid
EditView-->>Browser: 200 edit page with inline errors
end
Browser->>UpdateView: POST /update/ (legacy path)
UpdateView->>EditView: ProjectCustomPostTypeEditView.as_view()(request)
EditView-->>Browser: same response as /edit/ POST path
Last reviewed commit: "feat(posts): add ded..." |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
core/views.py
Outdated
| def dispatch(self, request, *args, **kwargs): | ||
| self.project = get_object_or_404(Project, pk=kwargs["pk"], profile=request.user.profile) | ||
| self.custom_post_type = get_object_or_404( | ||
| ProjectCustomPostType, | ||
| pk=kwargs["post_type_pk"], | ||
| project=self.project, | ||
| ) | ||
| return super().dispatch(request, *args, **kwargs) |
There was a problem hiding this comment.
dispatch() accesses request.user.profile before LoginRequiredMixin runs
The overridden dispatch() accesses request.user.profile on lines 1551–1556 before calling super().dispatch(), which is where LoginRequiredMixin performs the authentication check. For an unauthenticated request, request.user is AnonymousUser, which has no profile attribute — this raises AttributeError (a 500) instead of redirecting to the login page.
Compare this with the safe pattern used by ProjectCustomPostTypePostsView and ProjectCustomPostTypesView, where request.user.profile is only accessed inside get_queryset() / get_context_data(), both of which are called after LoginRequiredMixin has already verified the user is authenticated.
Fix: guard profile access or restructure to let super().dispatch() run first:
def dispatch(self, request, *args, **kwargs):
# Let LoginRequiredMixin redirect unauthenticated users first.
response = super().dispatch(request, *args, **kwargs)
if not request.user.is_authenticated:
return response
self.project = get_object_or_404(Project, pk=kwargs["pk"], profile=request.user.profile)
self.custom_post_type = get_object_or_404(
ProjectCustomPostType,
pk=kwargs["post_type_pk"],
project=self.project,
)
return self.get(request, *args, **kwargs) if request.method == "GET" else self.post(request, *args, **kwargs)Or, more cleanly, move the lookups into a helper that both get() and post() call after the mixin has checked auth.
core/views.py
Outdated
| class ProjectCustomPostTypeUpdateView(LoginRequiredMixin, View): | ||
| def post(self, request, pk, post_type_pk): | ||
| return ProjectCustomPostTypeEditView.as_view()(request, pk=pk, post_type_pk=post_type_pk) |
There was a problem hiding this comment.
/update/ endpoint now delegates to edit view
ProjectCustomPostTypeUpdateView now just proxies every POST to ProjectCustomPostTypeEditView. The list template was updated to link directly to the /edit/ route, so /update/ is no longer the canonical path for any UI action.
This leaves a duplicate endpoint that subtly changes the old behaviour: a failed form submission on /update/ now returns a full 200 HTML page (the edit template) instead of the previous redirect-with-messages to the list page. Any client (e.g. integration tests, external consumers) that relied on the old redirect-on-failure contract will silently get HTML instead of a 302.
Consider either removing the /update/ URL entry (and this class) entirely — or, if backward compat is needed, explicitly documenting it as a deprecated alias with a note that it may be removed later.
| {% if custom_post_type.logo %} | ||
| <div class="flex gap-2 items-center"> | ||
| <input id="id_remove_logo" name="remove_logo" type="checkbox" value="true" {% if form.remove_logo.value %}checked{% endif %} class="w-4 h-4 text-gray-900 rounded border-gray-300 focus:ring-gray-500" /> | ||
| <label for="id_remove_logo" class="text-sm text-gray-700">Remove current logo</label> | ||
| </div> | ||
| {% for error in form.remove_logo.errors %}<p class="mt-1 text-sm text-red-600">{{ error }}</p>{% endfor %} | ||
| {% endif %} |
There was a problem hiding this comment.
remove_logo + error
Both the header preview image (line 12) and the "Remove current logo" checkbox visibility (line 47) are driven by custom_post_type.logo, which is the database state — not the form submission state. If a user checks "Remove current logo", also triggers another validation error (unlikely but possible), and the form re-renders, they will see the old logo in the header and the checkbox still available, even though their intent was to remove it. The checkbox will be pre-checked (because form.remove_logo.value is True) so re-submitting works correctly — but the stale logo preview can look confusing.
This is a minor UX polish item: consider updating the header preview to use the form's current state rather than the DB instance logo, e.g. hiding it when form.remove_logo.value is truthy.
![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
🧹 Nitpick comments (1)
core/tests/test_custom_post_types.py (1)
194-247: Add one regression for the new remove-vs-upload validation branch.The new coverage exercises successful edits and duplicate-name failures, but it never submits both
remove_logoandlogo, so the new edit-specific validation path is still unpinned.🧪 Suggested test
+@pytest.mark.django_db +@override_settings(MEDIA_ROOT="/tmp/tuxseo-test-media") +def test_edit_custom_post_type_rejects_remove_logo_and_new_logo_together(client): + user = User.objects.create_user( + "owner-conflict-logo", + "owner-conflict-logo@example.com", + "secret", + ) + project = Project.objects.create(profile=user.profile, name="Site", url="https://site.test") + post_type = ProjectCustomPostType.objects.create( + project=project, + name="Tutorial", + prompt_guidance="Step-by-step instructional style.", + logo=SimpleUploadedFile("existing.png", TINY_PNG_BYTES, content_type="image/png"), + ) + + client.force_login(user) + response = client.post( + reverse("project_custom_post_type_edit", kwargs={"pk": project.id, "post_type_pk": post_type.id}), + { + "name": "Tutorial", + "prompt_guidance": "Updated instructional style.", + "remove_logo": "true", + "logo": SimpleUploadedFile("logo.png", TINY_PNG_BYTES, content_type="image/png"), + }, + ) + + assert response.status_code == 200 + assert "Choose either remove logo or upload a new logo, not both." in response.content.decode("utf-8")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/tests/test_custom_post_types.py` around lines 194 - 247, Add a regression test that posts both "remove_logo" and a new "logo" together to exercise the remove-vs-upload validation branch: create a user/project/PostType with an existing logo, log in, POST to the same "project_custom_post_type_edit" endpoint with form data including "remove_logo": "true" and a SimpleUploadedFile(...) under "logo", then assert the response indicates a validation error (no 302 redirect), the form contains the expected error message, and the post_type.logo remains unchanged; name the test something like test_edit_custom_post_type_remove_and_upload_logo_conflict to mirror the existing tests.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@core/forms.py`:
- Around line 400-408: The save method currently deletes instance.logo
immediately when self.cleaned_data.get("remove_logo") is true; instead, capture
the existing file object (old_logo = instance.logo), set instance.logo = None so
the DB will be updated, then perform instance.save() and, only if commit is
True, schedule the file deletion inside transaction.on_commit(lambda:
old_logo.delete(save=False)); do not delete the file when commit is False.
Reference: save(), self.cleaned_data.get("remove_logo"), instance.logo, and
transaction.on_commit.
---
Nitpick comments:
In `@core/tests/test_custom_post_types.py`:
- Around line 194-247: Add a regression test that posts both "remove_logo" and a
new "logo" together to exercise the remove-vs-upload validation branch: create a
user/project/PostType with an existing logo, log in, POST to the same
"project_custom_post_type_edit" endpoint with form data including "remove_logo":
"true" and a SimpleUploadedFile(...) under "logo", then assert the response
indicates a validation error (no 302 redirect), the form contains the expected
error message, and the post_type.logo remains unchanged; name the test something
like test_edit_custom_post_type_remove_and_upload_logo_conflict to mirror the
existing tests.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 9ead4034-2bbc-44c8-91b6-941768b0a7bb
📒 Files selected for processing (7)
CHANGELOG.mdcore/forms.pycore/tests/test_custom_post_types.pycore/urls.pycore/views.pyfrontend/templates/project/project_custom_post_type_edit.htmlfrontend/templates/project/project_custom_post_types.html
1 participant
Summary\n- add a dedicated edit route/view for existing custom post types\n- add an edit page for updating name, prompt guidance, and logo with explicit logo removal support\n- simplify custom post types list UI to use an Edit action instead of inline edit forms\n- extend regression tests for edit validation, persistence, and logo updates/removal\n- update CHANGELOG with shipped user-facing behavior\n\n## Testing\n- \n- Local pytest/docker run is not available in this runtime (), so CI is the gate for full test execution.\n
Summary by CodeRabbit
Release Notes
New Features
Tests