Closed Bug 153578 Opened 22 years ago Closed 22 years ago

Attachment modified date is meant to be attachment creation date

Categories

(Bugzilla :: Creating/Changing Bugs, defect)

2.17
x86
Windows 98
defect
Not set
minor

Tracking

()

RESOLVED FIXED
Bugzilla 2.18

People

(Reporter: MatsPalmgren_bugz, Assigned: bbaetz)

References

()

Details

(Whiteboard: [schema])

Attachments

(1 file, 2 obsolete files)

Attachment 'Modified' date does not match 'View Activity' date for change.

In the given URL, I added an attchment that I later obsoleted.
In the 'Modified' column it says:        2002-06-22 05:56
In the 'View Bug Activity' list it says: 2002-06-22 05:58:36

The comment that was created when I first attached it says: 2002-06-22 05:56

My guess is that 'Modified' really is 'Created' ?
Its the created time in the source. Except that setting is_obsolete when
creating a new bug will update it. Oops

If its meant to be the creation time, we should fix that by having the field not
be a timestamp, and using now() on the INSERT. (We should also retroactivly fix
this my searching for the initial 'created an attachment' comment for all
obsolete attachments)

If its meant to be the modification time, then we should remove the explicit
creation_ts=creation_ts from attachment.cgi

I'm not sure which makes more sense to me. Creation is probably what we want,
though, I think. Its definately what was originally intended, and theres alwasy
teh activity log if someone wants more.

myk?
I think the value should be the creation time, and the obsoletion code is buggy,
which is easy for timestamps because they update automatically unless you
explicitly tell them not to.  We should fix this not only by fixing the
obsoletion code and the corrupt data but also by making that field not be
automatically updated (i.e. make it a different field type like a datetime field).
Yeah, it should be a datetime type. OK.
I have a patch for this.
Assignee: myk → bbaetz
Keywords: patch, review
Whiteboard: [schema]
Target Milestone: --- → Bugzilla 2.18
Attached patch v1 (obsolete) — Splinter Review
This recreates all of the creation times. I don't think I need a progress meter
thingy, although I would like to know how long this does take on a large db.

Since the now() for the comment time and the now() for the attachment time may
be different with mysql, I guess this could change stuff by a second or two. I
don't care about that, though - the number isn't meant to be that exact,
anyway. I tested this on landfill, and no numbers were changed.

I removed the formatting stuff since mysql's datetime uses that format anyway.
Once we support multiple dbs, we'll have to pull that code out somewhere and
use Date::Format on it, or something.
Comment on attachment 94920 [details] [diff] [review]
v1


Looks good; r=preed, with the following nits:

>Index: checksetup.pl
>===================================================================
>RCS file: /cvsroot/mozilla/webtools/bugzilla/checksetup.pl,v

>+    # Restrict this as much as possible in order to avoid false positives, and
>+    # keep the db search time down
>+    my $sth2 = $dbh->prepare("SELECT bug_when, thetext FROM longdescs WHERE bug_id=? AND who=? AND thetext LIKE ? ORDER BY bug_when LIMIT 1");

Why do you select thetext? I don't see you using it anywhere below.

>+    while (my ($bug_id, $attach_id, $submitter_id) = $sth->fetchrow_array()) {
>+        $sth2->execute($bug_id, $submitter_id, "Created an attachment (id=$attach_id)%");
>+        my ($when, $text) = $sth2->fetchrow_array();
>+        if ($when) {
>+            $dbh->do("UPDATE attachments SET creation_ts='$when' WHERE attach_id=$attach_id");

As you note in your comment, I think the status meter thing is relatively
important, especially on larger Bugzillas.

To make it simpler, maybe you could just put a print statement in here like
"attachment $attach_id updated" or something, maybe?

Granted, that'd be a lot of text and it might obscure the warning... but I
still think it's important. If we can find a simple way to do that, it would be
nice, but if not...

>+        } else {
>+            print "Warning - could not determine correct creation time for attachment $attach_id on bug $bug_id\n";
>+        }
>+    }
Attachment #94920 - Flags: review+
Attached patch v2 - add counter (obsolete) — Splinter Review
Getting thetext was a side effect from when I handled the seaching in perl.

I'll print updates every 1000, but I don't kno wif thats too much/too
little/whatever.
Attachment #94920 - Attachment is obsolete: true
Comment on attachment 95063 [details] [diff] [review]
v2 - add counter

My r= will carry forward, but considering that v2 is the exact same patch as
v1, I'm going to needs-work v2. :-)

I'd say every 500... but you're right... it's a hard call to make.

Bleh... whatever you think works...
Attachment #95063 - Flags: review-
Attached patch v2.1Splinter Review
oops - try this
Attachment #95063 - Attachment is obsolete: true
Comment on attachment 95243 [details] [diff] [review]
v2.1

Much better! :-)

r=preed
Attachment #95243 - Flags: review+
Comment on attachment 95243 [details] [diff] [review]
v2.1

2xr from joel
Attachment #95243 - Flags: review+
Fixed, and changing summary to match what ended up happening.
Status: NEW → RESOLVED
Closed: 22 years ago
Resolution: --- → FIXED
Summary: Attachment 'Modified' date does not match 'View Activity' date for change → Attachment modified date is meant to be attachment creation date
QA Contact: matty_is_a_geek → default-qa
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: