bugs.resolution needs a default value

RESOLVED FIXED in Bugzilla 2.20

Status

()

RESOLVED FIXED
14 years ago
14 years ago

People

(Reporter: Tomas.Kopal, Assigned: Tomas.Kopal)

Tracking

2.19.2
Bugzilla 2.20
Dependency tree / graph
Bug Flags:
approval +
blocking2.20 -

Details

Attachments

(1 attachment, 1 obsolete attachment)

V2
1.82 KB, patch
mkanat
: review+
Details | Diff | Splinter Review
(Assignee)

Description

14 years ago
There is still one problem left preventing entry of a new bug on Postgres: the
resolution column does not have any default value, is declared as NOT NULL, and
at bug creation time, we are not passing any value.
I think that there is no need to specify the value at creation time (the bug
should never have any resolution when created), so default value '' (empty
string) in the DB is probably appropriate.
(Assignee)

Updated

14 years ago
Summary: Resolution need default value → Resolution needs default value
(Assignee)

Updated

14 years ago
Flags: blocking2.20?
Target Milestone: --- → Bugzilla 2.20
(Assignee)

Updated

14 years ago
Assignee: general → Tomas.Kopal
(Assignee)

Comment 1

14 years ago
Posted patch V1 (obsolete) — Splinter Review
Attachment #178309 - Flags: review?(mkanat)
(Assignee)

Updated

14 years ago
Status: NEW → ASSIGNED
Comment on attachment 178309 [details] [diff] [review]
V1

Due to a bug in bz_change_field_type, I think that change will run every time
you run checksetup. Try it out, though, and see.
(Assignee)

Comment 3

14 years ago
(In reply to comment #2)
> (From update of attachment 178309 [details] [diff] [review] [edit])
> Due to a bug in bz_change_field_type, I think that change will run every time
> you run checksetup. Try it out, though, and see.
> 

Hmmm, what is the bug in bz_change_field_type about? Wouldn't it be easier to
fix that bug first (if possible), than working around it?
You can try. I think the bug is that it doesn't take defaults into account.

I didn't fix the bug because I'm going to eliminate that function very soon.

If you'd like, we can just hold off on this bug until bug 285111 is resolved,
maybe. Or you can check this in, and then we can live with it for a few days
until we have a later change to wrap it in (like how I did it for the other
default changes).
(Assignee)

Comment 5

14 years ago
Comment on attachment 178309 [details] [diff] [review]
V1

Ok, I'll wait.
Attachment #178309 - Flags: review?(mkanat)

Updated

14 years ago
Depends on: 285111
Flags: blocking2.20? → blocking2.20+
"If it's not a regression from 2.18 and it's not a critical problem with
something that's already landed, let's push it off." - Dave
Flags: blocking2.20+
Whiteboard: [wanted for 2.20]
Flags: blocking2.20-
(Assignee)

Comment 7

14 years ago
Actually, the dependecy is on 285748, not on 285111.
Depends on: 285748
No longer depends on: 285111
(Assignee)

Comment 8

14 years ago
Posted patch V2Splinter Review
Well, here is updated version which seems to work correctly, as far as I was
able to test.
Attachment #178309 - Attachment is obsolete: true
Attachment #181739 - Flags: review?(mkanat)
Comment on attachment 181739 [details] [diff] [review]
V2

Yeah, that will work, but I'd be more comfortable doing it in chronological
sequence at the bottom, instead of up there.
Attachment #181739 - Flags: review?(mkanat) → review-
(Assignee)

Comment 10

14 years ago
(In reply to comment #9)
> (From update of attachment 181739 [details] [diff] [review] [edit])
> Yeah, that will work, but I'd be more comfortable doing it in chronological
> sequence at the bottom, instead of up there.
> 

Well, that's not so easy. If you just add the line changing the definition at
the end, you end up with changing the type back and forth on every execution of
checksetup (been there, done that). Or, you can remove the line I modified and
add the new one at the end - then it's bit confusing why all the conversions
from enum to tables are not at the same place. So the approach I took seems to
be most sensible to me.
What do you recommend, Max?
(In reply to comment #10)
> What do you recommend, Max?

  Put it at the bottom, but wrap it in an "if !exists
$dbh->bz_column_info(blah)->{DEFAULT}"

Comment on attachment 181739 [details] [diff] [review]
V2

OK, I looked at checksetup a bit more, and I've decided that this is actually
OK. It would be a big pain to go back and wrap stuff in the if statements, and
there are no bugs.resolution schema changes between this one and now. The
column never even gets touched.

So this is fine.
Attachment #181739 - Flags: review- → review+

Updated

14 years ago
Flags: approval?
Flags: approval? → approval+
Checking in checksetup.pl;
/cvsroot/mozilla/webtools/bugzilla/checksetup.pl,v  <--  checksetup.pl
new revision: 1.399; previous revision: 1.398
done
Checking in Bugzilla/DB/Schema.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/DB/Schema.pm,v  <--  Schema.pm
new revision: 1.27; previous revision: 1.26
done
Status: ASSIGNED → RESOLVED
Last Resolved: 14 years ago
Resolution: --- → FIXED
Summary: Resolution needs default value → bugs.resolution needs a default value
Whiteboard: [wanted for 2.20]
Version: unspecified → 2.19.2
You need to log in before you can comment on or make changes to this bug.