N°8692 - Notification - placeholder attributesubitem#778
N°8692 - Notification - placeholder attributesubitem#778accognet wants to merge 1 commit intosupport/3.2from
Conversation
There was a problem hiding this comment.
Pull request overview
Fixes formatting of StopWatch deadline subitems when the raw value is provided as a numeric string (notably for notification placeholders).
Changes:
- Update
AttributeStopWatch::GetSubItemAsPlainText()to accept numeric-string timestamps and cast them before callingdate(). - Add a PHPUnit test covering StopWatch deadline display formatting.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| tests/php-unit-tests/unitary-tests/core/AttributeDefinitionTest.php | Adds a unit test asserting the plain-text rendering of a StopWatch deadline subitem. |
| core/attributedef.class.inc.php | Makes StopWatch “deadline” subitem plain-text rendering tolerant to numeric-string timestamps. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| public function testDisplayStopwatch() | ||
| { | ||
| $aUserRequestCustomParams = [ |
There was a problem hiding this comment.
This new test method is indented with spaces, while the rest of the file uses tabs (eg. existing methods above). Please align indentation to the file's convention to avoid noisy diffs and style inconsistencies.
| $iStartDate = time() - 200; | ||
|
|
||
| $oStopwatch = $oUserRequest->Get('ttr'); | ||
| $oStopwatch->DefineThreshold(100, $iStartDate); |
There was a problem hiding this comment.
Variable name $iStartDate is misleading here: it's used as the threshold deadline timestamp passed to DefineThreshold(), not a start date. Renaming (eg. $iDeadlineTs) would make the test intent clearer.
| $iStartDate = time() - 200; | |
| $oStopwatch = $oUserRequest->Get('ttr'); | |
| $oStopwatch->DefineThreshold(100, $iStartDate); | |
| $iDeadlineTs = time() - 200; | |
| $oStopwatch = $oUserRequest->Get('ttr'); | |
| $oStopwatch->DefineThreshold(100, $iDeadlineTs); |
| $value = $oUserRequest->Get('ttr_escalation_deadline'); | ||
| $sRet = $oAttDef->GetAsPlainText($value, $oUserRequest); | ||
| self::assertEquals('Missed by 3 min', $sRet); |
There was a problem hiding this comment.
This test doesn't currently exercise the new behavior in GetSubItemAsPlainText() (handling numeric timestamps provided as strings), because Get('ttr_escalation_deadline') returns an int from the in-memory ormStopWatch. To cover the regression, coerce $value to a string (or otherwise simulate the SQL/string code path) before calling GetAsPlainText() and keep the assertion on the formatted output.
| if (is_numeric($value)) { | ||
| if (!is_int($value)) { | ||
| $value = intval($value); | ||
| } | ||
| $sDate = date(AttributeDateTime::GetInternalFormat(), $value); | ||
| $sRet = AttributeDeadline::FormatDeadline($sDate); | ||
| } else { | ||
| $sRet = $value; | ||
| } | ||
| } else { | ||
| $sRet = $value; | ||
| } |
There was a problem hiding this comment.
The added block uses spaces for indentation whereas this file consistently uses tabs in surrounding code. Please re-indent these lines to match the existing convention to keep diffs clean and avoid style drift.
| if ($value) { | ||
| if (is_int($value)) { | ||
| if (is_numeric($value)) { | ||
| if (!is_int($value)) { | ||
| $value = intval($value); | ||
| } | ||
| $sDate = date(AttributeDateTime::GetInternalFormat(), $value); | ||
| $sRet = AttributeDeadline::FormatDeadline($sDate); |
There was a problem hiding this comment.
GetSubItemAsPlainText() is now tolerant of numeric-string timestamps for deadline, but GetSubItemAsHTML() still calls date(..., $value) without casting/validation, which can still throw a TypeError on PHP 8 when $value is a string (common when the subitem value comes from SQL). Consider applying the same numeric-string handling in the HTML branch to keep behavior consistent and prevent runtime errors.
2 participants
internal