Skip to content
This repository was archived by the owner on Jan 29, 2022. It is now read-only.

Conversation

georgiana-b
Copy link
Contributor

@vitorbaptista vitorbaptista temporarily deployed to ot-api-next-pr-143 January 30, 2017 17:03 Inactive
@coveralls
Copy link

Coverage Status

Coverage increased (+0.09%) to 92.308% when pulling e1e997d on feature/replace-doc-type-with-category into 777c6ae on master.

Copy link
Member

@victor-nitu victor-nitu left a comment

Choose a reason for hiding this comment

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

I don't like that we have database IDs hardcoded in our migrations. This way, if database IDs in either Explorer or API ever change, the migrations will silently fail.

Can't we replace the IDs with name+group pairs, and instead of doing operations by ID we do them by getting the ID in a subquery using the pairs above?


const documentCategories = [
{
id: 19,
Copy link
Member

Choose a reason for hiding this comment

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

I really don't like the idea of hardcoding IDs into migrations. Isn't there any other way of doing it? Maybe we can further match explorer and API categories by name+group pairs, which are unique.

const Promise = require('bluebird');

const typeToCategoryMapping = {
blank_consent_form: 33,
Copy link
Member

Choose a reason for hiding this comment

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

Same issue I have with hardcoded IDs into migrations.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@nightsh I figured the name and group would probably change more often than the ID, wouldn't you agree?

@georgiana-b georgiana-b merged commit 517825e into master Feb 6, 2017
@georgiana-b georgiana-b deleted the feature/replace-doc-type-with-category branch February 6, 2017 16:00
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.

4 participants