N°1681 - Add new triggers for attachment creation and removal #534
Conversation
There was a problem hiding this comment.
Discussed IRL with Anne-Catherine, several things to change:
Point 1
Hacking the standard "Trigger on update" with JS to remove the "Attachment" class from proposed value doesn't seem like a good option.
Instead we would rather improve the \AttributeClass class to add a class_exclusion_list like we do for \AttributeClassAttCodeSet with attribute_definition_exclusion_list. Check with @rquetiez or @eespie , they can guide you.
Point 2
The way it is currently done, you can't limit the trigger to attachments of a specific class only, which is most likely something we'll want to do.
Point 3
Because of the previous points, you can't rely on the ComputeValues method and the fact it derived from the standard TriggerOnObjectUpdate, you'll need to call your trigger when attachments are finalized. We might want to add an event for that actually, check with Eric.
Molkobain
changed the title
|
May I ask why the existing triggers aren’t sufficient? |
|
It does but to get an attachment creation notification, we need to create an "on object update" trigger on the attachment with the temp_id field. It's not really logical. |
|
During the technical review, we'll need to discuss the possible impacts of adding the
|
core/attributedef.class.inc.php
Outdated
| @@ -3736,7 +3736,7 @@ class AttributeClass extends AttributeString | |||
|
|
|||
| public static function ListExpectedParams() | |||
| { | |||
| return array_merge(parent::ListExpectedParams(), array("class_category", "more_values")); | |||
| return array_merge(parent::ListExpectedParams(), array('class_category', 'more_values', 'class_exclusion_list')); | |||
There was a problem hiding this comment.
Discussed during technical review: We'll actually remove it from expected params as it would force any AttributeClass usage to provide it, which would lead to regression on any module / custos using it.
We'll just add a comment to inform that this option exists.
Molkobain
changed the title
accognet
force-pushed
the
feature/1681-Notification_on_Attachment
branch
from
b71d2f1
to
bfbf85c
Compare
| "db_finalclass_field" => "", | ||
| ); | ||
| MetaModel::Init_Params($aParams); | ||
| MetaModel::Init_InheritAttributes(); | ||
| MetaModel::Init_AddAttribute(new AttributeClass("target_class", array("class_category" => "bizmodel", "more_values" => "User,UserExternal,UserInternal,UserLDAP,UserLocal", "sql" => "target_class", "default_value" => null, "is_null_allowed" => false, "depends_on" => array()))); | ||
| MetaModel::Init_AddAttribute(new AttributeClass("target_class", | ||
| array("class_category" => "bizmodel", "more_values" => "User,UserExternal,UserInternal,UserLDAP,UserLocal", "sql" => "target_class", "default_value" => null, "is_null_allowed" => false, "depends_on" => array(), "class_exclusion_list" => "Attachment"))); |
There was a problem hiding this comment.
Just a note: Since there is no TriggerOnAttachmentCreate actually, I know of some iTop instances that use a TriggerOnObject with a target class Attachment.
Your implementation will make this impossible in the future, so you are forcing these people to migrate. At least this should be documented, as it may be a breaking change for some iTop instances?
I assume that in such a case (Triggers with target class Attachment already exist) the setup will not break, but there will be a warning in the DB tools? Have you checked this?
There was a problem hiding this comment.
One effect for the end-user (verified) is that, when opening the form to edit the trigger, the "class" value is reset and prevents the user from modifying the trigger (mandatory field).
In order to preserve this constraint, a solution would be to migrate automatically the existing triggers at setup time (ModuleInstaller). Could be implemented in the form of a helper in ModuleInstallerAPI.
Create a new trigger "on object's attachment creation"