Validate AccountRequest parameters

betagouv / betaGouvBot

Automated assistant for beta.gouv.fr administrative tasks on slack.

https://beta.gouv.fr

MIT License

4 stars 10 forks source link

Validate AccountRequest parameters #60

Closed bonjourmauko closed 7 years ago

bonjourmauko commented 7 years ago

Thanks to approve or tell me if you think is RTM for you.

Morendil commented 7 years ago

Expressing it this way, it strikes me that we might want to spec out the Slack interactions at some point... It's likely that Slack will grow in importance as a way of interacting with BGB.

bonjourmauko commented 7 years ago

Good feedback, said otherwise : « When I'm requesting a badge in Slack, and I'm following the instructions given to me, and Slack tells me the badge request was unsuccessful, I want to know what went wrong, so to resubmit my command, or ask for help with clearly identified problem, and have it solved ASAP »

bonjourmauko commented 7 years ago

@Morendil Done, dunno how to test though...

Morendil commented 7 years ago

You can route a test command to a review app, using the Slack settings panel for the team. Go to "Manage integrations" then "Slack commands".

Using the Sinatra error handling is neat, but looking at the code doesn't leave me very confident that the external behaviour is preserved by this transformation, so I'd definitely do some testing via Slack.

bonjourmauko commented 7 years ago

@Morendil Ended up adding integration testing for /compte, was way easier...

bonjourmauko commented 7 years ago

Could you try and test my last commit ?

Morendil commented 7 years ago

On it - in the meantime you can also try it yourself on Slack!

Morendil commented 7 years ago

No parameters: /compte-test-validation => Darn – that slash command didn't work (error message: 500_service_error). (1st try only), plus "Zut, il y a une erreur: Le format du nom doit être prenom.nom"
One parameter: /compte-test-validation toto => Zut, il y a une erreur: Email invalide, typo ?
Two parameters: /compte-test-validation toto titi => Zut, il y a une erreur: Email invalide, typo ?
Ditto for three parameters all invalid
Two parameters, first valid: /compte-test-validation laurent.bossavit titi => Zut, il y a une erreur: Email invalide, typo ?
Two parameters, first two valid: /compte-test-validation laurent.bossavit a@b.com => Zut, il y a une erreur: Mot de passe requis
Three parameters: /compte-test-validation laurent.bossavit a@b.com truc => OK, création de compte en cours ! (Should fail with password error, probably doesn't because I used my own name for testing)

Morendil commented 7 years ago

By now this PR has, to my eyes, outgrown the intent originally expressed by the branch name ("refactor account request"), possibly a hint to split it into refactoring vs new feature development.

Morendil commented 7 years ago

As experienced on Slack, we should probably also suppress in-channel output to Slack if the command results in any error (whether by failing validation or by external failure).

bonjourmauko commented 7 years ago

rebase + fixup

bonjourmauko commented 7 years ago

The original intent of this PR is effectively to start refactoring ActionRequest. Feedback is precisely a feature/solution so I decided not to address it here by now (I wrote the job story somewhere else if we want to come back to this later). As it stands, I think the only thing this PR does is to « Validate AccountRequest parameters ». Thanks to confirm (or not) this is RTM now.