Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[fix] add adapter for fix relationfield deserializer #86

Merged
merged 8 commits into from
Dec 28, 2023

Conversation

eikichi18
Copy link
Member

Problema con il deseriazer dei relationfield perché di base di andava a prendere l'id di un oggetto, che veniva poi utilizzato come path, per poi utilizzarlo all'interno di un restrictedTraverse per andare a recuperarsi l'oggetto.

Questa cosa però creava problemi nel momento in cui degli utenti avevano accesoo solo a certe sezioni del sito non potendo accedere a quelle superiori

@eikichi18 eikichi18 requested review from mamico and cekk December 14, 2023 14:44
@eikichi18
Copy link
Member Author

le cofigurazioni di flake e black di questo pacchetto vanno in conflitto

@coveralls
Copy link

coveralls commented Dec 14, 2023

Pull Request Test Coverage Report for Build 7340529013

  • 40 of 46 (86.96%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.8%) to 63.961%

Changes Missing Coverage Covered Lines Changed/Added Lines %
src/redturtle/volto/restapi/deserializer/relationfield.py 40 46 86.96%
Totals Coverage Status
Change from base Build 6907137559: 0.8%
Covered Lines: 843
Relevant Lines: 1318

💛 - Coveralls

Copy link
Member

@cekk cekk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add tests and changelog please.

@eikichi18
Copy link
Member Author

@cekk per quanto riguarda i test, io penso che i test di plone.restapi all'interno di test_dxfield_deserializer per quando riguarda i campi relazione, coprano tutte le casistiche.

@cekk
Copy link
Member

cekk commented Dec 19, 2023

@cekk per quanto riguarda i test, io penso che i test di plone.restapi all'interno di test_dxfield_deserializer per quando riguarda i campi relazione, coprano tutte le casistiche.

però non intercettavano il problema che hai sistemato tu, o sbaglio?

@eikichi18
Copy link
Member Author

è vero, ma con la modifica che abbiamo fatto il problema non è più replicabile, dalle prove che ho fatto non arriviamo più li con un path

@cekk
Copy link
Member

cekk commented Dec 20, 2023

si ma il test serve proprio per mostrare quello che hai sistemato.
In teoria dovresti farlo prima del fix, in modo che si rompa. Poi metti il fix e vedi che diventa verde.
Così poi lo prendi e lo metti anche nella pr di restapi (c'è?)

@eikichi18
Copy link
Member Author

Su plone.restapi c'è il test che prova il funzionamento del deserializer del RelationField.

L'unico modo che mi viene in mente per testare l'errore attualmente, sarebbe mockure il deserializer per farlo rompere nel test e poi usare quello modificato per mostrare che adesso lo fa bene.

Mentre secondo me i test all'interno di plone.restapi, che testano già quel deserializer, dovrebbero già coprire la funzionalità che abbiamo modificato.

@cekk
Copy link
Member

cekk commented Dec 21, 2023

allora non la testano bene perché avrebbero dovuto rompersi ;)
E' questo quello che intendo: se siete arrivati a replicare questo errore, vuol dire che ci si può scrivere sopra un test (che evidentemente prima non lo copriva).
Facendolo girare senza il fix deve rompersi, mentre aggiungendo il fix, dare luce verde

@mamico
Copy link
Contributor

mamico commented Dec 21, 2023

E' una funzionalità nostra o una fix per plone.restapi? Nel secondo caso serve una pull request su quel prodotto.

@eikichi18
Copy link
Member Author

Io questa la farei come fix su plone.restapi, però intanto ho bisogno di rilasciarla per alcuni clienti che sono bloccati

@eikichi18 eikichi18 merged commit 887049a into master Dec 28, 2023
17 of 18 checks passed
@pnicolli pnicolli deleted the us49605_pdt_in_venue branch June 7, 2024 15:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants