feat/t26 add view save first notice - #23
Merged
Merged
Conversation
Contributor
Reviewer's GuideAdds Unfold card-based informational notices to the FormEntry add view explaining that form elements and form handlers are only available after saving, wires these via custom inline templates, and introduces tests to assert the notice presence on add view and absence on change view, along with a patch version bump and locale updates. Sequence diagram for FormEntry add view inline notice behaviorsequenceDiagram
actor AdminUser
participant DjangoAdmin
participant FormEntryAddView
participant FormElementsInlineTemplate
participant FormHandlersInlineTemplate
AdminUser->>DjangoAdmin: Request FormEntry add view
DjangoAdmin->>FormEntryAddView: Dispatch add view
FormEntryAddView-->>DjangoAdmin: Context with add = True
DjangoAdmin->>FormElementsInlineTemplate: Render form_elements_tabular.html
FormElementsInlineTemplate-->>FormElementsInlineTemplate: Include admin/edit_inline/tabular.html
FormElementsInlineTemplate-->>FormElementsInlineTemplate: Check add flag
alt add is True
FormElementsInlineTemplate-->>FormElementsInlineTemplate: Render Unfold card notice
end
DjangoAdmin->>FormHandlersInlineTemplate: Render form_handlers_tabular.html
FormHandlersInlineTemplate-->>FormHandlersInlineTemplate: Include admin/edit_inline/tabular.html
FormHandlersInlineTemplate-->>FormHandlersInlineTemplate: Check add flag
alt add is True
FormHandlersInlineTemplate-->>FormHandlersInlineTemplate: Render Unfold card notice
end
DjangoAdmin-->>AdminUser: HTML with tabs and save-first notices on active tab
Flow diagram for inline template rendering of save-first noticesflowchart TD
A[Render_FormElementEntryInline_or_FormHandlerEntryInline] --> B[Load_corresponding_tabular_template]
B --> C[Include_admin_edit_inline_tabular_template]
C --> D{Is_add_flag_true_for_FormEntry?}
D -- Yes_add_view --> E[Render_full_width_Unfold_card_notice]
E --> F[Bind_card_visibility_to_activeTab_matching_inline_prefix]
F --> G[Return_HTML_with_info_card_on_add_view]
D -- No_change_view --> H[Do_not_render_save_first_notice]
H --> I[Return_standard_inline_rows_on_change_view]
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- The new add-view tests for the notice rely on the literal
"tabular-table"class and exact message strings in the rendered HTML, which makes them quite brittle against template or copy changes; consider asserting on more stable structure or IDs, or scoping the checks to the new card container instead. - The two new inline templates for form elements and form handlers are nearly identical apart from the messages, so it may be worth extracting a shared partial or macro/component to avoid duplication and keep future styling or behavioral changes in one place.
- Since the notices are wrapped in
x-show="activeTab == ...", it might be useful to add a small comment in the templates explaining the expectation about theactiveTabAlpine state, to make the dependency on the surrounding tab implementation clearer to future maintainers.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The new add-view tests for the notice rely on the literal `"tabular-table"` class and exact message strings in the rendered HTML, which makes them quite brittle against template or copy changes; consider asserting on more stable structure or IDs, or scoping the checks to the new card container instead.
- The two new inline templates for form elements and form handlers are nearly identical apart from the messages, so it may be worth extracting a shared partial or macro/component to avoid duplication and keep future styling or behavioral changes in one place.
- Since the notices are wrapped in `x-show="activeTab == ..."`, it might be useful to add a small comment in the templates explaining the expectation about the `activeTab` Alpine state, to make the dependency on the surrounding tab implementation clearer to future maintainers.
## Individual Comments
### Comment 1
<location path="tests/admin/test_add_view.py" line_range="41-45" />
<code_context>
+ "Save this form first to add form elements."
+ )
+
+ def test_change_view_does_not_render_add_notice(self, admin_client, form_entry):
+ response = admin_client.get(get_admin_edit_url(form_entry.pk))
+ content = response.content.decode()
+ assert "Save this form first to add form elements." not in content
+ assert "Save this form first to add form handlers." not in content
+
</code_context>
<issue_to_address>
**suggestion (testing):** Also assert the change view status code to strengthen the regression check
As with the add-view test, please also assert `response.status_code == 200` so this regression test confirms the notices are absent on a successful change view, not on an unexpected redirect or 404.
```suggestion
def test_change_view_does_not_render_add_notice(self, admin_client, form_entry):
response = admin_client.get(get_admin_edit_url(form_entry.pk))
assert response.status_code == 200
content = response.content.decode()
assert "Save this form first to add form elements." not in content
assert "Save this form first to add form handlers." not in content
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+41
to
+45
| def test_change_view_does_not_render_add_notice(self, admin_client, form_entry): | ||
| response = admin_client.get(get_admin_edit_url(form_entry.pk)) | ||
| content = response.content.decode() | ||
| assert "Save this form first to add form elements." not in content | ||
| assert "Save this form first to add form handlers." not in content |
Contributor
There was a problem hiding this comment.
suggestion (testing): Also assert the change view status code to strengthen the regression check
As with the add-view test, please also assert response.status_code == 200 so this regression test confirms the notices are absent on a successful change view, not on an unexpected redirect or 404.
Suggested change
| def test_change_view_does_not_render_add_notice(self, admin_client, form_entry): | |
| response = admin_client.get(get_admin_edit_url(form_entry.pk)) | |
| content = response.content.decode() | |
| assert "Save this form first to add form elements." not in content | |
| assert "Save this form first to add form handlers." not in content | |
| def test_change_view_does_not_render_add_notice(self, admin_client, form_entry): | |
| response = admin_client.get(get_admin_edit_url(form_entry.pk)) | |
| assert response.status_code == 200 | |
| content = response.content.decode() | |
| assert "Save this form first to add form elements." not in content | |
| assert "Save this form first to add form handlers." not in content |
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 Sourcery
Add save-first guidance to the form add view for form elements and handlers, and wire it into the Unfold admin inlines.
New Features:
Enhancements:
Tests: