PnX-SI / UsersHub-authentification-module

Module Flask d'authentification de UsersHub
GNU General Public License v3.0
5 stars 13 forks source link

add unique constraint on email_role #56

Open lpofredc opened 1 year ago

lpofredc commented 1 year ago

Ajout d'une contrainte d'unicité sur le champ email de la table utilisateurs.t_roles comme évoqué sur cette issue de UsersHub (https://github.com/PnX-SI/UsersHub/issues/122)

fix https://github.com/PnX-SI/UsersHub/issues/122

codecov[bot] commented 1 year ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Comparison is base (0d269c8) 37.67% compared to head (992619f) 37.67%. Report is 55 commits behind head on develop.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## develop #56 +/- ## ======================================== Coverage 37.67% 37.67% ======================================== Files 12 12 Lines 698 698 ======================================== Hits 263 263 Misses 435 435 ``` | [Flag](https://app.codecov.io/gh/PnX-SI/UsersHub-authentification-module/pull/56/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=PnX-SI) | Coverage Δ | | |---|---|---| | [pytest](https://app.codecov.io/gh/PnX-SI/UsersHub-authentification-module/pull/56/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=PnX-SI) | `37.67% <ø> (ø)` | | Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=PnX-SI#carryforward-flags-in-the-pull-request-comment) to find out more.

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

camillemonchicourt commented 1 year ago

Peut-être que la migration doit gérer le cas où 2 utilisateurs ont le même email ? L'indiquer clairement et s’arrêter ? Ou alors supprimer les emails en doublon, mais ça peut être problématique car on supprimerait des emails sans l'indiquer ?

mvergez commented 1 year ago

Je rebondis sur cette PR dans le cadre d'une prestation pour l'Agence Régionale de la Biodiversité en île de France.

Je ne pense pas qu'il faille supprimer les emails en doublon mais plutôt avertir l'administrateur. Si on applique cette migration, on a une erreur du type :

sqlalchemy.exc.IntegrityError: (psycopg2.errors.UniqueViolation) could not create unique index "t_roles_email_un"

Mais sous une backtrace énorme.

Donc :

camillemonchicourt commented 1 year ago

Pour moi, c'est un soucis que les migrations puissent échouer, et que les administrateurs doivent comprendre pourquoi, et ensuite lancer des commandes Alembic qu'ils ne maîtrisent pas.

mvergez commented 1 year ago

Alors il faut les prévenir dans une note de version de GeoNature, je ne vois pas comment on pourrait faire autrement. Parce que même si on met un message le plus clair possible dans alembic, ils ne vont pas savoir comment relancer les migrations.

Qu'en penses-tu ?

camillemonchicourt commented 1 year ago

Oui