From dba9292814b7210d56daaadf85eab009fa3d030c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20Pasternak?= Date: Sat, 25 Jul 2026 00:25:26 +0200 Subject: [PATCH 1/2] =?UTF-8?q?fix(pbn):=20WillNotExportError=20to=20brak?= =?UTF-8?q?=20danych,=20nie=20awaria=20=E2=80=94=20czytelny=20komunikat?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit sprobuj_wyslac_do_pbn ma osobną gałąź except dla każdego spodziewanego przypadku (SameDataUploadedRecently, AccessDeniedException, PKZeroExportDisabled, NeedsPBNAuthorisationException, ResourceLockedException, PBNValidationError) — każda z konkretnym komunikatem dla redaktora i bez raportu do Rollbara. Pozostałe podklasy WillNotExportError (DOIorWWWMissing, LanguageMissingPBNUID, StatementsMissing, CharakterFormalnyMissingPBNUID) takiej gałęzi nie miały, więc wpadały do `except Exception` — opatrzonego komentarzem "w sumie nie wiadomo, co to za problem na tym etapie". Skutki: redaktor dostawał generyczne "Kod błędu: Musi być DOI lub adres WWW", a Rollbar item na każde wystąpienie (#1475, #1473), mimo że system działał poprawnie, a poprawka leży po stronie danych rekordu. Komunikat wyjątku jest już napisany po polsku i wprost mówi, czego brakuje — podajemy go redaktorowi wprost (escape'owany), z prośbą o uzupełnienie danych. PKZeroExportDisabled zostaje na swojej wcześniejszej gałęzi: to podklasa WillNotExportError, ale ma inny, konfiguracyjny komunikat. Kolejność gałęzi to gwarantuje, a istniejący test tego pilnuje. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01NcAqeqyqBzNEkkVnhpHDaH --- src/bpp/admin/helpers/pbn_api/common.py | 25 +++++++++ ...l-not-export-czytelny-komunikat.bugfix.rst | 5 ++ src/pbn_api/tests/test_bpp_admin_helpers.py | 51 ++++++++++++++++++- 3 files changed, 80 insertions(+), 1 deletion(-) create mode 100644 src/bpp/newsfragments/+pbn-will-not-export-czytelny-komunikat.bugfix.rst diff --git a/src/bpp/admin/helpers/pbn_api/common.py b/src/bpp/admin/helpers/pbn_api/common.py index 041996def..e2cef354e 100644 --- a/src/bpp/admin/helpers/pbn_api/common.py +++ b/src/bpp/admin/helpers/pbn_api/common.py @@ -16,6 +16,7 @@ PraceSerwisoweException, ResourceLockedException, SameDataUploadedRecently, + WillNotExportError, ) from pbn_api.models import SentData @@ -246,6 +247,30 @@ def sprobuj_wyslac_do_pbn( # noqa: C901 return + except WillNotExportError as e: + # Brak danych w rekordzie (DOI/WWW, odpowiednik języka w PBN, + # oświadczenia...), a NIE awaria kodu. Komunikat wyjątku jest już + # napisany po polsku i wprost mówi, czego brakuje — podajemy go + # redaktorowi zamiast generycznego „Kod błędu: …" z gałęzi niżej. + # + # Bez tej gałęzi wpadało to do `except Exception`, opatrzonego + # komentarzem „nie wiadomo, co to za problem" — i zakładało w + # Rollbarze item na każde wystąpienie (#1475, #1473), mimo że system + # działał poprawnie, a poprawka leży po stronie danych. + # + # UWAGA: `PKZeroExportDisabled` (też podklasa WillNotExportError) ma + # własną gałąź WYŻEJ i musi tam zostać — ma inny, konfiguracyjny + # komunikat. + notificator.warning( + f'Rekord "{link_do_obiektu(obj)}" nie zostanie wysłany do PBN: {escape(e)}. ' + f"Uzupełnij dane rekordu i spróbuj ponownie. " + f"{open_in_pbn_link}{open_in_pi_link}" + ) + if raise_exceptions: + raise e + + return + except Exception as e: try: link_do_wyslanych = reverse( diff --git a/src/bpp/newsfragments/+pbn-will-not-export-czytelny-komunikat.bugfix.rst b/src/bpp/newsfragments/+pbn-will-not-export-czytelny-komunikat.bugfix.rst new file mode 100644 index 000000000..df685f06e --- /dev/null +++ b/src/bpp/newsfragments/+pbn-will-not-export-czytelny-komunikat.bugfix.rst @@ -0,0 +1,5 @@ +Przy wysyłce rekordu do PBN z panelu redagowania brakujące dane dają teraz +konkretny komunikat („Musi być DOI lub adres WWW", „Brak odpowiednika języka +w PBN") zamiast generycznego „Kod błędu: …". Dotyczy sytuacji, w których +rekordu po prostu nie da się wysłać, dopóki nie uzupełni się danych — +to nie jest awaria systemu i nie jest już jako awaria zgłaszane. diff --git a/src/pbn_api/tests/test_bpp_admin_helpers.py b/src/pbn_api/tests/test_bpp_admin_helpers.py index 4cdc648b6..f4f08010e 100644 --- a/src/pbn_api/tests/test_bpp_admin_helpers.py +++ b/src/pbn_api/tests/test_bpp_admin_helpers.py @@ -21,7 +21,12 @@ PBN_GET_INSTITUTION_STATEMENTS, PBN_GET_PUBLICATION_BY_ID_URL, ) -from pbn_api.exceptions import AccessDeniedException, PBNValidationError +from pbn_api.exceptions import ( + AccessDeniedException, + DOIorWWWMissing, + LanguageMissingPBNUID, + PBNValidationError, +) from pbn_api.models import Publication, SentData from pbn_api.tests.utils import middleware @@ -511,3 +516,47 @@ def test_sprobuj_wyslac_do_pbn_przychodzi_inny_pbn_uid_dla_starego_rekordu( msg = get_messages(req) assert "Wg danych z PBN zmodyfikowano PBN UID tego rekordu " in list(msg)[0].message + + +@pytest.mark.django_db +@pytest.mark.parametrize( + "wyjatek,fragment_komunikatu", + [ + (DOIorWWWMissing("Musi być DOI lub adres WWW"), "Musi być DOI lub adres WWW"), + (LanguageMissingPBNUID("Brak odpowiednika języka"), "Brak odpowiednika języka"), + ], +) +def test_sprobuj_wyslac_do_pbn_will_not_export_czytelny_komunikat_bez_rollbar( + pbn_wydawnictwo_zwarte_z_charakterem, + pbn_client, + rf, + pbn_uczelnia, + mocker, + wyjatek, + fragment_komunikatu, +): + """``WillNotExportError`` to brak danych w rekordzie, nie awaria kodu. + + Regresja (Rollbar #1475, #1473): te wyjątki wpadały do gałęzi + ``except Exception``, opatrzonej komentarzem „nie wiadomo, co to za + problem" — redaktor dostawał generyczne „Kod błędu: …", a Rollbar item + per wystąpienie. Tymczasem to zwykły komunikat walidacyjny: brakuje DOI, + brakuje odpowiednika języka w PBN. Sąsiednie gałęzie (``PKZeroExportDisabled``, + ``PBNValidationError``) od dawna robią to poprawnie. + """ + req = rf.get("/") + + report = mocker.patch("bpp.admin.helpers.pbn_api.common.rollbar.report_exc_info") + mocker.patch.object(pbn_client, "sync_publication", side_effect=wyjatek) + + with middleware(req): + sprobuj_wyslac_do_pbn_gui( + req, pbn_wydawnictwo_zwarte_z_charakterem, pbn_client=pbn_client + ) + + text = list(get_messages(req))[0].message + assert fragment_komunikatu in text, ( + "Redaktor musi zobaczyć KONKRETNY powód, nie 'Kod błędu: ...'" + ) + assert "Kod błędu" not in text + report.assert_not_called() # brak danych w rekordzie to NIE błąd kodu From eb3eea47d5dab04d513d4ad4f5b488803a0bfc0a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20Pasternak?= Date: Sat, 25 Jul 2026 00:41:46 +0200 Subject: [PATCH 2/2] =?UTF-8?q?fix(pbn):=20domknij=20self-review=20?= =?UTF-8?q?=E2=80=94=20test=20kolejno=C5=9Bci=20ga=C5=82=C4=99zi,=20escape?= =?UTF-8?q?=20i=20sufiks?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Twierdziłem w opisie, że "istniejący test pilnuje kolejności gałęzi". NIEPRAWDA: ciąg "Eksport prac z PK=0" nie występował w żadnym teście, a testy kolejki idą ścieżką raise_exceptions=True, gdzie obie gałęzie robią `raise e` — są więc na kolejność niewrażliwe. Przestawienie gałęzi nie wywaliłoby niczego. Dokładam PKZeroExportDisabled do parametryzacji; zweryfikowane mutacją (przeniesienie WillNotExportError ponad PKZeroExportDisabled → test pada). - escape(e) dało się usunąć bez żadnego czerwonego testu. Komunikaty zawierają dane sterowane przez użytkownika (LanguageMissingPBNUID wstrzykuje nazwę języka, CharakterFormalnyMissingPBNUID nazwę charakteru formalnego — oba ze słowników edytowalnych w adminie), a admin/base.html renderuje komunikaty przez `|safe`. Dokładam test escapowania, lustro istniejącego ..._validation_error_escapuje_html. - Sufiks "Uzupełnij dane rekordu" był mylący dla dwóch przypadków: CharakterFormalnyMissingPBNUID (poprawka w słowniku, nie w rekordzie) oraz gołego WillNotExportError podnoszonego przez Uczelnia.pbn_client() przy braku autoryzacji w PBN — ta ścieżka JEST osiągalna wewnątrz try, bo authorize() woła się leniwie przy 403. Teraz: "Popraw dane rekordu lub konfigurację". Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01NcAqeqyqBzNEkkVnhpHDaH --- src/bpp/admin/helpers/pbn_api/common.py | 28 ++++++++------ src/pbn_api/tests/test_bpp_admin_helpers.py | 41 +++++++++++++++++++++ 2 files changed, 58 insertions(+), 11 deletions(-) diff --git a/src/bpp/admin/helpers/pbn_api/common.py b/src/bpp/admin/helpers/pbn_api/common.py index e2cef354e..d0d7d75de 100644 --- a/src/bpp/admin/helpers/pbn_api/common.py +++ b/src/bpp/admin/helpers/pbn_api/common.py @@ -248,22 +248,28 @@ def sprobuj_wyslac_do_pbn( # noqa: C901 return except WillNotExportError as e: - # Brak danych w rekordzie (DOI/WWW, odpowiednik języka w PBN, - # oświadczenia...), a NIE awaria kodu. Komunikat wyjątku jest już - # napisany po polsku i wprost mówi, czego brakuje — podajemy go - # redaktorowi zamiast generycznego „Kod błędu: …" z gałęzi niżej. + # Rekordu nie da się wysłać, dopóki ktoś czegoś nie uzupełni — a NIE + # awaria kodu. Komunikat wyjątku jest już napisany po polsku i wprost + # mówi, czego brakuje, więc podajemy go redaktorowi zamiast + # generycznego „Kod błędu: …" z gałęzi niżej. # # Bez tej gałęzi wpadało to do `except Exception`, opatrzonego - # komentarzem „nie wiadomo, co to za problem" — i zakładało w - # Rollbarze item na każde wystąpienie (#1475, #1473), mimo że system - # działał poprawnie, a poprawka leży po stronie danych. + # komentarzem „nie wiadomo, co to za problem" — i szło do Rollbara + # (itemy Rollbar #1475, #1473), mimo że system działał poprawnie. # - # UWAGA: `PKZeroExportDisabled` (też podklasa WillNotExportError) ma - # własną gałąź WYŻEJ i musi tam zostać — ma inny, konfiguracyjny - # komunikat. + # „lub konfigurację" w komunikacie jest celowe: ta gałąź łapie też + # przypadki, w których poprawka NIE leży w rekordzie — + # `CharakterFormalnyMissingPBNUID` (słownik charakterów formalnych) + # oraz gołe `WillNotExportError` podnoszone przez + # `Uczelnia.pbn_client()` przy braku autoryzacji w PBN. + # + # UWAGA NA KOLEJNOŚĆ: `PKZeroExportDisabled` (też podklasa + # WillNotExportError) ma własną gałąź WYŻEJ i musi tam zostać — ma + # inny, konfiguracyjny komunikat. Pilnuje tego parametryzacja + # testu `..._will_not_export_czytelny_komunikat_bez_rollbar`. notificator.warning( f'Rekord "{link_do_obiektu(obj)}" nie zostanie wysłany do PBN: {escape(e)}. ' - f"Uzupełnij dane rekordu i spróbuj ponownie. " + f"Popraw dane rekordu lub konfigurację i spróbuj ponownie. " f"{open_in_pbn_link}{open_in_pi_link}" ) if raise_exceptions: diff --git a/src/pbn_api/tests/test_bpp_admin_helpers.py b/src/pbn_api/tests/test_bpp_admin_helpers.py index f4f08010e..278a9301b 100644 --- a/src/pbn_api/tests/test_bpp_admin_helpers.py +++ b/src/pbn_api/tests/test_bpp_admin_helpers.py @@ -26,6 +26,7 @@ DOIorWWWMissing, LanguageMissingPBNUID, PBNValidationError, + PKZeroExportDisabled, ) from pbn_api.models import Publication, SentData from pbn_api.tests.utils import middleware @@ -524,6 +525,14 @@ def test_sprobuj_wyslac_do_pbn_przychodzi_inny_pbn_uid_dla_starego_rekordu( [ (DOIorWWWMissing("Musi być DOI lub adres WWW"), "Musi być DOI lub adres WWW"), (LanguageMissingPBNUID("Brak odpowiednika języka"), "Brak odpowiednika języka"), + # PKZeroExportDisabled JEST podklasą WillNotExportError, ale ma własną + # gałąź WYŻEJ. Ten przypadek pilnuje kolejności gałęzi — bez niego + # przestawienie ich nie wywaliłoby żadnego testu, a redaktor + # dostawałby zły komunikat. + ( + PKZeroExportDisabled("nieużywane, liczy się komunikat z gałęzi"), + "Eksport prac z PK=0 jest wyłączony", + ), ], ) def test_sprobuj_wyslac_do_pbn_will_not_export_czytelny_komunikat_bez_rollbar( @@ -560,3 +569,35 @@ def test_sprobuj_wyslac_do_pbn_will_not_export_czytelny_komunikat_bez_rollbar( ) assert "Kod błędu" not in text report.assert_not_called() # brak danych w rekordzie to NIE błąd kodu + + +@pytest.mark.django_db +def test_sprobuj_wyslac_do_pbn_will_not_export_escapuje_html( + pbn_wydawnictwo_zwarte_z_charakterem, pbn_client, rf, pbn_uczelnia, mocker +): + """Komunikat wyjątku trafia do `{{ message|safe }}` — musi być escape'owany. + + Treść NIE jest w pełni nasza: ``LanguageMissingPBNUID`` wstrzykuje nazwę + języka, a ``CharakterFormalnyMissingPBNUID`` nazwę charakteru formalnego — + oba ze słowników edytowalnych w adminie. Lustro istniejącego + ``..._validation_error_escapuje_html``. + """ + req = rf.get("/") + + mocker.patch("bpp.admin.helpers.pbn_api.common.rollbar.report_exc_info") + mocker.patch.object( + pbn_client, + "sync_publication", + side_effect=LanguageMissingPBNUID( + 'Język "" nie ma odpowiednika w PBN' + ), + ) + + with middleware(req): + sprobuj_wyslac_do_pbn_gui( + req, pbn_wydawnictwo_zwarte_z_charakterem, pbn_client=pbn_client + ) + + text = list(get_messages(req))[0].message + assert "