Closed Bug 425668 Opened 16 years ago Closed 15 years ago

Changing a bug (via process_bug) omits body classes and other header information when displaying the current/next bug

Categories

(Bugzilla :: User Interface, defect, P2)

Product:
Bugzilla
Component:
User Interface
Version:
3.1.3
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
Bugzilla 3.6

People

(Reporter: LpSolit, Assigned: mkanat)

References

Details

Attachments

(1 file, 3 obsolete files)

v4
4.03 KB, patch
LpSolit
: review+
Details | Diff | Splinter Review 
When you commit changes to a bug, the list of classes set to <body> are missing. You have to load the page again to see them (i.e. show_bug.cgi has them, but process_bug.cgi hasn't).
This is another good reason why we need a special show_bug => 1 for header.html.tmpl.
This will get fixed with the other show_bug header flag fix.
This bug is annoying for all those who have, like me or dveditz, added stuff to userContent.css to display security bugs differently. Everytime the bug is processed, it's being displayed as a normal bug. Should be trivial enough to fix.
Flags: blocking3.2?
Not a blocker, but feel free to fix it and we can take it on all branches back to 3.0 if it's fixed before we release 3.2.
Severity: normal → minor
Flags: blocking3.2? → blocking3.2-
The Bugzilla 3.0 branch is now locked to security bugs and dataloss fixes only. This bug doesn't fit into one of these two categories and is retargetted to 3.2 as part of a mass-change. To catch bugmails related to this mass-change, use lts081207 in your email client filter.
Target Milestone: Bugzilla 3.0 → Bugzilla 3.2
pyrzak, could you have a look?
Priority: -- → P2
Summary: Changing bugs omit body classes → Changing bugs omit body classes (centralize show_bug header stuff in templates)
Target Milestone: Bugzilla 3.2 → Bugzilla 3.6
No longer blocks: 455810, 530015
Summary: Changing bugs omit body classes (centralize show_bug header stuff in templates) → Changing bugs omit body classes
Summary: Changing bugs omit body classes → Changing a bug (via process_bug) omits body classes and other header information when displaying the current/next bug
Depends on: 530009
Attached patch v1 (obsolete) — — Splinter Review 
This depends on the patch from the blocker.
Assignee: ui → mkanat
Status: NEW → ASSIGNED
Attachment #414145 - Flags: review?(LpSolit)
Comment on attachment 414145 [details] [diff] [review]
v1

We now get midair collisions all the time, and if you click "submit my changes anyway", then the last comment is missing.
Attachment #414145 - Flags: review?(LpSolit) → review-
Attached patch v2 (obsolete) — — Splinter Review 
Ah hah! The problem was caused by a NOW() in process_bug where there should have been a LOCALTIMESTAMP(0).
Attachment #414145 - Attachment is obsolete: true
Attachment #416484 - Flags: review?(LpSolit)
Comment on attachment 416484 [details] [diff] [review]
v2

The midair collision problem is gone, but the comment you just added isn't displayed.
Attachment #416484 - Flags: review?(LpSolit) → review-
Whiteboard: [needs new patch asap]
Attached patch v3 (obsolete) — — Splinter Review 
So, it turns out that the problem was that a Bugzilla::Bug doesn't update its internal structure well-enough during an update(). 

I tried, at first, modifying $bug->update() to delete all the stuff that it needed to delete, but it actually ended up being a lot of changes, and it seemed so fragile--there are so many interdependencies with a bug's values inside of the object itself (like how $self->user depends on so many things, for example) that it would be impossible to know what to delete, and we'd have to delete everything and re-load the bug from the database entirely, which seemed like a wasteful thing to do at the end of every single update (particularly because I would have put it in Bugzilla::Object, and that would have caused it to go everywhere in Bugzilla).

So instead I just create a new bug before displaying the template, which is much simpler.
Attachment #416484 - Attachment is obsolete: true
Attachment #416787 - Flags: review?(LpSolit)
Whiteboard: [needs new patch asap]
Attachment #416787 - Flags: review?(LpSolit) → review-
Comment on attachment 416787 [details] [diff] [review]
v3

wrong patch. It's the same as the previous one.
Attached patch v4 — — Splinter Review 
Ah, thanks. :-) This is the updated patch.
Attachment #416787 - Attachment is obsolete: true
Attachment #416856 - Flags: review?(LpSolit)
Comment on attachment 416856 [details] [diff] [review]
v4

>Index: process_bug.cgi

>     # Include both action = 'same_bug' and 'nothing'.
>     else {
>-        $vars->{'bug'} = {bug_id => $cgi->param('id')};
>+        $vars->{'bug'} = $bug_objects[0];
>     }

Nit: you could use $first_bug instead of $bug_objects[0] (same object, but as we have it...).


>+elsif ($action eq 'next_bug' or $action eq 'same_bug') {

Another nice cleanup would be to move lines below

  # End the response page.
  unless (Bugzilla->usage_mode == USAGE_MODE_EMAIL) {

into this block too as all other conditions do not trigger this code. This would make it more obvious when this code is called or not, for both developers and reviewers.



>Index: template/en/default/bug/process/header.html.tmpl

> [% PROCESS global/header.html.tmpl 
>   # We don't have a bug object at this point, unfortunately, so we can't
>   # actually display all the proper header info.
>-  header_addl_info = '' 
> %]

The comment is no longer relevant. Please remove it on checkin.


Works fine. r=LpSolit
Attachment #416856 - Flags: review?(LpSolit) → review+
Flags: approval+
(In reply to comment #20)
> Another nice cleanup would be to move lines below
> 
>   # End the response page.
>   unless (Bugzilla->usage_mode == USAGE_MODE_EMAIL) {
> 
> into this block too as all other conditions do not trigger this code. This
> would make it more obvious when this code is called or not, for both developers
> and reviewers.

  Hmm. Unless I'm misunderstanding you, I think that that code is called at other times--sometimes the action is "nothing" and the same footer is printed whether there's a valid action or the action is "nothing".

  I'll fix everything else on checkin, thanks for pointing them out. :-)
Checking in process_bug.cgi;
/cvsroot/mozilla/webtools/bugzilla/process_bug.cgi,v  <--  process_bug.cgi
new revision: 1.426; previous revision: 1.425
done
Checking in template/en/default/bug/process/header.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/bug/process/header.html.tmpl,v  <--  header.html.tmpl
new revision: 1.16; previous revision: 1.15
done
Status: ASSIGNED → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
(In reply to comment #21)
>   Hmm. Unless I'm misunderstanding you, I think that that code is called at
> other times--sometimes the action is "nothing" and the same footer is printed
> whether there's a valid action or the action is "nothing".

Oh, you are totally right. The last test is ($action ne 'nothing') but I read equals instead of not equals. So the code has to stay where it is now. Oops! :)
Blocks: 429457
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: