Updated crud_for_model and crud_for_app functions

[A4TIF]

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

#114

[oscarmlage]

First of all, thanks for the PR.

Appart from the cosmetical pep8 things (hehe), I was taking a look and thinking a bit about this "breaking change". I mean, all the people using modelforms should "update" and call the crud_for_app in the new - modelconfig - way. I'm in doubt.

What do you think if we allow to call crud_for_app with both params? The one who wants to use modelforms could do it as before, and the one that wants to use the new modelconfig way could too. I'm not 100% convinced but I guess is the best solution (or at least the best that came to my mind this morning).

Need some feedback from you 😄 (and thanks again for the amazing work).

[oscarmlage]

This line breaks pep8

[A4TIF]

fixed

[oscarmlage]

This line breaks pep8 (and next two too) :)

[A4TIF]

fixed

[A4TIF]

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

[oscarmlage]

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]

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]

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]

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]

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.