feat: Session adding notes and recordings #8084
Conversation
There was a problem hiding this comment.
This is really good, and close, but it is introducing some new behavior with edges we should talk about more.
If it turns out that any changes based on that discussion aren't quick to resolve, lets consider splitting showing the existing recordings on the materials page into a separate PR as it would have immediate utility even before adding the ability to upload more recordings.
And this is all about recordings - please scrub mentions of notes away in the new code (existing code should be linking to notes in that section - we'll address that it isn't separately).
| @@ -16,6 +16,7 @@ def get_redirect_url(self, *args, **kwargs): | |||
| safe_for_all_meeting_types = [ | |||
| url(r'^session/(?P<acronym>[-a-z0-9]+)/?$', views.session_details), | |||
| url(r'^session/(?P<session_id>\d+)/drafts$', views.add_session_drafts), | |||
| url(r'^session/(?P<session_id>\d+)/notes_and_recordings$', views.add_session_notes_and_recordings), | |||
There was a problem hiding this comment.
lets make this endpoint just "recordings"
| @@ -2538,6 +2538,55 @@ def add_session_drafts(request, session_id, num): | |||
| 'form': form, | |||
| }) | |||
|
|
|||
| class SessionNotesAndRecordingsForm(forms.Form): | |||
| if request.method == 'POST': | ||
| form = SessionNotesAndRecordingsForm(request.POST,already_linked=already_linked) | ||
| if form.is_valid(): | ||
| title = form.cleaned_data['title'] |
There was a problem hiding this comment.
Currently, for the meetecho uploads, the title is set by automata to look something like, e.g.,Video recording for tools on 2024-10-15 at 18:00:00. We should probably suggest a title of that form rather than starting with an empty control. Giving chairs the ability to pick a title here will be a new thing, but I think it's ok.
| problems = set(url).intersection(set(self.already_linked)) | ||
| if problems: | ||
| raise forms.ValidationError("Already linked: %s" % ', '.join([d.name for d in problems])) |
There was a problem hiding this comment.
This has a different semantic than the meetecho upload API, which can update a link in case there was an error. We should think through what a chair would do if they entered the wrong link and wanted to fix it.
There was a problem hiding this comment.
I have a couple technical comments/suggestions inline.
The handling of already_linked feels a little strange to me. It makes it a little tricky to describe the new form class. It's not really logically a SessionRecordingForm because it doesn't actually know anything about the Session class, but it's not quite a more general RecordingForm because it's expecting this extra piece of data that is pulled out of the Session.
I guess I'd slightly prefer that either the form take a session parameter and then be responsible for knowing how to pull the already_linked data out of the session, or not take an extra parameter and do the already_linked checking in the view.
All that said, I'm not sure that a change is necessary, just something to consider.
| def clean(self): | ||
| url = self.cleaned_data['url'] | ||
| parsed_url = urlparse(url) | ||
| if parsed_url.hostname not in ['www.youtube.com', 'youtube.com', 'youtu.be', 'm.youtube.com', 'youtube-nocookie.com', 'www.youtube-nocookie.com']: |
There was a problem hiding this comment.
Make the allowed list into a settings entry?
| self.already_linked = kwargs.pop('already_linked') | ||
| super(self.__class__, self).__init__(*args, **kwargs) | ||
|
|
||
| def clean(self): |
There was a problem hiding this comment.
Since this is really only validating the url field, it should probably be def clean_url(). In addition to a semantic hint about what you're validating, it'll correctly route your ValidationError exceptions into field-specific errors. You'll need to change the return value to be just the value for url rather than the entire cleaned_data.
Doc link for the form validation process: https://docs.djangoproject.com/en/5.1/ref/forms/validation/
(If you want to keep this as a clean() method, you may want to call self.add_error() so you can indicate the field to which errors apply)
| </div> | ||
| {% endif %} | ||
| <div class="alert alert-info my-3"> | ||
| This form will link additional Notes and Recordings to this session with a revision of "Current at time of presentation". For more fine grained control of versions, or to remove Notes and Recordings from a session, adjust the sessions associated with an Internet-Draft from the Internet-Draft's main page. |
There was a problem hiding this comment.
Sorry - missed commenting on this earlier: it's copied from the workflow that lets chairs associate drafts with sessions, and the lifecycle of such things is not the same. With drafts, chairs sometimes want to associate an older version with a session. Here, older recordings aren't really something to point to. It's a good indication that recording as a type of document may have been a mis-model. But in any case, please just remove this paragraph.
fixes #8033
Notes and recordings
Adding notes and recordings
