N°3366 - Multiple target state on approbal-extended

[accognet]

need change in approval base (branche feature/3366-Add_status_informations_on_approval )

Allows you to configure which classes are affected by the approval and from which statuses. Multiple starting statuses are allowed for a class.

new configuration for this module:

_'combodo-approval-extended' => array (
	'targets' => array (
	  'UserRequest' => 
	  array (
	    'target_states' => ['new','assigned'],
	    'bypass_profiles' => 'Service Manager',
	    'reuse_previous_answers' => true,
	  ),
	)
),_

You can configure different triggers to send different messages for approval:

Different approval stages are visible on the object in the Approval tab with the starting and ending status (thanks to the functionality implemented in the approval-base)

[Hipska]

Sounds interesting, does that mean it could be used to have a ruleset for NormalChange objects to go from new to validated/rejected and also from planned to approved/notapproved?

In your example I see it went from assigned towards escalated_ttr. Is there a way to specify what the target state should be?

[accognet]

Yes, this is the target.
The target status is defined in XML as the current approval with 2 transitions: ev_approve and ev_reject

[Hipska]

Okay, so the transitions from starting state towards ending state should be named exactly ev_approve and ev_reject?

[accognet]

There are a lot of logs, because the code is not finished yet. I still have to do a lot of tests to validate this code. To not forget what I did I have already created the PR. I will have time in August to finish it.

[Hipska]

Sure, I gave suggestions to apply correct line indentations and log debug messages as actual debug, not error ;)

[Molkobain]

Support PRs review:

• Don't forget to add the conf migration during setup, see combodo-autoclose-tickets for an equivalent example
• Change target_state to target_states
• Change target_states format from a CSV list to an array (will work out of the box when config edition wil be through GUI)
• Mind to add the target_states default value in HideButtonsPlugin::OnDisplayProperties()

Good :

• Store start/end states instead of computing them on the fly, that way what has been done won't change even in case of conf / lifecycle changes
• New conf format

Sounds nice! Will take a closer look.

[Hipska]

Indentations sometimes still not correct. (Is your IDE correctly configured?)

Also, what with users that already have customised their config? Shouldn't there be a migration method during iTop Setup? Currently the old config is ignored and reverted to default.

[Molkobain]

Also, what with users that already have customised their config? Shouldn't there be a migration method during iTop Setup? Currently the old config is ignored and reverted to default.

Totally, the config migration was asked during the support PRs review:

• Don't forget to add the conf migration during setup, see combodo-autoclose-tickets for an equivalent example

[Hipska]

The installer looks good 👍

[Hipska]

Thanks, the code looks much more readable now. Some minor remarks:

[Hipska]

I would remove this section, since you said this: https://github.com/Combodo/combodo-approval-extended/pull/7/files#r1763440755

[Hipska]

Still some indentation problem, a typo and to keep backward compatibility, the admin profile should also be included when migrating.

Suggested change

	$newConfiguration = [
	'UserRequest' => [
	'target_states' => [$oConfiguration->GetModuleSetting('combodo-approval-extended', 'target_states', null) ],
	'bypass_profiles' =>$oConfiguration->GetModuleSetting('combodo-approval-extended', 'bypass_profiles', 'Service Manager') ,
	'reuse_previous_answers' => $oConfiguration->GetModuleSetting('combodo-approval-extended', 'reuse_previous_answers', true) ,
	]
	];
	$newConfiguration = [
	'UserRequest' => [
	'target_states' => [$oConfiguration->GetModuleSetting('combodo-approval-extended', 'target_state', 'new')],
	'bypass_profiles' => $oConfiguration->GetModuleSetting('combodo-approval-extended', 'bypass_profiles', 'Administrator, Service Manager'),
	'reuse_previous_answers' => $oConfiguration->GetModuleSetting('combodo-approval-extended', 'reuse_previous_answers', true),
	]
	];

[Hipska]

It would make more sense later in the code when this var is named differently..

Suggested change

	$aAllowedClasses = MetaModel::GetConfig()->GetModuleSetting('combodo-approval-extended', 'targets', ApprovalConfiguration::DEFAULT_TARGET);
	if (array_key_exists(get_class($oObject), $aAllowedClasses) === false) {
	$aTargets = MetaModel::GetConfig()->GetModuleSetting('combodo-approval-extended', 'targets', ApprovalConfiguration::DEFAULT_TARGET);
	if (array_key_exists(get_class($oObject), $aTargets) === false) {