Bugzilla should produce JS version of Components/Products etc.

RESOLVED FIXED in Bugzilla 2.18

Status

()

task
P3
normal
RESOLVED FIXED
18 years ago
7 years ago

People

(Reporter: gerv, Assigned: myk)

Tracking

(Blocks 2 bugs)

2.11
Bugzilla 2.18
x86
All
Dependency tree / graph
Bug Flags:
approval +

Details

Attachments

(2 attachments, 11 obsolete attachments)

11.32 KB, patch
gerv
: review+
Details | Diff | Splinter Review
2.09 KB, patch
gerv
: review+
Details | Diff | Splinter Review
We have continual trouble updating the Bugzilla Helper to keep in sync with 
component changes - the results of falling behind are scary messages about 
"database corruption". What we really need is for Bugzilla to generate a .js 
file with data structures in to hold this information. This could then be used 
by the Bugzilla query form and things like Bugzilla Helper.

The query form already has something like this in it; splitting them off into a 
separate file shouldn't be too hard. Filing this bug to keep track of the idea, 
but it's probably 2.16 at the earliest. :-(

Gerv
Since AFAICS this is an independent new feature, and it isn't urgent to go in
the main distribution, you should feel free to implement it in the 2.14
timeline.  =)
Target Milestone: --- → Bugzilla 2.16
Assignee

Comment 3

18 years ago
The attached patch recreates a file called queryarrays.js in the bugzilla
directory whenever a component, version, or milestone is added, updated, or
deleted.  It adds functions to globals.pl for doing this, patches
editcomponents.cgi, editversions.cgi, and editmilestones.cgi so those functions
get called on any change, and modifies query.cgi to use the functions in
globals.pl rather than doing the job itself.
Status: NEW → ASSIGNED
my main concern was going to be to make sure the static file didn't wind up with 
private groups in it, but looks like you got that covered. :-)  Has my seal of 
approval for that portion, however I'm not a javascript expert, so I won't r= 
this.

Excellent :-) I'll transition Bugzilla Helper to use this when I get back from Easter break, and we'll have no more out-of-sync woes :-)

CCing Andreas in case he has a use for this in a search tool.

Gerv

Comment 6

18 years ago
nice! two comments tho.

make checksetup.pl create the initial queryarrays.js.

add the product and component descriptions too. perhaps in a separate file.
That would allow the helper be a lot more helpful.

Comment 7

18 years ago
It appears that the JavaScript (and the perl that generates it) was pretty much
copied from query.cgi into the new sub in globals.pl, so there shouldn't be any
problems there.  I applied that patch on my local install and everything
functioned as expected (including the recent fix for SELECTing the Target
Milestone, Version and Component.  My only comment is that it doesn't seem
necessary to declare "my ($c, $v, $m, $p);" instead of using "foreach my $c",
etc. (athough that is admittedle minor and the same as what's happening now).

I think making checksetup.pl create the JS files may be more work than it's
worth.  checksetup.pl doesn't use globals.pl so it would mean maintaining two
copies of the JS generating code, and it is done automagically evertime a
component is changed (including initial owner, etc.).

Also, reassigning to myk as I think that's what he intended to do when he
clicked "accept bug".
Assignee: tara → myk
Status: ASSIGNED → NEW
Assignee

Comment 9

18 years ago
Changes in the new patch:

1. Added product and component descriptions;
2. Implemented Jake's suggestion re: "my ($p, $c, $v, $m)";
3. Changed the bz_components, bz_versions, and bz_milestones arrays to hashes to
reflect how they are actually being used.


Status: NEW → ASSIGNED

Comment 10

18 years ago
+  $str =~ s/\r\n/\\n/g;
+  $str =~ s/\n\r/\\n/g;
+  $str =~ s/\r/\\n/g;
+  $str =~ s/\n/\\n/g;

Won't that turn \r\n into \\\n?
Assignee

Comment 11

18 years ago
> + $str =~ s/\r\n/\\n/g;
> + $str =~ s/\n\r/\\n/g;
> + $str =~ s/\r/\\n/g;
> + $str =~ s/\n/\\n/g;
> 
> Won't that turn \r\n into \\\n?

I don't think so.  These regular expressions turn carriage return and new line
characters into the literal sequence backslash-n.  They don't find any instances
of that literal sequence itself, however.




Comment 12

18 years ago
Also, 'queryarrays.js' should get added to .cvsignore so CVS doesn't complain
about it.
Nice. What I'm interested in for QuickSearch is mainly keywords, however.
Have a look at localconfig.js, e.g. in attachment 26160 [details].
Priority: -- → P3
Keywords: patch, review
moving to new product = Bugzilla
Component: Bugzilla → Administration
Product: Webtools → Bugzilla
Version: Bugzilla 2.11 → 2.11

Comment 16

18 years ago
Myk, this looks great, and this is actually what bug 96983 proposes to do (I'd
mark a dupe, actually). If you resync this patch, remember to take into account
the changes done for bug 96534 that make the JS much simpler.
Blocks: 99955
afranke, was this what had to be done to get customised resolutions and
quicksearch to work together?
Matty: yes, this should solve it. (Note: I'm not reading bugmail anymore, so
mail me if you want me to comment.)

Myk: Great work. Thanks a lot.
OK, guys, it would be nice if you could generate a list of resolutions.  I
suggest it be called queryable_resolutions or some such.  It won't show all the
resolutions in the system - inactive resolutions on no bugs will not show.

Once this is done we can fix up quicksearch to work 100% with customised
resolutions.
Comment on attachment 54983 [details] [diff] [review]
patch v1: templatized version w/js and rdf outputs (apply w/-p0 option to patch)

Looks pretty good to me; the code for working out the valid products etc. is
shared with 
enter_bug.cgi and/or query.cgi - this should be factored out into globals.pl or
something.

Is now an appropriate time to do that?

Gerv
We are currently trying to wrap up Bugzilla 2.16.  We are now close enough to
release time that anything that wasn't already ranked at P1 isn't going to make
the cut.  Thus this is being retargetted at 2.18.  If you strongly disagree with
this retargetting, please comment, however, be aware that we only have about 2
weeks left to review and test anything at this point, and we intend to devote
this time to the remaining bugs that were designated as release blockers.
Target Milestone: Bugzilla 2.16 → Bugzilla 2.18
Assignee

Updated

18 years ago
Blocks: 123030
Assignee

Updated

18 years ago
Blocks: 128613
Assignee

Updated

17 years ago
Blocks: 132181
No longer blocks: 99955
I imagine there's nothing stopping this going in now, however, is there any use
for it anymore?

Bugzilla Helper will become an enter_bug template that doesn't even use JS
anymore as a part of bug #133559, whereas QuickSearch will become Perl as a part
of bug #70907.
I'd say it's not worth it, myself. Does anyone have a use for this?

Gerv
Myk marked this as blocking bug 132181 (the 'upgrade bmo' bug), so he presumably
did want this.
If it's not much work, I'd be interested in this because of the keywords list
for quicksearch. I may try to maintain the javascript version for a while even
when the perl version is used by default, if time permits. 
Assignee

Comment 30

17 years ago
This is worthwhile because it allows developers to build external Bugzilla
clients.  QuickSearch may no longer need it, but other clients will.
Assignee

Comment 31

17 years ago
Posted patch patch v2: unrotted (obsolete) — Splinter Review
Attachment #30826 - Attachment is obsolete: true
Attachment #31030 - Attachment is obsolete: true
Attachment #31179 - Attachment is obsolete: true
Attachment #54608 - Attachment is obsolete: true
Attachment #54609 - Attachment is obsolete: true
Attachment #54610 - Attachment is obsolete: true
Attachment #54983 - Attachment is obsolete: true
Assignee

Updated

17 years ago
No longer blocks: 132181

Updated

17 years ago
Severity: normal → enhancement
Assignee

Comment 32

17 years ago
Also "--makeall" killed since the "getformat" check-in killed code it relied
on.
Attachment #84932 - Attachment is obsolete: true
Assignee

Comment 33

17 years ago
Attachment #104221 - Attachment is obsolete: true
- I'm not sure if your use vars() will work; the form which works elsewhere is:
use vars qw(@legal_priority, @legel_severity, ...);

- Can we use template comments in the JS to cut down on file size?

- Are follow-up bugs going to be required to convert some users of this data to
use config.cgi?

- Use of | instead of FILTER - one of the reasons for using TT is readability
and customisability by admins. Should we be using the English versions of TT
keywords always, both for that reason and consistency with everywhere else?

- Some of the comments (e.g. the one for keywords) seem a bit odd. They either
don't apply any more, or are very QuickSearch-specific, even on entries which
aren't QuickSearch-specific. This is probably not the right place for
documentation on how QuickSearch works. :-)

- Should the values of component_exceptions and product_exceptions be
b.m.o.-specific customisations?

- When custom fields turns up, presumably these files will have to automatically
export the configurations and options for fields with arbitrary names. Also,
currently-existing fields may get converted to custom ones. If this is the case,
then I would suggest that the RDF names particularly, and possibly other
variable names in the JS as well, be the DB column names for that field (because
I suspect that this will be equal to the canonical internal name for the field
in the custom fields system.) Yes, this does mean:

+  <bz:op_sys>
+    <Seq>
+      [% FOREACH item = operatingsystems %]
+        <li>[% item FILTER html %]</li>
+      [% END %]
+    </Seq>
+  </bz:op_sys>

which is singular rather than plural, but I think that the forward-compatibility
 might be worth it.

Gerv

Updated

17 years ago
Attachment #104226 - Flags: review?
Assignee

Comment 35

16 years ago
>- I'm not sure if your use vars() will work; the form which works elsewhere
is:
>use vars qw(@legal_priority, @legel_severity, ...);

It worked in my testing, but I've fixed it to use the form you suggest.

>- Can we use template comments in the JS to cut down on file size?

Per your comment below about the comments being odd, I deleted most of them. 
I'm not sure it's worth making what remains into template comments, but tell me
if you think I'm wrong.

>- Are follow-up bugs going to be required to convert some users of this data
to
>use config.cgi?

No one should have to change the way they use localconfig.js (if they do it's a
bug), although some variable names in it have been deprecated and notice served
that they'll go away in the future.  localconfig.rdf has never been in a
released version of Bugzilla, so changes to it are an installation-specific
(f.e. b.m.o) problem.  Note bug 200837, however, which is discussed further
below.

>- Use of | instead of FILTER

Ok, done.

>- Some of the comments (e.g. the one for keywords) seem a bit odd.

Right.	I've removed the ones that were QuickSearch-specific (i.e. most of them
:->).

>- Should the values of component_exceptions and product_exceptions be
>b.m.o.-specific customisations?

Yeah, good point.  I've commented them out but left them in there to serve as
examples.

>- When custom fields turns up, presumably these files will have to
automatically
>export the configurations and options for fields with arbitrary names. Also,
>currently-existing fields may get converted to custom ones. If this is the
case,
>then I would suggest that the RDF names particularly, and possibly other
>variable names in the JS as well, be the DB column names for that field
(because
>I suspect that this will be equal to the canonical internal name for the field

>in the custom fields system.)

Right you are, but this calls for changing the bass-ackwards database names
more than it does changing the good names, especially now that we're
advertising interfaces to the world instead of just using them internally. 
That's why I translated names when I rewrote a large hunk of buglist.cgi, a
change that got reversed in bug 176461 and which I'd like back (one way or
another) in bug 200837.
Attachment #104226 - Attachment is obsolete: true
One thing: can we add "version" and "maintainer" to this CGI? 

There's a bug on this somewhere, but I think it would be really handy to be able
to query Bugzilla for these two things; then, in the future, we can turn a "list
of public Bugzillas" (such as the one we are constructing) into a "list of
people to mail about a major security issue" really easily.

I'll review this properly later.

Gerv
Attachment #119575 - Flags: review?(gerv)
Comment on attachment 119575 [details] [diff] [review]
patch v5: unrotted, Gerv's comments addressed

> Index: config.cgi
> ===================================================================
> +# Pass a bunch of Bugzilla configuration to the templates.
> +$vars->{'priorities'}  = \@::legal_priority;
> +$vars->{'severities'}  = \@::legal_severity;
> +$vars->{'platforms'}   = \@::legal_platform;
> +$vars->{'op_syses'}    = \@::legal_opsys;
In other templates, we actually tend to use the singular (i.e. the field name)
for these variables. E.g. query.cgi:

$vars->{'rep_platform'} = \@::legal_platform;
$vars->{'op_sys'} = \@::legal_opsys;
$vars->{'priority'} = \@::legal_priority;
$vars->{'bug_severity'} = \@::legal_severity;

It may not be gramatically excellent, but I think there's value in consistency.

> Index: globals.pl
> ===================================================================
>  # This function returns an alphabetical list of product names to which
> -# the user can enter bugs.
> +# the user can enter bugs.  If the $by_id parameter is true, also retrieves IDs
> +# and pushes them onto the list as id, name [, id, name...] for easy slurping
> +# into a hash by the calling code.
>  sub GetSelectableProducts {

Perhaps a wrapper function GetSelectableProductsByID to make calling code
clearer? After all, there's a GetSelectableProductsHash(). Or call it as
GetSelectableProducts("by_id")?

> Index: template/en/default/config.rdf.tmpl
> ===================================================================
> +  <bz:status>
> +    <Seq>
> +      [% FOREACH item = statuses %]
> +        <li>[% item FILTER html %]</li>
> +      [% END %]
> +    </Seq>
> +  </bz:status>
> +
> +  <bz:status_open>

Surely whether it's an open or closed status should be an attribute on the
<bz:status> tag?

> +  <bz:resolution>
> +    <Seq>
> +      [% FOREACH item = resolutions %]
> +        <li>[% item FILTER html %]</li>

Do we not have a FILTER xml, then?

> +            [% IF Param('usetargetmilestone') %]
> +              <bz:target_milestones>

<bz:milestones>? :-)

Can I also repeat my request for the addition of the maintainer and the
Bugzilla version to either the JS or RDF formats?

Nothing particularly serious here, so r=gerv if you implement all these tweaks.
:-)

Gerv
Attachment #119575 - Flags: review?(gerv) → review+
Assignee

Comment 38

16 years ago
>In other templates, we actually tend to use the singular (i.e. the field name)

>for these variables.

>It may not be gramatically excellent, but I think there's value in
consistency.

I don't see much consistency, since the legal_* variables delete sillinesses
like the "rep_" in "rep_platform" and have varying plurality (i.e.
"legal_platform" but "legal_products"), and I think sensibleness is more
important than consistency, but, ok, I've made them "consistent".

>Perhaps a wrapper function GetSelectableProductsByID to make calling code
>clearer?

Oops, sorry, that was another patch; removed.

>Surely whether it's an open or closed status should be an attribute on the
><bz:status> tag?

No, actually, because <bz:status> and <bz:status type="open"> both refer to the
same thing when placed inside the <bz:installation> object, so saying this is
akin to just saying <bz:status type="open">.

We can, however, add an attribute to each individual status that indicates
whether it is open or closed.  I'm not sure whether this is more useful than
the current approach; I think we should wait for some consumer of this
interface to want it that way, if then.

>Do we not have a FILTER xml, then?

No, but we don't need one, since the HTML filter does just what we want (HTML
being an XML language in its most recent incarnations).

><bz:milestones>? :-)

Not per our "standard" database column name, but we should change that.

>Can I also repeat my request for the addition of the maintainer and the
>Bugzilla version to either the JS or RDF formats?

Yes, done.

>Nothing particularly serious here, so r=gerv if you implement all these
tweaks.

Here ya go.
Assignee

Updated

16 years ago
Attachment #119575 - Attachment is obsolete: true
Assignee

Updated

16 years ago
Attachment #122048 - Flags: review?(gerv)
Comment on attachment 122048 [details] [diff] [review]
patch v6: more of Gerv's comments addressed

r=gerv.

Gerv
Attachment #122048 - Flags: review?(gerv) → review+
Assignee

Comment 40

16 years ago
RCS file: /cvsroot/mozilla/webtools/bugzilla/config.cgi,v
done
Checking in config.cgi;
/cvsroot/mozilla/webtools/bugzilla/config.cgi,v  <--  config.cgi
initial revision: 1.1
done
RCS file: /cvsroot/mozilla/webtools/bugzilla/template/en/default/config.js.tmpl,v
done
Checking in template/en/default/config.js.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/config.js.tmpl,v  <-- 
config.js.tmpl
initial revision: 1.1
done
RCS file: /cvsroot/mozilla/webtools/bugzilla/template/en/default/config.rdf.tmpl,v
done
Checking in template/en/default/config.rdf.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/config.rdf.tmpl,v  <-- 
config.rdf.tmpl
initial revision: 1.1
done
Status: ASSIGNED → RESOLVED
Last Resolved: 16 years ago
Flags: approval+
Resolution: --- → FIXED

Comment 41

16 years ago
> #!/usr/bonsaitools/bin/perl -w

Shouldn't this be rather
  #!/usr/bin/perl -wT

(Path change + -T)?
Assignee

Comment 43

16 years ago
Comment on attachment 122122 [details] [diff] [review]
clean-up patch: uses correct Perl path and documents experimental nature of interface

This is a clean-up patch that makes config.cgi use /usr/bin/perl instead of
/usr/bonsaitools/bin/perl and adds taint checking.

I also added some comments about the output being experimental and subject to
change so that people are forewarned (since it's likely to happen what with bug
200837 and the fact that this interface is relatively new and untested).
Attachment #122122 - Flags: review?(gerv)
Comment on attachment 122122 [details] [diff] [review]
clean-up patch: uses correct Perl path and documents experimental nature of interface

r=gerv.

Gerv
Attachment #122122 - Flags: review?(gerv) → review+
Assignee

Comment 45

16 years ago
Clean-up patch checked in:

Checking in config.cgi;
/cvsroot/mozilla/webtools/bugzilla/config.cgi,v  <--  config.cgi
new revision: 1.2; previous revision: 1.1
done
Checking in template/en/default/config.js.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/config.js.tmpl,v  <-- 
config.js.tmpl
new revision: 1.3; previous revision: 1.2
done
Checking in template/en/default/config.rdf.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/config.rdf.tmpl,v  <-- 
config.rdf.tmpl
new revision: 1.3; previous revision: 1.2
done

Updated

16 years ago
Attachment #104226 - Flags: review?
QA Contact: matty_is_a_geek → default-qa
You need to log in before you can comment on or make changes to this bug.