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

[#610] Add is_primary to record #131

Merged
merged 2 commits into from Dec 14, 2016
Merged

[#610] Add is_primary to record #131

merged 2 commits into from Dec 14, 2016

Conversation

georgiana-b
Copy link
Contributor

@vitorbaptista vitorbaptista temporarily deployed to ot-api-next-pr-131 December 14, 2016 08:38 Inactive
@coveralls
Copy link

Coverage Status

Coverage decreased (-0.2%) to 92.164% when pulling 2da328a on feature/add_is_primary_to_record into ceafb5f on master.

@georgiana-b georgiana-b force-pushed the feature/add_is_primary_to_record branch from 2da328a to e70942d Compare December 14, 2016 09:13
@vitorbaptista vitorbaptista temporarily deployed to ot-api-next-pr-131 December 14, 2016 09:13 Inactive
@coveralls
Copy link

Coverage Status

Coverage increased (+0.03%) to 92.351% when pulling e70942d on feature/add_is_primary_to_record into ceafb5f on master.

nightsh
nightsh reviewed Dec 14, 2016
View reviewed changes
Copy link
Member

@nightsh nightsh left a comment

Choose a reason for hiding this comment

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

Questions raised about the usage of true/null values.

Another question: do we need the is_primary field in the ES indexer?

migrations/20161213144433_add_is_primary_to_records.js
knex.schema
.table('records', (table) => {
table.boolean('is_primary')
.nullable();
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't this default to false, instead of being nullable?

seeds/trials.js
@@ -468,6 +469,7 @@ exports.seed = (knex) => {
trial_id: trials[1].id,
source_id: sources.isrctn.id,
source_url: 'http://www.isrctn.com/ISRCTN11631712',
is_primary: null,
Copy link
Member

Choose a reason for hiding this comment

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

Why not use true/false instead of true/null?

@coveralls
Copy link

Coverage Status

Coverage remained the same at 92.322% when pulling 401a880 on feature/add_is_primary_to_record into ceafb5f on master.

@georgiana-b
Copy link
Contributor Author

georgiana-b commented Dec 14, 2016 •
edited

@nightsh I added a default to false for is_primary as you mentioned and removed the logic to only expose the field when its value is true, so now it will show in the payload for all records whether it's true or false.
Please review latest changes.

Another question: do we need the is_primary field in the ES indexer?

As far as I've seen we don't have an index dedicated to records. We only index trials and discrepancies so there's no room for is_primary.

Also, coveralls is giving mixed messages and I don't really understand what bothers him. If you do, let me know.

@georgiana-b georgiana-b merged commit 08bff69 into master Dec 14, 2016
@georgiana-b georgiana-b deleted the feature/add_is_primary_to_record branch December 14, 2016 15:21
@georgiana-b georgiana-b mentioned this pull request Jan 9, 2017
Closed
@georgiana-b georgiana-b mentioned this pull request Jan 17, 2017
Closed
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@nightsh nightsh nightsh approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
4 participants
@georgiana-b @coveralls @nightsh @vitorbaptista