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

fix(backend): oData queries with input filter #656

Merged
merged 8 commits into from
Jan 5, 2021

Conversation

jr01
Copy link
Collaborator

@jr01 jr01 commented Dec 18, 2020

@ghiscoding there is absolutely no rush to get this out, so please leave this until after your holiday :) And I'm of course more than happy to change things.

Fixes #650 and more:

  • Improved escaping/normalizing search terms for string/text/readonly fields.
  • Invalid characters from integer/float/number fields with string search terms are now removed.
  • Added a decimalInputSeparators filter param for integer/float/number field searching. For example, in the Netherlands (NL) users are accustomed to both typing 10.23 or 10,23 when searching for numbers. The reason is the ',' is the decimal separator in NL and commonly the (physical) 'en-US' keyboard is used which has a '.' on the numeric keypad.
filter: {
    params: {
      decimalInputSeparators: ',.'
    }
  };
  • Added support for range filter .. to function without an upper bound. When for example 2.. is input then field ge 2 or field gt 2 is send to the backend, depending on the defaultFilterRangeOperator
  • Added support for range filter .. to function without a lower bound. When for example ..2 is input field le 2 or field lt 2 is send, depending on the defaultFilterRangeOperator

@codecov
Copy link

codecov bot commented Dec 18, 2020

Codecov Report

Merging #656 (d9aab76) into master (2e8e245) will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff            @@
##            master      #656   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files          154       154           
  Lines        10392     10397    +5     
  Branches      3664      3668    +4     
=========================================
+ Hits         10392     10397    +5     
Impacted Files Coverage Δ
...s/angular-slickgrid/services/grid-odata.service.ts 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2e8e245...d9aab76. Read the comment docs.

@ghiscoding
Copy link
Owner

ghiscoding commented Dec 18, 2020

Wow I rarely get good PRs like yours, incredible job there, I took a quick glance at it and left couple of comments.
Things to note though, I recently added 2 new options for formatterOptions in my other lib (Slickgrid-Universal) but forgot to add it in this lib as well which was to add decimal formatter prefix/suffix (ref commit), I leave this here mostly as a reminder for me to look into it after holidays... but you can see that I would prefer to have a common place for decimal separator and such. The name formatterOptions might not be ideal to represent OData query but is still a good way to globalize the option. I'm open to suggestion if it makes sense to use the formatterOptions or rename it somehow.

BTW I'm in Canada (Montreal) and we use both metrics and US system because we are close to them, so yes I understand the pain of having to handle different decimal separator.

Also it looks like adding the switch/case was nice but now codecov reports a missing line coverage, I really wish to keep my 100% coverage since I worked really hard on achieving it, so if you have a way to add the missing line coverage, that would be great.

Now it looks like I have to do in also replicating some of that code in my GraphQL implementation.

Again thanks for this great contribution and yes I will only merge it after in January 😉

image

@jr01
Copy link
Collaborator Author

jr01 commented Dec 18, 2020

, I really wish to keep my 100% coverage since I worked really hard on achieving it, so if you have a way to add the missing line coverage, that would be great.

Pardonnez moi :) - I did look at the coverage before creating the PR, but I somehow missed it. I'll fix this for sure!

@ghiscoding
Copy link
Owner

Pas de problème ;)

Codecov checks slightly more than Jest (istanbul) coverage, so it happens sometime that it shows 100% in Jest but in reality it's missing a line or 2, that most often happens in switch/case.

@ghiscoding
Copy link
Owner

ghiscoding commented Jan 1, 2021

I've reviewed the PR while applying the changes to the GraphQL Service too (in my other lib that is) and apart from the test description rename it seems all good. There's also still 1 line of coverage missing, if you could change 2 of the duration columns to be of FieldType.integer and float that should be enough to be back to full coverage.

I'll be ready for a merge after that
Thanks again for this great contribution, it's always nice to have contributions.

Happy New Year 🍾

@ghiscoding
Copy link
Owner

@jr01
Hi there, I'm not sure if you've seen the last comments I've put. I'd like couple of small changes done before I merge the PR, whenever you get a chance. Thanks again.

@jr01
Copy link
Collaborator Author

jr01 commented Jan 5, 2021

Hi @ghiscoding - I've done the changes you requested.

Happy new year! 😃

@ghiscoding ghiscoding merged commit 48b4ebc into ghiscoding:master Jan 5, 2021
@ghiscoding
Copy link
Owner

Excellent, expect a week or two before a new version, I got a few PRs to look into
Happy busy New Year 😉

ghiscoding-SE pushed a commit that referenced this pull request Jan 5, 2021
* Improved escaping/normalizing search terms for string/text/readonly fields.
* Invalid characters from integer/float/number fields with string search terms are now removed.
* Added support for range filter `..` to function without an upper bound. When for example `2..` is input then `field GE 2` or `field GT 2` is send to the backend, depending on the `defaultFilterRangeOperator`
* Added support for range filter `..` to function without a lower bound. When for example `..2` is input `field LE 2` or `field LT 2` is send, depending on the `defaultFilterRangeOperator`
- ref issue #656
@ghiscoding
Copy link
Owner

ghiscoding commented Jan 5, 2021

As per some discussions in this PR, I also made this PR #660 to export all Editors, Filters & Formatters as Public. So you'll be able to extends InputFilter which is easier and less confusing compare to extends Filters.inputText

Thanks for the feedback (and contribution), it helps to improve the lib 😉

Oh I have also applied your OData changes to the GraphQL Service as well in PR #659 which is also great 😄

... and I will report back when the new release is out

@ghiscoding
Copy link
Owner

@jr01 I just realized that I might have forgot to inform that this was released last month, in the meantime I just released another version 2.26.0 today.

Cheers and thanks again for contributions. 🍻

@jr01
Copy link
Collaborator Author

jr01 commented Feb 18, 2021

@ghiscoding - you informed me here #650 (comment) .
I remember leaving a thank you, but can't find that one now...so (perhaps again): thank you!🥇 😄

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.

invalid oData queries with operators in input filter
2 participants