From e2e837d6e7dd18baa26a7dc41fcdb245b0383298 Mon Sep 17 00:00:00 2001 From: Jennifer Richards Date: Fri, 8 Mar 2024 13:06:25 -0400 Subject: [PATCH 1/2] fix: better Meetecho API "order" management --- ietf/meeting/tests_views.py | 9 ++---- ietf/meeting/views.py | 3 +- ietf/utils/meetecho.py | 17 ++++++++++++ ietf/utils/tests_meetecho.py | 53 ++++++++++++++++++++++++++++++++++-- 4 files changed, 71 insertions(+), 11 deletions(-) diff --git a/ietf/meeting/tests_views.py b/ietf/meeting/tests_views.py index 6385395c2c..81f82075a7 100644 --- a/ietf/meeting/tests_views.py +++ b/ietf/meeting/tests_views.py @@ -6477,16 +6477,11 @@ def test_upload_slides(self, mock_slides_manager_cls): self.assertEqual(replacement_sp.document.rev,'01') self.assertEqual(mock_slides_manager_cls.call_count, 1) self.assertEqual(mock_slides_manager_cls.call_args, call(api_config="fake settings")) - self.assertEqual(mock_slides_manager_cls.return_value.delete.call_count, 1) + self.assertEqual(mock_slides_manager_cls.return_value.revise.call_count, 1) self.assertEqual( - mock_slides_manager_cls.return_value.delete.call_args, + mock_slides_manager_cls.return_value.revise.call_args, call(session=session2, slides=sp.document), ) - self.assertEqual(mock_slides_manager_cls.return_value.add.call_count, 1) - self.assertEqual( - mock_slides_manager_cls.return_value.add.call_args, - call(session=session2, slides=replacement_sp.document, order=2), - ) def test_upload_slide_title_bad_unicode(self): session1 = SessionFactory(meeting__type_id='ietf') diff --git a/ietf/meeting/views.py b/ietf/meeting/views.py index 280e3ca313..1fb775169a 100644 --- a/ietf/meeting/views.py +++ b/ietf/meeting/views.py @@ -2955,8 +2955,7 @@ def upload_session_slides(request, session_id, num, name=None): sp.rev = doc.rev sp.save() if sm is not None: - sm.delete(session=sess, slides=doc) # removes the existing deck with previous revision - sm.add(session=sess, slides=doc, order=sp.order) + sm.revise(session=sess, slides=doc) else: max_order = ( sess.presentations.filter(document__type="slides").aggregate( diff --git a/ietf/utils/meetecho.py b/ietf/utils/meetecho.py index 9879014723..37de2f099b 100644 --- a/ietf/utils/meetecho.py +++ b/ietf/utils/meetecho.py @@ -493,12 +493,29 @@ def add(self, session: "Session", slides: "Document", order: int): ) def delete(self, session: "Session", slides: "Document"): + """Delete a slide deck from the session""" + # remove, leaving a hole self.api.delete_slide_deck( wg_token=self.wg_token(session.group), session=str(session.pk), id=slides.pk, ) + self.send_update(session) # adjust order to fill in the hole + def revise(self, session: "Session", slides: "Document"): + """Replace existing deck with its current state""" + sp = session.presentations.filter(document=slides).first() + if sp is None: + raise MeetechoAPIError(f"Slides {slides.pk} not in session {session.pk}") + order = sp.order + # remove, leaving a hole in the order on Meetecho's side + self.api.delete_slide_deck( + wg_token=self.wg_token(session.group), + session=str(session.pk), + id=slides.pk, + ) + self.add(session, slides, order) # fill in the hole + def send_update(self, session: "Session"): self.api.update_slide_decks( wg_token=self.wg_token(session.group), diff --git a/ietf/utils/tests_meetecho.py b/ietf/utils/tests_meetecho.py index eaec0ae8cc..741b03ea00 100644 --- a/ietf/utils/tests_meetecho.py +++ b/ietf/utils/tests_meetecho.py @@ -561,10 +561,12 @@ def test_add(self, mock_add, mock_wg_token): ), ) + @patch("ietf.utils.meetecho.MeetechoAPI.update_slide_decks") @patch("ietf.utils.meetecho.MeetechoAPI.delete_slide_deck") - def test_delete(self, mock_delete, mock_wg_token): + def test_delete(self, mock_delete, mock_update, mock_wg_token): sm = SlidesManager(settings.MEETECHO_API_CONFIG) - slides = SessionPresentationFactory(document__type_id="slides") + slides = SessionPresentationFactory(document__type_id="slides", order=17) + slides_doc = slides.document sm.delete(slides.session, slides.document) self.assertTrue(mock_wg_token.called) self.assertTrue(mock_delete.called) @@ -572,6 +574,53 @@ def test_delete(self, mock_delete, mock_wg_token): mock_delete.call_args, call(wg_token="atoken", session=str(slides.session.pk), id=slides.document_id), ) + self.assertTrue(mock_update.called) + self.assertEqual( + mock_update.call_args, + call( + wg_token="atoken", + session=str(slides.session.pk), + decks=[ + { + "id": slides_doc.pk, + "title": slides_doc.title, + "url": slides_doc.get_versionless_href(), + "rev": slides_doc.rev, + "order": 17, + }, + ] + ) + ) + + @patch("ietf.utils.meetecho.MeetechoAPI.delete_slide_deck") + @patch("ietf.utils.meetecho.MeetechoAPI.add_slide_deck") + def test_revise(self, mock_add, mock_delete, mock_wg_token): + sm = SlidesManager(settings.MEETECHO_API_CONFIG) + slides = SessionPresentationFactory(document__type_id="slides", order=23) + slides_doc = slides.document + sm.revise(slides.session, slides.document) + self.assertTrue(mock_wg_token.called) + self.assertTrue(mock_delete.called) + self.assertEqual( + mock_delete.call_args, + call(wg_token="atoken", session=str(slides.session.pk), id=slides_doc.pk), + ) + self.assertTrue(mock_add.called) + self.assertEqual( + mock_add.call_args, + call( + wg_token="atoken", + session=str(slides.session.pk), + deck={ + "id": slides_doc.pk, + "title": slides_doc.title, + "url": slides_doc.get_versionless_href(), + "rev": slides_doc.rev, + "order": 23, + }, + ), + ) + @patch("ietf.utils.meetecho.MeetechoAPI.update_slide_decks") def test_send_update(self, mock_send_update, mock_wg_token): From 5998f638fdfbf9c910a25d582af98510ec365a2d Mon Sep 17 00:00:00 2001 From: Jennifer Richards Date: Fri, 8 Mar 2024 13:34:35 -0400 Subject: [PATCH 2/2] fix: no PUT if there are no slides after DELETE --- ietf/utils/meetecho.py | 11 ++++++++++- ietf/utils/tests_meetecho.py | 27 ++++++++++++++++++++++----- 2 files changed, 32 insertions(+), 6 deletions(-) diff --git a/ietf/utils/meetecho.py b/ietf/utils/meetecho.py index 37de2f099b..8c35b1e8e4 100644 --- a/ietf/utils/meetecho.py +++ b/ietf/utils/meetecho.py @@ -480,6 +480,8 @@ class SlidesManager(Manager): avoid this requirement.) """ def add(self, session: "Session", slides: "Document", order: int): + # Would like to confirm that session.presentations includes the slides Document, but we can't + # (same problem regarding unsaved Documents discussed in the docstring) self.api.add_slide_deck( wg_token=self.wg_token(session.group), session=str(session.pk), @@ -494,13 +496,20 @@ def add(self, session: "Session", slides: "Document", order: int): def delete(self, session: "Session", slides: "Document"): """Delete a slide deck from the session""" + if session.presentations.filter(document=slides).exists(): + # "order" problems are very likely to result if we delete slides that are actually still + # linked to the session + raise MeetechoAPIError( + f"Slides {slides.pk} are still linked to session {session.pk}." + ) # remove, leaving a hole self.api.delete_slide_deck( wg_token=self.wg_token(session.group), session=str(session.pk), id=slides.pk, ) - self.send_update(session) # adjust order to fill in the hole + if session.presentations.filter(document__type_id="slides").exists(): + self.send_update(session) # adjust order to fill in the hole def revise(self, session: "Session", slides: "Document"): """Replace existing deck with its current state""" diff --git a/ietf/utils/tests_meetecho.py b/ietf/utils/tests_meetecho.py index 741b03ea00..b3319cc7c2 100644 --- a/ietf/utils/tests_meetecho.py +++ b/ietf/utils/tests_meetecho.py @@ -565,32 +565,49 @@ def test_add(self, mock_add, mock_wg_token): @patch("ietf.utils.meetecho.MeetechoAPI.delete_slide_deck") def test_delete(self, mock_delete, mock_update, mock_wg_token): sm = SlidesManager(settings.MEETECHO_API_CONFIG) - slides = SessionPresentationFactory(document__type_id="slides", order=17) + # Test scenario: we had a session with two slide decks and we already deleted the SessionPresentation + # for one and are now updating Meetecho + slides = SessionPresentationFactory(document__type_id="slides", order=1) # still attached to the session + session = slides.session slides_doc = slides.document - sm.delete(slides.session, slides.document) + removed_slides_doc = DocumentFactory(type_id="slides") + + with self.assertRaises(MeetechoAPIError): + sm.delete(session, slides_doc) # can't remove slides still attached to the session + self.assertFalse(any([mock_wg_token.called, mock_delete.called, mock_update.called])) + + sm.delete(session, removed_slides_doc) self.assertTrue(mock_wg_token.called) self.assertTrue(mock_delete.called) self.assertEqual( mock_delete.call_args, - call(wg_token="atoken", session=str(slides.session.pk), id=slides.document_id), + call(wg_token="atoken", session=str(session.pk), id=removed_slides_doc.pk), ) self.assertTrue(mock_update.called) self.assertEqual( mock_update.call_args, call( wg_token="atoken", - session=str(slides.session.pk), + session=str(session.pk), decks=[ { "id": slides_doc.pk, "title": slides_doc.title, "url": slides_doc.get_versionless_href(), "rev": slides_doc.rev, - "order": 17, + "order": 1, }, ] ) ) + mock_delete.reset_mock() + mock_update.reset_mock() + + # Delete the other session and check that we don't make the update call + slides.delete() + sm.delete(session, slides_doc) + self.assertTrue(mock_delete.called) + self.assertFalse(mock_update.called) @patch("ietf.utils.meetecho.MeetechoAPI.delete_slide_deck") @patch("ietf.utils.meetecho.MeetechoAPI.add_slide_deck")