fix: Stop interim edit from clobbering Meetecho remote_instructions (#10689)

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
 

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')

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):

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):