Skip to content
This repository was archived by the owner on Nov 20, 2025. It is now read-only.

create GET /ideas?filter[title][like]#62

Merged
agatatalita merged 3 commits intomasterfrom
ideas-keyword
Apr 10, 2018
Merged

create GET /ideas?filter[title][like]#62
agatatalita merged 3 commits intomasterfrom
ideas-keyword

Conversation

@agatatalita
Copy link
Copy Markdown
Member

There is one test which is not passing.
@mrkvon look at it please (any time, it is not blocking me from anything)

@agatatalita agatatalita requested a review from mrkvon April 8, 2018 11:27
@agatatalita
Copy link
Copy Markdown
Member Author

@mrkvon thank you for the fix commit!

@mrkvon
Copy link
Copy Markdown
Member

mrkvon commented Apr 9, 2018

@agatatalita my pleasure.

When this is ready for review, will you, @agatatalita, remove the "WIP"?

@agatatalita
Copy link
Copy Markdown
Member Author

agatatalita commented Apr 9, 2018

@mrkvon and @ditup/glodne-czyzyki It is ready for review now : )

@agatatalita agatatalita changed the title WIP create GET /ideas?filter[title][like] create GET /ideas?filter[title][like] Apr 9, 2018
Copy link
Copy Markdown
Member

@mrkvon mrkvon left a comment

Choose a reason for hiding this comment

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

Nice, clean work, almost ready to merge. Only the json-schema validation for a query item could be improved and some test(s) added for this.

Note to self: we should validate the json-schemas we use. Now invalid json-schemas seem to pass.

},
keywordsList: {
type: 'array',
items: 'string',
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Is 'string' a valid json schema? Seems that https://www.jsonschemavalidator.net/ is not able to parse it as such.

Would be also nice to see some test for this, i.e. not allow empty strings and have maximum length of a string, perhaps also strings may want to be separate words? How do you @agatatalita imagine valid items? What do they represent? (word, a few words, ...)

  • what is the minimum and maximum length of the query item?
  • can query item contain a blank space? (i.e. is it a separate word?)
  • does query item have other requirements? (i.e. what about leters, numbers, special characters, upper or lowercase)

If I receive the specification, I'd happily implement this.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

  • what is the minimum and maximum length of the query item?
    @mrkvon do you have any suggestions? what I would be thinking now, the maximum is for sure title.length maximum, but maybe it should be shorter? I believe there are some prepared ways to do it already, we may have it in mind - to optimize words search? So far we should choose something quite quick and simple.
  • can query item contains a blank space? (i.e. is it a separate word?)
    I was thinking it can, So it will be possible to look for 'big dogs', 'cats'. I was probably imagining using it with one keyword now or having something like adding keywords so: the user types 'big dogs' and again to the blank input - 'cats'. So the user can type the whole title and she should be able to find the proper idea.
  • does query item have other requirements? (i.e. what about letters, numbers, special characters, upper or lowercase)
    I was not thinking about. Probably it is the same as for idea title.

Comment thread test/ideas.list.js
});
});

describe('GET /ideas?filter[title][like]=string1,string2,string3', () => {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

So do I understand correctly that the client will be responsible for splitting the user's raw input into separate words (and send them as separate query items)?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I was thinking about it as about keywords, where each keyword is added separately and consciously by the user on the client side. Not like dividing a string into smaller pieces.
I am happy here with every way which will work and be useful for the user.

Comment thread models/idea/index.js
* @param {integer} offset - pagination offset
* @param {integer} limit - pagination limit
* @returns {Promise<Idea[]>} - list of found ideas
*/
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

we could also think whether we need substring search or just word or perfix search (those can be faster)

Copy link
Copy Markdown
Member

@mrkvon mrkvon Apr 10, 2018

Choose a reason for hiding this comment

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

More efficient search can be nice, actually. Especially if we get a lot of ideas and if the search has lower computational complexity (O(n) vs. O(1)?). It can be done later, when it's needed, can't it?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Arango supports word-based prefix matching from what I got on slack(this has probably some index support). If we want something different we can explore it later.

FOR idea IN FULLTEXT(ideas, "title", CONCAT("prefix:", CONCAT_SEPARATOR(",|prefix:", @Keywords)))
RETURN idea

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

opened issue #65 for this

},
keywordsList: {
type: 'array',
items: 'string',
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

  • what is the minimum and maximum length of the query item?
    @mrkvon do you have any suggestions? what I would be thinking now, the maximum is for sure title.length maximum, but maybe it should be shorter? I believe there are some prepared ways to do it already, we may have it in mind - to optimize words search? So far we should choose something quite quick and simple.
  • can query item contains a blank space? (i.e. is it a separate word?)
    I was thinking it can, So it will be possible to look for 'big dogs', 'cats'. I was probably imagining using it with one keyword now or having something like adding keywords so: the user types 'big dogs' and again to the blank input - 'cats'. So the user can type the whole title and she should be able to find the proper idea.
  • does query item have other requirements? (i.e. what about letters, numbers, special characters, upper or lowercase)
    I was not thinking about. Probably it is the same as for idea title.

Comment thread test/ideas.list.js
});
});

describe('GET /ideas?filter[title][like]=string1,string2,string3', () => {
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I was thinking about it as about keywords, where each keyword is added separately and consciously by the user on the client side. Not like dividing a string into smaller pieces.
I am happy here with every way which will work and be useful for the user.

Copy link
Copy Markdown
Member

@mrkvon mrkvon left a comment

Choose a reason for hiding this comment

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

I'm happy with this as it is now. Will you squash+merge @agatatalita ?

Copy link
Copy Markdown
Member Author

@agatatalita agatatalita left a comment

Choose a reason for hiding this comment

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

Thank you for the quick and neat fixes @mrkvon !

@agatatalita agatatalita merged commit 70d2cc7 into master Apr 10, 2018
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.

2 participants