oscarmlage / django-cruds-adminlte

django-cruds is simple drop-in django app that creates CRUD for faster prototyping
BSD 3-Clause "New" or "Revised" License
424 stars 81 forks source link

Updated crud_for_model and crud_for_app functions #119

Open A4TIF opened 4 years ago

A4TIF commented 4 years ago

Updated crud_for_model and crud_for_app functions to include all the CRUDView attributes when generating URLs

114

A4TIF commented 4 years ago

Added modelformswith deprecation warning, hopefully, that should resolve any breaking change issue for now :)

oscarmlage commented 4 years ago

After taking some time testing I've found that there are some arguments (fields, list_fields, display_fields) that are not working properly, i.e.:

model_config = {
    'publication': {
        'fields': ['description'],
        'list_fields': ['description'],
        'display_fields': ['description'],
    }
}
urlpatterns += crud_for_app(app_name, check_perms=True, namespace="ns",
                            modelconfig=model_config)

In this case, if you don't set field and/or list_fields in the custom CRUDView the crud is being generated with all the fields instead of taking in account model_config ones. Dunno if I'm missing something, maybe you could take a look on this later?.

OTOH after merge the PR I'll remove the warning because I'm not sure this new modelconfig will be more usable than the old modelform (we should check at least that all the arguments are working on both ways), so now that we have two options, let the people use one or the other on their own.

Other than that it's ok with me. Anyway I'll merge the PR and we can add a new issue to fix this pending stuff.

A4TIF commented 4 years ago

In this case, if you don't set field and/or list_fields in the custom CRUDView the crud is being generated with all the fields instead of taking in account model_config ones. Dunno if I'm missing something, maybe you could take a look on this later?.

Well, if you don't set field or list_fields, it is designed to take all fields right? It is working as designed, as I just tested it. I added a single field, and it worked fine for me here, displaying one single field in list and display. If I remove fields, and list_fields from the model's config, then it only displays single field in display page. So seems fine here. Maybe I didnt understand your issue properly. Can you please explain further so that I can look into it?

Thanks

A4TIF commented 4 years ago

I've tested with multiple models, something similar to this and it is working perfectly fine for me in all the cases

modelconfig={
    'product': {
        'list_fields': ['name'],
    },
    'message': {
        'list_fields': ['product', 'message', 'image'],
        'list_filter': ['product'],
    },
    'contact': {
        'list_fields': ['created', 'name', 'email', 'phone', 'products', 'user'],
        'display_fields': ['id', 'name', 'email', 'phone', 'other_phone', 'products',
                           'location',
                           'user', 'created', 'modified'],
        'list_filter': ['user', 'products'],
        'add_form': forms.ContactForm,
        'update_form': forms.ContactForm,
        'search_fields': ['name__icontains', 'email__icontains', 'phone__icontains'],
    },
    'user': {
        'list_fields': ['username', 'first_name', 'last_name', 'is_staff',
                        'is_active', ],
        'display_fields': ['id', 'username', 'first_name', 'last_name', 'is_staff',
                           'is_active', ],
        'search_fields': ['username__icontains', 'email__icontains'],
        'add_form': forms.UserAddForm,
        'update_form': forms.UserUpdateForm,
    }
})
telenieko commented 4 years ago

I was reading this PR and I cannot honestly see its purpose.

First, the description of the PR talks about including information on the URLs, but the code does not alter the url generation algorithms at all. The only "url" in the PR is that the function is in the urls.py file.

Then, it looks to me that this modelconfig introduced provides, pretty much, the same functionality as inheriting from the CRUDView class. crud_for_app and crud_for_model are shortcut functions like django's render_to_string or redirect_to. Being a shortcut, it should not attemp to implement every possible use case of the CRUD generation, that is why the class inheritance mechanism exists.

I do not think those two functions should implement more than they already do. The most they should do is accept **kwargs and update the dict with the kwargs.

Please, update the PR description to explain what this PR does exactly and its motivation so we can all be on the same page

A4TIF commented 4 years ago

Hi @telenieko

The main goal of this package is "to make prototyping faster". This is listed in the first few lines of the README.

The general purpose of this PR was to provide a single shortcut to get everything up and running easily without bothering with inheriting the CRUDView class. For the purpose of the goal that this package offers, having a single shortcut works like a charm for me as shown in the example above.

Similarly, bringing mixins was a great initiative, although we have had some breaks on it recently and if that can be worked out with this shortcut, it provides a solid foundation to prototyping extremely fast.

Do note, that modelconfig still works with previous settings, but it just covers everything that CRUDView class offers out of the box. If I needed more than what this package offers, I then have the option to inherit the class.

So I believe this PR works well for the goal of the package. Apart from that, if you feel the PR doesn't solve any purpose, then feel free to close it.