Bugfix: Validation consumption by giovanny07 · Pull Request #225 · pluginsGLPI/credit

giovanny07 · 2026-04-24T16:57:31Z

Checklist before requesting a review

I have performed a self-review of my code.
I have added tests (when available) that prove my fix is effective or that my feature works.
I have updated the CHANGELOG with a short functional description of the fix or new feature.

Description

This PR centralizes credit consumption validation so both the manual ticket form and the hook-driven followup/task/solution flow
use the same server-side rules. It also prevents selecting vouchers outside their valid begin/end date window and fixes the
ticket-tab JavaScript that broke the quantity field when selecting a voucher.

Validation performed:

find inc front ajax -name '*.php' -print0 | xargs -0 -n1 php -l
git diff --check
Manual UI validation of voucher selection and quantity behavior in the ticket form

stonebuzz · 2026-05-04T06:54:35Z

-        ERROR,
-    );
-    Html::back();
+if (!Session::haveRight('ticket', UPDATE) && !Session::haveRight(Entity::$rightname, UPDATE)) {


Why add a validation check when modifying an entity?
A technician updating a ticket (including data coming from the credit plugin) does not necessarily have the permissions required to update the associated entity.

stonebuzz · 2026-05-04T07:00:21Z

+        $ticket_id = filter_var(
+            $input['tickets_id'] ?? null,
+            FILTER_VALIDATE_INT,
+            ['options' => ['min_range' => 1]],
+        );
+        if ($ticket_id === false) {
+            Session::addMessageAfterRedirect(
+                __s('Ticket is mandatory.', 'credit'),
+                true,
+                ERROR,
+            );
+            return false;
+        }
+
+        $credit_id = filter_var(
+            $input['plugin_credit_entities_id'] ?? null,
+            FILTER_VALIDATE_INT,
+            ['options' => ['min_range' => 1]],
+        );
+        if ($credit_id === false) {
+            Session::addMessageAfterRedirect(
+                __s('Credit voucher entity must be selected.', 'credit'),
+                true,
+                ERROR,
+            );
+            return false;
+        }
+
+        $consumed = filter_var(
+            $input['consumed'] ?? null,
+            FILTER_VALIDATE_INT,
+            ['options' => ['min_range' => 1]],
+        );
+        if ($consumed === false) {
+            Session::addMessageAfterRedirect(
+                __s('Credit voucher quantity must be greater than 0.', 'credit'),
+                true,
+                ERROR,
+            );
+            return false;
+        }


Redundant code with prepareInputForUpdate
This logic duplicates functionality already present in prepareInputForUpdate.
It could be refactored into a dedicated checkInput function, or merged into getValidatedConsumptionInput, which should be renamed accordingly to better reflect its broader responsibility.

stonebuzz · 2026-05-04T07:02:05Z


 - Improve consumed credits modal readability and open related tickets in a new tab
 - Show credit vouchers list in entity tab for read-only users while keeping add/config forms restricted to editable contexts.
+- Centralize credit consumption validation, prevent invalid voucher selections outside the validity window, and fix the ticket tab quantity field behavior.


Suggested change

- Centralize credit consumption validation, prevent invalid voucher selections outside the validity window, and fix the ticket tab quantity field behavior.

- Improve credit validation and voucher handling.

Rom1-B · 2026-04-29T13:27:35Z


 use Glpi\Exception\Http\BadRequestHttpException;

-Session::haveRight("ticket", UPDATE);


Suggested change

Session::haveRight("ticket", UPDATE);

Session::checkRight(\Ticket::class, UPDATE);

Rom1-B · 2026-04-29T13:30:27Z

-if ($_REQUEST['plugin_credit_entities_id'] == 0) {
-    Session::addMessageAfterRedirect(
-        __s('Credit voucher entity must be selected.', 'credit'),
-        true,
-        ERROR,
-    );
-    Html::back();
-} elseif ($_REQUEST['plugin_credit_quantity'] == 0) {
-    Session::addMessageAfterRedirect(
-        __s('Credit voucher quantity must be greater than 0.', 'credit'),
-        true,
-        ERROR,
-    );
-    Html::back();


Why remove these checks?

stonebuzz · 2026-05-05T06:39:43Z

Hi @giovanny07

can you rebase ?

giovanny07 · 2026-05-11T16:05:42Z

Hi @stonebuzz, rebased on top of latest main.

stonebuzz · 2026-05-12T07:05:05Z

please fix CI

Herafia · 2026-06-10T07:07:23Z

Hi @giovanny07 could you resolve the requests and the CI ?

giovanny07 · 2026-06-22T17:31:57Z

Hi team!

Sorry for the delay, I was on vacation. I've fixed it now.

Regards

stonebuzz · 2026-06-22T19:33:20Z

Let me check tomorrow =)

stonebuzz

The test checkbox is unchecked, and given that checkInput centralizes logic that was previously scattered and untested, it would be worth adding at least:

a test for consumeVoucher on an already-solved ticket (this is the exact regression this PR introduces)
a test for prepareInputForAdd with an out-of-window voucher (begin_date in the future, end_date in the past)

stonebuzz · 2026-06-23T08:00:45Z

+            && !in_array($ticket->fields['status'], array_merge(Ticket::getSolvedStatusArray(), Ticket::getClosedStatusArray()));
+    }
+
+    private static function checkInput(array $input, ?self $current_credit = null): array|false


Suggested change

private static function checkInput(array $input, ?self $current_credit = null): array|false

private static function checkInput(array $input, ?self $current_credit = null, bool $skip_status_check = false): array|false

canUpdateCreditsForTicket returns false for non-entity-admins when the ticket is already solved or closed. consumeVoucher is registered on the item_add hook (setup.php line 74-76). For ITILSolution, GLPI sets the ticket status to "solved" inside ITILSolution::add(), before any item_add hooks fire.

stonebuzz · 2026-06-23T08:01:27Z

+        $ticket = new Ticket();
+        if (
+            !$ticket->getFromDB($ticket_id)
+            || !$ticket->can($ticket_id, READ)
+            || !self::canUpdateCreditsForTicket($ticket)
+        ) {


Suggested change

$ticket = new Ticket();

if (

!$ticket->getFromDB($ticket_id)

|| !$ticket->can($ticket_id, READ)

|| !self::canUpdateCreditsForTicket($ticket)

) {

$ticket = new Ticket();

if (

!$ticket->getFromDB($ticket_id)

|| !$ticket->can($ticket_id, READ)

|| (!$skip_status_check && !self::canUpdateCreditsForTicket($ticket))

) {

stonebuzz · 2026-06-23T08:02:27Z

 use Glpi\Exception\Http\BadRequestHttpException;

-Session::haveRight("ticket", UPDATE);
+Session::checkRight(Ticket::class, UPDATE);


Suggested change

Session::checkRight(Ticket::class, UPDATE);

Session::checkRight(\Ticket::class, UPDATE);

Please fix here

stonebuzz · 2026-06-23T08:03:57Z

+        }
+
+        $input = $validated_input + $input;
+        $input['users_id'] = (int) Session::getLoginUserID();


Suggested change

$input['users_id'] = (int) Session::getLoginUserID();

ALready set in front/ticket.form.php line 42

giovanny07 · 2026-06-23T13:19:12Z

Hi @stonebuzz, thanks for the detailed review.

Addressed in the latest pushed commit:

Added a skip_status_check parameter to PluginCreditTicket::checkInput() and use it only from consumeVoucher(). This keeps
the normal status/editability checks for manual add/update paths, while allowing the ITILSolution hook path to consume credits after GLPI has already moved the ticket to solved.
Removed the redundant users_id assignment from prepareInputForAdd(); it remains set by the front controller / caller input.
Added PHPUnit coverage for consumeVoucher() on an already-solved ticket through the solution-hook path.
Added PHPUnit coverage for prepareInputForAdd() rejecting vouchers outside their validity window: one with begin_date in
the future and one with end_date in the past.

On the Session::checkRight() suggestion: I kept Ticket::class without the leading slash because Rector failed the CI when the
code used \Ticket::class and explicitly rewrote it to Ticket::class.

Local validation done:

find inc front ajax tests -name '*.php' -print0 | xargs -0 -n1 php -l
git diff --check

I could not run PHPUnit locally because this checkout does not include the GLPI core vendor/; the plugin CI should now run the
tests because phpunit.xml is present.

stonebuzz · 2026-06-23T13:42:46Z

 use Glpi\Exception\Http\BadRequestHttpException;

-Session::haveRight("ticket", UPDATE);
+Session::checkRight(Ticket::class, UPDATE);


Please fix here

stonebuzz

LGTM

stonebuzz requested review from Rom1-B and stonebuzz April 27, 2026 07:41

stonebuzz requested changes May 4, 2026

View reviewed changes

Rom1-B requested changes May 4, 2026

View reviewed changes

giovanny07 added 3 commits May 11, 2026 11:04

fix credit consumption validation

4513f81

update changelog for credit validation fix

a240bfc

address credit validation review

9f28a7f

giovanny07 force-pushed the bugfix/validation-consumption branch from 373344a to 9f28a7f Compare May 11, 2026 16:04

fix rector ticket class reference

9661e81

stonebuzz requested changes Jun 23, 2026

View reviewed changes

add credit validation tests

976bf03

stonebuzz requested changes Jun 23, 2026

View reviewed changes

fix ticket right class reference

862b9db

stonebuzz requested review from Rom1-B and stonebuzz June 23, 2026 13:46

stonebuzz approved these changes Jun 23, 2026

View reviewed changes

giovanny07 added 3 commits June 23, 2026 08:53

fix phpunit bootstrap initialization

ddbf3fd

fix credit date test fixtures

9970336

skip normalized voucher dates in tests

bec00c8

	- Centralize credit consumption validation, prevent invalid voucher selections outside the validity window, and fix the ticket tab quantity field behavior.
	- Improve credit validation and voucher handling.


		use Glpi\Exception\Http\BadRequestHttpException;

		Session::haveRight("ticket", UPDATE);

	Session::haveRight("ticket", UPDATE);
	Session::checkRight(\Ticket::class, UPDATE);

	private static function checkInput(array $input, ?self $current_credit = null): array\|false
	private static function checkInput(array $input, ?self $current_credit = null, bool $skip_status_check = false): array\|false

	Session::checkRight(Ticket::class, UPDATE);
	Session::checkRight(\Ticket::class, UPDATE);

Uh oh!

Conversation

giovanny07 commented Apr 24, 2026

Checklist before requesting a review

Description

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stonebuzz commented May 5, 2026

Uh oh!

giovanny07 commented May 11, 2026

Uh oh!

stonebuzz commented May 12, 2026

Uh oh!

Herafia commented Jun 10, 2026

Uh oh!

giovanny07 commented Jun 22, 2026

Uh oh!

stonebuzz commented Jun 22, 2026

Uh oh!

stonebuzz left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

giovanny07 commented Jun 23, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stonebuzz left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants