fix: Show recordings for interims#7197

richsalz · 2024-03-16T05:03:48Z

List the recordings if the "meeting numnber" starts with "interim"

List the recordings if the "meeting numnber" starts with "interim" Fixes: ietf-tools#6543

richsalz · 2024-03-16T05:18:05Z

Should the same fix be made for the buttons, at https://github.com/ietf-tools/datatracker/blob/main/ietf/templates/meeting/session_buttons_include.html#L139 ?

rjsparks · 2024-03-16T05:22:42Z

ietf/templates/meeting/session_details_panel.html

@@ -320,7 +320,7 @@ <h3 class="mt-4">Notes and recordings</h3>
                </tr>
            {% endif %}
            {# Recordings #}
-            {% if meeting.number|add:"0" >= 80 %}
+            {% if meeting.number|add:"0" >= 80  or "interim" in meeting.number %}


Suggested change

{% if meeting.number|add:"0" >= 80 or "interim" in meeting.number %}
{% if meeting.type_id == 'ietf' and meeting.number|add:"0" >= 80 or meeting.type_id != 'ietf' %}

The above suggestion is a bit awkward - it requires knowing the precedence of operators as documented at https://docs.djangoproject.com/en/5.0/ref/templates/builtins/#boolean-operators. It might be better to move this into the view code to get away from the |add:"0" type-coersion-by-hack-trick anyhow.

I thought of something like that. In addition to the one mentioned above, the other tests against meeting 60. What's the significance of meetings 60 and 80? I'd like to remove the magic numbers, replacing with a reasonable function name.

they aren't magic (though they aren't complete) - they are places where the shape of proceedings and what was captured at meetings fundamentally changes. 60 was when we first started capturing chatlogs (jabber at the time). 80 was a point at where recording tech changed significantly.

That said, we don't have any Documents of type "recording" before ietf-100, so the code being touched here could could (for meetings of type ietf) not show the recordings block at all for meeting numbers < 100.

Improve the test to be more general. Co-authored-by: Robert Sparks <rjsparks@nostrum.com>

codecov · 2024-03-16T05:34:52Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 88.93%. Comparing base (187c2c5) to head (f414ae4).
Report is 79 commits behind head on main.

❗ Current head f414ae4 differs from pull request most recent head a4c3198. Consider uploading reports for the commit a4c3198 to get more accurate results

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #7197      +/-   ##
==========================================
- Coverage   88.98%   88.93%   -0.05%     
==========================================
  Files         291      291              
  Lines       40717    41063     +346     
==========================================
+ Hits        36233    36521     +288     
- Misses       4484     4542      +58

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

rjsparks · 2024-03-16T05:39:38Z

Should the same fix be made for the buttons, at https://github.com/ietf-tools/datatracker/blob/main/ietf/templates/meeting/session_buttons_include.html#L139 ?

Looks like it.

This really needs a new (or updated) test.

Put same "records available?" test in the buttons that we did in the interim meeting status page. Fixes: ietf-tools#6543

Add filters to meeting to test if we have recordings and have_notes, rather than inline examination of the meeting instance and tests.

Remove the use_notes parameter from the various templates. Instead, add a new meeting filter, has_notes, and use that in the templates.

rjsparks

This is shaping up nicely. It should go a litter further as noted inline.

rjsparks · 2024-03-22T18:06:32Z

ietf/doc/templatetags/ietf_filters.py

@@ -881,3 +881,19 @@ def badgeify(blob):
            )

    return text
+
+@register.filter
+def has_chat_logs(meeting):


Should this get the same "Return true for interims" treatment?

It's not clear that these should be filters - why aren't they methods on Meeting instead? (those would be visible to the templates). I think you were on the way to discovering that with the partial removal of the has_notes filter, and the redundant argument passed to the templates in the views later.

Happy to make them methods on the meeting if you prefer. I didn't know templates could invoke methods directly :)

Should this get the same "Return true for interims" treatment?

I guess? I changed it in the commit 466a630

rjsparks · 2024-03-22T18:07:14Z

ietf/doc/templatetags/ietf_filters.py

+#register.filter
+#define has_notes(meeting):
+    return meeting.uses_notes()


This looks incomplete - if the intent was to remove the filter L899 should also go.

(and I suspect all of these should just be removed once the functionality is moved to Meeting.

yes, this was just checking in a snapshot before i got on the plane. will move to templates and fix shortly.

Also fix has_notes() to be consistent with has_recordings()

This is for consistency with the meeting uses_notes method

fix: Show recordings for interims

becd236

List the recordings if the "meeting numnber" starts with "interim" Fixes: ietf-tools#6543

rjsparks reviewed Mar 16, 2024

View reviewed changes

Update ietf/templates/meeting/session_details_panel.html

ef87281

Improve the test to be more general. Co-authored-by: Robert Sparks <rjsparks@nostrum.com>

richsalz added 3 commits March 16, 2024 04:12

fix: Update test in session buttons

f414ae4

Put same "records available?" test in the buttons that we did in the interim meeting status page. Fixes: ietf-tools#6543

refactor: add two filters

423dd4c

Add filters to meeting to test if we have recordings and have_notes, rather than inline examination of the meeting instance and tests.

refactor; Replace use_notes param with a filter

65d8394

Remove the use_notes parameter from the various templates. Instead, add a new meeting filter, has_notes, and use that in the templates.

rjsparks requested changes Mar 22, 2024

View reviewed changes

richsalz added 2 commits March 22, 2024 17:55

fix: Replace filters with methods

466a630

Also fix has_notes() to be consistent with has_recordings()

refactor: chante agenda.useNotes to uses_notes

a4c3198

This is for consistency with the meeting uses_notes method

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: Show recordings for interims#7197

fix: Show recordings for interims #7197

richsalz commented Mar 16, 2024

richsalz commented Mar 16, 2024

rjsparks Mar 16, 2024

rjsparks Mar 16, 2024

richsalz Mar 16, 2024

rjsparks Mar 20, 2024

codecov bot commented Mar 16, 2024 •
edited

rjsparks commented Mar 16, 2024

rjsparks left a comment

rjsparks Mar 22, 2024

rjsparks Mar 22, 2024

richsalz Mar 22, 2024

richsalz Mar 22, 2024

rjsparks Mar 22, 2024

rjsparks Mar 22, 2024

richsalz Mar 22, 2024

	{% if meeting.number\|add:"0" >= 80 or "interim" in meeting.number %}
	{% if meeting.type_id == 'ietf' and meeting.number\|add:"0" >= 80 or meeting.type_id != 'ietf' %}

fix: Show recordings for interims#7197

Are you sure you want to change the base?

fix: Show recordings for interims #7197

Conversation

richsalz commented Mar 16, 2024

richsalz commented Mar 16, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Mar 16, 2024 • edited

Codecov Report

rjsparks commented Mar 16, 2024

rjsparks left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Mar 16, 2024 •
edited