#Refocus get /subjects filtering by tag working #68
Conversation
| in: query | ||
| items: | ||
| type: string | ||
| maxLength: 60 |
There was a problem hiding this comment.
Why 60 here? (Might need to allow for longer since you could build up a list of includes or excludes.)
There was a problem hiding this comment.
Each tag length is max 60 char long, as restricted by model.
Also, the changes in this PR only handles includes at sql level like v1/subjects?tags=mytag.
Do we need excludes here as well? I assumed the story was about includes only to get trust data working. I can create another PR if we need excludes as well.
There was a problem hiding this comment.
ok we can get this through as is, but i think we want to follow up with story to allow v1/subjects?tags=tag1,tag2,tag3 or v1/subjects?tags=-tag10,tag11 for parity with the analogous functionality on the /hierarchy endpoint
There was a problem hiding this comment.
Sounds good, let me create a story then.
There was a problem hiding this comment.
the /hierarchy endpoint filters are case insensitive too.
There was a problem hiding this comment.
@shriramshankar Just checked, we can add different case tags to a subject, like I can have
tags = [
"mytag",
"Cali",
"mytaG",
"MYTAG"
]
Is this expected behavior?
Also, if we need to filter case insensitive, there are 2 ways:
- Filter on API layer instead of query level, like hierarchy endpoint. Disadvantage: get all subjects first and then extra processing to filter.
- store tags in lowercase in db. This is more efficient way to do, if we agree.
I can add this feature to the new story to filter by excludes as well.
There was a problem hiding this comment.
yes you can have mixed cases for tags.
There was a problem hiding this comment.
So, if we can have mixed tags, then option 2 won't work and we will have to do filtering on API layer? What is the use case of having mixed tags?
There was a problem hiding this comment.
Since filtering for the hierarchy end point was done in the API layer, it made sense to store the tags the way it was created by the user.
| @@ -19,6 +19,7 @@ const u = require('./utils'); | |||
| const Subject = tu.db.Subject; | |||
| const path = '/v1/subjects'; | |||
| const expect = require('chai').expect; | |||
| const filterSubjByTags = require('../../../../config').filterSubjByTags; | |||
There was a problem hiding this comment.
will always be false unless we explicitly set env var when we call the test (see examples in package.json)
pallavi2209
force-pushed
the
subject-filter-by-tag
branch
from
4d25141
to
f3d4ae4
Compare
pallavi2209
force-pushed
the
subject-filter-by-tag
branch
from
f3d4ae4
to
30cf7ae
Compare
pallavi2209
force-pushed
the
subject-filter-by-tag
branch
2 times, most recently
from
0e3cd1d
to
8d6489b
Compare
iamigo
dismissed
their stale review
pushing additional work into separate story
|
uh oh coverage decreased! :( |
|
@iamigo @shriramshankar updated the PR |
|
@iamigo So, I looked into it because I have tests for the new code. I think the test coverage does not cover the tests written for specific environment. So, coverage identifies the new code as not covered. @harshkothari410 can you confirm this? |
|
Yes, that's right--Harsh is going to add all those in this week I hope. |
|
Yes environment test are remaining. It covers API and DB test only right now. I will add those because that requires some research because we need to compile some stuff for view test or change environment for env related test. @pallavi2209 @iamigo |
Using env var for new changes.
When env var us enables, subjects are filtered by tags, like GET /v1/subjects?tags=tag1,tag2
Added tests behind config var.