Change the filter params #7051
Change the filter params #7051
Conversation
There was a problem hiding this comment.
Very nice PR
Thank you @aeSouid
I know we have not always been thorough enough about testing the Front-End.
But this feature is very abstract and all the edge case that could slip into encoding/decoding values should be tested.
Let's chat it you want to learns the antics of FE testing tomorrow.
| @@ -42,7 +44,10 @@ export const encodeParams = (filters, OPTIONS) => { | |||
| }, {}); | |||
| }; | |||
|
|
|||
| export const decodeParams = (query, include, OPTIONS) => { | |||
| export const decodeParams = (path, query, include, OPTIONS) => { | |||
There was a problem hiding this comment.
I would love these two functions to be unit tested using vitest.
Create a spec file named tests/unit/filter/utils/helpers.spec.ts and add the test casses like what has been done in flowUtils and yamlUtils.
There was a problem hiding this comment.
Alaa asked me about tests, I've told him to skip them for now as they were not existing already, but you do make a good point here. As we're not rushing with this for the 0.21.0 release (if I'm not mistaken) we can do the test coverage, also.
There was a problem hiding this comment.
@elevatebart @MilosPaunovic thanks for the review, i will add the tests
There was a problem hiding this comment.
i pushed the helpers.spec.ts for testing, and i will add other case tests
There was a problem hiding this comment.
We've now lost the ability to chose more than one value, example is Flows -> Scope. If the comparator is not multiple, but there are two comparators for the particular filter type, we should allow the selection of both of them, if I'm not mistaken.
So, on the case of Flows -> Scope, we should allow users to select one value for compartor IS and one for the IS NOT.
| IN: buildComparator(t(comparator("in"))), | ||
| BETWEEN: buildComparator(t(comparator("between"))), | ||
| STARTS_WITH: buildComparator(t(comparator("starts_with"))), | ||
| EQUALS: buildComparator(t(comparator("is")), "$eq"), |
There was a problem hiding this comment.
Potentially, might be a good idea to uniform the expressions - to not show label IS for EQUALS, but the EQUALS itself.
Although, this is more of a question for a product team, so pinging @Ben8t here for an opinion.
There was a problem hiding this comment.
Not sure to understand: does it shows "IS" here? Can you give me a bit more context please?
There was a problem hiding this comment.
It's the comparator chooser of filters:
For example, what is right now IS is going to be sent to backend as EQUALS when this is merged. Do we want to align the wording on that, so it's not labeled as IS anymore, but as EQUALS?
| return params; | ||
| }; | ||
|
|
||
| export const isSearchPath = (path: string) =>["/flows", "/executions", "/logs", "/dashboard"].includes(path); |
There was a problem hiding this comment.
Be aware here, as we might have search in some of the nested pages somewhere (not completely sure, needs checking), so it might be a better idea here to check the full route name in compassion.
There was a problem hiding this comment.
Also, do we need this method exported, as I believe it's used only in this file?
There was a problem hiding this comment.
i will see how we can apply the filter encoding only when we call the search endpoint
|
As far as I'm concerned, this is ready to be merged, just sync it with the merge of PRs for the BE changes to have it all available at the same time, and make sure to NOT backport to previous release branches, as we're not including this in Good job here @aeSouid! 🚀 |
aeSouid
force-pushed
the
feat/query-filter-client
branch
4 times, most recently
from
a0cb678 to
6b2ceb5
Compare
Change the filters to the new search format chore(translations): auto generate values for languages other than english fix the date display in the search bar chore(translations): auto generate values for languages other than english chore(translations): auto generate values for languages other than english
aeSouid
force-pushed
the
feat/query-filter-client
branch
from
21d35f4 to
97b2264
Compare
|
CHnage the parsing of the query parms from url to the new format