fix: Stop interim edit from clobbering Meetecho remote_instructions (#10689)#10703
fix: Stop interim edit from clobbering Meetecho remote_instructions (#10689)#10703ekr wants to merge 2 commits intoietf-tools:mainfrom
Conversation
…10689) Editing an interim meeting backed by an auto-created Meetecho conference used to lose the Remote instructions URL whenever the subsequent Meetecho API call failed (and silently orphan the old conference when it succeeded). The WG chair was left with an interim that pointed nowhere and had to escalate to the secretariat to manually recover — losing uploaded slides in the process. Root cause: `InterimSessionModelForm.clean()` unconditionally blanked `remote_instructions` whenever the form was submitted with `remote_participation=='meetecho'`, on the assumption that the post-save helper would then fill in a fresh Meetecho URL. That assumption held for the create flow but falls apart on edit: if the helper's API call to Meetecho fails for any reason (network, auth, rate limit), the URL is already gone from the in-memory form data *and* has been persisted by `ModelForm.save()`, leaving nothing to fall back on. Fixes: - `forms.py`: stop blanking `remote_instructions` in `clean()`. The helper layer decides whether to overwrite the URL and only does so on successful Meetecho creation. Also initialize the `remote_participation` radio from the stored URL on edit so a no-op resubmit doesn't flip modes. - `helpers.py`: only (re)create a Meetecho conference when one of the scheduling fields (`date`, `time`, `requested_duration`, `end_time`) actually changed. Merely editing the agenda now leaves the existing conference alone. On a successful recreate, the old conference is fetched and deleted so it is not orphaned on the Meetecho side. - Tests in `tests_forms.py` and `tests_helpers.py` cover: - form-level: clean() preserves the URL across a Meetecho-mode edit; - helper-level: failure path (exception and empty result) preserves the URL; success path deletes the previous conference; - post-save filter: skip when only non-timing fields changed, recreate when any timing field changed, always create when no URL exists yet. Note: the "is this a Meetecho URL" check currently uses a substring match on "meetecho"; a dedicated flag on Session would be cleaner but requires a migration and is out of scope for this bugfix. Fixes ietf-tools#10689. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
ekr
changed the title
|
Hmm.. This didn't fail locally. Debugging. |
|
Pretty hard to see how this PR could have created this bug. I can't seem to re-kick off the CI, but maybe try doing that and see if it's an intermittent somehow? |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #10703 +/- ##
==========================================
+ Coverage 88.44% 88.47% +0.02%
==========================================
Files 330 330
Lines 44365 44392 +27
==========================================
+ Hits 39237 39274 +37
+ Misses 5128 5118 -10 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
The best-effort delete of the replaced Meetecho conference catches any exception from fetch/delete and logs it, to ensure a successful recreate is never undone by a cleanup hiccup. Add a test that forces fetch() to raise and asserts the new URL still lands, delete_conference is not invoked, and nothing propagates out. Closes the remaining uncovered lines flagged by codecov on ietf-tools#10703. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
| confs = meetecho_manager.create( | ||
| group=session.group, | ||
| session_id=session.pk, | ||
| description=str(session), | ||
| start_time=ts.utc_start_time(), | ||
| duration=ts.duration, | ||
| ) |
There was a problem hiding this comment.
If a Meetecho session already exists with that session_id, you cannot use the create api to update it: it will always fail.
We may need to implement an update api on the Meetecho side if we want to safely change timing-related parameters.
Otherwise you may call the delete api first and then create, but in this case a guard is needed to avoid deleting a session that has already started (which would kick everybody out of the room).
There was a problem hiding this comment.
update seems better. Do you have a sense of how long that would take?
There was a problem hiding this comment.
Having an update API (as it's easy to implement) is fine, but we need to be careful about when/how to use it. There's been a lot of pressure to never remove a scheduled session from a published calendar (like /meeting/upcoming, or the main meeting agenda for ietf meetings). People that saw the original announcement/made plans have asked that there be a clear "canceled" or "moved" notification at the place they originally expected the session to be.
But this is wandering away from this particular issue as David wasn't trying to change the time parameters.
| # Best-effort: clean up the conference we just replaced so it | ||
| # doesn't get left orphaned on the Meetecho side. Failures here | ||
| # must not undo the successful recreate. | ||
| if previous_url and 'meetecho' in previous_url and previous_url != confs[0].url: | ||
| try: | ||
| for conference in meetecho_manager.fetch(session.group): | ||
| if conference.url == previous_url: | ||
| conference.delete() | ||
| break | ||
| except Exception as err: | ||
| log.log( | ||
| f'Exception deleting old Meetecho conference {previous_url} ' | ||
| f'for {session}: {err}' | ||
| ) |
There was a problem hiding this comment.
I don't think you can never get to this code because it's not possible to have multiple meetecho sessions with the same session_id=session.pk (see previous comment).
4 participants
This PR was written by @claude but reviewed by @ekr
Editing an interim meeting backed by an auto-created Meetecho conference used to lose the Remote instructions URL whenever the subsequent Meetecho API call failed (and silently orphan the old conference when it succeeded). The WG chair was left with an interim that pointed nowhere and had to escalate to the secretariat to manually recover — losing uploaded slides in the process.
Root cause:
InterimSessionModelForm.clean()unconditionally blankedremote_instructionswhenever the form was submitted withremote_participation=='meetecho', on the assumption that the post-save helper would then fill in a fresh Meetecho URL. That assumption held for the create flow but falls apart on edit: if the helper's API call to Meetecho fails for any reason (network, auth, rate limit), the URL is already gone from the in-memory form data and has been persisted byModelForm.save(), leaving nothing to fall back on.Fixes:
forms.py: stop blankingremote_instructionsinclean(). The helper layer decides whether to overwrite the URL and only does so on successful Meetecho creation. Also initialize theremote_participationradio from the stored URL on edit so a no-op resubmit doesn't flip modes.helpers.py: only (re)create a Meetecho conference when one of the scheduling fields (date,time,requested_duration,end_time) actually changed. Merely editing the agenda now leaves the existing conference alone. On a successful recreate, the old conference is fetched and deleted so it is not orphaned on the Meetecho side.Tests in
tests_forms.pyandtests_helpers.pycover:Note: the "is this a Meetecho URL" check currently uses a substring match on "meetecho"; a dedicated flag on Session would be cleaner but requires a migration and is out of scope for this bugfix.
Fixes #10689.