Closed
Bug 232705
Opened 20 years ago
Closed 18 years ago
FlagType::normalize() not used; remove it or use it where appropriate
Categories
(Bugzilla :: Attachments & Requests, enhancement)
Bugzilla
Attachments & Requests
Tracking
()
RESOLVED
FIXED
Bugzilla 3.0
People
(Reporter: myk, Assigned: LpSolit)
References
Details
Attachments
(1 file, 2 obsolete files)
1.95 KB,
patch
|
myk
:
review+
|
Details | Diff | Splinter Review |
The FlagType::normalize() function isn't used anywhere in Bugzilla. Presumably it has a purpose and should be called where appropriate. If that's not the case (i.e. it had a purpose in earlier versions of the request tracker patch but not anymore) it should be removed from the code.
Assignee | ||
Comment 1•19 years ago
|
||
This function should be called from Bugzilla::Bug::process() and editflagtypes.cgi. Actually, the code within Bugzilla::Bug::normalized() is duplicated at least twice!!!
Assignee: myk → LpSolit
Target Milestone: --- → Bugzilla 2.20
Assignee | ||
Comment 2•19 years ago
|
||
(In reply to comment #1) > This function should be called from Bugzilla::Bug::process() and > editflagtypes.cgi. Actually, the code within Bugzilla::Bug::normalized() is > duplicated at least twice!!! > Why am I writing "::Bug::" ??? I meant: This function should be called from Bugzilla::Flag::process() and editflagtypes.cgi. Actually, the code within Bugzilla::FlagType::normalized() is duplicated at least twice!!!
Status: NEW → ASSIGNED
Assignee | ||
Comment 3•19 years ago
|
||
We should move normalize() into Flag.pm because a part of this function calls Flag::clear() and only the 'flags' table is used, not the 'flagtypes' one.
Reporter | ||
Comment 5•19 years ago
|
||
Comment on attachment 178990 [details] [diff] [review] move FlagType::normalize() into Flag.pm, v1 This is a good fix and welcome simplification of the code. The only thing it would need is object-specific accessor methods to separate common code from object-specific code. In other words, instead of calling the code like this (the following examples are pseudo-code): normalize($bug_id, "bugs"); normalize($flagtype_id, "flags"); We call it like this: $bug->normalize(); $flagType->normalize(); The "normalize" methods then call Flag->normalize(), passing it a class-specific database field and instance-specific record ID: Bug::normalize() { $flag->normalize($this->id, "bugs.bug_id"); } FlagType::normalize() { $flag->normalize($this->id, "flags.type_id"); } Flag::normalize($id, $field) { <common code> } This polymorphic approach is the standard way of handling a situation like this where two different objects need access to the same functionality. The common code gets factored into a class to which both objects have access, and the object-specific code gets put into object-specific accessor methods. Ideally, we'd put Flag->normalize into a separate class that both the bug and flag type objects inherit, but that's probably overkill for this particular fix. Note that the bug accessor method should go into the pseudo-object created by Flag::GetBug(). Note also that "normalize" is probably not a good name for this anymore, since it doesn't convey what the function does to a reader who isn't already familiar with it. "delete_invalid_flags" would be better.
Attachment #178990 -
Flags: review?(myk)
Assignee | ||
Comment 6•19 years ago
|
||
OK, this is a bigger change than what I expected. Delaying it to 2.22 as it offers nothing useful for 2.20, and we are frozen already. Thanks for this interesting review. :)
Target Milestone: Bugzilla 2.20 → Bugzilla 2.22
Assignee | ||
Comment 7•19 years ago
|
||
I won't have time to play with it before the 2.22 freeze
Target Milestone: Bugzilla 2.22 → ---
Assignee | ||
Updated•18 years ago
|
Severity: normal → enhancement
Target Milestone: --- → Bugzilla 2.24
Assignee | ||
Comment 8•18 years ago
|
||
Till we have real flag objects, that's the way I'm going to handle Bugzilla::Flag::normalize(). Now FlagType.pm no longer depends on Flag.pm.
Attachment #178990 -
Attachment is obsolete: true
Attachment #223866 -
Flags: review?(myk)
Assignee | ||
Comment 9•18 years ago
|
||
myk, please wait for bug 339750 to be fixed before reviewing this patch. They conflict.
Depends on: 339750
Assignee | ||
Comment 10•18 years ago
|
||
Comment on attachment 223866 [details] [diff] [review] patch, v2 Let's wait a bit with this one...
Attachment #223866 -
Flags: review?(myk)
Updated•18 years ago
|
QA Contact: mattyt-bugzilla → default-qa
Assignee | ||
Comment 11•18 years ago
|
||
Due to the way I rewrote some parts of Flag.pm, the code in Flag.pm and editflagtypes.cgi is now different enough that normalize() can go away.
Attachment #223866 -
Attachment is obsolete: true
Attachment #227890 -
Flags: review?(myk)
Reporter | ||
Updated•18 years ago
|
Attachment #227890 -
Flags: review?(myk) → review+
Assignee | ||
Updated•18 years ago
|
Flags: approval?
Reporter | ||
Updated•18 years ago
|
Flags: approval? → approval+
Assignee | ||
Comment 12•18 years ago
|
||
Checking in Bugzilla/FlagType.pm; /cvsroot/mozilla/webtools/bugzilla/Bugzilla/FlagType.pm,v <-- FlagType.pm new revision: 1.30; previous revision: 1.29 done
Status: ASSIGNED → RESOLVED
Closed: 18 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•