fix: Show recordings for interims by richsalz · Pull Request #7197 · ietf-tools/datatracker

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

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

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

            {% 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.

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

Codecov Report

Attention: Patch coverage is 85.71429% with 3 lines in your changes missing coverage. Please review.

Project coverage is 88.77%. Comparing base (c7f6bde) to head (9c1dd11).
Report is 30 commits behind head on main.

Files	Patch %	Lines
ietf/meeting/models.py	78.57%	3 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #7197      +/-   ##
==========================================
- Coverage   88.78%   88.77%   -0.02%     
==========================================
  Files         296      299       +3     
  Lines       41320    41355      +35     
==========================================
+ Hits        36687    36712      +25     
- Misses       4633     4643      +10

☔ 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.

rjsparks

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

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

    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

+#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.

richsalz · 2024-07-22T21:33:44Z

Rebased, squashed, fixed the tests. This is ready for review now.

Add methods uses_notes(), has_recordings(), and uses_chat_logs() to the meeting object (with semantically correct tests) and use them consistently throughout. List the recordings if the "meeting numnber" starts with "interim" Fixes: ietf-tools#6543

jennifer-richards

Overall looks good - thank you for getting rid of the kludgy template code! There are a few nits to adjust that I've noted inline but I don't see any big issues.

jennifer-richards · 2024-07-25T18:18:24Z

+        if self.type_id != 'ietf':
+            return True
+        num = self.get_number()
+        return num != None and num >= 108


Generally preferable to write the first test as num is not None

jennifer-richards · 2024-07-25T18:27:51Z

+        return num != None and num >= 80
+
+    def has_chat_logs(self):
+        num = self.get_number()


For symmetry with the other has_* methods, I think it'd be good to make clear how this behaves with non-"ietf" meetings

jennifer-richards · 2024-07-25T20:34:32Z

        "categories": filter_organizer.get_filter_categories(),
        "isCurrentMeeting": is_current_meeting,
-        "useNotes": meeting.uses_notes(),
+        "uses_notes": meeting.uses_notes(),


Should also stay as a camelCase key (usesNotes) to match the rest of the structure

style: Use "is not None" instead of "!=" For non-IETF meetings assume chat logs exist

richsalz · 2024-08-02T20:33:46Z

@jennifer-richards I believe I addressed all your feedback, PTAL.

jennifer-richards

One question inline

rjsparks

Let me know if I misread the request for keeping the js variables in camelCase.

Also add comment about meeting number field in tests

jennifer-richards

I think there's still one variable name change needed

jennifer-richards · 2024-08-06T15:08:09Z

I think there was still disagreement about useNotes vs usesNotes. Easier to push a commit than to point at them, but please review that I'm not mixing up similarly named variables in 4e8ce54

rjsparks reviewed Mar 16, 2024

View reviewed changes

rjsparks requested changes Mar 22, 2024

View reviewed changes

richsalz force-pushed the fix-6543 branch 5 times, most recently from 5a99e85 to 89449d7 Compare July 22, 2024 21:32

fix: Show recordings for interims

1fb3b40

Add methods uses_notes(), has_recordings(), and uses_chat_logs() to the meeting object (with semantically correct tests) and use them consistently throughout. List the recordings if the "meeting numnber" starts with "interim" Fixes: ietf-tools#6543

richsalz force-pushed the fix-6543 branch from 89449d7 to 1fb3b40 Compare July 22, 2024 21:44

richsalz mentioned this pull request Jul 23, 2024

fix: Hide last modified field in agenda when unavailable #7722

Merged

jennifer-richards requested changes Jul 25, 2024

View reviewed changes

richsalz added 3 commits July 29, 2024 12:18

style: Use "is not" and "is" for None comparisons

c03f9b4

None comparison and non-IETF meetings

ab9ae1b

style: Use "is not None" instead of "!=" For non-IETF meetings assume chat logs exist

fix: Restore useNotes for JS fields

ea28b5a

jennifer-richards reviewed Aug 2, 2024

View reviewed changes

Comment thread ietf/meeting/models.py

rjsparks requested changes Aug 5, 2024

View reviewed changes

Comment thread client/agenda/store.js

Comment thread ietf/meeting/tests_views.py Outdated

fix: uses_notes->useNotes (in JavaScript)

44d426c

Also add comment about meeting number field in tests

jennifer-richards requested changes Aug 5, 2024

View reviewed changes

Comment thread client/agenda/AgendaScheduleList.vue Outdated

richsalz and others added 2 commits August 6, 2024 14:43

Missed a uses_notes->useNotes edit

29817a8

fix: useNotes->usesNotes

4e8ce54

jennifer-richards previously approved these changes Aug 7, 2024

View reviewed changes

rjsparks previously approved these changes Aug 7, 2024

View reviewed changes

Merge remote-tracking branch 'ietf-tools/main' into fix-6543

9c1dd11

rjsparks dismissed their stale review via 9c1dd11 August 7, 2024 15:42

rjsparks dismissed jennifer-richards’s stale review via 9c1dd11 August 7, 2024 15:42

rjsparks approved these changes Aug 7, 2024

View reviewed changes

rjsparks merged commit 0c8db80 into ietf-tools:main Aug 7, 2024

richsalz deleted the fix-6543 branch August 7, 2024 16:27

github-actions Bot locked as resolved and limited conversation to collaborators Aug 12, 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' %}

Conversation

richsalz commented Mar 16, 2024 • edited by rjsparks Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

richsalz commented Mar 16, 2024

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 Mar 16, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

rjsparks commented Mar 16, 2024

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!

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!

richsalz commented Jul 22, 2024

Uh oh!

jennifer-richards left a comment

Choose a reason for hiding this comment

Uh oh!

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!

richsalz commented Aug 2, 2024

Uh oh!

jennifer-richards left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

rjsparks left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

jennifer-richards left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jennifer-richards commented Aug 6, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

richsalz commented Mar 16, 2024 •

edited by rjsparks

Loading

codecov Bot commented Mar 16, 2024 •

edited

Loading