feat: Add session recordings #8218
Conversation
ietf/meeting/views.py
Outdated
| today = datetime.datetime.now() | ||
| initial = {'title': f"Video recording for {session.group.acronym} on {today.strftime('%b-%d-%Y at %H:%M:%S')}"} |
There was a problem hiding this comment.
Video recording for tools on 2024-10-15 at 18:00:00. We should probably suggest a title of that form - @rjsparks
Done. Do timezones matter? Should it be UTC?
There was a problem hiding this comment.
I don't know what the existing TZ convention (if any) is, but I think UTC is the right choice. I'd probably add "... UTC" to the default name, though it'd mean a break from existing filename convention.
There was a problem hiding this comment.
Why today?
The recording is for a working group session and the times should be the times for that session, not the time of the upload of the recording.
We should consider capturing the moment of "upload" (really, setting a URL) as a docevent, but I don't think we're currently doing that when meetecho reports uploads to youtube, so maybe this is a separate issue for improving both.
But yes, we should present in UTC by default - some views offer browser or meeting local time as well - this upload doesn't need to though, I think.
There was a problem hiding this comment.
Done, it now looks like this
There was a problem hiding this comment.
Based on the assumption that session.meeting.date is already UTC. Is that correct?
There was a problem hiding this comment.
Sadly, I don't think so, it'll be the date in the meeting timezone.
There was a problem hiding this comment.
Meeting.start_datetime() will give you a timezone-aware midnight in the meeting timezone. If you have a Session (which I think you do), then the way to get the formatted start time of that session is:
session.official_timeslotassignment().timeslot.utc_start_time().strftime(...)There was a problem hiding this comment.
Also, this needs a guard against official_timeslotassignment() not existing unless it's guaranteed to exist. It should exist for anything that is on an agenda.
| if delete: | ||
| delete_recording(pk=delete, session=session) |
There was a problem hiding this comment.
We should think through what a chair would do if they entered the wrong link and wanted to fix it. @rjsparks
I've added a delete button. Is that ok?
There was a problem hiding this comment.
Regarding the 'Revision' column is that valid for recordings? How would someone update the revision of a recording anyway?
There was a problem hiding this comment.
It's valid as long as we're modeling recordings as Document objects.
To date, we have not revised a recording document. Doesn't mean we couldn't (with the current model anyhow). An -01 for youtube would just point to a different URL the same as an -01 for a draft points to a different filename.
There was a problem hiding this comment.
Thoughts on the delete button? Should it have a confirmation modal?
There was a problem hiding this comment.
I'm going to assume we need a confirmation modal.
There was a problem hiding this comment.
Done. It looks like this. I did it in plain JS with a <dialog> rather than Bootstrap as we're looking to migrate away from that.
There was a problem hiding this comment.
I have minor heartburn that we're allowing a Document object to be deleted. It violates an assumption about what Document objects are for, but I suppose we have that problem with these recording documents-that-are-just-external-links already and some future project should migrate them to some other model instead.
I'll request one change and we can move forward with this.
holloway
requested review from
rjsparks and
jennifer-richards
and removed request for
rjsparks
… delete recording
| initial = { | ||
| 'title': f"Video recording for {session.group.acronym} on {session.meeting.date.strftime('%b-%d-%Y at %H:%M:%S')}" | ||
| } |
There was a problem hiding this comment.
To be more concrete about my comments elsewhere, I think something like this is what is needed. The assertion() call is not bulletproof, there'll still be a 500 if this comes up.
| initial = { | |
| 'title': f"Video recording for {session.group.acronym} on {session.meeting.date.strftime('%b-%d-%Y at %H:%M:%S')}" | |
| } | |
| timeslot = session.official_timeslotassignment() | |
| assertion("timeslot is not None") | |
| initial = { | |
| 'title': f"Video recording for {session.group.acronym} on {timeslot.utc_start_time().strftime('%b-%d-%Y at %H:%M:%S')}" | |
| } |
| </table> | ||
| </form> | ||
| <dialog id="delete_confirm_dialog"> | ||
| <p>Really delete <a href="#" id="delete_confirm_link"></a>?</p> |
There was a problem hiding this comment.
Could we say Really delete the link to instead? We don't want to leave anyone with the impression that this will remove the recording at youtube.
Replaces #8084