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

Playlist token #2342

Closed
wants to merge 14 commits into from
Closed

Playlist token #2342

wants to merge 14 commits into from

Conversation

kernicPanel
Copy link
Member

@kernicPanel kernicPanel commented Jul 21, 2023

Purpose

Implementation for #2286

Separated in #2370 #2371 #2372

@kernicPanel kernicPanel force-pushed the playlist-token branch 7 times, most recently from 4747f5b to 7a9a6d6 Compare August 3, 2023 09:35
Copy link
Member

@lunika lunika left a comment

Choose a reason for hiding this comment

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

Did you plan to replace the resource property in a resource token by playlist ? There is no reason anymore to keep any resource reference. But maybe this can be made in two steps for a smoother transition.

@@ -155,7 +155,9 @@ def test_api_video_instructor_initiate_live_with_playlist_token(self):
playlist__title="foo bar",
playlist__lti_id="course-v1:ufr+mathematics+00001",
)
jwt_token = PlaylistLtiTokenFactory(playlist=video.playlist)
jwt_token = PlaylistLtiTokenFactory(
resource=video.playlist, playlist=video.playlist
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 weird to read this

)
except (AttributeError, ObjectDoesNotExist):
try:
return models.Playlist.objects.filter(id=playlist_id).exists() and (
Copy link
Member

Choose a reason for hiding this comment

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

Should not be better to have a mixin on models implementing the playlist_id property method ?

Copy link
Member

Choose a reason for hiding this comment

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

if playlist_id property does not exists, we will have a 500 and we will know we have to implement it. WDYT ?

src/backend/marsha/bbb/api.py Show resolved Hide resolved
src/backend/marsha/core/api/base.py Show resolved Hide resolved
src/backend/marsha/markdown/permissions.py Show resolved Hide resolved
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.
Also, old routes can't be used anymore, as token resource is now a
playlist.
As we are now using playlist tokens, it has to be allowed on apis.
Also, old routes can't be used anymore, as token resource is now a
playlist.
As we are now using playlist tokens, it has to be allowed on apis.
Also, old routes can't be used anymore, as token resource is now a
playlist.
As we are now using playlist tokens, it has to be allowed on apis.
Also, old routes can't be used anymore, as token resource is now a
playlist.
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 playlist-token branch 4 times, most recently from f85e5af to 595018d Compare August 7, 2023 11:53
As the classroom document serializer sends only a classroom id, it has
been reflected to attribute name for clarity.
As the deposited file serializer sends only a file depository id, it has
been reflected to attribute name for clarity.
As we want to only use API nested routes, our generic components needs
to handle them.
As we want to only use API nested routes, our video components needs
to handle them.
As we want to only use API nested routes, our classroom components needs
to handle them.

Also, classroom has been renamed and retyped to classroom_id in
classroom document and recording, because the API actually sends it as a
uuid string, and not as a serialized object.
As we want to only use API nested routes, our deposit components needs
to handle them.

Also, filedepository has been renamed and retyped to filedepository_id
in deposited file, because the API actually sends it as a uuid string,
and not as a serialized object.
As we want to only use API nested routes, our markdown components needs
to handle them.
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.

2 participants