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)

enhancement
Not set
normal

Tracking

()

RESOLVED FIXED
Bugzilla 3.0

People

(Reporter: myk, Assigned: LpSolit)

References

Details

Attachments

(1 file, 2 obsolete files)

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.
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
(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
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.
Per my previous comment.
Attachment #178990 - Flags: review?(myk)
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)
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
I won't have time to play with it before the 2.22 freeze
Target Milestone: Bugzilla 2.22 → ---
Severity: normal → enhancement
Target Milestone: --- → Bugzilla 2.24
Attached patch patch, v2 (obsolete) — Splinter Review
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)
myk, please wait for bug 339750 to be fixed before reviewing this patch. They conflict.
Depends on: 339750
Comment on attachment 223866 [details] [diff] [review]
patch, v2

Let's wait a bit with this one...
Attachment #223866 - Flags: review?(myk)
QA Contact: mattyt-bugzilla → default-qa
Attached patch patch, v3Splinter Review
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)
Attachment #227890 - Flags: review?(myk) → review+
Flags: approval?
Flags: approval? → approval+
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.

Attachment

General

Created:
Updated:
Size: