fix: Hide last modified field in agenda when unavailable by microamp · Pull Request #7722 · ietf-tools/datatracker

microamp · 2024-07-21T00:52:34Z

The cut-over date is set to 1980-01-01 to be consistent with the behaviour of the JS client.

const updatedDatetime = DateTime.fromISO(agendaStore.meeting.updated).setZone(agendaStore.timezone)
if (!updatedDatetime.isValid || updatedDatetime < DateTime.fromISO('1980-01-01')) {
  return false
}

(in client/agenda/Agenda.vue)

microamp · 2024-07-21T01:02:45Z

The cut-over date is set to 1980-01-01 to be consistent with the behaviour of the JS client.

We may choose to implement the filter in the backend code to ensure consistency across all clients in the future.

codecov · 2024-07-21T01:24:14Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 88.80%. Comparing base (b00dfd3) to head (f121127).
Report is 15 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #7722      +/-   ##
==========================================
+ Coverage   88.79%   88.80%   +0.01%     
==========================================
  Files         296      296              
  Lines       41319    41330      +11     
==========================================
+ Hits        36688    36703      +15     
+ Misses       4631     4627       -4

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

microamp · 2024-07-21T02:02:19Z

            (slot.time + slot.duration).astimezone(meeting.tz()).strftime("%H%M"),
        ))
        self.assertContains(r, f"shown in the {meeting.tz()} time zone")
+        updated = meeting.updated().astimezone(meeting.tz()).strftime("%Y-%m-%d %H:%M:%S %Z")


Please double check for me that this isn't prone to race condition. Same with line 314 below. Otherwise, I'm happy to patch it with a mock return value instead of directly calling meeting.updated().

P.S. I encountered some errors locally earlier, but I haven't been able to reproduce them since, which made me think that it might be related to race condition.

the tests all run in a single thread in a single process for now - you can't be fighting a race.

It would be nice to be able to use threading, but we have dependencies that would have to change (and we would have to rewrite our custom harness around the django test framework.)

Thanks. Maybe it was a bug I accidentally introduced while updating the code locally. It doesn't appear to be there anymore.

Faster tests would be nice in the future.

jennifer-richards · 2024-07-22T23:19:13Z

 {% filter center:72 %}{{ schedule.meeting.agenda_info_note|striptags|wordwrap:72|safe }}{% endfilter %}
 {% endif %}
 {% filter center:72 %}{{ schedule.meeting.date|date:"F j" }}-{% if schedule.meeting.date.month != schedule.meeting.end_date.month %}{{ schedule.meeting.end_date|date:"F " }}{% endif %}{{ schedule.meeting.end_date|date:"j, Y" }}{% endfilter %}
+{% if updated|date:"Y-m-d" >= "1980-01-01" %}


You're creating a y10k bug here, aren't you?

Unfortunately, the only way I can see to do this in the template is updated.timestamp >= 315532800 which is a pretty awful magic number (it's the number of seconds between 1970-01-01T00:00:00Z and 1980-01-01T00:00:00Z).

I wonder if we can move the check into the view and avoid passing bad data to the template / javascript

Thanks for having a look.

You're creating a y10k bug here, aren't you?

I'm not sure. But updated will always be a datetime object, and the year must be four digits (based on the C standard). https://docs.python.org/3/library/datetime.html#strftime-and-strptime-format-codes

Unfortunately, the only way I can see to do this in the template is updated.timestamp >= 315532800 which is a pretty awful magic number (it's the number of seconds between 1970-01-01T00:00:00Z and 1980-01-01T00:00:00Z).

I wonder how 1980-01-01 was selected as the cutover date. The default value ietf.meeting.models.Meeting.updated uses is 1970-01-01. If that was the edge case to cater for, updated.timestamp > 0 would've been sufficient, and more readable. I may be missing some context here though.

I wonder if we can move the check into the view and avoid passing bad data to the template / javascript

I can return ~~"date": null~~ (typo) "updated": null back if the date is too old (i.e. pre-1980), and both the JS client and the template can check for null. But this means that the behaviour of the API will change.

You're creating a y10k bug here, aren't you?

I'm not sure. But updated will always be a datetime object, and the year must be four digits (based on the C standard). https://docs.python.org/3/library/datetime.html#strftime-and-strptime-format-codes

Yeah - in the year 10000, four digits is no longer adequate so it'll roll over and the check will fail

>>> "10000-01-01" < "1980-01-01" True

(Obviously it's not very likely this code will still be running in ~ 8000 years, so this is pretty pedantic)

Unfortunately, the only way I can see to do this in the template is updated.timestamp >= 315532800 which is a pretty awful magic number (it's the number of seconds between 1970-01-01T00:00:00Z and 1980-01-01T00:00:00Z).

I wonder how 1980-01-01 was selected as the cutover date. The default value ietf.meeting.models.Meeting.updated uses is 1970-01-01. If that was the edge case to cater for, updated.timestamp > 0 would've been sufficient, and more readable. I may be missing some context here though.

I suspect it's just an arbitrary date that's long enough ago that we don't care about the distinction and isn't right up against the Unix epoch start so that time zone handling errors won't crop up.

I wonder if we can move the check into the view and avoid passing bad data to the template / javascript

I can return ~~"date": null~~ (typo) "updated": null back if the date is too old (i.e. pre-1980), and both the JS client and the template can check for null. But this means that the behaviour of the API will change.

The API exists only to support the agenda, so as long as we update that we're ok.

The Meeting.updated() method is also used to set m.cached_updated that's used in a couple of templates - those will also need a tweak to handle a None value..

Discussed with Robert and the only real concern is that we end up with a method that returns Optional[datetime.datetime], which is a more complicated signature. I think that's ok, because it reflects that this method really may not have a valid response. Better to use None than to use a magic timestamp.

Thanks for the explanation.

What I meant to say is that the edge case wouldn't have been possible, as the year cannot be more than four digits.

datetime(10000, 1, 1)

Traceback (most recent call last): File "<stdin>", line 1, in <module> ValueError: year 10000 is out of range

datetime(9999, 12, 31) + timedelta(days=1)

Traceback (most recent call last): File "<stdin>", line 1, in <module> OverflowError: date value out of range

Re: Meeting.updated(), I'll update it as per the discussion above, and update the callers of the method accordingly. Thanks for pointing out that m.cached_updated also needs attention!

I took a closer look at what m.cached_updated is being used for - it's for the DTSTAMP property in the .ics files. e.g. https://github.com/ietf-tools/datatracker/blob/main/ietf/templates/meeting/important_dates_for_meeting.ics#L6

According to RFC 5545, the DTSTAMP property, https://www.rfc-editor.org/rfc/rfc5545#section-3.8.7.2, is required.

eventprop = *( ; ; The following are REQUIRED, ; but MUST NOT occur more than once. ; dtstamp / uid / ; ... )

https://www.rfc-editor.org/rfc/rfc5545#section-3.6.1

So, unlike in agenda.txt, we wouldn't be able to hide the value if ietf.meeting.models.Meeting.updated returned None. Let me know if I'm reading it wrong, or if there's a later RFC that says otherwise. (I couldn't find any.)

It's probably better to leave ietf.meeting.models.Meeting.updated as-is. Let me know what you think.

Wow, I'm disappointed to learn that datetime is limited that way.

I still think we should make the Meeting.updated() method return None if it doesn't have a date, just so it's not lying. In the specific case where we need a date, I think filling in a sensible fake value as close to the place we need it as possible is the thing to do. Here, maybe when computing cached_updated or in the template tag would be a good place.

Does that seem sensible?

Does that seem sensible?

It does. Thanks for your comment. I'll make changes accordingly.

Extra commits have been pushed. The Meeting.updated() method now returns Optional[datetime.datetime] instead of datetime.datetime.

Let me know if I missed anything.

P.S. I wrote some additional tests for DTSTAMP, but they need some further work. I will create a separate PR when they're ready.

richsalz · 2024-07-23T17:45:04Z

I would suggest adding "semantic-oriented" methods to the right object. See #7197 (the later comments; it took me awhile to get to this point :)

jennifer-richards · 2024-07-23T21:54:48Z

I would suggest adding "semantic-oriented" methods to the right object. See #7197 (the later comments; it took me awhile to get to this point :)

Good point. This one is on the Meeting object, which is I think a reasonable place in the hierarchy, though it annoyingly has to pull most of the information from other models.

microamp · 2024-07-24T09:12:30Z

@richsalz Thanks for your comment. I'll have a look at your PR, and see what can be improved here. 👍

rjsparks · 2024-07-26T16:40:55Z

-        return max(timeslots_updated, sessions_updated, assignments_updated)
+            assignments_updated = SchedTimeSessAssignment.objects.filter(schedule__in=[self.schedule, self.schedule.base if self.schedule else None]).aggregate(Max('modified'))["modified__max"]
+        dts = [timeslots_updated, sessions_updated, assignments_updated]
+        if valid_only := [dt for dt in dts if dt is not None]:


why do we need the walrus? (we've been avoiding it in the project so far)

(we've been avoiding it in the project so far)

I wasn't aware of that. We don't need it - it's just sugar. It's now removed.

The walrus operator is used a lot in bibxml-service. So I probably assumed it's okay/recommended to use elsewhere.

rjsparks · 2024-08-02T17:56:26Z

        dts = [timeslots_updated, sessions_updated, assignments_updated]
-        if valid_only := [dt for dt in dts if dt is not None]:
-            return max(valid_only)
-        return None
+        valid_only = [dt for dt in dts if dt is not None]
+        return max(valid_only) if valid_only else None


Consider:

dts = set([timeslots_updated, sessions_updated, assignments_updated]) dts.discard(None) return max(dts) if len(dts)>0 else None

(this is a pretty small style/readability nit)

I'll merge this without the above suggestion - if you agree with it, please make another PR.

TIL discard. Thanks. I'll see if a PR is necessary.

microamp added 2 commits July 21, 2024 11:15

test: Update tests to check for Updated field in agenda.txt

7f62d7f

fix: Hide Updated in agenda.txt if too old

e57a0a9

microamp marked this pull request as ready for review July 21, 2024 01:36

microamp requested review from jennifer-richards and rjsparks July 21, 2024 01:37

microamp commented Jul 21, 2024

View reviewed changes

jennifer-richards reviewed Jul 22, 2024

View reviewed changes

microamp added 2 commits July 26, 2024 09:38

test: Remove confusing tests on CSV agenda

057a800

refactor: Make updated() return None when no valid timestamp found

e71be3e

microamp changed the title ~~fix: Hide Updated in agenda.txt if too old~~ fix: Hide last modified field in agenda when unavailable Jul 26, 2024

rjsparks reviewed Jul 26, 2024

View reviewed changes

refactor: Remove walrus operator

f121127

jennifer-richards approved these changes Jul 27, 2024

View reviewed changes

rjsparks reviewed Aug 2, 2024

View reviewed changes

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

github-actions Bot locked as resolved and limited conversation to collaborators Aug 12, 2024

Conversation

microamp commented Jul 21, 2024

Uh oh!

microamp commented Jul 21, 2024

Uh oh!

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

Uh oh!

Codecov Report

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!

microamp Jul 23, 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!

microamp Jul 24, 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!

richsalz commented Jul 23, 2024

Uh oh!

jennifer-richards commented Jul 23, 2024

Uh oh!

microamp commented Jul 24, 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

codecov Bot commented Jul 21, 2024 •

edited

Loading

microamp Jul 23, 2024 •

edited

Loading

microamp Jul 24, 2024 •

edited

Loading