feat: Add session recordings by holloway · Pull Request #8218 · ietf-tools/datatracker

holloway · 2024-11-17T21:49:47Z

Replaces #8084

Allows a secretariat to add and delete session recordings (youtube urls)

holloway · 2024-11-17T21:50:57Z

+    today = datetime.datetime.now()
+    initial = {'title': f"Video recording for {session.group.acronym} on {today.strftime('%b-%d-%Y at %H:%M:%S')}"}


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?

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.

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.

Done, it now looks like this

Based on the assumption that session.meeting.date is already UTC. Is that correct?

Sadly, I don't think so, it'll be the date in the meeting timezone.

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

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.

holloway · 2024-11-17T22:10:17Z

+        if delete:
+            delete_recording(pk=delete, session=session)


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?

Regarding the 'Revision' column is that valid for recordings? How would someone update the revision of a recording anyway?

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.

Thoughts on the delete button? Should it have a confirmation modal?

I'm going to assume we need a confirmation modal.

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.

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.

… delete recording

…o be clearer

jennifer-richards · 2024-11-19T00:43:21Z

+    initial = {
+        'title': f"Video recording for {session.group.acronym} on {session.meeting.date.strftime('%b-%d-%Y at %H:%M:%S')}"
+    }


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.

Suggested change

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

}

Noticed I botched the timeslotassignment.timeslot step - glad you caught it!

rjsparks · 2024-11-19T15:45:42Z

+        </table>
+    </form>
+    <dialog id="delete_confirm_dialog">
+        <p>Really delete <a href="#" id="delete_confirm_link"></a>?</p>


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.

Ok, it looks like this now

codecov · 2024-11-19T23:52:32Z

Codecov Report

Attention: Patch coverage is 92.59259% with 4 lines in your changes missing coverage. Please review.

Project coverage is 88.82%. Comparing base (4f53cad) to head (d801631).
Report is 41 commits behind head on main.

Files with missing lines	Patch %	Lines
ietf/meeting/utils.py	70.00%	3 Missing ⚠️
ietf/meeting/views.py	97.72%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #8218      +/-   ##
==========================================
+ Coverage   88.81%   88.82%   +0.01%     
==========================================
  Files         312      312              
  Lines       40876    40908      +32     
==========================================
+ Hits        36304    36337      +33     
+ Misses       4572     4571       -1

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

rjsparks

I think this will work now (and it seems fine with manual testing). We need CI tests though.

rjsparks · 2024-11-20T14:40:58Z

+        delete = request.POST.get('delete', False)
+        if delete:
+            delete_recording(pk=delete, session=session)


It would be better to utilize the form framework here to get back a Document instead of a pk. As written, I think a malicious post could delete an arbitrary document.

rjsparks · 2024-11-20T14:43:29Z

+    '''
+    document = Document.objects.filter(pk=pk, group=session.group).first()
+    if document:
+        document.delete()


This is what caught my eye. This function would be better accepting a Document object rather than a pk, and perhaps should live in the Session model so that the connection to Session is naturally enforced.

jennifer-richards · 2024-11-20T16:13:12Z

This is a suggestion following up @rjsparks' comments inline regarding deletion of the Document and the interface for doing so. The inline conversation was long enough I think this might be easier to follow.

I think the "Delete the link to..." action should delete the SessionPresentation that links to the Document rather than the Document itself. This avoids the worry about changing the historical policy about Document lifetimes. It also lets the delete_recording() action restrict itself to the Session rather than using session.group as a proxy and trusting the discretion/awareness of materials managers.

When deleting the SessionPresentation, the Document should have its "recording" state set to "deleted" rather than actually being deleted from the database.

As far as pk vs instances, I don't think the modeling will let us use ModelForm in the way @rjsparks was suggesting. If it can, that'd be better, but SessionPresentation is really just an annotated through model between Document and Session. If this was planned to be a traditional HTML forms interface, the right approach would be a FormSet that could do that magic, but as an endpoint for a JS front end I think we have to roll our own.

rjsparks · 2024-11-20T17:42:41Z

This particular view isn't using a js frontend.

jennifer-richards · 2024-11-21T18:53:09Z

This particular view isn't using a js frontend.

Ah, I guess I'd mixed it up with work in other PRs. In that case we might want to use a formset

…dings-mk2

*shakes fist at pycharm*

plus slight rephrase

Tests were perturbed in a way that led to a test getting an interim instead of an IETF meeting. This exposed a bug reversing the URL for the materials_document() view. This splits it into two patterns that are equivalent to the original.

holloway added 3 commits November 15, 2024 13:46

feat: add session recordings

d4052f9

feat: add session recordings

420e296

feat: deleting recordings

061b68a

holloway commented Nov 17, 2024

View reviewed changes

feat: deleting recordings and initial form values

2c15423

holloway commented Nov 17, 2024

View reviewed changes

holloway requested review from jennifer-richards and rjsparks and removed request for rjsparks November 17, 2024 22:10

holloway mentioned this pull request Nov 17, 2024

feat: Session adding notes and recordings #8084

Closed

holloway requested a review from rjsparks November 18, 2024 05:00

holloway added 4 commits November 19, 2024 09:54

feat: use meeting date rather than today for initial title field. Fix…

f0598e9

… delete recording

feat: confirm delete recordings modal. fix server utils delete recording

3cc968e

fix: removing debug console.log

ce04b71

feat: change button name from 'Ok' to 'Delete' for confirm deletion t…

606bf6d

…o be clearer

jennifer-richards reviewed Nov 19, 2024

View reviewed changes

rjsparks requested changes Nov 19, 2024

View reviewed changes

holloway added 4 commits November 20, 2024 07:34

feat: UTC time in string and delete modal text

bda7061

fix: django html validation tests

7b7caa2

fix: django html validation tests

fd4547e

fix: django html validation tests

7497306

holloway requested a review from rjsparks November 20, 2024 01:37

rjsparks approved these changes Nov 20, 2024

View reviewed changes

rjsparks requested changes Nov 20, 2024

View reviewed changes

rjsparks reviewed Nov 20, 2024

View reviewed changes

rjsparks marked this pull request as draft November 22, 2024 14:15

jennifer-richards and others added 10 commits January 14, 2025 09:40

Merge branch 'main' into add-recordings-mk2

bca12a0

refactor: Work with SessionPresentations

01c8e8a

fix: better ordering

4783c43

chore: drop rev, hide table when empty

25a8eeb

test: test delete_recordings method

761eb36

fix: debug delete_recordings

9b293b1

Merge branch 'ietf-tools:main' into add-recordings

4dca900

test: test add_session_recordings view

67536d6

fix: better permissions handling

d0bc1ae

Merge remote-tracking branch 'holloway/add-recordings' into add-recor…

33ae6b7

…dings-mk2

jennifer-richards marked this pull request as ready for review January 17, 2025 16:47

jennifer-richards added 8 commits January 30, 2025 10:35

fix: only delete recordings for selected session

0145581

refactor: inline script -> js module

8d2297a

chore: remove accidental import

f2e14ca

*shakes fist at pycharm*

fix: consistent timestamp format

6f9f337

plus slight rephrase

style: Black

9c66886

chore: remove comment

50a2ff8

test: update test to match

3af8938

fix: reversible url pattern for materials

d801631

Tests were perturbed in a way that led to a test getting an interim instead of an IETF meeting. This exposed a bug reversing the URL for the materials_document() view. This splits it into two patterns that are equivalent to the original.

rjsparks approved these changes Jan 31, 2025

View reviewed changes

rjsparks merged commit db7d307 into ietf-tools:main Jan 31, 2025

github-actions Bot locked as resolved and limited conversation to collaborators Feb 4, 2025

		today = datetime.datetime.now()
		initial = {'title': f"Video recording for {session.group.acronym} on {today.strftime('%b-%d-%Y at %H:%M:%S')}"}

Conversation

holloway commented Nov 17, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jennifer-richards Nov 18, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jennifer-richards Nov 19, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

holloway Nov 18, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codecov Bot commented Nov 19, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

rjsparks left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jennifer-richards commented Nov 20, 2024

Uh oh!

rjsparks commented Nov 20, 2024

Uh oh!

jennifer-richards commented Nov 21, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

holloway commented Nov 17, 2024 •

edited

Loading

jennifer-richards Nov 18, 2024 •

edited

Loading

jennifer-richards Nov 19, 2024 •

edited

Loading

holloway Nov 18, 2024 •

edited

Loading

codecov Bot commented Nov 19, 2024 •

edited

Loading