Added 'selectable' column type

[mohsin]

The 'dropdown', 'radio', 'balloon-selector' and ‘checkboxlist’ type form fields do not have a list form field equivalent and so they show the internally stored key instead of the translated value. So, a new column type called 'selectable' is introduced here to overcome this limitation.

The workaround for this limitation was mentioned on the OctoberCMS forum but I found it unsatisfactory as it's a global model-level attribute conversion which doesn't solve the issue--using that technique breaks the fields at the form level as their update forms now gets the converted value and so the dropdown, balloon-selector, etc. fields show the incorrect value. Also, when writing RESTful APIs using OctoberCMS, the model runs the same attribute conversions to show the translated value than the key which isn't effective either--most RESTful APIs actually send the keys itself to the client device and the value's are filled in at the client-level as it's more network efficient that way, so that's another disadvantage of using the method, that was suggested in the forum. A backend list-level solution is what's needed and this new column type helps to solve that.

Fixes #2510.

[LukeTowers]

@SaifurRahmanMohsin I'm assuming you copied the getOptionsFromModel from the Form widget? Perhaps we should create a new \Backend\Traits\ModelOptions trait to be used by the Form widget and the Lists widget and then put that code there.

[LukeTowers]

@SaifurRahmanMohsin feel free to make that change on your branch so it shows up here.

[mohsin]

There is just one issue with that... I remember editing some code that is not common to both. I’ll check and see if I can make a common trait, if not I’ll comment back here.

[mohsin]

@LukeTowers Hey, I've moved getOptionsFromModel to its own trait as you've suggested and made the List and Form widgets implement the trait. I've already tested it with the dropdown widget for form and list and it doesn't break anything but could you cross verify that it works fine at your end as well to ensure I didn't mess anything else up? Thanks

[mohsin]

@bennothommo I have verified your suggestions and they're all good and I've included them in the PR. Thank you.

[bennothommo]

Thanks for that @SaifurRahmanMohsin. Just one last change, would you mind removing these 2 lines, as they won't be needed with my suggested change:

october/modules/backend/widgets/Lists.php

Lines 1163 to 1164 in bdeda4d

	$fieldName = $column->columnName;
	$fieldOptions = $column->config['options'] ?? null;

[mohsin]

@LukeTowers I have tested the code using 5 different possible ways of defining the field values for dropdown, radio, and balloon selector fields (4 of which are mentioned in the
OctoberCMS documentation) and everything works fine so I guess this is good to go.

[github-actions]

This pull request will be closed and archived in 3 days, as there has been no activity in the last 60 days.
If this is still being worked on, please respond and we will re-open this pull request.
If this pull request is critical to your business, consider joining the Premium Support Program where a Service Level Agreement is offered.

[bennothommo]

@SaifurRahmanMohsin are you still interested in finalising this PR? If so, would you mind reviewing this comment and see if you can implement those final changes?

[bennothommo]

@SaifurRahmanMohsin @LukeTowers I've been testing this out, as I have a project that will find some use for it, but it doesn't seem to support static field options from a fields.yaml file, which I feel is going to be the more common scenario for most field setups. It only supports callbacks, either directly to the model or to an external class.

It is possible this is the only way it'll work, treating the model as a single source of truth for the options, given that technically you could have more that one form that populates the model data, each potentially having their own set of options available for a field.

EDIT: I should clarify - @SaifurRahmanMohsin has made it so that the column config also passes a options parameter to the getOptionsFromModel method, however, this means you have to set the same set of options in two locations and ensure they are in sync.

[mohsin]

@bennothommo What do you mean by two locations? I remember setting this to take it from the form/list model class the same way the form's dropdown, balloon-selector, etc. fields take their values so it's already a single source of truth for both form and column definitions. Am I missing something? Hmm

[bennothommo]

@SaifurRahmanMohsin what I mean is, say I have the following field set up in fields.yaml for the form:

status:
    label: Status
    type: dropdown
    options:
        pending: Pending
        processing: Processing
        completed: Complete

and then I wanted to use the selectable type for a column for that field in columns.yaml for a list:

status:
    label: Status
    type: selectable

With that sort of setup, it won't know what the options are, because the options are defined in the fields.yaml config for the form, not the list. You've made an allowance for this by specifying the options in the columns.yaml as well, like so:

status:
    label: Status
    type: selectable
    options:
        pending: Pending
        processing: Processing
        completed: Complete

But that means that we now have two locations for the options that have to be kept in sync. It's probably not so bad for fields with only a small number of options, but there is every chance that people will have fields with many static options. The only way the selectable column will work around this is to define the options in the model through the callbacks (ie. getDropdownOptions or getStatusOptions), but I believe there will be a lot of people who will try the selectable type with their forms and lists set up the way I initially said, and won't realise that they have to convert the static options to use one of the callbacks instead.

[mohsin]

A workaround maybe to read the form config and take those options but I'm not sure if that's the "best practice" coz now we have a list reading a form config.

[bennothommo]

@SaifurRahmanMohsin That's correct - it wouldn't take into account any events that might manipulate the form config (ie. backend.form.extendFields). You'd have to pretty much run the entire initialization process for a form to ensure the correct options are available, which would be overkill.

[bennothommo]

All that having been said, I'm not against the way it is set up now. It certainly is faster than having to write out a partial for these types of columns and customising the partial to display the "nicer-looking" value, which is what I've had to do every time I've set up columns like this.

[mohsin]

@bennothommo I am trying to write the unit test for this as you requested but I'm unable to figure out why the system behaves weirdly. The goal here is for me to call getColumnValue and assert that with "Developer" which should be the right returned column value, thus proving it works fine. But I encounter a weird issue that doesn't let me write the test.

I made a video showing what I'm facing here:
https://youtu.be/iE9Rg0uI5V0

And here's a ready-made zip having my files to test the same:
https://drive.google.com/file/d/195QqOCHmgiUD0yAmdWuQ_oa-ZhuZIpnm/view?usp=sharing

If you can help me fix this or point me in the right direction, I'll be happy to include the test as well to this PR.

[bennothommo]

@SaifurRahmanMohsin could you add what you have for the tests into the changes for this PR, and I'll be able to have a look into it.

[mohsin]

Ok, here's what I changed:

1. Set role_id to 1 here.
2. Added the selectable test:

public function testSelectableType()
{
    $user = new UserFixture;
    $list = $this->listWithSelectableFieldFixture($user);
    $selectableColumn = $list->getColumn('role_id');

    // Test if the type is set to selectable
    $this->assertEquals($selectableColumn->type, 'selectable');

    // Test if the value is processed correctly
    $this->assertEquals($list->getColumnValue($user, $selectableColumn), 'Developer');
}

protected function listWithSelectableFieldFixture($record)
{
    return new Lists(null, [
        'model' => $record,
        'columns' => [
            'id' => [
                'type' => 'text',
                'label' => 'ID'
            ],
            'role_id' => [
                'type' => 'selectable',
                'label' => 'Role',
                'options' => ['Client', 'Developer']
            ]
        ]
    ]);
}

Here I'm re-using the UserFixture since that's a model readily available in the tests framework to test this out. The role_id returns 1 and so the selection would be 'Developer'. However, I'm facing the weird bug which I mentioned in my last comment as the $list->getColumnValue($user, $selectableColumn); fails--basically $selectableColumn isn't the correct ListColumn item i.e. role_id on checking inside the method.

[bennothommo]

@SaifurRahmanMohsin I'd say it's because role_id is not fillable, as it's filled by the role relation. I'm having a look into this now.

[mohsin]

The weird behavior occurs even if I custom define a field besides role. But sure, please do check and tell me if there's a solution. I'll be happy to check in the test case and we can have this closed soon.

[bennothommo]

@SaifurRahmanMohsin I've deployed a unit test that works into this PR for you.

[mohsin]

@bennothommo Excellent. The test passes on my machine as well. Thank you. Let me know if anything is left, otherwise you can go ahead and merge the PR.

[bennothommo]

@SaifurRahmanMohsin I've done some changes to this PR to improve support for both form fields and list columns - would you mind giving that a test and letting me know if that works?

@LukeTowers I've also added in changes to the callback methods to match up the signatures with the docs. Let me know your thoughts, or if you want me to roll them back.

[LukeTowers]

@bennothommo could you highlight the changes to the API you've made?

[bennothommo]

@LukeTowers I've given it some thought and I'm going to roll it back. While I like a consistent API, this is too much of a breaking change.

[daftspunk]

🔥🔥🔥 This is a crazy good feature @SaifurRahmanMohsin. Thank you! It is clear a lot of work went into building it. Although upon peer review, this implementation is an example where the code liability exceeds the underlying benefit; in other words, it adds too much technical debt to justify the feature. It is a great piece of work, just not the right implementation...

However, after some refactoring, I was able to get it to work. The final approach is to acknowledge that the list column is leveraging a form field's behaviour. So the workflow goes something like this

Lists Widget -> List Column -> type: selectable -> Form Field -> getOptions

The options logic then had to be bumped from the Form widget to the Form Field, which is a natural progression anyway. Previously there was just no reason to do it, now we have one

This feature should arrive in October CMS once the next major release comes out

Thanks again 🙏 All the best