Closed Bug 518293 Opened 15 years ago Closed 14 years ago

Improve and Simplify the QuickSearch Documentation

Categories

(Bugzilla :: Query/Bug List, enhancement)

enhancement
Not set
normal

Tracking

()

RESOLVED FIXED
Bugzilla 3.6

People

(Reporter: mkanat, Assigned: mkanat)

References

Details

Attachments

(2 files, 4 obsolete files)

After bug 518024, the quicksearch docs will need to be changed, to incorporate the fact that it now accepts all Bugzilla fields. Also, it could use some general modernization and simplification.
Attached patch Work In Progress (obsolete) — Splinter Review
Assignee: query-and-buglist → mkanat
Status: NEW → ASSIGNED
Does it make sense to add quicksearch_field_names in Template.pm when you only need it in the documentation? You could copy this in page.cgi.
I'm going to base the documentation on how QS will work after bug 518459.
Depends on: 518459
Attached patch v1 (obsolete) — Splinter Review
Okay, this is a complete re-write of the QuickSearch docs--hopefully one that should be sensible and accessible to all Bugzilla users, not just the advanced ones! :-) I'd like to see more people using QuickSearch, I think, particularly now that we've worked on simplifying it and since we're going to integrate it into fulltext search.

I also changed the copyright and contributor statement in the file, not to discredit anybody (in fact, I really don't like doing that), but because there was no Initial Developer statement in the file, and "N.N.", the first listed contributor, I can't figure out who that would be in order to credit them with an original copyright. Since I re-wrote the entire contents of the file and didn't retain any of the old content (other than the quicksearch form, which is identical to the quicksearch form elsewhere in the code) I just took the copyright and Initial Developer credit, which solves the whole problem.
Attachment #402297 - Attachment is obsolete: true
Attachment #402475 - Flags: review?(dkl)
Attachment #402475 - Flags: review?(colin.ogilvie)
(In reply to comment #4)
> I also changed the copyright and contributor statement in the file, not to
> discredit anybody (in fact, I really don't like doing that), but because there
> was no Initial Developer statement in the file, and "N.N.", the first listed
> contributor, I can't figure out who that would be in order to credit them with
> an original copyright. Since I re-wrote the entire contents of the file and
> didn't retain any of the old content (other than the quicksearch form, which is
> identical to the quicksearch form elsewhere in the code) I just took the
> copyright and Initial Developer credit, which solves the whole problem.

You should still list wurblzap as a contributor. Completely altering a file doesn't give you the right to remove him from the contributor list, AFAIK. And probably he will know who N.N. is.
(In reply to comment #5)
> You should still list wurblzap as a contributor. 

  I'm not sure that anything he contributed remains, but I'm willing to list him if he's responsible for the quicksearch form, which I suspect he is.

> Completely altering a file
> doesn't give you the right to remove him from the contributor list, AFAIK.

  Mmm, if the file is entirely my original content (which it is, theoretically, except for the quicksearch form) then actually I can, if I want to. But I'd be happy to put Wurblzap back if there is code in the quicksearch form that's his, which in fact I suspect there is. I can do it on checkin, if it passes review.
In any case, the point of modifying the license block (an incredibly trivial part of this otherwise very large patch) was to find a way to comply with the MPL, and since I had the opportunity to do so, I did. I really am fine with adding anybody back to the Contributor list who contributed any significant amount of code that remains in the file.
> except for the quicksearch form) then actually I can, if I want to. But I'd be
> happy to put Wurblzap back if there is code in the quicksearch form that's his,
> which in fact I suspect there is. I can do it on checkin, if it passes review.

There is no significant original code of mine in there; in bug 70907, I simply ported Bugzilla's old static quicksearch.html to template/en/default/pages/quicksearch.html.tmpl, replaced the js stuff by the necessities to do it server-side, and I did some minor markup cleanup. Same for quicksearchhack. As far as I can see, all that's left is <form name="f" action="buglist.cgi" method="get">, which I don't really cling to.

Oh, and I don't think you should drop the “Load Search Form” functionality.
Comment on attachment 402475 [details] [diff] [review]
v1

Only thing that jumped out at me from reading the patch was a bit I'd reword...

>Index: template/en/default/pages/quicksearch.html.tmpl
>+  <li>[% terms.Bugzilla %] searches not just for the exact word you put in,
>+    but also for any word that <strong>contains</strong> the word you put in.

Bugzilla does not just search for the exact word that you put in, but also for any word that contains that word.

>+    So, for example, searching for "cat" would also find [% terms.bugs %]
>+    that contain "<strong>cat</strong>ch" and 
>+    "certifi<strong>cat</strong>e".</li>

So, for example, searching for "cat" would also find bugs that contain it - for example, a bug mentioning "<strong>cat</strong>ch" or "certifi<strong>cat</strong>e".

I'll take a quick look later at it in HTML format later.
Attachment #402475 - Flags: review?(dkl) → review?(LpSolit)
Blocks: 527641
Blocks: 448609
Blocks: 529201
This is now a blocker since the stuff that changed QuickSearch was checked in, but these docs that explain the new system haven't been.
Flags: blocking3.6+
Attachment #402475 - Flags: review?(colin.ogilvie)
Attachment #402475 - Flags: review?(LpSolit)
Attachment #402475 - Flags: review-
Comment on attachment 402475 [details] [diff] [review]
v1

bitrotten in page.cgi (the hook name changed) and user-error.html.tmpl (doesn't match what landed in the blocker)
Whiteboard: [needs new patch]
Attached patch v2 (obsolete) — Splinter Review
Okay, this fixes the bitrot and also modifies the language as Colin suggested.

I also added [?] link next to the header/footer quicksearch, that links to this page.
Attachment #402475 - Attachment is obsolete: true
Attachment #420427 - Flags: review?(LpSolit)
Whiteboard: [needs new patch]
Attached image Full-Page Screenshot
Here's a screenshot of the full page for anybody who wants to read the docs without applying the patch.
Attached patch v3 (obsolete) — Splinter Review
Noticed that &emdash; doesn't work--it's &mdash;.
Attachment #420427 - Attachment is obsolete: true
Attachment #420436 - Flags: review?(LpSolit)
Attachment #420427 - Flags: review?(LpSolit)
Comment on attachment 420436 [details] [diff] [review]
v3

>Index: skins/contrib/Dusk/.cvsignore

>+page.css
> params.css
> release-notes.css

Shouldn't this last item be removed now that release-notes.css no longer exists? Also, shouldn't you remove release-notes.css from contrib/Dusk/ and custom/ as they are now irrelevant?



>Index: template/en/default/pages/quicksearch.html.tmpl

This template doesn't pass tests 8 and 9:

#   Failed test '(en/default) template/en/default/pages/quicksearch.html.tmpl has unfiltered directives:                                                                            
#   43: field_descs.product
#   44:+ field_descs.component
#   45:+ field_descs.keywords
#   46:+ field_descs.short_desc
#   47: field_descs.status_whiteboard
#   48: field_descs.longdesc
#   71: field_descs.longdesc
#   105: first_field.name
#   106: first_field.name.replace('^cf_')
# --ERROR'
#   at t/008filter.t line 132.


not ok 98 - template/en/default/pages/quicksearch.html.tmpl contains invalid bare words (e.g. 'bug') --WARNING
224:  with the highest priority.
        "P2" means the second-highest priority, and so on.
        <p>Searching for "<strong>P1-P3</strong>" will find bugs in any of the
          three highest priorities, and so on.</p>
#   at t/009bugwords.t line 90.



>+<ul class="qs_help">
>+  <li>If you just put a word or series of words in the search box, 
>+    [%+ terms.Bugzilla %] will search the [% field_descs.product %],
>+    [%+ field_descs.component %], 
>+    [%+ IF use_keywords %][%+ field_descs.keywords %],[% END %]
>+    [%+ field_descs.short_desc %], 
>+    [%+ IF Param('usestatuswhiteboard') %][% field_descs.status_whiteboard %],[% END %]
>+    and [% field_descs.longdesc %] fields for your word or words.</li>

It always looks for the status whiteboard, independently of the 'usestatuswhiteboard' parameter. Also, all these fields must be HTML filtered, see above.


>+  <li>Typing just a <strong>number</strong> in the search box will take
>+    you directly to the [% terms.bug %] with that id. 

Nit: I prefer to read id uppercase. Being a non-native english speaker, I not immediately think ID, but 'it' with a typo. :)


>+      Also, just typing the <strong>alias</strong> of [% terms.abug %]
>+      will take you to do that [% terms.bug %].

s/do// ? Or I don't understand the meaning of this sentence. :)


>+    "<strong>cat</strong>ch" or "certifi<strong>cat</strong>e". It will
>+    not find partial words in the [% field_descs.longdesc %] field 
>+    though&mdash;only full words are matched, there.</li>

Same for keywords, unfortunately.


>+[% USE Bugzilla %]
>+[% IF Bugzilla.active_custom_fields.size %]

Move [% USE Bugzilla %] at the top of the file, so that we don't inadvertendly duplicate it if we later need another Bugzilla.foo earlier in the code and don't notice this one.


>+    <kbd>cf_</kbd>. So for example, <kbd>[% first_field.name %]</kbd> can be
>+    referred to as <kbd>[% first_field.name.replace('^cf_') %]</kbd>,

Must be HTML filtered too. Cool idea to take a real custom field as an example! :)


>+[% FOREACH field = quicksearch_field_names.keys %]
>+  [% description = field_descs.$field || bug_fields.${field}.description %]

field_descs.none.tmpl already does this for you, so field_descs.$field will always be defined.


>+  [% values = quicksearch_field_names.${field} %]
>+  [% field_table.$description = quicksearch_field_names.${field} %]

'values' is unused. I'm pretty sure this makes this FOREACH loop useless.


>+          [% FOREACH nickname = field_table.$desc %]
>+            <kbd>[% nickname FILTER html %]</kbd>
>+            [% ",&nbsp; " UNLESS loop.last %]
>+          [% END %]

Nit: could be done as <kbd>[% field_table.$desc.join(', ') FILTER html %]</kbd>.


>+        making <kbd>FIX</kbd> the first word of your search will find all
>+        [% terms.bugs %] with a resolution of <kbd>FIXED</kbd> .

Must be [%+ terms.bugs %]


>+      <td class="field_nickname">"<strong>P1</strong>" (as a word anywhere in
>+        the search) means "find [% terms.bugs %] with the highest priority. 

Really? When I type P1 alone, all bugs are returned and this string is ignored. Either the logic behind it has been removed, or there is a bug in the QS code.
Attachment #420436 - Flags: review?(LpSolit) → review-
Whiteboard: [new patch needed]
(In reply to comment #15)
> Shouldn't this last item be removed now that release-notes.css no longer
> exists?

  Ah, yeah, done.

> Also, shouldn't you remove release-notes.css from contrib/Dusk/ and
> custom/ as they are now irrelevant?

  Mmm, they might have been modified, as unlikely as that seems.

> It always looks for the status whiteboard, independently of the
> 'usestatuswhiteboard' parameter.

  Yes, but if the parameter isn't on, there's a pretty good chance that users won't have any idea what that field is.

> 'values' is unused. I'm pretty sure this makes this FOREACH loop useless.

  Ah, yeah, "values" was unused, but the loop is still important so that we can create a data structure where we can sort by the description.

> >+          [% FOREACH nickname = field_table.$desc %]
> >+            <kbd>[% nickname FILTER html %]</kbd>
> >+            [% ",&nbsp; " UNLESS loop.last %]
> >+          [% END %]
> 
> Nit: could be done as <kbd>[% field_table.$desc.join(', ') FILTER html
> %]</kbd>.

  Ah, yeah, I didn't because I didn't want the commas in the monospace font.

> Really? When I type P1 alone, all bugs are returned and this string is ignored.
> Either the logic behind it has been removed, or there is a bug in the QS code.

  Ah, yes, that would be a bug; it should be filed separately.
Here's a new patch with all comments addressed. I generated this with "bzr diff", so it contains a rename that won't be done automatically if you use "bzr patch" or simply "patch -p0". I renamed release-notes.css to page.css (so now you can see what I actually added, instead of just the whole file).
Attachment #420436 - Attachment is obsolete: true
Attachment #425669 - Flags: review?(LpSolit)
Comment on attachment 425669 [details] [diff] [review]
v4 (contains rename)

r=LpSolit
Attachment #425669 - Flags: review?(LpSolit) → review+
Flags: approval3.6+
Flags: approval+
Committing to: bzr+ssh://bzr.mozilla.org/bugzilla/trunk/                       
modified .bzrignore
modified page.cgi
renamed skins/standard/release-notes.css => skins/standard/page.css
modified template/en/default/global/common-links.html.tmpl
modified template/en/default/global/user-error.html.tmpl
modified template/en/default/pages/quicksearch.html.tmpl
missing template/en/default/pages/quicksearchhack.html.tmpl
modified template/en/default/pages/quicksearchhack.html.tmpl
modified template/en/default/pages/release-notes.html.tmpl
Committed revision 6966.


Committing to: bzr+ssh://bzr.mozilla.org/bugzilla/3.6/                         
modified .bzrignore
modified page.cgi
renamed skins/standard/release-notes.css => skins/standard/page.css
modified template/en/default/global/common-links.html.tmpl
modified template/en/default/global/user-error.html.tmpl
modified template/en/default/pages/quicksearch.html.tmpl
missing template/en/default/pages/quicksearchhack.html.tmpl
modified template/en/default/pages/quicksearchhack.html.tmpl
modified template/en/default/pages/release-notes.html.tmpl                     
Committed revision 6962.
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
Whiteboard: [new patch needed]
Blocks: 498295
Keywords: relnote
Added to the release notes in bug 547466.
Keywords: relnote
You need to log in before you can comment on or make changes to this bug.