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

Backend playlist token #2372

Merged
merged 7 commits into from
Aug 22, 2023
Merged

Backend playlist token #2372

merged 7 commits into from
Aug 22, 2023

Conversation

kernicPanel
Copy link
Member

@kernicPanel kernicPanel commented Aug 7, 2023

Purpose

Implementation for #2286

Fixes #1342

@kernicPanel kernicPanel force-pushed the frontend-nested-urls branch from ff96f1e to 57d6714 Compare August 7, 2023 13:37
@kernicPanel kernicPanel force-pushed the backend-playlist-token branch from 2dfb27a to fed15ce Compare August 7, 2023 13:41
@kernicPanel kernicPanel changed the base branch from frontend-nested-urls to remove-old-urls August 7, 2023 14:57
@kernicPanel kernicPanel force-pushed the backend-playlist-token branch 2 times, most recently from 1df045b to be26101 Compare August 7, 2023 15:23
@kernicPanel kernicPanel force-pushed the backend-playlist-token branch 2 times, most recently from 7f785d8 to 6bab60e Compare August 7, 2023 16:02
@kernicPanel kernicPanel mentioned this pull request Aug 8, 2023
@kernicPanel kernicPanel force-pushed the backend-playlist-token branch from 6bab60e to b005e64 Compare August 8, 2023 07:14
@kernicPanel kernicPanel changed the base branch from remove-old-urls to frontend-nested-urls August 8, 2023 07:15
@kernicPanel kernicPanel marked this pull request as ready for review August 8, 2023 09:15
@kernicPanel kernicPanel requested review from lunika, AntoLC and polyhb August 8, 2023 09:15
Copy link
Contributor

@AntoLC AntoLC left a comment

Choose a reason for hiding this comment

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

I don't think my review has too much weight.

@kernicPanel kernicPanel force-pushed the frontend-nested-urls branch from 219a294 to dc7204f Compare August 10, 2023 16:08
@kernicPanel kernicPanel force-pushed the backend-playlist-token branch from b005e64 to 1565c87 Compare August 10, 2023 16:08
@kernicPanel kernicPanel force-pushed the frontend-nested-urls branch from dc7204f to 6300736 Compare August 16, 2023 07:16
@kernicPanel kernicPanel force-pushed the backend-playlist-token branch from 1565c87 to b78ac65 Compare August 16, 2023 07:16
@kernicPanel kernicPanel force-pushed the frontend-nested-urls branch from 6300736 to b7133b4 Compare August 16, 2023 09:03
@kernicPanel kernicPanel force-pushed the backend-playlist-token branch 4 times, most recently from f246a3f to f30c747 Compare August 17, 2023 14:44
Comment on lines 172 to 188
try:
return models.Playlist.objects.filter(id=playlist_id).exists() and (
str(view.get_queryset().get(id=view.get_object_pk()).playlist_id)
== playlist_id
)
except (AttributeError, ObjectDoesNotExist):
try:
return models.Playlist.objects.filter(id=playlist_id).exists() and (
str(
view.get_queryset()
.get(id=view.get_object_pk())
.video.playlist_id
)
== playlist_id
)
except ObjectDoesNotExist:
return False
Copy link
Member

Choose a reason for hiding this comment

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

Does it work if you replace with this ?

playlist_exists = models.Playlist.objects.filter(id=playlist_id).exists()
            try:
                return (
                    playlist_exists
                    and view.get_queryset()
                    .filter(id=view.get_object_pk(), playlist__id=playlist_id)
                    .exists()
                )
            except FieldError:
                return playlist_exists and (
                    view.get_queryset()
                    .filter(id=view.get_object_pk(), video__playlist__id=playlist_id)
                    .exists()
                )

),
).exists()
and request.resource.id == view.get_object_pk()
and models.Playlist.objects.filter(pk=request.resource.id).exists()
Copy link
Member

Choose a reason for hiding this comment

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

Here you are using pk= and line 173 you are using id=. Both are good IMO, we should probably choose the one we use everywhere.

@kernicPanel kernicPanel force-pushed the backend-playlist-token branch from 66895ba to fba89e9 Compare August 22, 2023 13:05
@kernicPanel kernicPanel force-pushed the frontend-nested-urls branch from a12d4cb to 9899b51 Compare August 22, 2023 15:38
@kernicPanel kernicPanel force-pushed the backend-playlist-token branch from fba89e9 to d8a338d Compare August 22, 2023 15:39
@kernicPanel kernicPanel force-pushed the frontend-nested-urls branch from 9899b51 to 0a8c738 Compare August 22, 2023 15:59
@kernicPanel kernicPanel force-pushed the backend-playlist-token branch from d8a338d to a681cc8 Compare August 22, 2023 16:00
@kernicPanel kernicPanel force-pushed the frontend-nested-urls branch from 0a8c738 to 024941a Compare August 22, 2023 16:21
@kernicPanel kernicPanel force-pushed the backend-playlist-token branch from a681cc8 to 3dcba1d Compare August 22, 2023 16:22
@kernicPanel kernicPanel force-pushed the frontend-nested-urls branch from 024941a to 5618e0f Compare August 22, 2023 16:30
@kernicPanel kernicPanel force-pushed the backend-playlist-token branch from 3dcba1d to 4e5b7fb Compare August 22, 2023 16:30
Base automatically changed from frontend-nested-urls to master August 22, 2023 16:46
We want to use playlist token instead of resource ones.
See #1342
As we are now using playlist tokens, it has to be allowed on video apis.
As we are now using playlist tokens, it has to be allowed on apis.
As we are now using playlist tokens, it has to be allowed on apis.
As we are now using playlist tokens, it has to be allowed on apis.
As we are now using playlist tokens, it has to be allowed on apis.
When run locally, if the front has been built, this test fails.
Overriding the static directory ensures the behavior when it is missing.
@kernicPanel kernicPanel force-pushed the backend-playlist-token branch from 4e5b7fb to 42fcb94 Compare August 22, 2023 16:47
@kernicPanel kernicPanel enabled auto-merge (rebase) August 22, 2023 16:48
@kernicPanel kernicPanel merged commit f990b05 into master Aug 22, 2023
@kernicPanel kernicPanel deleted the backend-playlist-token branch August 22, 2023 17:03
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.

JWT token scope extended to a playlist instead of a resource
3 participants