TrangPham / django-admin-confirm

AdminConfirmMixin is a mixin for ModelAdmin that adds confirmations to changes, additions and actions.
Other
129 stars 16 forks source link

confirmation_fields not taken in consideration #44

Closed barbouche closed 11 months ago

barbouche commented 1 year ago

Hello Dear,

I just noticed that the confirmation_fields list are not taken in consideration, in my case although I defined only one field in the list, but the confirmation is applyed to all the model fileds.

Cordially, Ahmed

TrangPham commented 1 year ago

Hi @barbouche

Could you provide me with these details:

aminenafdou commented 1 year ago

Hello @TrangPham i'm trying this addon to extend admin site functions,

i'm reproducing the same issue reported by @barbouche on :

Django version : Django==3.2 Django admin confirm version : django-admin-confirm==1.0.0

This is the model related to the ModelAdmin that i'm trying to test on this extension...

class ExpensesArchive(models.Model):

    class Meta:
        verbose_name = "Archive of file expenses"
        verbose_name_plural = "Archives of file expenses"

    type = models.IntegerField(
        choices=ARCHIVE_TYPE.values_to_names.value,
        default=ARCHIVE_TYPE.t_shine.value,
    )

    archive = PrivateFileField(
        "File",
        max_length=500,
        upload_to=calculate_archive_upload_relative_path,
    )

    extracted_archive_directory_fullpath = models.CharField(
        max_length=500,
        null=True,
        blank=True,
    )

    state = models.IntegerField(
        choices=EXPENSE_ARCHIVE_STATE.values_to_names.value,
        default=EXPENSE_ARCHIVE_STATE.notimported.value,
        editable=False,
    )

    # Define a RegexValidator for the CharField
    short_description_regex_validator = RegexValidator(
        regex=r'^****_\d{6}$',
        message="Enter a valid value in the format '****'.",
    )

    short_description = models.CharField(
        max_length=100,
        null=True,
        blank=True,
        unique=True,
        validators=[short_description_regex_validator],
        help_text="ex. '****'",
        default='****',
    )

    long_description = models.TextField(
        null=True,
        blank=True,
    )

    uploaded_at = models.DateTimeField(
        auto_now=True,
        editable=False,
    )

    business = models.ForeignKey(
        Business,
        on_delete=models.SET_NULL,
        null=True,
        related_name="expenses_archives",
    )

and this is the source code of the ModelAdmin concerned

class ExpensesArchiveAdmin(admin.ModelAdmin, AdminConfirmMixin):
    autocomplete_fields = ["business"]
    search_fields = ["short_description",
                     "business__short_description"]
    readonly_fields = ["state"]
    fields = ["short_description", "long_description",
              "business", "state", "archive"]
    list_display = ["short_description", "business", "type", "custom_is_imported", "uploaded_at"]
    actions = ["delete_expenses_related_to_archive", "import_archive_of_expenses"]
    actions_on_top = True
    actions_on_bottom = True
    confirm_change = True
    confirm_add = True
    confirmation_fields = ['short_description', 'long_description']
    inlines = [
        ArchiveExpensesInline,
    ]

    def custom_is_imported(self, obj) -> Union[bool, None]:
        if obj.state == EXPENSE_ARCHIVE_STATE.imported.value: 
            return True
        elif obj.state == EXPENSE_ARCHIVE_STATE.notimported.value: 
            return False
        return None

    custom_is_imported.short_description = "is imported"
    custom_is_imported.boolean = True

    def get_readonly_fields(self, request, obj=None):
        # Specify fields that are read-only based on conditions
        if obj:
            # Make "read_only_field" read-only if an instance exists (i.e., during editing)
            return ["short_description"] + self.readonly_fields
        return self.readonly_fields  # Keep fields read-only if creating a new instance

    @confirm_action
    @admin.action(description="Delete the expenses related to the selected archive(s)")
    def delete_expenses_related_to_archive(self, request, queryset):
       pass

    @confirm_action
    @admin.action(description="Import the content of expenses archive(s)")
    def import_archive_of_expenses(self, request, queryset):
       pass

i tried to apply it first on all the fields but it didn't work so i applied it only on two simple fields (short_description and long_description). and by the way, it's working fine on customs actions....

i cannot provide the full source code because it's not an open source code yet.

If you need more informations, don't hesistate (y) and thank you for your contribution

TrangPham commented 1 year ago

@aminenafdou when you say "it doesn't work" do you mean that no confirmation page appears/is triggered?

aminenafdou commented 1 year ago

@TrangPham: Yes ! exactly.

I change the values of the fields (confirmation_fields = ['short_description', 'long_description']) once i click on the save button a confirmation page doesn't appear..

when i execute the custom action 'import_archive_of_expenses', a confirmation page appear and works perfectly...

TrangPham commented 1 year ago

Thanks for the clarification! I should have some time to check this out this week or this weekend.

Thanks for using this project :) and reporting an issue

TrangPham commented 1 year ago

Hi @aminenafdou

I believe the issue is in: class ExpensesArchiveAdmin(admin.ModelAdmin, AdminConfirmMixin):

In python, the order of the classes matter in multiple inheritance. The way to read the method precedence is: class MyClass(A,B,C) wherein A.method_one would "overwrite" B.method_one

I say "overwrite" in quotes as the documentation actually describes it as looking for the first implementation from A -> C depth first. See: https://docs.python.org/3/tutorial/classes.html#multiple-inheritance

Could you try again using: class ExpensesArchiveAdmin(AdminConfirmMixin, admin.ModelAdmin): where AdminConfirmMixin is the left most parent in the multiple-inheritance specification?

I was able to reproduce your issue by changing the order of the classes:

https://github.com/TrangPham/django-admin-confirm/assets/2117313/8cad6b57-bbd3-4a33-9d72-90eef1239728

aminenafdou commented 1 year ago

Hello @TrangPham thank you for your feedback.

I modified the precedence and i'm still reproducing the issue.

enclosed a recording of the test that i executed... 675faeee-ee95-459c-afd6-5e9825180002.webm

as you can see on the recoding :

Thanks,

TrangPham commented 1 year ago

Could you try commenting out/removing your one private file field?

One suspicion I have is the addition of the field may be the issue. Within this project, we cache files while a confirmation is occurring - ie, between click save and clicking confirm on confirmation page. The purpose is to not overwrite the existing file stored if cancel is clicked on the confirmation page.

aminenafdou commented 12 months ago

Hello @TrangPham i'm sorry for my late response.

I tried on another admin model without any private file field or file field...

Model

class Employee(models.Model):   
    CONST_UNKNOWN_FIRST_NAME_VALUE = 'UNKNOWN'
    CONST_UNKNOWN_LAST_NAME_VALUE = 'UNKNOWN'

    social_security_number = models.CharField(
        primary_key=True, 
        max_length=50, 
        null=False, 
        blank=False
    )

    first_name = models.CharField(
        max_length=30, 
        null=False, 
        blank=False,
        default=CONST_UNKNOWN_FIRST_NAME_VALUE,
    )

    last_name = models.CharField(
        max_length=30, 
        null=False, 
        blank=False,
        default=CONST_UNKNOWN_LAST_NAME_VALUE,
    )

    email = models.EmailField()

    business = models.ForeignKey(
        Business, 
        on_delete=models.SET_NULL, 
        null=True, 
        blank=True, 
        related_name='employees'
    )

Model admin

@admin.register(Employee)
class EmployeeAdmin(AdminConfirmMixin, admin.ModelAdmin):
    search_fields = ["last_name","first_name","social_security_number", "email", "business__short_description"]
    list_display = ["social_security_number","last_name","first_name", "email", "business"]
    actions_on_bottom = True
    actions_on_top = True

    confirm_change = True
    confirm_add = True
    confirmation_fields = ["social_security_number","last_name", "first_name"]

Same issue !

aminenafdou commented 12 months ago

@TrangPham good news ! found out why it wasn't working ! it's a conflict with another django official module...

In settings.py, when i place 'admin_confirm' app at the end of the list 'INSTALLED_APPS' after django official apps, 'admin_confirm' works only for custom actions...

Once i place it at the beginning of the list (before everything else).... it works perfectly, even used with private file field.

OK =>

# Application definition
INSTALLED_APPS = [
    'admin_confirm',
    'django.contrib.admin',
    'django.contrib.auth',
    'django.contrib.contenttypes',
    'django.contrib.sessions',
    'django.contrib.messages',
    'django.contrib.staticfiles',
    'private_storage',
]

NOK =>

# Application definition
INSTALLED_APPS = [
    'django.contrib.admin',
    'django.contrib.auth',
    'django.contrib.contenttypes',
    'django.contrib.sessions',
    'django.contrib.messages',
    'django.contrib.staticfiles',
    'admin_confirm',
    'private_storage',
]

NOK =>

# Application definition
INSTALLED_APPS = [
    'django.contrib.admin',
    'django.contrib.auth',
    'django.contrib.contenttypes',
    'django.contrib.sessions',
    'django.contrib.messages',
    'django.contrib.staticfiles',
    'private_storage',
    'admin_confirm',
]

Thank you very much for your help @TrangPham

TrangPham commented 12 months ago

Thanks for posting the solution! I'm glad it's working for you

TrangPham commented 11 months ago

I'm closing this issue. If you are experiencing this issue please ensure:

  1. Ordering of classes in your model admin has 'AdminCofirmMixin' listed first in list of superclasses, like so:

class ExpensesArchiveAdmin(AdminConfirmMixin, admin.ModelAdmin)

  1. Ordering of installed apps is correct. See comments above.
barbouche commented 11 months ago

Many Thanks to you dear @TrangPham for your effort and help. It's so useful library ^^ @aminenafdou Thanks to you also for resolving the issue and your time ^^

TrangPham commented 11 months ago

@barbouche were you able to get it working for yourself now?