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

corrected hierarchy filter query on perspective #78

Merged
merged 2 commits into from
Nov 1, 2016

Conversation

shriramshankar
Copy link
Contributor

No description provided.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 83.006% when pulling 5d758c1 on perspective-exclude-fix into 6926d4c on master.

@shriramshankar
Copy link
Contributor Author

@harshkothari410 looks like with the code coverage changes, the db tests does not run any more. Also, the db reset does not happen any more. Not sure if these were the intended; if not can you take a look any time you are free?

q += 'aspect' + sign + p.aspectFilter.join();
const sign = p.aspectFilterType === 'INCLUDE' ? '' : '-';
q += 'aspect' + '=' + sign +
p.aspectFilter.join().replace(/,/g, ',' + sign);
Copy link
Contributor

Choose a reason for hiding this comment

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

Does it really need the sign before each one?
wouldn't ...?aspect=-A,B,C set up an EXCLUDE filter for all three aspects without having to put the minus sign before each one ...?aspect=-A,-B,-C?

Copy link
Contributor

Choose a reason for hiding this comment

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

The API docs for GET /v1/subjects/.../hierarchy are ambiguous about this:

Comma-separated list of aspect names to include/exclude. For example, ?aspect=FOO,BAR will only return subjects in the hierarchy with samples for those two aspects (and all those subjects' ancestors up to the specified root of the requested hierarchy). Prefix the aspect name with a negative sign to indicate that a sample with that aspect should be excluded. For example, ?aspect=-BAZ will return only the subjects (and its hierarchy) that have samples with aspect name not equal to BAZ. Subjects without samples are not included in the result set

The api docs don't explicitly say anything about excluding multiple items.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 83.006% when pulling e449b20 on perspective-exclude-fix into 6926d4c on master.

@pallavi2209 pallavi2209 merged commit beb200d into master Nov 1, 2016
@pallavi2209 pallavi2209 deleted the perspective-exclude-fix branch November 1, 2016 22:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants