Skip to content
This repository was archived by the owner on Jan 19, 2022. It is now read-only.

support NOTed filtering#43

Closed
sh1r0 wants to merge 11 commits intomozilla:masterfrom
sh1r0:bug_NOT
Closed

support NOTed filtering#43
sh1r0 wants to merge 11 commits intomozilla:masterfrom
sh1r0:bug_NOT

Conversation

@sh1r0
Copy link
Contributor

@sh1r0 sh1r0 commented Oct 28, 2013

@camd
Copy link
Contributor

camd commented Oct 30, 2013

I like the idea of this. I think it would be a really useful feature. However, I think this PR doesn't quite cover the feature as it would need to be. See this tracker story: https://www.pivotaltracker.com/story/show/41414801

We need a UI component to be able to display this in a "chicklet" like the other filters, but show that it's excluding, rather than including items. (like a circle with a slash through it). It also looks like the only way to do this currently is to add the filter to the URL? We should provide a way for a user to invoke this in the UI. So, much like we have a button next to the filter value for "pinning" the filter, we could have the "negate" button there, too.

And this will need unit tests to cover the functionality.

But, again, this would be awesome to have. Especially for things like results, where you would like to see all tests except ones that have been already run, for example.

@sh1r0
Copy link
Contributor Author

sh1r0 commented Oct 31, 2013

Thanks, @camd .
I'm sorry that I have not seen that post before. And the concept of this pull request is similar to my another PR(#40). The original purpose of this patch is to support hidden function to let user able to turn on NOT filtering.

For example,
https://moztrap.mozilla.org/manage/cases/?pagenumber=1&pagesize=20&sortfield=created_on&sortdirection=desc&filter-product=16&filter-productversion=132,
the url above is to retrieve cases with product = firefox os & product version = firefox os v1.3.0. But maybe I want cases belong to firefox os but not v1.3.0, then I can just append &filter-not-productversion to url, that is,
https://moztrap.mozilla.org/manage/cases/?pagenumber=1&pagesize=20&sortfield=created_on&sortdirection=desc&filter-product=16&filter-productversion=132&filter-not-productversion, to get what I really want.

And sorry again, this PR is just a kind of partial solution, not complete at all.
Please feel free to let me know if I should close this PR, thanks.

@camd
Copy link
Contributor

camd commented Dec 7, 2013

hey @sh1r0 : I just got some time to play with this. I must admit, this is pretty dang cool! It wasn't exactly intuitive for discover-ability to know to click the body to do the negation. We might have the hover text mention that or something? Or just But it works great! and having the text be strike-out is great!

This actually worked for me with filtering the on "name" of a test case. You mentioned that keyword filtering though. Can you give me an example of what wasn't working?

I think QA would be ecstatic over this feature. :) Would you work to get the tests working in this branch? It would be great to push this through next week.

Thanks!!! This is awesome!
-Cam

@sh1r0
Copy link
Contributor Author

sh1r0 commented Dec 7, 2013

Hi @camd ,
Sorry about the misleading comment in Pivotal Tracker, since the comment is for bf66a0a commitment and I forgot to update that. Actually, it works on all kinds of keyword filters now except PrefixIDFilter, since it's a little different from others. I wonder if it's acceptable that id filter does not support negation filtering.

And, I'll try to revise or modify some test cases for this branch.

Thanks,
-shiro

@camd
Copy link
Contributor

camd commented Dec 9, 2013

@sh1r0 : Yeah, I don't see it as being useful for ids too much. Hmm... Well, I suppose that being able to filter by an id prefix would be cool in some circumstances. But let's not worry about that for now. Perhaps open a bug that it won't work for ids. But this is really cool! Thanks for doing this! :)

@camd
Copy link
Contributor

camd commented Dec 19, 2013

@sh1r0 This looks great! I think we just need to get the tests passing and we are good to merge. Thanks!

@sh1r0
Copy link
Contributor Author

sh1r0 commented Dec 20, 2013

Hi @camd ,
I've revised and added cases for this PR, but I'm still working on making PrefixIDFilter be supported, since I want to make this PR more complete :p
And, thanks a lot for all your reviews!!

@camd
Copy link
Contributor

camd commented Dec 20, 2013

Sounds great! I can't wait to see it. You're very welcome. :)

@sh1r0 sh1r0 closed this Feb 27, 2014
@sh1r0 sh1r0 mentioned this pull request Feb 27, 2014
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