Skip to content

Commit

Permalink
add _reopen in followup; see #247
Browse files Browse the repository at this point in the history
  • Loading branch information
@orthagh
orthagh committed Nov 16, 2015
1 parent a03da8d commit 8ed18da
Show file tree
Hide file tree
Showing 2 changed files with 33 additions and 12 deletions.
16 changes: 11 additions & 5 deletions inc/ticket.class.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2637,6 +2637,17 @@ static function getProcessStatusArray() {
return array(self::ASSIGNED, self::PLANNED);
}

/**
* Get the ITIL object closed, solved or waiting status list
*
* @since version 0.90.1
*
* @return an array
**/
static function getReopenableStatusArray() {
return array(self::CLOSED, self::SOLVED, self::WAITING);
}

/**
* Calculate Ticket TCO for an item
*
Expand Down Expand Up @@ -6704,11 +6715,6 @@ static function getSplittedSubmitButtonHtml($tickets_id, $action="add") {
$ticket->getFromDB($tickets_id);
$ticket_users = $ticket->getTicketActors();
$actor_type = $ticket_users[Session::getLoginUserID()];

// stupid control: assign a status if requester? done by commonitilactor
// if ($actor_type == CommonITILActor::REQUESTER) {
/// $ticket->fields['status'] = CommonITILObject::ASSIGNED;
// }
$all_status = Ticket::getAllowedStatusArray($ticket->fields['status']);

$html = "<div class='x-split-button' id='x-split-button'>
Expand Down
29 changes: 22 additions & 7 deletions inc/ticketfollowup.class.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -391,10 +391,16 @@ function post_addItem() {
$donotif = false; // Done for ticket update (new status)
}

//manage reopening of ticket
$reopened = false;
if (!isset($this->input['_status'])) {
$this->input['_status'] = $this->input["_job"]->fields["status"];
}
// if reopen set (from followup form) and status is reopenable and not changed in form
if (isset($this->input["_reopen"])
&& $this->input["_reopen"]
&& in_array($this->input["_job"]->fields["status"],
array(CommonITILObject::CLOSED, CommonITILObject::SOLVED, CommonITILObject::WAITING))) {
&& in_array($this->input["_job"]->fields["status"], Ticket::getReopenableStatusArray())
&& $this->input['_status'] == $this->input["_job"]->fields["status"]) {

if (($this->input["_job"]->countUsers(CommonITILActor::ASSIGN) > 0)
|| ($this->input["_job"]->countGroups(CommonITILActor::ASSIGN) > 0)
Expand All @@ -408,11 +414,12 @@ function post_addItem() {
// Use update method for history
$this->input["_job"]->update($update);
$donotif = false; // Done for ticket update (new status)
$reopened = true;
}

//change ticket status only if imput change
if (isset($this->input['_status'])
&& ($this->input['_status'] != $this->input['_job']->fields['status'])) {
if (!$reopened
&& $this->input['_status'] != $this->input['_job']->fields['status']) {

$update['status'] = $this->input['_status'];
$update['id'] = $this->input['_job']->fields['id'];
Expand Down Expand Up @@ -598,12 +605,20 @@ function showForm($ID, $options=array()) {
|| (isset($_SESSION["glpigroups"])
&& $ticket->haveAGroup(CommonITILActor::ASSIGN, $_SESSION['glpigroups'])));

$requester = ($ticket->isUser(CommonITILActor::REQUESTER, Session::getLoginUserID())
|| (isset($_SESSION["glpigroups"])
&& $ticket->haveAGroup(CommonITILActor::REQUESTER, $_SESSION['glpigroups'])));

$reopen_case = false;
if ($this->isNewID($ID)
&& in_array($ticket->fields["status"], $ticket->getClosedStatusArray())
&& $ticket->isAllowedStatus($ticket->fields['status'], Ticket::INCOMING)) {
&& (in_array($ticket->fields["status"], $ticket->getClosedStatusArray())
&& $ticket->isAllowedStatus($ticket->fields['status'], Ticket::INCOMING)
|| ($requester && !$tech)
&& in_array($ticket->fields['status'], Ticket::getReopenableStatusArray()))) {
$reopen_case = true;
echo "<div class='center b'>".__('If you want to reopen the ticket, you must specify a reason')."</div>";
if (in_array($ticket->fields["status"], $ticket->getClosedStatusArray())) {
echo "<div class='center b'>".__('If you want to reopen the ticket, you must specify a reason')."</div>";
}
}

if ($tech) {
Expand Down

10 comments on commit 8ed18da

@yllen
Copy link
Collaborator

@yllen yllen commented on 8ed18da Nov 17, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Il y a un problème dans les derniers contrôles de ce commit.
Le dernier if ne sert à rien vu que tu es déjà dans un if avec cette condition (614)

617 ne sert à rien vu que tu es déjà dans statut clos avec 614

615 à 617 : je pense que les && et || sont mal implémentés car là si $requester tu n'as meme plus le controle du statut

@orthagh
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Je suis en désaccord sur les 2 remarques.
617 n'es't pas en doublon justement a cause du || à la ligne 616

La condition indique qu'un ticket est ré-ouvrable si :

  • pour tout le monde, qu'il est dans un statut clos et que la matrice du profil permet de revenir de ce statut vers Nouveau (condition existant avant mon commit)

OU

  • pour le demandeur seulement, qu'il soit dans un statut ré-ouvrable (clos, résolu, en attente)

@yllen
Copy link
Collaborator

@yllen yllen commented on 8ed18da Nov 17, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oui, donc c'est bien l'implémentation avec les parenthèse qui n'est pas bon (je m'en doutais un peu).
En effet, suivant la Précédence des opérateurs PHP, && est évolué avant ||

@orthagh
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oui pour la priorité des opérateurs,
mais il n'y a, à mon sens, pas de problème, ni de manque de parenthèses

J'extrait la condition en enlevant le test sur newID() et je l'ai commenté un peu :

in_array($ticket->fields["status"], $ticket->getClosedStatusArray()) // le statut est clos
&&  
$ticket->isAllowedStatus($ticket->fields['status'], Ticket::INCOMING) // matrice profil OK
||
($requester && !$tech) // seulement le demandeur (d'ailleurs les parenthèses sont inutiles ici)
&&
in_array($ticket->fields['status'], Ticket::getReopenableStatusArray()) // statut ré-ouvrable

@yllen
Copy link
Collaborator

@yllen yllen commented on 8ed18da Nov 18, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Si il manque des parenthèses.
Là il va te faire
nouveau suivi
ET statut clos ET statut autorisé ET statut réouvrable (ne sert à rien avec la première condition)
OU demandeur ET pas tech
Tu devrais avoir une parenthèse englobant les 2 première conditions qui vont ensmble et une pour les 2 suivantes
if (is new
&& ((in array statut clos
&& statut autorisé)
|| (demandeur
&& pas tech
&& in array statut réouvrable)))

@orthagh
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mais non !

Comme tu le dis toi même :
suivant la Précédence des opérateurs PHP, && est évalué avant ||
-> http://php.net/manual/fr/language.operators.precedence.php

Ce qui veux dire que toute les parties entre && sont évaluées et donnent un booléen
et ensuite les || sont évalués sur ces résultats booléens

Pour exemple :

A && B || C && D   ===  (A && B) || (C && D)
A && B || C && D   !==   A && (B || C) && D

Donc je maintiens fortement que les parenthèses que tu veux ajouter peuvent l'être pour un souci de lisibilité (ce n'est pas mon point de vue) mais que coté logique, elles sont parfaitement inutiles.

Pour reprendre donc mon exemple :

in_array($ticket->fields["status"], $ticket->getClosedStatusArray()) // le statut est clos
&& $ticket->isAllowedStatus($ticket->fields['status'], Ticket::INCOMING) // matrice profil OK
// 1 - Toute la partie du dessus est évaluée pleinement

|| // 3 - le résultat des 2 parties est comparée par l'opérateur OU

// 2 - Toute la partie ci dessous est évaluée pleinement
$requester && !$tech // seulement le demandeur
&& in_array($ticket->fields['status'], Ticket::getReopenableStatusArray()) // statut ré-ouvrable

@remicollet
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry but I really don't understand this piece of code and its goal.

For me the only case where reopen (by requester) is needed

  • waiting => because answer the question
  • solved => when solution is not accepted.

And IIRC from mailgate, not closed ticket are auto-reopen if needed, closed ticket are dupliacted as a new one.

@yllen
Copy link
Collaborator

@yllen yllen commented on 8ed18da Nov 20, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Alors il ne faut plus respecter le coding standard qui permet de mieux comprendre le code sans connaite par coeur la précédence des opérateurs?

@tomolimo
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yllen
I agree with you, clarity is better than operator rules. Especially when as developpers we often switch between diffent languages (PHP, Javascript, C, VB, scripts,...) which don't have always the same rules for precedences.
my one cent contribution :)
Tomolimo

@orthagh
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fine, I'll re-add brackets for CS and clarity (it's a better argument than previous, thanks).

Please sign in to comment.