Fixed: Generic errors.ForbiddenError raised when election closed

[chriscool]

This should fix: #67

[TheoSabattie]

il y a une raison à "E5" (est-ce une convention quelconques?) ou est-ce arbitraire?

La structure semble différente que la fonction précédente (propriétés: message, details).
Je pense qu'il serait préférable de garder la même structure. (ou de la changer partout (mais ça implique de mettre à jour le front partout))

Note: j'ai l'impression PL se basait sur la description du message (propriété details). Pour afficher le message d'erreur qui correspondait. Est-ce que tu considères que c'est une mauvaise pratique et qu'il serait préférable de procéder autrement?

[chriscool]

J'ai repris les constantes qui existent déjà à https://github.com/MieuxVoter/majority-judgment-web-app/blob/master/services/api.ts#L448-L467:

export const UNKNOWN_ELECTION_ERROR = 'E1:';
export const ONGOING_ELECTION_ERROR = 'E2:';
export const NO_VOTE_ERROR = 'E3:';
export const ELECTION_NOT_STARTED_ERROR = 'E4:';
export const ELECTION_FINISHED_ERROR = 'E5:';
export const INVITATION_ONLY_ERROR = 'E6:';
export const UNKNOWN_TOKEN_ERROR = 'E7:';
export const USED_TOKEN_ERROR = 'E8:';
export const WRONG_ELECTION_ERROR = 'E9:';
export const API_ERRORS = [
  UNKNOWN_TOKEN_ERROR,
  ONGOING_ELECTION_ERROR,
  NO_VOTE_ERROR,
  ELECTION_NOT_STARTED_ERROR,
  ELECTION_FINISHED_ERROR,
  INVITATION_ONLY_ERROR,
  UNKNOWN_TOKEN_ERROR,
  USED_TOKEN_ERROR,
  WRONG_ELECTION_ERROR,
];

Tu as un exemple où on se base sur la description du message? Je ne sais pas ce que contient la description du message, mais si c'est juste un message d'erreur, ça me paraît fragile effectivement.

Je veux bien faire les modifs partout progressivement si on décide d'utiliser des codes d'erreurs comme ceux ci-dessus dans "services/api.ts".

Sinon je veux bien aussi faire comme dans des exemples existants si on pense que c'est plus simple, mais alors il faudrait supprimer ces codes d'erreur.

[TheoSabattie]

J'ai repris les constantes qui existent déjà à

J'ai l'impression qu'elles ne sont pas utilisées. Je ne trouve pas de références. Ni pour elle, ni pour la méthode apiErrors

Tu as un exemple où on se base sur la description du message? Je ne sais pas ce que contient la description du message, mais si c'est juste un message d'erreur, ça me paraît fragile effectivement.

https://github.com/MieuxVoter/majority-judgment-web-app/blob/db14a77120e3c6bfa437f1957c02b0c332f991c0/pages/results/%5Bpid%5D/%5B%5B...tid%5D%5D.tsx#L515

Sinon je veux bien aussi faire comme dans des exemples existants si on pense que c'est plus simple, mais alors il faudrait supprimer ces codes d'erreur.

Je pense que tu as raison sur le fait que ce baser sur le détail n'est pas hyper fiable.

J'aurais tendance à changer les messages d'erreurs:
body:

{ 
    "error": "THE_ERROR", 
    "message": "Explanation sentence"
}

error: chaîne de charactère sous forme de constante qui n'évoluera jamais
message: message descriptif

et éventuellement mettre à jour les erreur HTTP (400/401/403/409/422)

En bref: retirer les E1: qui ne semblent pas utile.

Et reprendre la logique de la méthode apiErrors en parsant la propriété error pour obtenir la localisation adéquate.

ça te semble ok?

[chriscool]

Oui, ça m'a l'air bien. Je vais faire ça alors. Merci.

[chriscool]

@TheoSabattie je viens d'envoyer une série de 19 commits sur cette PR qui implémentent ce que tu suggères. Ils ajoutent aussi des tests manquants, modifient quelques codes HTTP et corrigent quelques petites choses au passage. Dis moi ce que tu en penses. Pendant ce temps, je vais adapter le frontend à ces modifs.

[chriscool]

I have just sent a corresponding change to the frontend in MieuxVoter/majority-judgment-web-app#153

[TheoSabattie]

Bien joué 💪 👍

[TheoSabattie]

Je trouve que c'est dommage de rajouter un handler par erreur.

Est-ce que ça te semblerait OK d'avoir un handler pour une classe abstraite CustomError qui contiendrait une propriété pour l'id error?

(Je suppose que @app.exception_handler capte les classes enfants, à vérifier)

[chriscool]

Ok, je vais regarder ça.

[chriscool]

Voilà, la nouvelle version que je viens d'envoyer utilise une classe abstraite CustomError et donc un seul exception handler. CustomError est définie dans le premier commit de la branche et est utilisé systématiquement ensuite.

L'interface ne change pas donc le frontend adapté (MieuxVoter/majority-judgment-web-app#153) devrait marcher sans modification avec cette nouvelle version.

[TheoSabattie]

C'est la même chose que not playload["admin"] non? 👀
Si ça perturbe et que tu préfères, il y a également la synthaxe: "admin" not in payload (pareil en dessous)

[chriscool]

Non, ce n'est pas tout à fait la même chose. Quand la clé n'est pas définie dans un cas on a une KeyError et dans l'autre ça marche:

>>> payload = { "election": "XXX" }
>>> if not payload.get("admin"):
...     print("KO")
... 
KO
>>> if not payload["admin"]:
...     print("KO")
... 
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
KeyError: 'admin'
>>> 

En particulier dans le test test_update_election_as_non_admin je crois bien que ça plante le test sans cette modif.

"admin" not in payload marcherait probablement aussi dans l'application telle qu'elle est actuellement, mais si un jour on peut avoir payload["admin"] == False alors il faudrait soit not payload.get("admin") soit "admin" not in payload or not payload["admin"], donc je pense qu'utiliser not payload.get("admin") tout de suite est légèrement mieux.

[TheoSabattie]

ouki, ça marche 👍

Merci pour les explications ;)