Skip to content
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

Create an abstract model named BaseAttachment that can be overwritten #93

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

vieirafrancisco
Copy link

@vieirafrancisco vieirafrancisco commented Sep 16, 2022

Hi,

I felt the need of an abstract class of Attachment to overwrite the basic fields without creating two tables in the database.
Thought in something like that, having BaseAttachment without the "creator" field and assign it to the original model Attachment that overwrites the BaseAttachment model class.

I would like your opinion about this code.
thanks :)

@vieirafrancisco vieirafrancisco changed the title Create an abstract named BaseAttachment that can be overwritten Create an abstract model named BaseAttachment that can be overwritten Sep 16, 2022
Copy link
Collaborator

@atodorov atodorov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall looks good to me but I have 2 comments here:

  1. Needs rebase on top of latest in order to trigger tests

  2. Everything that uses the Attachment class will not work with your overriden/custom class that will inherit from BaseAttachment. At the very minimum I see: delete_stale_attachments.py, attachments_tags.py, admin.py, forms.py (used in views).

This second item pretty much breaks the entire out-of-the box experience of django-attachments because all of these places contain hard-coded references to the Attachment model itself instead of whatever custom model the developer provides in their app.

So IMO this can go in either of 2 directions:

  1. If you are building a custom app and care only about the model and not really anything else (iow you are handling all of the above mentioned functionality yourself) inheriting directly from Attachment and having another DB table should not be a big deal IMO.

  2. If you are aiming to provide a fully customizable experience for the developer this PR needs more work (could be split into smaller PRs which are easier to review). Something like:

  • add a setting and remove hard-coded references; possibly refactor tests a bit so they consume the setting. Similar to what Django does for the User model.
  • add the abstract BaseAttachment class
  • document the changes and how others can use them
  • add a custom attachment model class in the test suite and retest everything with it as well (could be a parameter to avoid code duplication).
  • fix any bugs if present

@atodorov atodorov requested a review from bartTC March 11, 2023 18:18
@atodorov atodorov marked this pull request as draft April 22, 2023 10:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants