We just added the operators to the API (cf. #2624870: Add operators for (NOT) IN and (NOT) BETWEEN), now we should add Views support for them.

CommentFileSizeAuthor
#13 add_support_for_not-2695627-13.patch12.44 KBdermario
#11 2695627-10--views_filters_between.patch12.32 KBdrunken monkey
#10 2695627-10--views_filters_between--interdiff.txt10.03 KBdrunken monkey
#8 add_support_for_not_between-2695627-8.patch3.93 KBdermario
#2 2695627-3--in_between_operators_for_views.patch4.86 KBdrunken monkey

Comments

drunken monkey created an issue. See original summary.

drunken monkey’s picture

drunken monkey
he/him
Primary language German
Location Vienna, Austria
commented
Status: Active » Needs review
StatusFileSize
new 2695627-3--in_between_operators_for_views.patch4.86 KB

This should be a first step (just produced as a by-product of a different issue).
I'm not sure, though, whether this will be enough to make "is between" working – and we should definitely verify that by adding tests as well. Not too difficult, though, just a few assertions in ViewsTest.

Also interesting would be the discussion in #2624870: Add operators for (NOT) IN and (NOT) BETWEEN, to see whether we want to support "NOT BETWEEN" after all.

drunken monkey’s picture

drunken monkey
he/him
Primary language German
Location Vienna, Austria
commented
Status: Needs review » Needs work
Issue tags: +Needs tests
dermario’s picture

dermario
Primary language German
Location Zürich
commented
Assigned: Unassigned » dermario

I will try to write some tests for this in the next days.

drunken monkey’s picture

drunken monkey
he/him
Primary language German
Location Vienna, Austria
commented

The follow-up to #2624870: Add operators for (NOT) IN and (NOT) BETWEEN got committed, so we should also add a "NOT BETWEEN" integration here.

drunken monkey’s picture

drunken monkey
he/him
Primary language German
Location Vienna, Austria
commented
Assigned: dermario » Unassigned
Issue tags: +release target

I guess I can un-assign you? Or are you still working on this?

dermario’s picture

dermario
Primary language German
Location Zürich
commented

Sorry for not replying for so long. I will dig into this in Dublin now.

dermario’s picture

dermario
Primary language German
Location Zürich
commented
Status: Needs work » Needs review
StatusFileSize
new add_support_for_not_between-2695627-8.patch3.93 KB

Ich rerolled/reapplied the change from the patch in #2, tested locally and added simpletests for it.

borisson_’s picture

borisson_
Primary language Dutch
Location Mechelen, 🇧🇪
commented
Status: Needs review » Reviewed & tested by the community
Issue tags: +Dublin2016

@dermario, this looks good!

drunken monkey’s picture

drunken monkey
he/him
Primary language German
Location Vienna, Austria
commented
Status: Reviewed & tested by the community » Needs review
StatusFileSize
new 2695627-10--views_filters_between--interdiff.txt10.03 KB

Thanks a lot, does look pretty good!

A few issues, though:

  • In SearchApiQuery::sanitizeOperator(), we should uppercase "not between", too.
  • In SearchApiFilterTrait::opHelper() you removed the return, which looks to me as though it would add the conditions twice – once using "or" and once "and" logic – if the operator is "or". That our tests don't catch that is a bit worrisome. I've added tests for that.
  • You also missed one commented-out test for the between filter in ViewsTest.

The attached patch should fix all that, please test/review!
And, in any case, thanks again to both of you!

drunken monkey’s picture

drunken monkey
he/him
Primary language German
Location Vienna, Austria
commented
StatusFileSize
new 2695627-10--views_filters_between.patch12.32 KB

Status: Needs review » Needs work

The last submitted patch, 11: 2695627-10--views_filters_between.patch, failed testing.

dermario’s picture

dermario
Primary language German
Location Zürich
commented
Status: Needs work » Needs review
StatusFileSize
new add_support_for_not-2695627-13.patch12.44 KB

Thank you @drunken monkey for your detailed feedback. That will help me - i'll try to be more precise with that. For now i rerolled the patch from #11. Hope it will apply now.

drunken monkey’s picture

drunken monkey
he/him
Primary language German
Location Vienna, Austria
commented
Status: Needs review » Fixed
Issue tags: -Needs tests

Ah, great. Thanks!
Seems like everyone's OK with this now, so: Committed.
Thanks again, everyone!

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.