Skip to content

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).

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')

@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.

@thibaudcolas thibaudcolas self-requested a review August 10, 2020 16:43
@thibaudcolas thibaudcolas force-pushed the chooser-upload-form-fix branch from 813607b to 470fdbb Compare August 14, 2020 11:13
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')

@thibaudcolas thibaudcolas force-pushed the chooser-upload-form-fix branch from a8885cf to 407ff3c Compare August 14, 2020 12:25
@thibaudcolas thibaudcolas force-pushed the chooser-upload-form-fix branch from 407ff3c to e1addd2 Compare August 14, 2020 12:26
@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