714527 - editflagtypes.cgi should use ->set_all

Status: ASSIGNED

(Bugzilla :: Administration, task)

Description

[Olav Vitters]

Attachment: Patch v1

Note:
 - not sure if ->set_all should be last; maybe better for extensions
 - the deactive, changed it to set_all, not sure if that is the right thing to do

Comment 1

[Frédéric Buclin]

Comment on attachment 585212 [details] [diff] [review]
Patch v1

Pushing this patch out of my radar till bug 714520 comment 1 is addressed. If your patch correctly works with VALIDATOR_DEPENDENCIES, please re-request review from me.

Comment 2

[Sunil Joshi [:sjoshi]]

Attachment: Patch-v2

Comment 3

[:glob ✱]

we shouldn't be making the method names less understandable -- there's no reason for them to match exactly what we have in the schema.

> set_is_specifically_requestable --> set_is_requesteeble
don't change this one

> set_grant_group --> set_grant_group_id
> set_request_group --> set_request_group_id
these are ok

Comment 4

[Frédéric Buclin]

(In reply to Byron Jones ‹:glob› from comment #3)
> we shouldn't be making the method names less understandable -- there's no
> reason for them to match exactly what we have in the schema.

Are you sure of that? VALIDATOR_DEPENDENCIES is called by both run_create_validators() and set_all() via Bugzilla::Object::_sort_by_dep() and so hash keys must match.

Anyway, setting approval- is not the right way to go. If you disagree with the patch, set r- on it. a- is similar to wontfix, at least when I was an approver.

Comment 5

[:glob ✱]

(In reply to Frédéric Buclin from comment #4)
> Are you sure of that? VALIDATOR_DEPENDENCIES is called by both
> run_create_validators() and set_all() via Bugzilla::Object::_sort_by_dep()
> and so hash keys must match.

sounds like that should be fixed first.
please file a bug for that and mark it blocking this one.

> Anyway, setting approval- is not the right way to go. If you disagree with
> the patch, set r- on it. a- is similar to wontfix, at least when I was an
> approver.

i'll add that to the approvers' documentation.

Comment 6

[Frédéric Buclin]

Flags: review+ → review-

And instead of that, simply leave simon's r+ alone and add your own r- besides his review. You are not allowed to override his review. r- wins over r+ anyway. ;)