Skip to content

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

Open
wants to merge 10 commits into
base: master
Choose a base branch
from

Conversation

ibrahimjaved12
Copy link
Contributor

@ibrahimjaved12 ibrahimjaved12 commented May 26, 2025

What are the relevant tickets?

https://github.com/mitodl/hq/issues/5686#issuecomment-2839573054

Description (What does it do?)

  1. Simplify the text to just "This item is referenced by other items and cannot be deleted."
  2. Check whether the item can be deleted when the modal page is loaded, and display the error message right away when the delete modal is opened.
  3. When the item cannot be deleted, hide the delete button.

Screenshots (if appropriate):

image image image

How can this be tested?

  1. Checkout to this branch
  2. Spin up ocw-studio locally
  3. Create two external resources and link one in a page.
  4. Now try deleting the external resources. The one linked in the page should error out immediately when you open the delete modal, with only Cancel button visible. The other external resource should be deletable via Delete button.
  5. You can also extend the testing by testing deletion of a page. First link a course page in the course's menu. Afterwards, try deleting the page and confirm its erroring same as above.

Copy link

@gemini-code-assist gemini-code-assist bot left a 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 the WebsiteContentSerializer. This field determines if an item can be deleted by checking if it is referenced by other content, controlled by the OCW_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 for referencing_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') and onAccept props for the Dialog component based on selectedContent?.is_deletable (lines 315-318).
  • static/js/types/websites.ts
    • Added an optional is_deletable boolean property to the WebsiteContentListItem interface (line 284).
  • websites/serializers.py
    • Imported is_feature_enabled from main.posthog (line 22).
    • Added a SerializerMethodField named is_deletable to WebsiteContentSerializer (line 472).
    • Implemented get_is_deletable method to check for prefetched references or query referencing_content based on a feature flag (lines 474-485).
    • Added is_deletable to the read_only_fields in the Meta class (line 495).
  • websites/views.py
    • Imported Prefetch from django.db.models (line 10).
    • Added prefetch_related for referencing_content to the WebsiteContent queryset in get_queryset (lines 610-615).
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

  1. 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.

Copy link

@gemini-code-assist gemini-code-assist bot left a 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 in websites/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 using SerializerMethodField, 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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants