273339 - attachment.cgi is not consistent with its activity log timestamps

Status: RESOLVED FIXED

(Bugzilla :: Attachments & Requests, defect)

Description

[GavinS]

Sometime it uses NOW(), and sometimes it uses it's local $timestamp variable.
All the changes should be grouped together with the same timestamp.

Comment 1

[GavinS]

Attachment: make the attachment change timestamps consistent

Comment 2

[Vlad Dascalu]

Comment on attachment 168238 [details] [diff] [review]
make the attachment change timestamps consistent

I think the change should have been made the other way around, in order to use
NOW(). avatraxiom said in IRC this can be found in the developer's guide.

Comment 3

[GavinS]

(In reply to comment #2)
> (From update of attachment 168238 [details] [diff] [review])
> I think the change should have been made the other way around, in order to use
> NOW(). avatraxiom said in IRC this can be found in the developer's guide.
> 

Are you saying that you would prefer NOW() to be used in every individual INSERT
or UPDATE statement?

Comment 4

[Vlad Dascalu]

> Are you saying that you would prefer NOW() to be used in every individual INSERT
> or UPDATE statement?

As a short term solution I think so. The right way (as specified by Avatraxiom
in chat) would be something in the lines of bug 98304 comment #48, that codes a
layer of DB-abstractization.

Comment 5

[GavinS]

(In reply to comment #4)
> 
> As a short term solution I think so. The right way (as specified by Avatraxiom
> in chat) would be something in the lines of bug 98304 comment #48, that codes a
> layer of DB-abstractization.

2 things:

a) I think that we really want to be able to specify the same timestamp for all
changes made at the same time. I believe this is the intention in
process_bug.cgi, and the various routines (AppendComment,Flag::process) which
store away activity information with a timestamp. We need all the timestamps to
be the same so we can list them together in the activity log, and also for the
few hacks that exist to detail the bug activity alongside the corresponding
change comment

b) regarding db compatability (as you have aluded to in the developers guide) -
is it not cross db compatible to read a timestamp in the db default format, and
then use that in subsequent statements? (I'm no cross db expert, but I would
hope that is OK to do)


I think this fix (to use the same timestamp for all the changes) is what Dave
was requesting in his message to the developers list earlier this week -- but if
I misunderstood, please let me know...

Comment 6

[Dave Miller [:justdave]]

Comment on attachment 168238 [details] [diff] [review]
make the attachment change timestamps consistent

This is exactly what I wanted.	We get the timestamp from the database ONCE and
keep using the same one.  For cross-db compatibility we need to wrap it in a
DATE_FORMAT() but I wouldn't worry about that now, because there's lots of
other places that'll have to be adjusted when the time comes (neither
DATE_FORMAT nor NOW is cross-db compatible in its own right, and will need
DB-specific code for both).

Requesting a second review as I haven't actually tested this, but it looks
correct to me.

Comment 7

[Vlad Dascalu]

> Are you saying that you would prefer NOW() to be used in every individual INSERT
> or UPDATE statement?

Yeah, but indirectly, via $timestamp or something.

Your patch does what I want. However, my brain thought that $timestamp was
obtained via Perl's localtime or something similar. I missed the line in your
patch where you did:

+  my ($timestamp) = Bugzilla->dbh->selectrow_array("SELECT NOW()"); 

Sloppy reviewing I guess :-). I guess my brain would have expected to see that
called $DBtimestamp or something like that. :)

Comment 8

[Vlad Dascalu]

Comment on attachment 168238 [details] [diff] [review]
make the attachment change timestamps consistent

Oh, I see it's $sql_timestamp now for the quoted value. Fair enough.

I've tested this and I don't see any problems with the patch applied, except
that unrelated "DBI::db=HASH(0x1cdeecc)->disconnect invalidates 1 active
statement handle" thing.

Comment 9

[Vlad Dascalu]

r=vladd

Comment 10

[Vlad Dascalu]

This doesn't apply cleanly to 2.18.

Comment 11

[GavinS]

Attachment: make the attachment change timestamps consistent for 2.18

Here is the patch for 2.18

Comment 12

[Vlad Dascalu]

Comment on attachment 168723 [details] [diff] [review]
make the attachment change timestamps consistent for 2.18

The irony of this is that we've ended up doing the hunk below for 2.18, but we
haven't done it in the tip version:

-  SendSQL("SELECT NOW()");
-  my $timestamp = FetchOneColumn();
+  my ($timestamp) = Bugzilla->dbh->selectrow_array("SELECT NOW()"); 
+  my $sql_timestamp = SqlQuote($timestamp); 

   # Record changes in the activity table.
-  my $sql_timestamp = SqlQuote($timestamp);

So we end up with having more DB clean code in 2.18 rather than in the tip.

Anyway, either version works. And the important thing here is the timestamp
clean-up.

Comment 13

[Vlad Dascalu]

Checking in attachment.cgi;
/cvsroot/mozilla/webtools/bugzilla/attachment.cgi,v  <--  attachment.cgi
new revision: 1.64; previous revision: 1.63
done
Checking in Bugzilla/Flag.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Flag.pm,v  <--  Flag.pm
new revision: 1.24; previous revision: 1.23
done

Checking in attachment.cgi;
/cvsroot/mozilla/webtools/bugzilla/attachment.cgi,v  <--  attachment.cgi
new revision: 1.58.2.3; previous revision: 1.58.2.2
done
Checking in Bugzilla/Flag.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Flag.pm,v  <--  Flag.pm
new revision: 1.18.2.4; previous revision: 1.18.2.3
done