datamade / bga-payroll

💰 How much do your public officials make?
4 stars 4 forks source link

Replace custom Upload interface with Django admin views #402

Closed hancush closed 4 years ago

hancush commented 4 years ago

Enable the Django admin interface for StandardizedFile objects.

By default, the Django admin site will generate a form to create new model instances with all model fields enabled. Now that you've enabled the admin interface, check out what the default interface for creating a new StandardizedFile looks like.

We want this interface to have the same fields and behavior as the current upload, i.e., it should:

The way this code is currently organized is not necessarily the best way. Some things you can do with the admin interface include excluding fields or marking them readonly. You can also do custom operations on save. If necessary, you can also configure the admin interface to use a custom form.

Read up on the admin interface and best practices for organizing model, form, and admin code, then propose an architecture that meets the above requirements, e.g., "I'll use X and Y model admin options to render the correct fields, I'll create a custom form to do A and B, etc."

P.S. The SourceFile admin may also be instructive for at least one of the requirements.

fgomez828 commented 4 years ago

Contain field for a file and a reporting year:

fgomez828 commented 4 years ago

Validate the incoming data current methods in validation include: _validate_fields, _validate_filetype, clean_standardized_file, and clean_reporting_year

https://docs.djangoproject.com/en/2.2/ref/contrib/admin/#adding-custom-validation-to-the-admin

Create an AdminForm for StandardizedFile These two articles look like they may contain answers for doing the logic in the 4 methods that currently exist in a more streamlined way:

Giving the file a unique name could be done in the validation step as well, if all validation passes

fgomez828 commented 4 years ago

give the file a unique name on extending the save method

https://docs.djangoproject.com/en/3.0/ref/contrib/admin/#django.contrib.admin.ModelAdmin.save_model

fgomez828 commented 4 years ago

Create an Upload object AND trigger copy_to_database task: took me a minute to find this one: https://docs.djangoproject.com/en/2.2/ref/class-based-views/mixins-editing/#django.views.generic.edit.ModelFormMixin.form_valid

this is a mixin method: saves object, sets it for view, and redirects to get_success_url()

this method is extended in the creation of the Upload object; if form is valid, call this method, create Upload object, and copy to database here

fgomez828 commented 4 years ago

@hancush Here is my proposed plan for addressing this issue: the Django admin interface by default shows the file and reporting_year fields as they are on the production site, so these can be left alone

To implement custom validation, I will use a custom ModelForm as described here; the private methods will be defined in the class, and the logic in clean_standardized_file and clean_reporting_year will be implemented by using the save_model() method on the form. In this method, the file can be given a unique name as well.

Finally, the creation of the Upload object and triggering the copy_to_database task can happen by extending form_valid(), which gets called after the form has been validated and goes on to call get_success_url()

I think I have a pretty solid idea of how this would work, but let me know if I missed anything. Thanks!

hancush commented 4 years ago

@fatima3558 This is sounding promising! Would you mind writing some pseudocode to illustrate your proposal? It can all be in one block, like:

class CustomForm():
    def save_model():
        # do x

   # ... etc. ...

class CustomAdmin():
    form = CustomForm

    def form_valid():
        # do y

    # ... etc. ...
fgomez828 commented 4 years ago

@hancush It would look something like this:

class CustomForm():
    def form_valid():
        # create Upload object
        # trigger copy_to_database task

class CustomAdmin():
    form = CustomForm

    def _validate_fields():
        # implement logic from _validate_fields

    def _validate_filetype():
        # implement logic from _validate_filetype

    def save_model():
        # do `clean_standardized_file` and `clean_reporting_year` logic
        # give model unique name
hancush commented 4 years ago

I see! Logically, I think operations that clean or validate the form data belong on the form class, while operations that add to the form data belong on the admin class. With that in mind, a couple of things:

What do you think? Does that distinction make sense to you/gel with what you've read in the docs?

fgomez828 commented 4 years ago

@hancush gotcha, yes, all your suggestions make sense. These patterns are new to me, so I definitely appreciate your guidance! Here's what I think makes sense based on your feedback:

class CustomForm():
    def _validate_fields():
        # implement logic from _validate_fields

    def _validate_filetype():
        # implement logic from _validate_filetype

    def clean_standardized_file():
        # breaking out this method and clean_reporting_year in order to avoid retain the modularity of the methods as they are in the current form & to move them out of the Admin model
        # implement logic from clean_standardized_file()

    def clean_reporting_year():
        # implement logic from clean_reporting_year

    def form_valid():
        # give file unique name
        # trigger copy_to_database task

class CustomAdmin():
    form = CustomForm

    def save_model():
        # create Upload object
hancush commented 4 years ago

That looks great, @fatima3558. The only thing I'd change is I'd fire copy_to_database in the save_model method. The reasoning there is we want to that to fire last, after the model has been saved.

fgomez828 commented 4 years ago
class CustomForm():
    def _validate_fields():
        # implement logic from _validate_fields

    def _validate_filetype():
        # implement logic from _validate_filetype

    def clean_standardized_file():
        # breaking out this method and clean_reporting_year in order to avoid retain the modularity of the methods as they are in the current form & to move them out of the Admin model
        # implement logic from clean_standardized_file()

    def clean_reporting_year():
        # implement logic from clean_reporting_year

    def form_valid():
        # give file unique name

class CustomAdmin():
    form = CustomForm

    def save_model():
        # create Upload object
        # trigger copy_to_database task
hancush commented 4 years ago

Looks good, @fatima3558!