Skip to content
This repository has been archived by the owner on Jan 9, 2024. It is now read-only.

Request badges for new members #7

Merged
merged 18 commits into from Apr 25, 2017
Merged

Request badges for new members #7

merged 18 commits into from Apr 25, 2017

Conversation

Morendil
Copy link
Contributor

@Morendil Morendil commented Jan 27, 2017

Current behaviour:

  • will request badges for members with a property office set to dinsic
  • will make a request as soon as _author entry is checked in, regardless of date
  • will make a new request on any change to end date
  • will mention the raw value of employer in the email sent to DINSIC secretariat

This may be good enough to go live. Future enhancements will include a better representation of the employer field, and perhaps explicit handling of contract renewals.

MattiSG
MattiSG previously requested changes Feb 1, 2017
Copy link
Member

@MattiSG MattiSG left a comment

Choose a reason for hiding this comment

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

Je me demande s'il est réellement nécessaire d'ajouter la dépendance à redis et à quelle point elle est lourde, mais je ne m'y connais pas suffisamment sur le sujet dans l'immédiat et n'ai pas le temps de rechercher pour avoir un avis.

C'est très enthousiasmant en tous cas !!

@@ -0,0 +1,6 @@
Bonjour,

Serait-il possible de fournir un badge à {{author.fullname}}, qui nous rejoint en tant que {{author.employer}} jusqu'au {{author.end}} au sein de l’incubateur ?
Copy link
Member

Choose a reason for hiding this comment

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

Au vu du format de {{author.employer}}, une formulation de type « par un contrat {{author.employer}} » me semblerait plus adaptée que « en tant que » (dinsic, independent/octo, service/octo…).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Je pensais réaliser un mapping, style formulations[author.employer.split('/')[0]] et ça va exiger un brin de refactoring du code qui établit le contexte pour Liquid...

Copy link
Member

Choose a reason for hiding this comment

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

Oui, on peut changer le code… ou sinon on peut juste changer le texte, non ? 😉

Copy link
Contributor Author

Choose a reason for hiding this comment

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

En fait il y a une étape intermédiaire: l'API à l'heure actuelle n'expose pas employer (oops)

Je démarre une PR sur beta.gouv.fr avec une montée de version en 1.2

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ou alors on peut considérer que ce n'est pas important de savoir en tant que quoi les gens nous rejoignent?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Tu as raison, et j'aurais dû t'écouter plus tôt. ;)

Serait-il possible de fournir un badge à {{author.fullname}}, qui nous rejoint en tant que {{author.employer}} jusqu'au {{author.end}} au sein de l’incubateur ?

Bonne journée,
L'Incubateur
Copy link
Member

Choose a reason for hiding this comment

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

« Le secrétariat de l'incubateur » ?
J'aime beaucoup l'idée du secrétariat robotique…

@MattiSG
Copy link
Member

MattiSG commented Feb 2, 2017 via email

@Morendil
Copy link
Contributor Author

Morendil commented Feb 2, 2017

Argh. Je vais revert le commit précédent.

@bonjourmauko
Copy link
Contributor

Je me demande s'il est réellement nécessaire d'ajouter la dépendance à redis et à quelle point elle est lourde, mais je ne m'y connais pas suffisamment sur le sujet dans l'immédiat et n'ai pas le temps de rechercher pour avoir un avis.

Le filesystem de Heroku est éphémère et ne permet pas d'écrire au disque, donc ça rend obligatoire d'écrire à une base de données qui ne soit pas server-less. Lorsque le job-to-be-done est de « ne pas être harcelé·e (en tant que personne responsable de faire de badges) par des demandes (répétées) de ceux-ci », il est besoin IMHO de savoir si l'email a été déjà envoyé avant.

Ceci dit, j'imagine rapidement les solutions :

  • Parser le log : il faut s'assurer que sa rotation soit < 1j
  • Demander à SendGrid si l'on a envoyé an email de demande de badge pour member les dernières 24 + 1 heures : semble compliqué, et il faudrait investiguer l'API
  • Utiliser une base des données : bingo. Pas investigué non plus, mais Redis semble OK.

Copy link
Contributor

@bonjourmauko bonjourmauko left a comment

Choose a reason for hiding this comment

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

  • Essaie de renommer BadgeBoy pour le rendre gender-less
  • Essaie de séparer plus clairement la logique de service (envoyer l'email) de de modèle (écrire et lire à la base de données). Bien que séparer en deux classes ne me semple pas forcement nécessaire, au moins je séparerait les méthodes.

.each { |member| maybe_request_badge(member) }
end

def maybe_request_badge(author)
Copy link
Contributor

Choose a reason for hiding this comment

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

This is a policy method, should be written as, for example, badge_requested?(author)

members
.map(&:with_indifferent_access)
.select { |member| member[:based] == 'dinsic' }
.each { |member| maybe_request_badge(member) }
Copy link
Contributor

Choose a reason for hiding this comment

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

request_badge(member) unless badge_requested?(member)

mailer.post(email)
end

def state_storage
Copy link
Contributor

Choose a reason for hiding this comment

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

Or just storage

@@ -24,6 +25,9 @@ class Webhook < Sinatra::Base
# Send reminders (if any)
Mailer.(schedule, RULES)

# Request badges (if any)
BadgeBoy.(members)
Copy link
Contributor

Choose a reason for hiding this comment

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

Or girl


it 'requests a badge for new members' do
described_class.(authors)
expect(described_class).to have_received(:request_badge).once
Copy link
Contributor

Choose a reason for hiding this comment

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

Or (haven't tried) :

expect(described_class).to have_received(:request_badge).with(ann).once

@Morendil
Copy link
Contributor Author

Morendil commented Apr 20, 2017

Before merging:

  • re-enable Redis in prod
  • change authors API so that v1.3 exposes office
  • update CONTRIBUTING and Wiki

@SGMAP-bot SGMAP-bot temporarily deployed to betagouvbot-pr-7 April 21, 2017 08:26 Inactive
@SGMAP-bot SGMAP-bot temporarily deployed to betagouvbot-pr-7 April 21, 2017 08:36 Inactive
@SGMAP-bot SGMAP-bot temporarily deployed to betagouvbot-pr-7 April 24, 2017 13:57 Inactive
@SGMAP-bot SGMAP-bot temporarily deployed to betagouvbot-pr-7 April 24, 2017 14:06 Inactive
@SGMAP-bot SGMAP-bot temporarily deployed to betagouvbot-pr-7 April 24, 2017 14:22 Inactive
@SGMAP-bot SGMAP-bot temporarily deployed to betagouvbot-pr-7 April 24, 2017 14:31 Inactive
@SGMAP-bot SGMAP-bot temporarily deployed to betagouvbot-pr-7 April 24, 2017 14:44 Inactive
@SGMAP-bot SGMAP-bot temporarily deployed to betagouvbot-pr-7 April 24, 2017 14:55 Inactive
@SGMAP-bot SGMAP-bot temporarily deployed to betagouvbot-pr-7 April 24, 2017 21:59 Inactive
@SGMAP-bot SGMAP-bot temporarily deployed to betagouvbot-pr-7 April 24, 2017 22:04 Inactive
@SGMAP-bot SGMAP-bot temporarily deployed to betagouvbot-pr-7 April 25, 2017 09:29 Inactive
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants