316096 - 'everconfirmed' is not included in the DTD and XML files

Status: RESOLVED FIXED

(Bugzilla :: Bug Import/Export & Moving, enhancement)

Description

[Greg Hendricks]

This is important if we want to import bugs and know whether they have been confirmed.

Comment 1

[Greg Hendricks]

Attachment: updated DTD

Comment 2

[Colin Ogilvie [:cso]]

Where do you plan on using this element? You also need to define where it should be in the grand scheme of things in the XML document and I don't see it in http://lxr.mozilla.org/mozilla/source/webtools/bugzilla/bugzilla.dtd

Comment 3

[Olav Vitters]

(In reply to comment #2)
> Where do you plan on using this element?

See the dependency bug 285614.

Comment 4

[Colin Ogilvie [:cso]]

(In reply to comment #3)
> (In reply to comment #2)
> > Where do you plan on using this element?
> 
> See the dependency bug 285614.
> 

That doesn't help - it doesn't show me where in the XML schema its going... and neither does this change to the DTD - it just adds an element definition, but doesn't show where the element is going to go.

Comment 5

[Frédéric Buclin]

This element will go in show.xml.tmpl and will be used by importxml.pl

Comment 6

[Colin Ogilvie [:cso]]

(In reply to comment #5)
> This element will go in show.xml.tmpl and will be used by importxml.pl

There is nothing in the DTD that shows where the new element will feature in the XML structure being returned by show.xml.tmpl that I can see. All this does is define <everconfirmed> as a valid element, but doesn't show where it is going to feature.

Compare it to, say https://bugzilla.mozilla.org/attachment.cgi?id=183403 which adds another element that was missed out - it also updates the list of where the element can appear.

Comment 7

[Frédéric Buclin]

Comment on attachment 202710 [details] [diff] [review]
updated DTD

Colin is right, everconfirmed is missing in the list of elements a bug must have.

Comment 8

[Greg Hendricks]

One of these days, I will remember that you have to add it in two places ;)

Comment 9

[Greg Hendricks]

Attachment: v2

Comment 10

[Colin Ogilvie [:cso]]

Comment on attachment 202994 [details] [diff] [review]
v2

Based on your example XML file, this DTD is wrong.

Comment 11

[Greg Hendricks]

(In reply to comment #10)
> (From update of attachment 202994 [details] [diff] [review] [edit])
> Based on your example XML file, this DTD is wrong.
> 
Can you ellaborate?

The example xml file is probably not the most accurate since it is a hodgepodge of xml outputs that I have used for testing importxml.pl. I have verified that ctype=xml does conform to this version of the dtd as I have submitted it. Indeed, this change is designed to make the dtd conform with what is already output by ctype=xml. 

As far as importxml is concerned, I do not strictly enforce conformance to the dtd since it is possible that the xml is coming from multiple sources. It is important that everconfirmed be included in the dtd as that is used to check which fields are expected. The order is not important to importxml.

Comment 12

[Colin Ogilvie [:cso]]

(In reply to comment #11)
> I have verified that ctype=xml does conform to this version
> of the dtd as I have submitted it.

The version currently on trunk at the moment?

> The order is not important to importxml.

It is important to other things though - the order specified in the DTD is the order that XML parsers should expect to see the XML.

Comment 13

[Frédéric Buclin]

/me is unhappy with the new patch from bug 285614.

First of all because it crashes PostgreSQL, and then because bug 285614 is about importxml.pl *only*. We opened this bug precisely to fix the DTD, which also involves fixing the XML file too (via Bug.pm). So please don't include this part in bug 285614.

FYI, 'everconfirmed' must appear in the GROUP BY clause in Bug.pm.

Comment 14

[Colin Ogilvie [:cso]]

(In reply to comment #13)
> First of all because it crashes PostgreSQL, and then because bug 285614 is
> about importxml.pl *only*. We opened this bug precisely to fix the DTD, which
> also involves fixing the XML file too (via Bug.pm). So please don't include
> this part in bug 285614.

Except, of course, prior to this comment there was no indication that this bug was also meant to fix the XML output. In fact, it was somewhat implied (at least to me reading the comments here) that the other bug already added the output to the XML!

Comment 15

[Greg Hendricks]

Attachment: v3

>Except, of course, prior to this comment there was no indication that this bug
>was also meant to fix the XML output. In fact, it was somewhat implied (at
>least to me reading the comments here) that the other bug already added the
>output to the XML!

This is in part because I myself was confused. I thought everconfirmed was exported. Hopefully this will clear things up.

I am putting this part of the review from bug 285614 here. 

(In reply to bug 285614 comment #37)
> (From update of attachment 207779 [details] [diff] [review] [edit])
> >Index: bugzilla.dtd
> 
> >+<!ELEMENT bug (bug_id, (alias?, creation_ts, short_desc, delta_ts, reporter_accessible, cclist_accessible, classification_id, classification, product, component, version, rep_platform, op_sys, bug_status, resolution?, bug_file_loc?, status_whiteboard?, keywords*, priority, bug_severity, target_milestone?, dependson*, blocked*, (votes, everconfirmed)?, reporter, assigned_to, qa_contact?, cc*, (estimated_time, remaining_time, actual_time, deadline)?, group*, flag*, long_desc*, attachment*)?)>
> 
> Nit: why setting everconfirmed as optional? If this parameter is missing, then
> all open bugs will be marked as UNCO. On the other hand, if everconfirmed = 0,
> show.xml.tmpl will not include it in the XML file. So maybe it's fine like
> this.
> 
Good point. Removed optional part of everconfirmed 



> >Index: Bugzilla/Bug.pm
> 
> >@@ -172,7 +172,7 @@
> 
> >-      delta_ts, COALESCE(SUM(votes.vote_count), 0),
> >+      delta_ts, COALESCE(SUM(votes.vote_count), 0), everconfirmed,
> 
> Adding everconfirmed in the SELECT clause of the SQL query is not enough.
> PostgreSQL requires that 'everconfirmed' also appears in the GROUP BY clause,
> else it crashes (that's what happened to me while testing your patch). That's
> the main reason why I deny review.

Done

Comment 16

[Frédéric Buclin]

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

r=LpSolit

Comment 17

[Frédéric Buclin]

Checking in bugzilla.dtd;
/cvsroot/mozilla/webtools/bugzilla/bugzilla.dtd,v  <--  bugzilla.dtd
new revision: 1.13; previous revision: 1.12
done
Checking in Bugzilla/Bug.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Bug.pm,v  <--  Bug.pm
new revision: 1.104; previous revision: 1.103
done