-
Notifications
You must be signed in to change notification settings - Fork 3
Refactor referenced items Delete Dialog #2578
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Refactor referenced items Delete Dialog #2578
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hello @ibrahimjaved12, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!
Summary of Changes
Gemini here, providing a summary of this pull request. This PR refactors the delete dialog functionality for repeatable content items. The core change introduces a check on the backend to determine if a content item is 'deletable' based on whether it is referenced by other content items. This check is controlled by a feature flag. On the frontend, the delete dialog is updated to use this is_deletable
flag. If an item is not deletable, the 'Delete' button is hidden, and a message is displayed to the user explaining why the item cannot be deleted. The backend includes performance improvements by prefetching related content to avoid N+1 queries when determining deletability.
Highlights
- Backend Deletability Logic: A new
is_deletable
field is added to theWebsiteContentSerializer
. This field determines if an item can be deleted by checking if it is referenced by other content, controlled by theOCW_STUDIO_CONTENT_DELETABLE_REFERENCES
feature flag. - Frontend Error Message: If a content item is not deletable (i.e.,
is_deletable
is false), a message is displayed in the delete dialog indicating that the item cannot be deleted because it is referenced. - Performance Optimization: The backend queryset for listing website content now includes a
prefetch_related
call forreferencing_content
to efficiently determine deletability without causing N+1 query issues.
Changelog
Click here to see the changelog
- static/js/components/RepeatableContentListing.tsx
- Added logic to check
content.is_deletable
when the delete button is clicked (lines 168-174). - Set a
deleteError
state with a message if the item is not deletable (line 169). - Conditionally render the
acceptText
('Delete') andonAccept
props for theDialog
component based onselectedContent?.is_deletable
(lines 315-318).
- Added logic to check
- static/js/types/websites.ts
- Added an optional
is_deletable
boolean property to theWebsiteContentListItem
interface (line 284).
- Added an optional
- websites/serializers.py
- Imported
is_feature_enabled
frommain.posthog
(line 22). - Added a
SerializerMethodField
namedis_deletable
toWebsiteContentSerializer
(line 472). - Implemented
get_is_deletable
method to check for prefetched references or queryreferencing_content
based on a feature flag (lines 474-485). - Added
is_deletable
to theread_only_fields
in theMeta
class (line 495).
- Imported
- websites/views.py
- Imported
Prefetch
fromdjango.db.models
(line 10). - Added
prefetch_related
forreferencing_content
to theWebsiteContent
queryset inget_queryset
(lines 610-615).
- Imported
Using Gemini Code Assist
The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.
Invoking Gemini
You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command>
or @gemini-code-assist <command>
. Below is a summary of the supported commands.
Feature | Command | Description |
---|---|---|
Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
Help | /gemini help |
Displays a list of available commands. |
Customization
To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/
folder in the base of the repository. Detailed instructions can be found here.
Limitations & Feedback
Gemini Code Assist is currently in preview and may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments to provide feedback.
You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.
Content linked tight,
Delete button takes flight,
Unless references hold,
A story untold.
Footnotes
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request effectively introduces a check for referenced items before deletion, enhancing data integrity. The backend now calculates an is_deletable
flag, leveraging prefetching for efficiency and a feature flag for control. The frontend consumes this flag to update the delete dialog, preventing users from deleting referenced content and providing an appropriate message.
The implementation is generally solid. I've noted a minor typo in a user-facing message and a consideration for future maintainability of the serializer.
Summary of Findings
- Typo in User-Facing Error Message: A typo ('hehe') was found in an error message displayed to the user in
static/js/components/RepeatableContentListing.tsx
. This should be corrected for professionalism. - Serializer Maintainability (N+1 Potential): The
get_is_deletable
method inwebsites/serializers.py
falls back to a database query if prefetched data isn't available. While handled correctly in this PR's view changes, it's a point of awareness for future uses of the serializer to ensure prefetching is implemented to prevent N+1 issues. - Conditional UI for Deletion: The frontend dialog now correctly hides the 'Delete' button and shows an informative message if an item is not deletable, based on the new
is_deletable
flag. This is a good UX improvement. - Backend Deletability Logic: The backend logic to determine
is_deletable
usingSerializerMethodField
, prefetching related content, and a feature flag (OCW_STUDIO_CONTENT_DELETABLE_REFERENCES
) is robust and provides good control over the feature.
Merge Readiness
The pull request is in good shape but has a minor typo in a user-facing error message that should be addressed. There's also a suggestion for an inline comment to aid future maintainability. Once the typo is fixed, this PR should be ready for merge. As an AI, I am not authorized to approve pull requests; please ensure further review and approval by authorized team members.
What are the relevant tickets?
https://github.com/mitodl/hq/issues/5686#issuecomment-2839573054
Description (What does it do?)
modalpage is loaded, and display the error message right away when the delete modal is opened.Screenshots (if appropriate):
How can this be tested?
Cancel
button visible. The other external resource should be deletable viaDelete
button.