As an adherent, I want to create my committee. #98

hhamon · 2017-01-18T11:08:42Z

No description provided.

ghost · 2017-01-19T22:58:02Z

app/Resources/translations/validators.fr.yml

@@ -28,6 +28,8 @@ common.conditions.not_accepted: Vous devez accepter la charte.

 common.recaptcha.invalid_message: La réponse au contrôle anti-spamm est incorrecte.

+common.twitter_nickname.invalid_format: Un identifiant Twitter ne peut contenir que des lettres, des chiffres et des underscores.


On utilise davantage le terme "caractère de soulignement" ou "tiret bas" en français.

Ou « tiret du 6 » lol ? ^^
@tgalopin @HeahDude vous préférez quoi comme traduction FR ?

Nan du 8 en plus lol

https://fr.wiktionary.org/wiki/caract%C3%A8re_de_soulignement
https://fr.wiktionary.org/wiki/tiret_bas

L'underscore est un anglicisme aujourd'hui ça me paraît assez courant, après le tiret c'est juste un "tiret" :D

ghost · 2017-01-19T22:58:59Z

app/Resources/views/adherent/create_committee.html.twig

+                    <form method="post" id="create-committee-form" class="form">
+                        {{ form_errors(form) }}
+
+                        <h3>Renseignez les informatins de votre comité local</h3>


informations

ghost · 2017-01-19T23:12:37Z

tests/AppBundle/Controller/AdherentControllerTest.php

+        $this->assertSame(9, $crawler->filter('#create-committee-form .form__errors > li')->count());
+        $this->assertSame("Cette valeur n'est pas un identifiant valide de ville française.", $crawler->filter('#create-committee-form > .form__errors > li')->text());
+        $this->assertSame('Cette chaine est trop courte. Elle doit avoir au minimum 2 caractères.', $crawler->filter('#field-name > .form__errors > li')->text());
+        $this->assertSame('Cette chaine est trop courte. Elle doit avoir au minimum 5 caractères.', $crawler->filter('#field-description > .form__errors > li')->text());


Bien qu'il s'agisse ici des tests, il conviendrait de corriger les 2 occurrences de "chaîne". Ou au moins de le noter dans les typos à corriger pour la prochaine fois ?

Ah oui bien vu!

hhamon · 2017-01-19T23:45:33Z

tests/AppBundle/Controller/MembershipControllerTest.php


        // Activate the user account
        $activateAccountUrl = sprintf('/inscription/finaliser/%s/%s', $adherent->getUuid(), $activationToken->getValue());
        $this->client->request(Request::METHOD_GET, $activateAccountUrl);

        $this->assertResponseStatusCode(Response::HTTP_FOUND, $this->client->getResponse());
+        $this->assertCount(1, $this->emailRepository->findMessages(AdherentAccountConfirmationMessage::class, 'paul@dupont.tld'));


@HeahDude @romain-pierre @tgalopin voici la nouvelle bonne manière pour tester qu'un email a été envoyé via Mailjet et donc stocké en BDD.

On pourrait imaginer faire une assertion du genre:

$this->assertEmailWasSent(AdherentAccountConfirmationMessage::class, 'paul@dupont.tld');

Pour une prochaine PR ;)

HeahDude · 2017-01-19T23:42:53Z

app/Resources/translations/validators.fr.yml

@@ -41,6 +43,15 @@ adherent.plain_password.min_length: Votre mot de passe doit comporter au moins 8
 adherent.activity_position.invalid_choice: Le statut d'activité n'est pas valide.

 #
+# Committee
+#
+committee.canonical_name.not_unique: Ce nom de comité est déjà choisi


missing dot?

HeahDude · 2017-01-19T23:47:16Z

app/Resources/views/adherent/create_committee.html.twig

+                                'attr': {
+                                    'class': 'form--full',
+                                    'placeholder': 'Nom du comité',
+                                    'max-length': '50',


If you pass null instead of an explicit form type class in the type, the value will be guessed from the constraint, that avoid changing values in many places.

HeahDude · 2017-01-20T09:07:56Z

src/AppBundle/Committee/CommitteeCreationCommandHandler.php

+        $command->setCommittee($this->factory->createFromCommitteeCreationCommand($command));
+
+        $adherent = $command->getAdherent();
+        $committee = $command->getCommittee();


No need for $adherent var IMO, and you should initialize $committee thanks to the return from the factory.

HeahDude · 2017-01-20T09:10:36Z

src/AppBundle/Committee/CommitteeFactory.php

+    {
+        $uuid = isset($data['uuid'])
+            ? Uuid::fromString($data['uuid'])
+            : Committee::createUuid($data['name']);


How sure are we that the name key will exist?

it's a requirement but yes I could throw an exception if it doesn't.

HeahDude · 2017-01-20T09:12:27Z

src/AppBundle/Committee/CommitteeFactory.php

+        }
+
+        $committee->setSocialNetworks(
+            !empty($data['facebook_page_url']) ? $data['facebook_page_url'] : null,


$data['facebook_page_url'] ?? null

HeahDude · 2017-01-20T13:22:57Z

src/AppBundle/Entity/CommitteeMembership.php

+     *
+     * @return CommitteeMembership
+     */
+    public static function createWithAdherent(


createForAdherent?

HeahDude · 2017-01-20T13:25:15Z

src/AppBundle/Entity/CommitteeMembership.php

+     */
+    public static function createWithHostPrivilege(
+        UuidInterface $committeeUuid,
+        UuidInterface $adherentUuid,


Should be Committee and Adherent as type hints IMO.

I can't because the method is used in the Committee class when I don't have access to the Adherent but only its UUID.

HeahDude · 2017-01-20T13:29:19Z

src/AppBundle/Repository/CommitteeMembershipRepository.php

+            ->getQuery()
+        ;
+
+        return (int) $query->getSingleScalarResult() >= 1;


return (bool) $query->getSingleScalarResult();?

Nope ! I want to cast the result of getSingleScalarResult() to int.

What for? You return a bool anyway...

I want to compare two numbers together. By default, getSingleScalarResult() returns a string.

Yeah a string sanding for a COUNT result so casting the call to a bool should work the same:

-return (int) $query->getSingleScalarResult() >= 1; +return (bool) $query->getSingleScalarResult();

Yes you're right this is because PHP is shit with numbers vs bool vs strings. IMO, from a cognitive perspective, it requires less efforts to compare numbers together rather than casting to a boolean. Also it's safer!

I mean it's explicit when you read the code!

HeahDude · 2017-01-20T13:29:47Z

src/AppBundle/Repository/CommitteeMembershipRepository.php

+            ->getQuery()
+        ;
+
+        return 1 === (int) $query->getSingleScalarResult();


Same as above.

Nope! Explanations is same as above. The cast applies on the returned value of $query->getSingleScalarResult();

But you're returning a bool! I don't get it :D

Well the two comparisons below are not equal:

1 === '1'

vs

1 === 1

The cast enforces the right operand to be a valid integer to compare with the left operand.

Back to basics

1 === ( (int) '1' )

Yeah but here we just need to know that it's not empty, so as above it should be the same:

-return 1 === (int) $query->getSingleScalarResult(); +return (bool) $query->getSingleScalarResult();

Am I missing something?

What if the getSingleScalarResult() method returns '2'? Your condition is wrong! I strictly want to compare that we have only one row matching. No less no more!

HeahDude · 2017-01-20T13:31:44Z

tests/AppBundle/Controller/MembershipControllerTest.php


        // Activate the user account
        $activateAccountUrl = sprintf('/inscription/finaliser/%s/%s', $adherent->getUuid(), $activationToken->getValue());
        $this->client->request(Request::METHOD_GET, $activateAccountUrl);

        $this->assertResponseStatusCode(Response::HTTP_FOUND, $this->client->getResponse());
+        $this->assertCount(1, $this->emailRepository->findMessages(AdherentAccountConfirmationMessage::class, 'paul@dupont.tld'));


[ADI] #98 Fix questions numerotation

hhamon added this to the sprint-2 milestone Jan 18, 2017

hhamon self-assigned this Jan 18, 2017

hhamon force-pushed the feat-create-committee branch 10 times, most recently from 5d764f9 to 8a8e7a3 Compare January 19, 2017 22:53

hhamon changed the title ~~[WIP] [AppBundle] create a committee.~~ [AppBundle] create a committee. Jan 19, 2017

hhamon force-pushed the feat-create-committee branch from 8a8e7a3 to 3d02bf4 Compare January 19, 2017 22:55

hhamon requested review from HeahDude, a user and tgalopin January 19, 2017 22:55

hhamon added the Ready for code review label Jan 19, 2017

hhamon force-pushed the feat-create-committee branch from 3d02bf4 to 00b5d74 Compare January 19, 2017 22:58

ghost reviewed Jan 19, 2017

View reviewed changes

hhamon force-pushed the feat-create-committee branch from 00b5d74 to 6224011 Compare January 19, 2017 23:08

ghost reviewed Jan 19, 2017

View reviewed changes

hhamon force-pushed the feat-create-committee branch 4 times, most recently from 6ca6b78 to fcf4579 Compare January 19, 2017 23:42

hhamon commented Jan 19, 2017

View reviewed changes

hhamon changed the title ~~[AppBundle] create a committee.~~ As an adherent, I want to create my committee. Jan 19, 2017

hhamon force-pushed the feat-create-committee branch 2 times, most recently from 6a34c8d to a349a63 Compare January 20, 2017 00:00

hhamon force-pushed the feat-create-committee branch from a349a63 to 8fe3b85 Compare January 20, 2017 11:00

HeahDude reviewed Jan 20, 2017

View reviewed changes

hhamon force-pushed the feat-create-committee branch 5 times, most recently from 748a88f to 70123cf Compare January 23, 2017 09:19

[AppBundle] create a committee from an Adherent authenticated account.

8a1ed10

hhamon force-pushed the feat-create-committee branch from 70123cf to 8a1ed10 Compare January 23, 2017 09:20

hhamon merged commit 9aa4732 into master Jan 23, 2017

hhamon deleted the feat-create-committee branch January 23, 2017 09:23

Haitaar added a commit that referenced this pull request Jan 15, 2019

Merge pull request #3119 from EnMarche/feature/abbeal/adi-98

1290b08

[ADI] #98 Fix questions numerotation

snyk-bot mentioned this pull request Dec 10, 2020

[Snyk] Upgrade node-sass-chokidar from 1.3.3 to 1.5.0 #5181

Closed

		@@ -28,6 +28,8 @@ common.conditions.not_accepted: Vous devez accepter la charte.

		common.recaptcha.invalid_message: La réponse au contrôle anti-spamm est incorrecte.

		common.twitter_nickname.invalid_format: Un identifiant Twitter ne peut contenir que des lettres, des chiffres et des underscores.

As an adherent, I want to create my committee. #98

As an adherent, I want to create my committee. #98

Conversation

hhamon commented Jan 18, 2017

ghost Jan 19, 2017 • edited by ghost

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghost Jan 19, 2017 •

edited by ghost