Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Communes by dep #47

Merged
merged 23 commits into from
Jun 17, 2016
Merged

Communes by dep #47

merged 23 commits into from
Jun 17, 2016

Conversation

tmerlier
Copy link
Contributor

Réf #31

@tmerlier tmerlier added this to the 1.0 milestone Jun 17, 2016
@tmerlier tmerlier self-assigned this Jun 17, 2016
@tmerlier tmerlier added code and removed feature labels Jun 17, 2016
@tmerlier
Copy link
Contributor Author

cc @jdesboeufs

@jdesboeufs
Copy link
Contributor

Quelle honte. -1.2%, pire que le CAC 40.

@@ -178,6 +178,33 @@ paths:
schema:
$ref: '#/definitions/Error'

/departements/{code}/communes:
get:
summary: Renvoi les communes du d'un département
Copy link
Contributor

Choose a reason for hiding this comment

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

Typo

@tmerlier
Copy link
Contributor Author

Better? @jdesboeufs

@@ -83,11 +83,19 @@ app.get('/departements/:code', initDepartementFields, function (req, res) {
});

app.get('/departements/:code/communes', initCommuneFields, initCommuneFormat, function (req, res) {
let communes = dbCommunes.queryByDep(req.params.code);
if (!communes) {
let departement = dbDepartements.queryByCode(req.params.code)[0];
Copy link
Contributor

@jdesboeufs jdesboeufs Jun 17, 2016

Choose a reason for hiding this comment

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

Si le résultat est vide, tu vas avoir une exception et donc une erreur 500.
Ça veut dire qu'il manque un test d'intégration avec la 404 attendue.

const matching = dbDepartements.queryByCode(req.params.code);
if (match.length === 0) {
    res.sendStatus(404);
} else {
    const departement = matching[0];
   /// ...
}

Enfin dans l'esprit quoi.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ou du coup :

if (dbDepartements.queryByCode(req.params.code) === 0) {
  // Booom
} else {
  // Good
}

@tmerlier
Copy link
Contributor Author

Last one? @jdesboeufs

@@ -83,11 +83,10 @@ app.get('/departements/:code', initDepartementFields, function (req, res) {
});

app.get('/departements/:code/communes', initCommuneFields, initCommuneFormat, function (req, res) {
let departement = dbDepartements.queryByCode(req.params.code)[0];
if (!departement) {
let communes = dbCommunes.queryByDep(req.params.code);
Copy link
Contributor

@jdesboeufs jdesboeufs Jun 17, 2016

Choose a reason for hiding this comment

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

On ne s'est pas compris.
On cherche à vérifier l'existence du département, pas qu'il contienne des communes.

@jdesboeufs
Copy link
Contributor

Il manque la recherche standard, à savoir via /communes?codeDepartement={codeDepartement}

@jdesboeufs
Copy link
Contributor

Fix #28

@jdesboeufs
Copy link
Contributor

Fini.
N'oublies pas de jeter un oeil aux changements.

@jdesboeufs jdesboeufs merged commit 1f77908 into master Jun 17, 2016
@jdesboeufs jdesboeufs deleted the communes-by-dep branch June 17, 2016 18:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants