647649 - Change the old "boolean charts" UI into the new AND/OR "custom search" UI

Status: RESOLVED FIXED

(Bugzilla :: Query/Bug List, enhancement)

Description

[Max Kanat-Alexander]

Now that the backend supports arbitrary AND/OR combinations with parentheses and all that stuff, it's time to make the UI support it as well.

Comment 1

[Max Kanat-Alexander]

Attachment: v1

Here we go, this is the new Custom Search UI. I need a review on this because I don't own the search UI (I only own the backend).

This requires JavaScript in order to add new boolean charts. It's going to be the end of 2011 by the time that we release 4.2, it's fine to require JavaScript for an advanced feature that would be considerably more complex if we also added non-JS support.

Comment 2

[Max Kanat-Alexander]

Pyrzak--note that this accomplishes the "add charts with JS" bug, because that was the only way I did it. (It was actually much easier to do it in JS than to do it without JS, with this new system.)

You're also welcome to comment on the UI, but any really major changes should wait until after this gets checked in (since right now it's blocking me from doing further work that has to get done before 4.2 freezes).

Comment 3

[Max Kanat-Alexander]

Attachment: v2

There was some code that I forgot to remove from query.cgi.

Comment 4

[Frédéric Buclin]

Could you attach a screenshot, please?

Comment 5

[Max Kanat-Alexander]

Attachment: v3

I forgot to remove the "jsmagic" variable from the template, which wasn't used. Also, there were some 008filter failures that I've fixed in this version.

Comment 6

[Max Kanat-Alexander]

Attachment: Screenshot of Basic UI

Here's what the UI looks like when you haven't entered anything.

Comment 7

[Max Kanat-Alexander]

Attachment: Screenshot of Active Use

Here's a screenshot of the UI in active use. This is essentially a search that says:

bug_id = 1
OR
bug_id = 2
OR
NOT ( bug_id = 3 )
OR 
(
  bug_id = 4
  AND
  assigned_to LIKE '%mkanat%'
)
OR
cc = mkanat
OR
(
  assigned_to = mkanat
  AND
  comment LIKE '%crash%'
)

Comment 8

[Max Kanat-Alexander]

One thing I've considered as a possibility, also, is to have an "advanced" link that causes the (, ), and "Not" bits to appear, and to hide them by default.

Comment 9

[Max Kanat-Alexander]

Comment on attachment 523963 [details] [diff] [review]
v3

Hmm, this still has a few bugs.

Comment 10

[Max Kanat-Alexander]

Attachment: v4

Okay, this one now actually works as intended. :-)

Comment 11

[Max Kanat-Alexander]

Attachment: Screenshot of Basic UI

I modified the UI a bit, here's the new version.

Comment 12

[Max Kanat-Alexander]

Attachment: Screenshot of Active Use

Here's an updated screenshot of the UI in heavy use. This is the same search as the last one, except that I changed the last criterion to:

OR NOT ( )

instead of just "OR ( )".

Comment 13

[Max Kanat-Alexander]

Attachment: v5

Ha, there was one more bug, having to do with closing parentheses. Fixed now.

Comment 14

[Max Kanat-Alexander]

Attachment: v5.1

I changed a magic number in the JS into a constant.

Comment 15

[Colin Ogilvie [:cso]]

Comment on attachment 523971 [details]
Screenshot of Active Use

(comment which contained the quoted contents of a binary attachment removed for your sanity)

Comment 16

[Colin Ogilvie [:cso]]

Err, d'oh - didn't mean to do that - was only updating the mimetype!

Comment 17

[timeless]

err. i use noscript w/ bugzilla and i use boolean charts regularly

Comment 18

[timeless]

colin: fwiw you probably clicked 'edit attachment as comment'

Comment 19

[Max Kanat-Alexander]

(In reply to comment #17)
> err. i use noscript w/ bugzilla and i use boolean charts regularly

  Sorry, you will no longer be able to do that.

Comment 20

[timeless]

surely you could provide a path which implements the old ui.

note that if you're going to use JS to do stuff you should dim "not" when it isn't checked. And it's probably better written as 'Negate' instead of 'Not'...

Comment 21

[Dave Miller [:justdave]]

(In reply to comment #1)
> This requires JavaScript in order to add new boolean charts. It's going to be
> the end of 2011 by the time that we release 4.2, it's fine to require
> JavaScript for an advanced feature that would be considerably more complex if
> we also added non-JS support.

This will break Bugzilla for several of Mozilla's users if it doesn't work without js.  The people who triage the security bugs run pretty regularly with Javascript disabled on bmo to avoid getting tripped up by testcases for exploits that get posted to the security bugs.  Regardless, that was one of our design goals was to keep the main features usable without Javascript, but add functionality when Javascript is present.

I don't see anything being done here in the UI that's any more complicated than the existing boolean charts is as far as form display goes (you submit the with with the open paren or the + button as the submit and it spits the form back out with the extra line or the indent - ought to be able to steal existing code from boolean charts for this part).

Comment 22

[Max Kanat-Alexander]

(In reply to comment #21)
> This will break Bugzilla for several of Mozilla's users if it doesn't work
> without js.

  Basically, though, what this means is that for less than 0.01% of all users, you're asking me to write and maintain 25% more code. "Bugzilla should work without JS" is an antiquated requirement that nowadays has important practical application *only* to people triaging security bugs at Mozilla.

  I'd estimate there are 10,000 Bugzilla installations. If we estimate that there are roughly 200 users per installation, that's 2 million users. (There are probably more.) Let's take a huge over-estimate and say that there are 50 people who have to turn off JS in this one Bugzilla for security reasons. That's %0.0025 of our userbase.

  Since the users are all at one installation (or only a few installations) I think the more appropriate solution would be to write an extension that you maintain, since you have the user requirements anyhow.

Comment 23

[timeless]

this would presumably also apply to bugzilla.webkit.org ...

Comment 24

[Tiago Mello [:timello]]

Comment on attachment 523973 [details] [diff] [review]
v5.1

For some reason when I applied the patch I got:

undef error - Can't use string ("") as an ARRAY ref while "strict refs" in use at Bugzilla/Template.pm line 865.

When trying to access query.cgi

Something wrong is being passed to Bugzilla::Template->lsearch

Comment 25

[Tiago Mello [:timello]]

(In reply to comment #24)
> For some reason when I applied the patch I got:
> 
> undef error - Can't use string ("") as an ARRAY ref while "strict refs" in use
> at Bugzilla/Template.pm line 865.
> 
> When trying to access query.cgi

Actually, it happens when you visit the simple search page.

Comment 26

[Tiago Mello [:timello]]

Comment on attachment 523973 [details] [diff] [review]
v5.1

Ok, another thing:

After building a search with more than one criteria and press "back" in the bug list page, it's unable to recreate the custom search. That is a problem because I may want to fix/change it and hit search again.

Comment 27

[Max Kanat-Alexander]

(In reply to comment #26)
> After building a search with more than one criteria and press "back" in the bug
> list page, it's unable to recreate the custom search. That is a problem because
> I may want to fix/change it and hit search again.

  FWIW, that won't happen with Firefox. There isn't really too much of a way to work around this if your browser doesn't cache JavaScript state along with the back/forward history. You *can* do "Edit Search" properly still, though.

Comment 28

[Tiago Mello [:timello]]

(In reply to comment #27)
> (In reply to comment #26)
> > After building a search with more than one criteria and press "back" in the bug
> > list page, it's unable to recreate the custom search. That is a problem because
> > I may want to fix/change it and hit search again.
> 
>   FWIW, that won't happen with Firefox. There isn't really too much of a way to
> work around this if your browser doesn't cache JavaScript state along with the
> back/forward history. You *can* do "Edit Search" properly still, though.

Actually, I tested with firefox 4.

Comment 29

[Max Kanat-Alexander]

(In reply to comment #28)
> Actually, I tested with firefox 4.

  Oh, hmm, okay. I'll look into that, then.

Comment 30

[Frédéric Buclin]

(In reply to comment #22)
> are probably more.) Let's take a huge over-estimate and say that there are 50
> people who have to turn off JS in this one Bugzilla for security reasons.

You have no data about that. It's a simple guess. There are some users at GCC who use Lynx to access Bugzilla. And AFAIK, Lynx doesn't support JS.

Comment 31

[Max Kanat-Alexander]

(In reply to comment #30)
> You have no data about that. It's a simple guess. There are some users at GCC
> who use Lynx to access Bugzilla. And AFAIK, Lynx doesn't support JS.

  We definitely do not officially support Lynx.

  What if it were 500 users out of 2 million? That's ten times my estimate. That would up it to a whopping %0.025 of our userbase. What if it were FIVE THOUSAND users? 0.25%. There is simply no justification there for requiring that Bugzilla operate properly without JS, except in very specific circumstances for which that group of people could write an extension if they so desired. This is one of the great reasons to have an extension system, so that tiny minority user groups can be well-supported.

  I'm perfectly happy to make Bugzilla operate without JS in situations where it improves the architecture. In fact, I've been a tremendous proponent of graceful degradation in all my UI reviews. But when supporting non-JS clients adds significant complexity to both the UI and the code (as is the case here) it's really not worth it.

Comment 32

[Max Kanat-Alexander]

(In reply to comment #26)
> After building a search with more than one criteria and press "back" in the bug
> list page, it's unable to recreate the custom search. That is a problem because
> I may want to fix/change it and hit search again.

  Okay. I've been investigating this and I can't quite track down the cause. I think it may actually be a separate bug--bug 584742 or bug 612574 (probably the second one). There's a chance it will be fixed by bug 636416. I think we should work on fixing those bugs and probably mark them 4.2 blockers. But this Search UI stuff is also a 4.2 blocker, so it should go in before them, even though it will have this problem until we fix those bugs.

Comment 33

[Max Kanat-Alexander]

Attachment: v6

Okay, I've fixed the simple-search error.

Comment 34

[Max Kanat-Alexander]

Attachment: v7

There was an HTML error in the previous patch (that was also there in the older versions of it, I believe).

Comment 35

[Tiago Mello [:timello]]

Comment on attachment 525355 [details] [diff] [review]
v7

Ok, here are some comments regarding the UI:

There is a bug in the UI: Start adding a criteria... for instance... Bug ID is equal to 3... and then click in the open parentheses... it cleans the value field... that happens for every last added criteria... but it works if you click in the parentheses first.

Also... it would be nice to have a button to reset the custom search and it's more intuitive than reload the entire page. Maybe a button to remove a specific criteria... imagine you are doing a very complex search... and you have a bunch of combinations and want to remove just one that you decide is not needed...

Speaking of very complex search... it looks scary when you have a lot of criterias... but I can't think how to improve it for now... also that can done later... I think.

I think a help link [?] is something good to have there too. It may not be intuitive for everyone how to use the custom search. But it's ok if we intend to do the polishing later.

I think we need a title for the parentheses buttons as we have for the + (Add a new row)

I haven't started looking at the code yet.

Comment 36

[Max Kanat-Alexander]

(In reply to comment #35)
> There is a bug in the UI: Start adding a criteria... for instance... Bug ID
> is equal to 3... and then click in the open parentheses... it cleans the
> value field... that happens for every last added criteria... but it works if
> you click in the parentheses first.

  Okay, found and fixed that! _cs_fix_ids was resetting the values.

> Maybe a button to remove a specific criteria...

  Yeah, I'm planning to add that in a future patch. :-)

> Speaking of very complex search... it looks scary when you have a lot of
> criterias... but I can't think how to improve it for now... also that can
> done later... I think.

  I'm going to make the system hide Not and the parens by default, and only show them if you click a link (which will be remembered by TUI).

> I think a help link [?] is something good to have there too. It may not be
> intuitive for everyone how to use the custom search. But it's ok if we
> intend to do the polishing later.

  That's a good idea. I've also added those titles for the buttons like you suggested, which should help.

Comment 37

[Max Kanat-Alexander]

Attachment: v8

Comment 38

[Max Kanat-Alexander]

Attachment: v9

I checked in the Search.pm part as a checkin fix for test failures caused by bug 656994, which was expecting the j_top parameter to already be supported. (This is okay because I own Search.pm right now--I just hadn't checked in that change before because it was only one line.)

Comment 39

[Tiago Mello [:timello]]

Comment on attachment 532337 [details] [diff] [review]
v9

Review of attachment 532337 [details] [diff] [review]:
-----------------------------------------------------------------

Try to do search like... bug is equal 1 OR bug is equal 2... then hit search... then clink in the 'edit search'. You should have only the search criteria you added before... and be able to get the same result just by hitting search again without editing anything. However, it adds a new blank row and when you hit search... you get no results.

::: Bugzilla/Install/Requirements.pm
@@ +66,4 @@
>  # parameters. Note that on Debian and Gentoo, there is an "apache2ctl",
>  # but it takes different parameters on each of those two distros, so we
>  # don't use apache2ctl.
> +use constant APACHE => qw(apache2ctl apachectl httpd apache2 apache);

The comment above should be updated since now we have apache2ctl here. right?

Comment 40

[Max Kanat-Alexander]

(In reply to comment #39)
> The comment above should be updated since now we have apache2ctl here. right?

  Oh, that was my error! That piece of the patch should not have been there.

Comment 41

[Max Kanat-Alexander]

Attachment: v10

Thanks for the review! It was a one-line change to fix the fact that OR became AND when you did Edit Search. :-)

The extra row is intentional, actually. It won't make the search URL longer or anything. Did it bother you or surprise you, as a user?

Comment 42

[Max Kanat-Alexander]

Attachment: v11

New version with better variable names and comments in the JS, as requested by timello on IRC.

Comment 43

[Tiago Mello [:timello]]

Comment on attachment 536169 [details] [diff] [review]
v11

It looks good!

We still need:

1. Remove the new line when editing a search;
2. Allow editing old saved searches

Comment 44

[Max Kanat-Alexander]

Woohoo! Thanks!

I'll file bugs for the other two things.

Comment 45

[Max Kanat-Alexander]

Committing to: bzr+ssh://bzr.mozilla.org/bugzilla/trunk/
modified buglist.cgi
modified query.cgi
modified Bugzilla/CGI.pm
added js/custom-search.js
modified template/en/default/filterexceptions.pl                               
modified template/en/default/search/boolean-charts.html.tmpl
modified template/en/default/search/form.html.tmpl                             
modified template/en/default/search/search-specific.html.tmpl
modified template/en/default/search/type-select.html.tmpl
Committed revision 7831.