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

Actions #133

Merged
merged 4 commits into from May 23, 2013
Merged

Actions #133

merged 4 commits into from May 23, 2013

Conversation

inglesp
Copy link
Contributor

@inglesp inglesp commented May 22, 2013

Here's an implementation of admin actions.

Am currently on a train and about to lose wifi access... I've got a bunch of things we need to discuss about this approach, and so will write up tonight.

This addresses #71.

@AndrewIngram
Copy link
Contributor

I think we could develop a BulkAction Generic CBV rather than used a function-based approach, dog-fooding and all that. Actually, that'd be something useful even outside of admin2, so I'd quite like to add it to django-extra-views (which I'm hoping to get into core at some point).

We could handle it as a later refactor though.

else:
return capfirst(action.__name__.replace('_', ' '))

def delete_selected(request, queryset):
Copy link
Member

Choose a reason for hiding this comment

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

This is a FBV, right? So why not make this a simple CBV built thus:

class DeleteSelectedView(View):

    def post(self, request, *args, **kwargs):
        # AWESOME CODE HERE

Does this work or is this doing something else?

@pydanny
Copy link
Member

pydanny commented May 22, 2013

In general I like this. I appreciate how the permissions stick to the default stuff, so @gregmuellegger's work on other tickets won't be impacted. Nevertheless, I do have a comment inline.

In addition, it wouldn't hurt to have this documented in the reference page: https://django-admin2.readthedocs.org/en/latest/reference.html 😉

@inglesp
Copy link
Contributor Author

inglesp commented May 22, 2013

Agreed that a class-based approach to handling bulk actions is the way to go -- the advantages to CBAs over FBAs are similar to those of CBVs over FBVs -- but I wanted to get some code out there for discussion first.

Firstly, note that delete_selected is not strictly a FBV, since it may return None.

The question we need to answer is how we dispatch to a class-based action. Currently, the user POSTs to a URL which is linked to a ModelListView, which then dispatches to a function according to the action parameter in the request. Is this the right thing to do? An alternative would be to have a different URL for each action, in which case we could hook a class-based action up to each URL.

We should also think about whether it is appropriate to expose actions over the API (I think we should), and also whether we should expose actions on single models (again, I think we should).

@gregmuellegger
Copy link
Contributor

As attractive it might be offer actions to the API as well, it will get tricky to implement them. Actions (in django.contrib.admin) have the possibility to return a HTML page containing a form. I'm not sure how we could make the same functionality available in the API.

However it's a topic that is worth exploring.

@inglesp
Copy link
Contributor Author

inglesp commented May 22, 2013

That's a good point, but not every action needs to return an HTML page. The delete_selected action for instance only returns a form (asking for confirmation) if the POST data does not include a confirmed parameter.

@pydanny
Copy link
Member

pydanny commented May 22, 2013

@gregmuellegger You have a good point about 'actions' in the REST API. We need to make certain that actions work on views AND in the rest framework. Any thoughts?

@AndrewIngram
Copy link
Contributor

I'm not a fan of the action parameter, but the alternative seems to be to use javascript trickery to POST the selected model ID's to different endpoints depending on which option is selected, which feels ugly. So I think the action parameter is the way to go unless someone has a better approach.

@pydanny pydanny merged commit ade9295 into jazzband:develop May 23, 2013
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.

None yet

4 participants