Notify of validation error to user

[bonjourmauko]

When I'm requesting an email account in Slack,
and I'm following the instructions given to me, 
and Slack tells me the 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

[Morendil]

I've tested this in Slack using the test command set up for that purpose. What happens is that if you return any HTTP status other than 200, Slack will output an uninformative error message, and suppress all those outputs we've worked so hard to emit. So, that's still not satisfactory...

[Morendil]

Other than that, lots to like in the PR too! (Wish I'd taken the time to write that earlier, but we were rushing to lunch...)

[bonjourmauko]

@Morendil So I've changed :

• 201 -> 200
• 4xx -> 200 + code in the body

Note : rebased

Would you be kind to test ? Running to the airport 😄

[Morendil]

Argh, I may have jumped to conclusions regarding the behaviour I saw in Slack - I didn't notice that owing to the change in PR, the command set up in Slack was pointing to an app that no longer existed.

[Morendil]

We should distinguish between validation errors and unexpected errors. Originally "Zut, il y a une erreur" was meant to catch unexpected cases; "zut" conveys both dismay and surprise.

For instance, a missing token generates "Zut, il y a une ou plusieurs erreur(s) : 'token' n'est pas valide". This won't help the Slack user as it's a configuration error. That should say something like "Zut, j'ai un problème inattendu ('token' n'est pas valide), merci de contacter un programmeur de BetaGouvBot."

For validation errors it would be more appropriate to say something like "J'ai du mal à interpréter votre commande correctement, car X, Y, Z".

Likewise when handling an empty command (equivalent to /command help) we should not prefix the output with "zut" but something like "Cette commande attend les paramètres suivants", etc.

The fullname validation regex thinks "bob69" is invalid, but has no problem with "bob" (no last name) or with "bob.joe.bart" (three parts instead of two). Personally, I think fullname validation is superfluous, but if we're going to have it, we should ensure it matches up with the validation in beta.gouv.fr code.

We should not include debug information in the messages sent to Slack channels - at present this is what a Slack user sees:

Zut, il y a une ou plusieurs erreur(s) : je ne vois pas de qui tu veux parler
{"code":404,"errors":["je ne vois pas de qui tu veux parler"]}

I think the way to fix that is to return an empty body if a token is present. If there is a token that means we're talking to Slack, and a response body will be displayed to the Slack user. If there is no token, we're in debug mode.

If you're going to be away for some time, let me know and I can probably take it from here.

[bonjourmauko]

@Morendil Thanks for the feedback. Some takeaways for me :

I've tested this in Slack using the test command set up for that purpose. What happens is that if you return any HTTP status other than 200, Slack will output an uninformative error message, and suppress all those outputs we've worked so hard to emit. So, that's still not satisfactory...

We should confirm that effectively something other than 200 is skipped by Slack

We should distinguish between validation errors and unexpected errors. Originally "Zut, il y a une erreur" was meant to catch unexpected cases; "zut" conveys both dismay and surprise.

For instance, a missing token generates "Zut, il y a une ou plusieurs erreur(s) : 'token' n'est pas valide". This won't help the Slack user as it's a configuration error. That should say something like "Zut, j'ai un problème inattendu ('token' n'est pas valide), merci de contacter un programmeur de BetaGouvBot."

Yeah, I've done it like this:

• 400 are configuration, so should definitely said something like "merci de contacter..." : it needs to be actionable for the user !
• 401 reserved for auth (also configuration but specific, should give same format as 400).
• 404 is self explanatory
• 422 for validation, including empty text
• For now, everything else unexpected is going up the stack and end up throwing a 500. If you want to handle those as well, let's make sure we capture the errors and not the 500 itself:

# bad
error 500 do
  #
end

# good
error RuntimeError do
  #
end

# good
error ArgumentError do
  # 
end

# good
error StandardError do
  # 
end

Likewise when handling an empty command (equivalent to /command help) we should not prefix the output with "zut" but something like "Cette commande attend les paramètres suivants", etc.

👍 Also, not that /command doesn't get sent without params, so the empty text case is never triggered as there is no client request being made. OK to change it to /command help and rewrite the message more appropriately.

The fullname validation regex thinks "bob69" is invalid, but has no problem with "bob" (no last name) or with "bob.joe.bart" (three parts instead of two). Personally, I think fullname validation is superfluous, but if we're going to have it, we should ensure it matches up with the validation in beta.gouv.fr code.

👍 Feel free to either only check for presence, or to improve the format validation.

We should not include debug information in the messages sent to Slack channels

👍 That's unintended, we should try either halt(4xx) or halt(200) to see what happens.

I think the way to fix that is to return an empty body if a token is present. If there is a token that means we're talking to Slack, and a response body will be displayed to the Slack user. If there is no token, we're in debug mode.

I have some alternative ideas for a debug / dry-run that do not require polluting our authentication logic. Tell me if they're feasible for de /slash command and if you like them :

• header (X-DRY-RUN or X-VALIDATE-ONLY)
• parameter (dry-run: true or execute: false)
• nested resource (my favorite) : either /compte/dry-run or /compte/validate

If you're going to be away for some time, let me know and I can probably take it from here.

Feel free to address those issues and mark me for review / ping me. I won't be off on-the-go, so a less available. Boarding now ✈️ .

[Morendil]

Another approach would be to buy (vs build): https://github.com/slack-ruby/slack-ruby-bot

[bonjourmauko]

Superseded by #92