Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Multiple display logics of a field not working #563

Open
xkrdudrlf opened this issue Oct 29, 2024 · 0 comments · May be fixed by #564
Open

Multiple display logics of a field not working #563

xkrdudrlf opened this issue Oct 29, 2024 · 0 comments · May be fixed by #564
Labels
Priority:Important Issues & PRs that are important; broken functions; errors; there are workarounds Severity: Moderate Minor Impact Status: Fix Proposed A issue that has a PR related to it that provides a possible resolution Type: Bug Something isn't working

Comments

@xkrdudrlf
Copy link

xkrdudrlf commented Oct 29, 2024

Issue

When a user configures multiple display logics for a field, currently it is not working properly.

For example, if we have set 2 display logics for a field 'status' under 'Lead' module such as follows:

$dictionary['Lead']['fields']['status']['displayLogic'] = [
    'readonly_when_converted' => [
        'key' => 'displayType',
        'modes' => ['edit'],
        'params' => [
            'fieldDependencies' => [
                'title'
            ],
            'targetDisplayType' => 'show',
            'activeOnFields' =>  [
                'title' => [ 'Converted' ]
            ]
        ]
    ],
    'readonly_when_dead' => [
        'key' => 'displayType',
        'modes' => ['edit'],
        'params' => [
            'fieldDependencies' => [
                'title'
            ],
            'targetDisplayType' => 'none',
            'activeOnFields' =>  [
                'title' => [ 'Dead' ]
            ]
        ] 
    ]
];

'status' field should be shown when 'title' field value is 'Converted' and should be hidden when 'title' field value is 'Dead'

However, the field is currently not being shown when the value is 'Converted' and it turns out that whenever any display logic condition holds, the field will always be hidden.

Possible Fix

I've found 2 problems in how we set "field.display" value in runAll() of 'FieldLogicDisplayManager' class

    runAll(field: Field, record: Record, mode: ViewMode): void {
        let toDisplay: DisplayType = 'show';

        if(!field.displayLogic) {
            return;
        }

        const validModeLogic = Object.values(field.displayLogic).filter(logic => {
            const allowedModes = logic['modes'] ?? [];
            return !!(allowedModes.length && allowedModes.includes(mode));
        });

        if (!validModeLogic || !validModeLogic.length) {
            field.display = toDisplay;
            return;
        }

        let defaultDisplay = field.defaultDisplay ?? 'show';

        let targetDisplay: DisplayType = 'none'; // <= P1. it does not reflect what's in displayLogic['params']['targetDisplayType']

        if (defaultDisplay === 'none') {
            targetDisplay = 'show';
        }

        const context = {
            record,
            field,
            module: record.module
        } as ActionContext;


        const isActive = validModeLogic.some(logic => {
            const data: FieldLogicDisplayActionData = this.buildActionData(logic, context);
            return this.actions[mode][logic.key].run(data, logic);
        });

        if (isActive) {
            defaultDisplay = targetDisplay; // <= P2. there can be multiple targetDisplay from multiple displayLogics

        }

        toDisplay = defaultDisplay as DisplayType;

        if (defaultDisplay === 'show') {
            toDisplay = 'show';
        }

        field.display = toDisplay;
    }

P1.
As we can see in the above, the targetDisplay does not reflect the value we set in displayLogic['params']['targetDisplayType']

P2.
Also, when there are multiple displayLogics for a field, according to the CRM official documentation, all these displayLogics will be applied as 'OR' condition.
For example, when displayLogic A is 'active'(which means the display logic condition holds), the displayLogic A's 'targetDisplayType' should be applied. Also, when displayLogic B is also 'active', the displayLogic B's 'targetDisplayType' should be applied too. My current fix is currently take the last specified displayLogic's targetDisplayType since there can be a conflict for deciding the value.
The current codebase does not handle the case where there can be multiple displayLogics for a field in terms of deciding targetDisplay.
So, I rewrote runAll() of FieldLogicDisplayManager to tackle aforementioned 2 problems as follows:

    runAll(field: Field, record: Record, mode: ViewMode): void {
       if (!field.displayLogic) {
           return;
       }

       // 1. Set default display 
       const defaultDisplay: DisplayType = (field.defaultDisplay ?? "show") as DisplayType;

       // 2. Set target display
       let targetDisplay: DisplayType = null;

       // 2-1. Get valid mode logics - only the mode logics relevant to the current 'mode'
       const validModeLogics = Object.values(field.displayLogic).filter(logic => {
           const allowedModes = logic['modes'] ?? [];
           return !!(allowedModes.length && allowedModes.includes(mode));
       });
       if (validModeLogics.length === 0) {
           return;
       }

       // 2-2. Set target display by applying the valid mode logics
       const context = {
           record,
           field,
           module: record.module
       } as ActionContext;        
       for (const modeLogic of validModeLogics) {
           const data: FieldLogicDisplayActionData = this.buildActionData(modeLogic, context);
           const isModeLogicActive: boolean = Boolean(this.actions[mode][modeLogic.key].run(data, modeLogic));
           // Take targetDisplayType of the last mode logic by overwriting targetDisplay in case there are multiple active mode logics
           if (isModeLogicActive) {
               targetDisplay = modeLogic['params']['targetDisplayType'];
           }
       }

       // 3. Set field display
       field.display = targetDisplay === null ? defaultDisplay : targetDisplay; 
   }

Steps to Reproduce the Issue

1. Try setting multiple displayLogics for any field as follows:

$dictionary['Lead']['fields']['status']['displayLogic'] = [
    'readonly_when_converted' => [
        'key' => 'displayType',
        'modes' => ['edit'],
        'params' => [
            'fieldDependencies' => [
                'title'
            ],
            'targetDisplayType' => 'show',
            'activeOnFields' =>  [
                'title' => [ 'Converted' ]
            ]
        ]
    ],
    'readonly_when_dead' => [
        'key' => 'displayType',
        'modes' => ['edit'],
        'params' => [
            'fieldDependencies' => [
                'title'
            ],
            'targetDisplayType' => 'none',
            'activeOnFields' =>  [
                'title' => [ 'Dead' ]
            ]
        ] 
    ]
];
  1. Test whether a field is shown correctly in the page according to the display type set in displayLogics

For example, in the above case, 'status' field should be shown when 'title' field value is 'Converted' and should be hidden when 'title' field value is 'Dead'



### Context

_No response_

### Version

8.7.0

### What browser are you currently using?

Chrome

### Browser Version

_No response_

### Environment Information

PHP 8.2

### Operating System and Version

Ubuntu 23.10
@xkrdudrlf xkrdudrlf added the Type: Bug Something isn't working label Oct 29, 2024
xkrdudrlf added a commit to xkrdudrlf/SuiteCRM-Core that referenced this issue Oct 29, 2024
… class

 - use ['params']['targetDisplayType'] of display logic for setting targetDisplay
 - handle the case where there are multiple display logics in terms of deciding targetDisplay value for field.display eventually.
@xkrdudrlf xkrdudrlf linked a pull request Oct 29, 2024 that will close this issue
3 tasks
@johnM2401 johnM2401 added Priority:Important Issues & PRs that are important; broken functions; errors; there are workarounds Status: Fix Proposed A issue that has a PR related to it that provides a possible resolution Severity: Moderate Minor Impact labels Nov 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Priority:Important Issues & PRs that are important; broken functions; errors; there are workarounds Severity: Moderate Minor Impact Status: Fix Proposed A issue that has a PR related to it that provides a possible resolution Type: Bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants