Job 192 : several call octopod when there are more than 50 projects #196
Conversation
| return project.kind === 'cost_reimbursable' || project.kind === 'fixed_price'; | ||
| } | ||
|
|
||
| function filterProjectByStatusAndKind(projects) { |
There was a problem hiding this comment.
Pourquoi cette fonction publique est au milieu de fonctions privée ?
Pas de convention d'ordre comme en java (fonctions publiques puis privées) ?
D'ailleurs, pourquoi cette fonction est publique ?
Elle ne m'a l'air d'être utilisée qu'ici
There was a problem hiding this comment.
Elle est privée => hop un underscore
En Javascript, on a : "pas d'appels de fonctions pas encore définies dans le fichier", c'est-à-dire l'inverse. On pourrait retirer cette config du linter.
Mais elle est là pour une raison : (je te laisse lire)
https://eslint.org/docs/rules/no-use-before-define
| .filter(project => _isStatusWantedOnJobBoard(project)) | ||
| .filter(project => _isKindOfProjectWantedOnJobBoard(project)); | ||
| } | ||
|
|
||
| async function _fetchAndCacheJobs() { |
There was a problem hiding this comment.
C'est une fonction privée, pourtant elle est exportée.
Tout comme _compareFetchedAndCachedJobs
Pourquoi ?
There was a problem hiding this comment.
car testée indépendamment.
Je pense qu'un gros refacto doit être réalisée dans ces fichiers pour faire apparaître un fichier qui contient _compareFetchedAndCachedJobs et _fetchAndCacheJobs
Je n'ai pas encore de meilleures solutions, donc je ne l'ai pas encore mis en place.
| @@ -29,20 +29,19 @@ const OctopodClient = { | |||
| }); | |||
| }, | |||
|
|
|||
There was a problem hiding this comment.
Commentaire général sur ce fichier : pourquoi est-il appelé octopod.js alors que quand tu l'importes tu nommes ta variable octopodClient ?
=> to rename octopod-client.js
server/src/infrastructure/octopod.js
Outdated
| || project.status === 'mission_accepted' | ||
| || project.status === 'proposal_sent'; | ||
| async fetchProjectsToBeStaffed(accessToken, projects = [], page = 1) { | ||
| if (projects.length === 100 * (page - 1)) { |
There was a problem hiding this comment.
const MAX_NUMBER_OF_PROJECTS_BY_OCTOPOD_PAGE = 100;
There was a problem hiding this comment.
const hasMoreProjectsOnOctopodApi = projects.length === MAX_NUMBER_OF_PROJECTS_BY_OCTOPOD_PAGE * (page - 1);
server/src/infrastructure/octopod.js
Outdated
| return new Promise((resolve, reject) => { | ||
| const options = { | ||
| url: `${config.OCTOPOD_API_URL}/v0/projects?staffing_needed=true&page=1&per_page=50`, | ||
| url: `${config.OCTOPOD_API_URL}/v0/projects?staffing_needed=true&page=${page}&per_page=100`, |
| const allProjects = projects.concat(moreProjects); | ||
| return this.fetchProjectsToBeStaffed(accessToken, allProjects, page + 1); | ||
| } | ||
| return projects; |
There was a problem hiding this comment.
Tu as déporté le filtrage dans le service, pour pouvoir importer tous les projets.
Mais tu pourrais filtrer ici.
Donc fondamentalement, qui porte la responsabilité de filtrer les projets ? OctopodClient qui te sert pour consommer l'api ou ton service qui est censé porter ton métier ?
There was a problem hiding this comment.
c'est une règle métier, c'est pour cela que je l'ai renvoyé vers le service
| callback(null, httpResponse); | ||
| }); | ||
| }); | ||
| describe('#fetchProjectsToBeStaffedPerPage', () => { |
There was a problem hiding this comment.
Nope, ce n'est pas cette fonction que tu testes, c'est #fetchProjectsToBeStaffed
|
|
||
| it('should call Octopod API "GET /projects"', () => { | ||
| describe('when there are more than 100 projects', () => { |
There was a problem hiding this comment.
Pas de test de quand il y en a moins de 100 ?
There was a problem hiding this comment.
avec la récurrence, c'est déjà testé. Donc j'ai pu retirer l'ancien test
| }); | ||
|
|
||
|
|
||
| describe('#fetchProjectsToBeStaffedPerPage', () => { |
There was a problem hiding this comment.
Personnellement, ça me dérange de tester directement cette fonction.
Elle devrait être privée, car elle n'est publique que pour les tests.
Tu pourrais la tester dans le describe supérieur, en appelant sa fonction mère fetchProjectsToBeStaffed
Là tu as un couplage fort avec ton code.
There was a problem hiding this comment.
Oui. J'avais commencé à faire cela.
Néanmoins, c'était très douloureux, et je n'ai pas réussi à le faire simplement.
C'est pour cela que j'ai sorti deux méthodes.
J'ai pensé à sortir ce bout de code, dans un autre fichier, mais je trouve que ce code doit rester ici.
il y a deux features ici : récupérer les résultats de l'api par page, et fixer le cas où j'ai plus de 100 requêtes.
Je n'ai pas de meilleures solutions actionnables facilement. Je laisse ça.
Mais je demanderai bien à un TL JS son avis ! ;-)
trollepierre
force-pushed
the
JOB-192
branch
from
21f17a5
to
36abb56
Compare
|
Changes Unknown when pulling 36abb56 on JOB-192 into ** on dev**. |
trollepierre
force-pushed
the
JOB-192
branch
from
36abb56
to
e8922f5
Compare
|
Changes Unknown when pulling e8922f5 on JOB-192 into ** on dev**. |
No description provided.