diff --git a/ietf/meeting/forms.py b/ietf/meeting/forms.py index e5b1697f86..9466bbb294 100644 --- a/ietf/meeting/forms.py +++ b/ietf/meeting/forms.py @@ -296,6 +296,13 @@ def __init__(self, *args, **kwargs): doc = self.instance.agenda() content = doc.text_or_error() self.initial['agenda'] = content + # Initialise remote_participation from the existing remote_instructions + # so that a no-op resubmit does not flip the session between modes. + existing_remote = self.instance.remote_instructions or '' + if hasattr(settings, 'MEETECHO_API_CONFIG') and 'meetecho' in existing_remote: + self.initial['remote_participation'] = 'meetecho' + else: + self.initial['remote_participation'] = 'manual' # set up remote participation choices choices = [] @@ -325,9 +332,13 @@ def clean_requested_duration(self): return duration def clean(self): - if self.cleaned_data.get('remote_participation', None) == 'meetecho': - self.cleaned_data['remote_instructions'] = '' # blank this out if we're creating a Meetecho conference - elif not self.cleaned_data['remote_instructions']: + # Preserve the existing remote_instructions across submits: the helper + # layer (create_interim_session_conferences) decides whether to + # overwrite it, and only does so on successful Meetecho creation. + # Blanking it here would destroy the URL before the helper even runs, + # leaving the session unrecoverable on any Meetecho failure. + if self.cleaned_data.get('remote_participation', None) != 'meetecho' \ + and not self.cleaned_data.get('remote_instructions'): self.add_error('remote_instructions', 'This field is required') return self.cleaned_data diff --git a/ietf/meeting/helpers.py b/ietf/meeting/helpers.py index 39d271ae6b..b60fe9466c 100644 --- a/ietf/meeting/helpers.py +++ b/ietf/meeting/helpers.py @@ -1082,10 +1082,11 @@ def sessions_post_save(request, forms): form.save_agenda() try: - create_interim_session_conferences( + create_interim_session_conferences([ form.instance for form in forms if form.cleaned_data.get('remote_participation', None) == 'meetecho' - ) + and _needs_meetecho_refresh(form) + ]) except RuntimeError: messages.warning( request, @@ -1095,11 +1096,30 @@ def sessions_post_save(request, forms): ) +# Form fields that, when changed, require a Meetecho conference to be +# (re)created because they alter the scheduled time or duration. +_MEETECHO_TIMING_FIELDS = frozenset(('date', 'time', 'requested_duration', 'end_time')) + + +def _needs_meetecho_refresh(form): + """Does this session's Meetecho conference need to be (re)created? + + If the session already has a Meetecho URL and no timing-related field + changed, we leave the existing conference alone — recreating it would + orphan the old one and is unnecessary. + """ + existing = form.instance.remote_instructions or '' + if 'meetecho' not in existing: + return True # no Meetecho conference yet (empty or manual URL) + return bool(_MEETECHO_TIMING_FIELDS.intersection(form.changed_data)) + + def create_interim_session_conferences(sessions): error_occurred = False if hasattr(settings, 'MEETECHO_API_CONFIG'): # do nothing if not configured meetecho_manager = meetecho.ConferenceManager(settings.MEETECHO_API_CONFIG) for session in sessions: + previous_url = session.remote_instructions or '' ts = session.official_timeslotassignment().timeslot try: confs = meetecho_manager.create( @@ -1116,7 +1136,23 @@ def create_interim_session_conferences(sessions): if len(confs) == 1: session.remote_instructions = confs[0].url session.save() + # 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}' + ) else: + # Leave session.remote_instructions alone: preserving the + # previous URL is the whole point of the failure path. error_occurred = True if error_occurred: raise RuntimeError('error creating meetecho conferences') diff --git a/ietf/meeting/tests_forms.py b/ietf/meeting/tests_forms.py index 61d0a1d2c8..cec0ec37ad 100644 --- a/ietf/meeting/tests_forms.py +++ b/ietf/meeting/tests_forms.py @@ -133,6 +133,45 @@ def test_edits_in_meeting_time_zone(self): session.official_timeslotassignment().timeslot.local_start_time().strftime('%H:%M'), ) + @override_settings(MEETECHO_API_CONFIG={}) + def test_edit_preserves_existing_meetecho_remote_instructions(self): + """Editing an existing Meetecho-backed interim must not blank the stored + remote_instructions URL. Regression test for #10689: previously + clean() unconditionally set remote_instructions='' whenever + remote_participation=='meetecho', so if the subsequent Meetecho API + call failed (or even when it succeeded, it would orphan the existing + conference) the session was left with an empty URL. + """ + existing_url = 'https://meetings.conf.meetecho.com/interim/?session=35206' + session = SessionFactory( + meeting__type_id='interim', + meeting__time_zone='America/Halifax', + remote_instructions=existing_url, + ) + ts = session.official_timeslotassignment().timeslot + local_start = ts.local_start_time() + form_data = { + 'date': local_start.date().strftime('%Y-%m-%d'), + 'time': local_start.time().strftime('%H:%M'), + 'requested_duration': '01:00', + 'end_time': '', + 'remote_participation': 'meetecho', + 'remote_instructions': existing_url, + 'agenda': 'new agenda content', + 'agenda_note': '', + } + form = InterimSessionModelForm(data=form_data, instance=session) + self.assertTrue( + form.is_valid(), + f'form should validate, got errors: {form.errors!r}', + ) + self.assertEqual( + form.cleaned_data['remote_instructions'], + existing_url, + 'editing a Meetecho-backed interim must not blank the existing ' + 'remote_instructions URL (issue #10689)', + ) + class InterimMeetingModelFormTests(TestCase): def test_enforces_authroles(self): diff --git a/ietf/meeting/tests_helpers.py b/ietf/meeting/tests_helpers.py index b118b9f041..24d09192c9 100644 --- a/ietf/meeting/tests_helpers.py +++ b/ietf/meeting/tests_helpers.py @@ -625,6 +625,191 @@ def test_sessions_post_save_creates_meetecho_conferences(self, mock_create_metho self.assertEqual(len(msgs), 1) self.assertIn('An error occurred', msgs[0]) + @patch('ietf.utils.meetecho.ConferenceManager') + def test_create_interim_session_conferences_preserves_url_on_failure(self, mock): + """A failed (re)create must not clobber the existing remote_instructions. + + Regression test for ietf-tools/datatracker#10689: the WG chair reported + that editing an interim meeting wiped the working Meetecho URL whenever + the API call failed. The previous URL must survive the failure so the + meeting is still reachable. + """ + mock_conf_mgr = mock.return_value + original_url = 'https://meetings.conf.meetecho.com/interim/?session=42' + session = SessionFactory( + meeting__type_id='interim', + remote_instructions=original_url, + ) + + # create() raises -> RuntimeError, URL untouched + mock_conf_mgr.create.side_effect = ValueError('meetecho down') + with self.assertRaises(RuntimeError): + create_interim_session_conferences([session]) + self.assertEqual( + Session.objects.get(pk=session.pk).remote_instructions, + original_url, + ) + + # create() returns [] -> RuntimeError, URL still untouched + mock_conf_mgr.create.side_effect = None + mock_conf_mgr.create.return_value = [] + with self.assertRaises(RuntimeError): + create_interim_session_conferences([session]) + self.assertEqual( + Session.objects.get(pk=session.pk).remote_instructions, + original_url, + ) + + @patch('ietf.utils.meetecho.ConferenceManager') + def test_create_interim_session_conferences_deletes_previous_conference(self, mock): + """On a successful recreate, the old Meetecho conference must be cleaned up. + + The issue report noted that the secretariat ended up with orphaned + interims because each edit created a brand new Meetecho session without + deleting the one it was replacing. + """ + mock_conf_mgr = mock.return_value + original_url = 'https://meetings.conf.meetecho.com/interim/?session=42' + session = SessionFactory( + meeting__type_id='interim', + remote_instructions=original_url, + ) + timeslot = session.official_timeslotassignment().timeslot + + new_conf = Conference( + manager=mock_conf_mgr, id=int(session.pk), public_id='new-uuid', + description='desc', start_time=timeslot.utc_start_time(), + duration=timeslot.duration, url='https://meetings.conf.meetecho.com/interim/?session=99', + deletion_token='please-delete-me', + ) + old_conf = Conference( + manager=mock_conf_mgr, id=int(session.pk), public_id='old-uuid', + description='desc', start_time=timeslot.utc_start_time(), + duration=timeslot.duration, url=original_url, + deletion_token='please-delete-old', + ) + mock_conf_mgr.create.return_value = [new_conf] + mock_conf_mgr.fetch.return_value = [old_conf] + + create_interim_session_conferences([session]) + + self.assertEqual( + Session.objects.get(pk=session.pk).remote_instructions, + new_conf.url, + ) + self.assertTrue(mock_conf_mgr.fetch.called) + self.assertEqual(mock_conf_mgr.fetch.call_args[0], (session.group,)) + # The old conference's delete() should have been invoked exactly once. + self.assertEqual(mock_conf_mgr.delete_conference.call_count, 1) + self.assertEqual( + mock_conf_mgr.delete_conference.call_args[0], + (old_conf,), + ) + + @patch('ietf.utils.meetecho.ConferenceManager') + def test_create_interim_session_conferences_cleanup_failure_is_swallowed(self, mock): + """If deleting the previous conference fails, the successful recreate must stand. + + The cleanup of the replaced Meetecho conference is best-effort - any + error in fetch/delete must be logged and swallowed, not propagated. + """ + mock_conf_mgr = mock.return_value + original_url = 'https://meetings.conf.meetecho.com/interim/?session=42' + session = SessionFactory( + meeting__type_id='interim', + remote_instructions=original_url, + ) + timeslot = session.official_timeslotassignment().timeslot + + new_conf = Conference( + manager=mock_conf_mgr, id=int(session.pk), public_id='new-uuid', + description='desc', start_time=timeslot.utc_start_time(), + duration=timeslot.duration, url='https://meetings.conf.meetecho.com/interim/?session=99', + deletion_token='please-delete-me', + ) + mock_conf_mgr.create.return_value = [new_conf] + mock_conf_mgr.fetch.side_effect = RuntimeError('meetecho unreachable') + + # Must not raise, and the new URL must still be persisted. + create_interim_session_conferences([session]) + + self.assertEqual( + Session.objects.get(pk=session.pk).remote_instructions, + new_conf.url, + ) + self.assertTrue(mock_conf_mgr.fetch.called) + self.assertFalse(mock_conf_mgr.delete_conference.called) + + @patch('ietf.meeting.helpers.create_interim_session_conferences') + def test_sessions_post_save_skips_meetecho_when_only_agenda_changed(self, mock_create_method): + """If the session already has a Meetecho URL and only non-timing fields + changed, sessions_post_save must not ask the helper to recreate it. + + This is the primary fix for ietf-tools/datatracker#10689: merely editing + the agenda must leave the existing conference alone. + """ + session = SessionFactory( + meeting__type_id='interim', + remote_instructions='https://meetings.conf.meetecho.com/interim/?session=42', + ) + mock_form = Mock() + mock_form.instance = session + mock_form.has_changed.return_value = True + mock_form.changed_data = ['agenda'] + mock_form.requires_approval = True + mock_form.cleaned_data = { + 'date': date_today(), + 'time': datetime.time(1, 23), + 'remote_participation': 'meetecho', + } + sessions_post_save(RequestFactory().post('/some/url'), [mock_form]) + self.assertTrue(mock_create_method.called) + self.assertCountEqual(mock_create_method.call_args[0][0], []) + + @patch('ietf.meeting.helpers.create_interim_session_conferences') + def test_sessions_post_save_recreates_meetecho_when_timing_changed(self, mock_create_method): + """If a timing field changed, the helper must be asked to recreate.""" + session = SessionFactory( + meeting__type_id='interim', + remote_instructions='https://meetings.conf.meetecho.com/interim/?session=42', + ) + mock_form = Mock() + mock_form.instance = session + mock_form.has_changed.return_value = True + mock_form.requires_approval = True + mock_form.cleaned_data = { + 'date': date_today(), + 'time': datetime.time(1, 23), + 'remote_participation': 'meetecho', + } + for field in ('date', 'time', 'requested_duration', 'end_time'): + mock_create_method.reset_mock() + mock_form.changed_data = [field] + sessions_post_save(RequestFactory().post('/some/url'), [mock_form]) + self.assertTrue(mock_create_method.called) + self.assertCountEqual( + mock_create_method.call_args[0][0], [session], + f'expected recreate when {field!r} changed', + ) + + @patch('ietf.meeting.helpers.create_interim_session_conferences') + def test_sessions_post_save_creates_meetecho_when_no_existing_url(self, mock_create_method): + """A session without any existing Meetecho URL must always be (re)created.""" + session = SessionFactory(meeting__type_id='interim', remote_instructions='') + mock_form = Mock() + mock_form.instance = session + mock_form.has_changed.return_value = True + mock_form.changed_data = ['agenda'] # no timing field changed + mock_form.requires_approval = True + mock_form.cleaned_data = { + 'date': date_today(), + 'time': datetime.time(1, 23), + 'remote_participation': 'meetecho', + } + sessions_post_save(RequestFactory().post('/some/url'), [mock_form]) + self.assertTrue(mock_create_method.called) + self.assertCountEqual(mock_create_method.call_args[0][0], [session]) + class HelperTests(TestCase): def test_get_ietf_meeting(self):