415541 - Implement $bug->set_flags() and $attachment->set_flags()

Status: RESOLVED FIXED

(Bugzilla :: Attachments & Requests, enhancement, P1)

Description

[Frédéric Buclin]

We should get rid of CGI data in Flag.pm entirely and implement $bug->set_flag() and $attachment->set_flag() instead. The would work the exact same way as other set_foo() methods, i.e. changes would be effective only when $bug->update (and $attachment->update when it's implemented) is called.

Too invasive for 3.2. Targetting to 4.0

Comment 1

[Max Kanat-Alexander]

This blocks the Bug.pm bug, but not the process_bug bug.

Comment 2

[Frédéric Buclin]

Attachment: patch, v1

Here is the patch I plan to check in, unless someone finds something really wrong in bits I don't own. I spent a lot of time testing my patch as much as possible, so it should be pretty bug-free (I also checked emails sent when editing bugs and attachments, and when deleting a user account). If there are still some bugs to fix, I prefer to do this in a separate bug.

I had to replace "SELECT NOW()" at some places by "SELECT LOCALTIMESTAMP(0)" to not trigger unwanted midair collisions. NOW() appends the timezone on PostgreSQL while LOCALTIMESTAMP(0) doesn't, which is what we want as all datetime fields in our DB schema have no timezone (see also bug 296668 about this topic). This way, bugs.delta_ts and attachments.modification_time can now correctly be compared with dates and times reported by "SELECT LOCALTIMESTAMP(0)" without having to recreate the bug or attachment object.

extract_flags_from_cgi() is a helper to extract all the required data from the CGI form, so that all the code in Flag.pm (outside this helper) no longer depends on CGI at all. This let's you edit flags from WebServices, and even pass partial data to process_bug.cgi, as with other fields. This also brings us one step closer to mass-edit flags.

Bugzilla::Flag now follows Bugzilla::Object as much as possible, but due to the complex way flags work, you can only create/edit/delete flags by using $bug->set_flags() and $attachment->set_flags(), whose role is comparable to $bug->set_all(). Validators are called internally as the order in which the validators are called is important (e.g. flags may not exist in some products or components, a flag can have a grant group or a request group, some flags are multiplicable and some are not, or requestable or not, etc...).

Comment 3

[Max Kanat-Alexander]

Comment on attachment 392593 [details] [diff] [review]
patch, v1

Wow, this is an enormous patch, and although I didn't read all of the new and old code, I'd imagine it's a definite improvement over what we had.

The order that validators are called in is important for nearly every instance of Bugzilla::Object. That doesn't excuse bypassing the normal VALIDATORS and set_ system. In particular, that makes it very difficult for customizers to create a flag object and just do something with it. Granted, that might not be that common, but the point is to grant flexibility for unpredictable requirements.

For example, ideally there would be a VALIDATOR for flag_type, and you could just call $class->run_create_validators if you want to do the validation and creation at separate times, during the creation process.

Ideally, instead of having a manual validator inside of set_requstee, we'd be using eval {} when we call it to implement skip_requestee_on_error.

The current implementation of _validate makes it impossible to subclass Bugzilla::Flag--something that could be a problem for extensions, customizers, or possibly even ourselves in the future. We should never be passing __PACKAGE__ or a raw string to bless(), we should *always* be passing $class.

Why did you do that $params->{$_} = $flag->{$_} thing in create()? That also makes it impossible to extend flags or to subclass them. If there are specific fields you *don't* want, instead you should delete them (or just check if all fields are in $class->DB_COLUMNS instead of a manual list).

Also, instead of having a separate $timestamp positional argument, just pass creation_date to create(), and set it if it's not already set.

Never call "ref $blah eq 'Anything'" -- instead do "blessed $blah and $blah->isa('Anything')". (That allows for subclassing.)

It might be better to just have two return values from extract_flags_from_cgi--one that gives already-existing flags and another that gives flags that need to be created. I know that I as a developer would find that more useful if I ever want to do anything with the return value other than what you currently do with it.

Comment 4

[Frédéric Buclin]

(In reply to comment #3)
> The order that validators are called in is important for nearly every instance
> of Bugzilla::Object. That doesn't excuse bypassing the normal VALIDATORS and
> set_ system.

I don't bypass validators and set_ system. I call set_* from _validate().


 In particular, that makes it very difficult for customizers to
> create a flag object and just do something with it. Granted, that might not be
> that common, but the point is to grant flexibility for unpredictable
> requirements.

You should NEVER call create() directly, because the creation of a flag may be forbidden depending on the flag type properties and on the number of flags already set for the bug/attachment.


> Ideally, instead of having a manual validator inside of set_requstee, we'd be
> using eval {} when we call it to implement skip_requestee_on_error.

This doesn't help as you still have to pass the attachment, and you still want to throw an error if the flag is not specifically requestable.


> The current implementation of _validate makes it impossible to subclass
> Bugzilla::Flag--something that could be a problem for extensions, customizers,
> or possibly even ourselves in the future. We should never be passing
> __PACKAGE__ or a raw string to bless(), we should *always* be passing $class.

Easy to fix. Note that I removed all instance of __PACKAGE__ which you added. ;) So that's not a regression; you couldn't subclass Bugzilla::Flag yet.


> Also, instead of having a separate $timestamp positional argument, just pass
> creation_date to create(), and set it if it's not already set.

Wow, I don't understand what you mean. Please reformulate. :)

Comment 5

[Max Kanat-Alexander]

(In reply to comment #4)
> Easy to fix. Note that I removed all instance of __PACKAGE__ which you added.
> ;) So that's not a regression; you couldn't subclass Bugzilla::Flag yet.

  Yeah, I know. I added __PACKAGE__ instead of 'Bugzilla::Flag' where that existed written manually, once upon a time.

> > Also, instead of having a separate $timestamp positional argument, just pass
> > creation_date to create(), and set it if it's not already set.
> 
> Wow, I don't understand what you mean. Please reformulate. :)

  For create(), just pass creation_date as one of its params--there's no need to add a separate $timestamp argument to create().

Comment 6

[Frédéric Buclin]

Attachment: patch, v2

Here is the patch I'm going to check in. I fixed most suggestions mkanat made in his previous comment (except for stuff related to VALIDATORS, which I have to think about a bit more, in a separate bug). In particular, in this new patch:

- extract_flags_from_cgi() now returns two separate arrayrefs, one for existing flags and one for new flags (I didn't update the POD yet, but I plan to do it in a separate bug; this patch is already big enough);
- Bugzilla::{Bug|Attachment}->set_flags() passes both arrayrefs separately, in case we want to handle them separately later;
- blessed($foo) && $foo->isa('Bugzilla::Foo') is used everywhere instead of ref($foo) eq 'Bugzilla::Foo';
- Hardcoded Bugzilla::Flag->foo() have all been replaced by $class->foo();
- _validate() is now called using $class->_validate(). Same for notify();
- Bugzilla::Flag has been replaced by $class everywhere;
- Hardcoded fields have been replaced by $class->DB_COLUMNS in create(). I still explicitly set the timestamp there for creation_ts and modification_date as it must match the timestamp passed by update_flags().

So unless I miss something, you can now subclass Bugzilla::Flag. Cleanup, small fixes and bugs should be filed as separate bugs, depending on this one.

Comment 7

[Frédéric Buclin]

Checking in attachment.cgi;                   
/cvsroot/mozilla/webtools/bugzilla/attachment.cgi,v  <--  attachment.cgi
new revision: 1.157; previous revision: 1.156                           
done                                                                    
Checking in editflagtypes.cgi;                                          
/cvsroot/mozilla/webtools/bugzilla/editflagtypes.cgi,v  <--  editflagtypes.cgi
new revision: 1.56; previous revision: 1.55                                   
done                                                                          
Checking in editusers.cgi;                                                    
/cvsroot/mozilla/webtools/bugzilla/editusers.cgi,v  <--  editusers.cgi        
new revision: 1.154; previous revision: 1.153                                 
done                                                                          
Checking in post_bug.cgi;                                                     
/cvsroot/mozilla/webtools/bugzilla/post_bug.cgi,v  <--  post_bug.cgi          
new revision: 1.200; previous revision: 1.199                                 
done                                                                          
Checking in process_bug.cgi;                                                  
/cvsroot/mozilla/webtools/bugzilla/process_bug.cgi,v  <--  process_bug.cgi    
new revision: 1.419; previous revision: 1.418                                 
done                                                                          
Checking in Bugzilla/Attachment.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Attachment.pm,v  <--  Attachment.pm
new revision: 1.63; previous revision: 1.62
done
Checking in Bugzilla/Bug.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Bug.pm,v  <--  Bug.pm
new revision: 1.287; previous revision: 1.286
done
Checking in Bugzilla/Flag.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Flag.pm,v  <--  Flag.pm
new revision: 1.103; previous revision: 1.102
done
Checking in Bugzilla/Hook.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Hook.pm,v  <--  Hook.pm
new revision: 1.26; previous revision: 1.25
done
Checking in extensions/example/code/flag-end_of_update.pl;
/cvsroot/mozilla/webtools/bugzilla/extensions/example/code/flag-end_of_update.pl,v  <--  flag-end_of_update.pl
new revision: 1.2; previous revision: 1.1
done
Checking in template/en/default/global/code-error.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/global/code-error.html.tmpl,v  <--  code-error.html.tmpl
new revision: 1.114; previous revision: 1.113
done
Checking in template/en/default/global/user-error.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/global/user-error.html.tmpl,v  <--  user-error.html.tmpl
new revision: 1.283; previous revision: 1.282
done
Checking in template/en/default/request/email.txt.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/request/email.txt.tmpl,v  <--  email.txt.tmpl
new revision: 1.22; previous revision: 1.21
done