N°6111 - Display in read-only mode a field in transition #750
Conversation
There was a problem hiding this comment.
Pull request overview
This PR implements functionality to display read-only fields during state transitions in iTop's workflow management system. The change enables fields marked with the OPT_ATT_READONLY flag to be displayed to users during transitions, providing visibility into field values that cannot be edited.
Changes:
- Added logic to handle
OPT_ATT_READONLYflagged attributes in theDisplayStimulusFormmethod - Refactored variable initialization to move
$oAttDefearlier in the code flow - Fixed code style issues with brace formatting
- Removed redundant variable reassignments
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| 'value' => "<span id=\"field_att_$iFieldIndex\">$sHTMLValue</span>", | ||
| ); | ||
| } | ||
| if (($iExpectCode & OPT_ATT_READONLY) ){ |
There was a problem hiding this comment.
The logic here has a potential issue. When a field has both MUSTCHANGE/MUSTPROMPT flags AND READONLY flags, the first block (lines 3275-3324) will execute and create an editable form field, but then the second block (lines 3325-3332) will overwrite $aAttrib with a readonly display. This means the READONLY flag takes precedence, which may not be the intended behavior. Consider using 'else if' instead of a separate 'if' to ensure mutual exclusivity between editable and readonly display modes.
| if (($iExpectCode & OPT_ATT_READONLY) ){ | |
| elseif (($iExpectCode & OPT_ATT_READONLY) ){ |
| if (($iExpectCode & OPT_ATT_READONLY) ){ | ||
| $bIsPrompt = true; | ||
| $sHTMLValue = $this->GetAsHTML($sAttCode); | ||
| $aAttrib = array( | ||
| 'label' => '<span>'.$oAttDef->GetLabel().'</span>', | ||
| 'value' => "<span id=\"field_att_$iFieldIndex\">$sHTMLValue</span>", | ||
| ); | ||
|
|
||
| } |
There was a problem hiding this comment.
When a field has the OPT_ATT_READONLY flag, the variables $sInputId and $sInputType are undefined, but they are used later at lines 3346-3347 when $bIsPrompt is true. This will result in PHP notices or undefined variable errors. These variables need to be initialized before the readonly block, similar to how they are initialized in the first condition block at line 3306.
| $aAttrib = array( | ||
| 'label' => '<span>'.$oAttDef->GetLabel().'</span>', | ||
| 'value' => "<span id=\"field_att_$iFieldIndex\">$sHTMLValue</span>", | ||
| ); | ||
| } | ||
| if (($iExpectCode & OPT_ATT_READONLY) ){ | ||
| $bIsPrompt = true; | ||
| $sHTMLValue = $this->GetAsHTML($sAttCode); | ||
| $aAttrib = array( | ||
| 'label' => '<span>'.$oAttDef->GetLabel().'</span>', | ||
| 'value' => "<span id=\"field_att_$iFieldIndex\">$sHTMLValue</span>", | ||
| ); |
There was a problem hiding this comment.
There is code duplication between lines 3320-3323 and lines 3328-3331. Both blocks create the same $aAttrib array structure with 'label' and 'value' keys. The only difference is how $sHTMLValue is obtained. Consider refactoring to avoid this duplication by computing $sHTMLValue conditionally and then constructing $aAttrib once.
| ); | ||
|
|
||
| } | ||
| if($bIsPrompt) { |
There was a problem hiding this comment.
Missing space after 'if' keyword. The codebase follows a consistent style of having a space between 'if' and the opening parenthesis, as seen on lines 3275, 3280, 3299, and throughout the file.
| if($bIsPrompt) { | |
| if ($bIsPrompt) { |
| 'value' => "<span id=\"field_att_$iFieldIndex\">$sHTMLValue</span>", | ||
| ); | ||
| } | ||
| if (($iExpectCode & OPT_ATT_READONLY) ){ |
There was a problem hiding this comment.
Extra space after opening parenthesis. The codebase consistently places the opening parenthesis immediately after the condition without a trailing space, as seen throughout the file (e.g., lines 3275, 3280, 3299).
| if (($iExpectCode & OPT_ATT_READONLY) ){ | |
| if (($iExpectCode & OPT_ATT_READONLY)) { |
internal