[#575] [FEATURE] Classer les acquis par odre de difficulté décroissante lors de la récupération pour un utilisateur donné - (US-880) #575

florianEnoh · 2017-11-02T10:24:01Z

No description provided.

MelanieMEB · 2017-11-02T10:42:30Z

api/lib/domain/services/user-service.js

 }

-function _getCompetenceById(competences, competenceId) {
-  return _(competences).find((competence) => competence.id === competenceId);
+function _getCompetenceById(competences, challenge) {


A renommer peut être ? getCompetenceByChallengeCompetenceId ?

J'aime pas quand tu as raison 👍

MelanieMEB · 2017-11-02T10:50:49Z

api/lib/domain/services/user-service.js

-function _getCompetenceById(competences, competenceId) {
-  return _(competences).find((competence) => competence.id === competenceId);
+function _getCompetenceById(competences, challenge) {
+  return competences.find((competence) => challenge && competence.id === challenge.competence);


Ne retourne rien si pas de challenge. Utilité de vérifié la présence d'un challenge pour chaque compétences ? Le faire avant (dans la fonction) ou à l'appel de la fonction ?

MelanieMEB · 2017-11-02T10:57:00Z

api/lib/domain/services/user-service.js

+  return take(sortedSkills, 3);
+}
+
+function _sortCompetencesSkillsInDesc(competences) {


renommer pour préciser qu'il ne prend que les 3 plus grand : _sortAndExtractThreeMostDifficultsCompetencesSkills?

MelanieMEB · 2017-11-02T11:04:36Z

api/lib/domain/services/user-service.js

+  return challenges.find((challenge) => challenge.id === answer.challengeId);
+}
+
+function addSkillsToCompetence(competence, challenge) {


Pas trouvé de test dessus
Fonction utilisé que dans ce fichier ? (manque un "_" ?)

Oui c'est une coquille

qcattez · 2017-11-07T14:21:01Z

api/lib/domain/services/user-service.js

-  assessments.forEach((assessment) => {
-    answersPromises.push(answerRepository.findByAssessment(assessment.id));
-  });
+function _getRightAnswersByAssesments(assessments) {


Du coup ça devient _findRightAnswersByAssesments(assessments) c'est ça ?

Et on peut remplacer Right par Correct

qcattez · 2017-11-07T14:26:49Z

api/lib/domain/services/user-service.js

 }

-function _getCompetenceById(competences, competenceId) {
-  return _(competences).find((competence) => competence.id === competenceId);
+function _getCompetenceByChallengeCompetenceId(competences, challenge) {


Vu ce que vous passez en paramètre, c'est plutôt _getCompetenceByChallenge(competences, challenge)

qcattez · 2017-11-07T14:30:17Z

api/lib/infrastructure/repositories/answer-repository.js

-        .then(answers => resolve(answers.models))
-        .catch(reject);
-    });
+  getRightAnswersByAssessment(assessmentId) {


ça devient findCorrectAnswersByAssessment du coup

qcattez · 2017-11-07T14:31:17Z

api/lib/infrastructure/repositories/answer-repository.js

-        .where({ challengeId, assessmentId })
-        .fetch()
-    );
+    return Answer


👍 pour le refacto ;)

qcattez · 2017-11-07T14:34:58Z

api/lib/domain/services/user-service.js

+}
+
+function _getRelatedChallengeById(challenges, answer) {
+  return challenges.find((challenge) => challenge.id === answer.challengeId);


Il vaut mieux utiliser le get de l'object Bookshelf que d'accéder directement à l'attribut : answer.get('challengeId')

qcattez · 2017-11-07T14:38:57Z

api/lib/domain/services/user-service.js

-            });
-          }
+      .then(([challenges, competences, answers]) => {
+        answers.map((answer) => {


On peut remplacer map par forEach

qcattez · 2017-11-07T14:45:30Z

api/lib/domain/services/user-service.js

-    challengeRepository.list(), competenceRepository.list(), answersByAssessments
-  ]);
+function _sortThreeMostDifficultSkillsInDesc(skills) {
+  const skillsWithExtractedLevel = skills.map((skill) => {


Du coup en utilisant le modèle de JJ embarquant déjà la notion de difficulty, pas besoin de créer cet objet ?

qcattez · 2017-11-07T14:48:04Z

api/tests/unit/infrastructure/repositories/answer-repository_test.js

@@ -87,7 +90,9 @@ describe('Unit | Repository | AnswerRepository', function() {
    });

    after(function(done) {
-      knex('answers').delete().then(() => {done();});
+      knex('answers').delete().then(() => {
+        done();


done inutile, on peut return knex('answers').delete()

Akhilian · 2017-11-07T14:44:41Z

api/lib/domain/services/user-service.js

+  return challenges.find((challenge) => challenge.id === answer.challengeId);
+}
+
+function _addSkillsToCompetence(competence, challenge) {


Pour moi, cette fonction privée n'est pas super utile.
Je met une proposition plus bas.

Akhilian · 2017-11-07T14:46:02Z

api/lib/domain/services/user-service.js

+        answers.map((answer) => {
+          const challenge = _getRelatedChallengeById(challenges, answer);
+          const competence = _getCompetenceByChallengeCompetenceId(competences, challenge);
+          _addSkillsToCompetence(competence, challenge);


answers.forEach((answer) => { const challenge = _getRelatedChallengeById(challenges, answer); const competence = _getCompetenceByChallengeCompetenceId(competences, challenge); if (challenge && competence) { challenge.knowledgeTags.forEach((skill) => { competence.addSkill(new Skill(skill)); }); } });

Akhilian · 2017-11-07T14:52:20Z

api/lib/domain/services/user-service.js

-    challengeRepository.list(), competenceRepository.list(), answersByAssessments
-  ]);
+function _sortThreeMostDifficultSkillsInDesc(skills) {
+  const skillsWithExtractedLevel = skills.map((skill) => {


Cette notion de difficulty devrait être porté par l'objet Skill.

Akhilian · 2017-11-07T14:53:53Z

api/lib/domain/services/user-service.js

+  return take(sortedSkills, 3);
+}
+
+function _sortAndExtractThreeMostDifficultsCompetencesSkills(competences) {


Ce que fait cette fonction _sortAndExtractThreeMostDifficultsCompetencesSkills, c'est _limitSkillsToTheThreeHighest. Pour la moi, la notion de sort est presque anecdotique.

Akhilian · 2017-11-07T14:54:44Z

api/lib/domain/services/user-service.js

+
+function _sortAndExtractThreeMostDifficultsCompetencesSkills(competences) {
+  competences.forEach((competence) => {
+    competence.skills = _sortThreeMostDifficultSkillsInDesc(competence.skills);


Du coup, on peux rename ici aussi.

Akhilian · 2017-11-07T15:14:22Z

api/tests/unit/infrastructure/repositories/answer-repository_test.js

@@ -39,7 +40,9 @@ describe('Unit | Repository | AnswerRepository', function() {
    });

    after(function(done) {
-      knex('answers').delete().then(() => {done();});
+      knex('answers').delete().then(() => {


On peux enlever les done()

…croissante lors de la récupération pour un utilisateur donné - (US-880)"

qcattez · 2017-11-07T16:10:04Z

ETQ Consommateur de l'API, JV connaître les Acquis à retenir par Compétence

florianEnoh assigned MBrandone and florianEnoh Nov 2, 2017

florianEnoh added [Func] Changes required [Tech] Review asked [Func] Review asked and removed [Func] Changes required labels Nov 2, 2017

MelanieMEB reviewed Nov 2, 2017

View reviewed changes

hypernikao added 5 commits November 2, 2017 14:51

Add Sorted skills

1f24021

litlle refacto of user-service_test

4c50948

Massive function rename

6d88bcd

refacto

8bbd2a7

refacto

f130278

florianEnoh force-pushed the 880-sort-in-desc-three-highests-skills branch from acaca50 to f130278 Compare November 2, 2017 13:52

MBrandone added 2 commits November 6, 2017 16:07

[US-880] resolve Bug from func review (only skills from 1st assessment)

484bd08

[US-880] delete unused function

8c79e7a

qcattez reviewed Nov 7, 2017

View reviewed changes

Akhilian reviewed Nov 7, 2017

View reviewed changes

qcattez added [Tech] Changes required and removed [Tech] Review asked labels Nov 7, 2017

Quentin Cattez added 2 commits November 7, 2017 16:53

[US-880] integrating review comments

4101315

Fixing linter

8e7c30b

qcattez merged commit 7bbb51f into dev Nov 7, 2017

qcattez pushed a commit that referenced this pull request Nov 7, 2017

Revert "[#575] [FEATURE] Classer les acquis par odre de difficulté dé…

5ec7248

…croissante lors de la récupération pour un utilisateur donné - (US-880)"

qcattez deleted the 880-sort-in-desc-three-highests-skills branch November 7, 2017 16:07

qcattez added [Func] Changes validated [Tech] Changes validated and removed [Func] Review asked [Tech] Changes required labels Nov 7, 2017

Akhilian added this to the v1.28.0 milestone Nov 17, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[#575] [FEATURE] Classer les acquis par odre de difficulté décroissante lors de la récupération pour un utilisateur donné - (US-880) #575

[#575] [FEATURE] Classer les acquis par odre de difficulté décroissante lors de la récupération pour un utilisateur donné - (US-880) #575

florianEnoh commented Nov 2, 2017

MelanieMEB Nov 2, 2017

florianEnoh Nov 2, 2017

MelanieMEB Nov 2, 2017

florianEnoh Nov 2, 2017

MelanieMEB Nov 2, 2017

florianEnoh Nov 2, 2017

MelanieMEB Nov 2, 2017

florianEnoh Nov 2, 2017

qcattez Nov 7, 2017

qcattez Nov 7, 2017

qcattez Nov 7, 2017

qcattez Nov 7, 2017

qcattez Nov 7, 2017

qcattez Nov 7, 2017

qcattez Nov 7, 2017

qcattez Nov 7, 2017

qcattez Nov 7, 2017

Akhilian Nov 7, 2017

Akhilian Nov 7, 2017

Akhilian Nov 7, 2017

Akhilian Nov 7, 2017

Akhilian Nov 7, 2017

Akhilian Nov 7, 2017

qcattez commented Nov 7, 2017

[#575] [FEATURE] Classer les acquis par odre de difficulté décroissante lors de la récupération pour un utilisateur donné - (US-880) #575

[#575] [FEATURE] Classer les acquis par odre de difficulté décroissante lors de la récupération pour un utilisateur donné - (US-880) #575

Conversation

florianEnoh commented Nov 2, 2017

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

qcattez commented Nov 7, 2017