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

Add chooser upload form #97

Merged
merged 10 commits into from
Aug 14, 2020
Merged

Conversation

teixas
Copy link
Contributor

@teixas teixas commented May 29, 2020

Copy link
Member

@thibaudcolas thibaudcolas left a comment

Choose a reason for hiding this comment

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

This is working well @teixas! I haven’t reviewed the tests yet, only spotted two things to fix so far.

One of the two things I spotted is an issue that’s also with the document chooser (wagtail/wagtail#6126), so would be good to fix this here in the same way that it will be fixed in Wagtail.

If you have time to fix some or all of those issues (and add corresponding tests) in your PR then great, otherwise I might try to pick it up at some point (can’t commit to anything).

wagtailmedia/admin_urls.py Outdated Show resolved Hide resolved
else:
media = Media(uploaded_by_user=request.user, type=media_type)
form = MediaForm(
instance=media, user=request.user, prefix='media-chooser-upload')
Copy link
Member

Choose a reason for hiding this comment

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

I don’t really get in what scenario this view would ever be request with GET rather than POST? I couldn’t test it, but doesn’t seem worth blocking the PR for that.

Copy link
Contributor Author

@teixas teixas Jun 8, 2020

Choose a reason for hiding this comment

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

I think this is required for rendering form before 'post'ing it.

edit:
oh nevermind! The upload form is rendered by chooser view

form = MediaForm(
instance=media, user=request.user, prefix='media-chooser-upload')

media_files = Media.objects.order_by('-created_at')
Copy link
Member

Choose a reason for hiding this comment

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

This would need to implement the changes done to the chooser view, adding a way to modify its queryset and enforcing permissions:

    media_files = permission_policy.instances_user_has_any_permission_for(
        request.user, ['change', 'delete']
    )
    # allow hooks to modify the queryset
    for hook in hooks.get_hooks('construct_media_chooser_queryset'):
        media_files = hook(media_files, request)

Copy link
Member

Choose a reason for hiding this comment

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

This isn’t going to work as-is, it overrides media_files defined above. Needs to be:

Suggested change
media_files = Media.objects.order_by('-created_at')
media_files = media_files.order_by('-created_at')

wagtailmedia/views/chooser.py Outdated Show resolved Hide resolved
@teixas
Copy link
Contributor Author

teixas commented Jun 12, 2020

I've fixed all issues and also removed code regarding GET request as well as (unnecessary) respective tests.

@thibaudcolas
Copy link
Member

thibaudcolas commented Aug 10, 2020

This looks good to me at a high level – I should be able to get it merged on Friday this week after a more thorough look.

Copy link
Member

@thibaudcolas thibaudcolas left a comment

Choose a reason for hiding this comment

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

Thank you @Eixas, looking great! I spotted a couple things but I can address them myself.

form = MediaForm(
instance=media, user=request.user, prefix='media-chooser-upload')

media_files = Media.objects.order_by('-created_at')
Copy link
Member

Choose a reason for hiding this comment

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

This isn’t going to work as-is, it overrides media_files defined above. Needs to be:

Suggested change
media_files = Media.objects.order_by('-created_at')
media_files = media_files.order_by('-created_at')

wagtailmedia/views/chooser.py Show resolved Hide resolved
@thibaudcolas thibaudcolas merged commit 19452fd into torchbox:master Aug 14, 2020
@thibaudcolas thibaudcolas mentioned this pull request Aug 14, 2020
@thibaudcolas
Copy link
Member

Thank you @teixas :) It’s great to see this feature finally implemented and merged in – now working on a 0.6.0 release!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants