feat: show existing recordings on materials page#8102
feat: show existing recordings on materials page#8102rjsparks merged 8 commits intoietf-tools:mainfrom
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #8102 +/- ##
==========================================
+ Coverage 88.78% 88.90% +0.11%
==========================================
Files 296 304 +8
Lines 41320 41351 +31
==========================================
+ Hits 36687 36763 +76
+ Misses 4633 4588 -45 ☔ View full report in Codecov by Sentry. |
| {% endif %} | ||
| {% endfor %} | ||
| {% endif %} | ||
| {% if session.session_recording_url %} |
There was a problem hiding this comment.
Move this out of the with.
Putting the meetecho recording name directly on session is very new, and stands independent of whether there are any documents of type recording associated with the session.
| <td> | ||
| <a href="{{ session.session_recording_url }}"> | ||
| <i class="bi bi-file-slides"></i> | ||
| {% if 'https://meetecho-player.ietf.org/' in session.session_recording_url %} |
There was a problem hiding this comment.
Good futureproofing, but the literal should come out of settings. Currently the configuration looks like input for interpolation:
MEETECHO_SESSION_RECORDING_URL = "https://meetecho-player.ietf.org/playout/?session={session_label}"
I suggest either changing settings.py to
MEETECHO_PLAYER_URL = "https://meetecho-player.ietf.org"
MEETECHO_SESSION_RECORDING_URL = MEETECHO_PLAYER_URL+"/playout/?session={session_label}"
and then chainging this if to be
{% if settings.MEETECHO_PLAYER_URL in session.session_recording_url %}
or backing the complexity out altogether as YNGNI (when we have something else coming out of the session_recording_url function, this code will likely need to be touched again anyhow).
There was a problem hiding this comment.
after typing all that, I lean pretty strongly towards saying remove the if and always say Meetecho until we have a need to say otherwise.
There was a problem hiding this comment.
Concur re: leaving it out.
|
@rjsparks @jennifer-richards any more changes needed on this PR? |
No - sorry it took so long to merge |
3 participants
See #8084
This shows existing recordings on materials page
existing recordings, on materials page
