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: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

Rom1-B · 2026-06-29T07:46:09Z

+            'users_id'                  => Session::getLoginUserID(),
+        ]));
+    }
+}


No test covers the overconsumption paths in checkInput (lines 287-298 of inc/ticket.class.php): when overconsumption_allowed = 1, a WARNING should be emitted but the add should succeed; when overconsumption_allowed = 0, the add should fail. Can you add a test for each branch?

The deduction logic in checkInput (lines 278-282): when updating an existing credit entry for the same voucher, $quantity_consumed is reduced by the current record's consumed value so the user is not double-charged. This path has no test. Can you add a test for prepareInputForUpdate where the updated quantity stays within the adjusted remaining budget?

Thanks! you're rigth Co-authored-by: Romain B. <8530352+Rom1-B@users.noreply.github.com>

giovanny07 · 2026-06-30T19:07:38Z

Those checks weren't dropped, they were centralized into PluginCreditTicket::checkInput() (inc/ticket.class.php), which both prepareInputForAdd() and prepareInputForUpdate() call. The same messages are still emitted there:

plugin_credit_entities_id not selected → Credit voucher entity must be selected. (ERROR)
consumed <= 0 → Credit voucher quantity must be greater than 0. (ERROR)

The goal was to have one source of truth for validation shared by the manual form and the hook-driven path (followup/task/solution), instead of duplicating the entity/quantity checks in the front controller.

Covers the two branches flagged by Rom1-B: WARNING+success when overconsumption_allowed=1, ERROR+failure when disabled, and the prepareInputForUpdate path where an existing record's consumption is deducted before re-checking the remaining budget.

stonebuzz · 2026-07-01T07:21:08Z

@giovanny07

can you rebase to solve conflicts ?

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

Rom1-B requested changes Jun 29, 2026

View reviewed changes

Update front/ticket.form.php

20fefe4

Thanks! you're rigth Co-authored-by: Romain B. <8530352+Rom1-B@users.noreply.github.com>

giovanny07 force-pushed the bugfix/validation-consumption branch from a21d0c3 to 941f09d Compare June 30, 2026 19:08

	- 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.

	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!

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!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

giovanny07 commented Jun 30, 2026

Uh oh!

stonebuzz commented Jul 1, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants