ADO-3145 Predefined list of available fields from each data source fo…#388
ADO-3145 Predefined list of available fields from each data source fo…#388sozhanggov1 wants to merge 1 commit intodevfrom
Conversation
sozhanggov1
requested review from
DavidOkulski,
JohYoshidaBCGov and
bzimonjaSDPR
brysonjbest
left a comment
brysonjbest
left a comment
There was a problem hiding this comment.
Please see comments:
- update permissions
- update integration with form element data binding model to ensure that pathing is retained.
Some questions for @bzimonjaSDPR, @DavidOkulski, @JohYoshidaBCGov:
- I believe some verification of the intended process might also be required. Is it a requirement that users are still be able to manually enter databindings in the form element page? Or is the intent to have this fully controlled by the data binding mapping resource object in order to avoid worker input error? The original tech story indicates a searchable dropdown, which implies different functionality from the currently implemented datalist. A searchable dropdown would limit users to existing databindings created through the Data Binding Mapping resource page, and would provide one singular source of truth, instead of the current 'per-form-version' implemenation.
- Require clarifying if it is desired that the databindings are centrally controlled through the data binding mapping resource/model. Or, are these simply 'templates' that can be copied to a form element, and must be customized per element? The current implementation here treats them primarily as templates. Either way, @sozhanggov1, I think that updates to the form_element_data_bindings may be required in order to incorporate either the path_labels and repeating_path properties, or revised to include a foreignId association with the data_binding_mappings entries, dependent on the requirements.
- Keeping the current 'template' version of the implementation is the most straightforward for minimal effort in migrating, but it also maintains the possibility of user error when entering data bindings.
| protected static ?string $slug = 'data-bindings'; | ||
| protected static ?string $navigationIcon = 'heroicon-o-link'; | ||
|
|
||
| // Admin-only gate |
There was a problem hiding this comment.
The admin view access has been properly implemented which is great, but would prefer to follow our practice of policies for management of access on the Model level. ~./app/Policies demonstrates permissions management for this.
The Readme reviews the workflow process for adding new models: https://github.com/bcgov/klamm?tab=readme-ov-file#general-developer-workflows-for-adding-new-content
| : ''; | ||
| }; | ||
|
|
||
| $findRepeating = static function (string $src, string $label): ?string { |
There was a problem hiding this comment.
findRepeating doesn't appear to be in use. As well, the functionality from resolvesource and compose appears to be similarly defined/duplicated in the pathlabelfield function.
| ->required(!$disabled) | ||
| ->disabled($disabled) | ||
| ->live(onBlur: true), | ||
| // Path label |
There was a problem hiding this comment.
I'm experiencing some errors when reviewing data bindings that have been connected to a form element. The path label does not appear to save - which I believe is due to the path label not being saved to the form_element_data_bindings? In this setup, the path label, while used for search, also appears to be what creates the data binding itself if a mapping doesn't exist? The behaviour of this field should be consolidated with the existing form_element_data_bindings.
| ->dehydrated(false) | ||
| ->helperText("Composed as: $.['{Data source}'].['{Path label}']") | ||
| // When the form loads (view/edit), compose once so users see the preview | ||
| ->afterStateHydrated(function (Set $set, Get $get) use ($resolveSource, $compose) { |
There was a problem hiding this comment.
The state hydration doesn't appear to be loading for me. Data path is not hydrating on view/edit. I believe this is related to the pathLabelField function loading a blank string - and therefore hydrating this field with blank. afterStateHydrated may not be needed for this field if the data is pulled in from the 'path' as is.
There was a problem hiding this comment.
UserType model is defined under Models/FormMetadata/UserType
2 participants
…r forms in KLAMM
What changes did you make?
added new migrations, models, filament resources and pages; also updated DataBindingshelper file
Why did you make these changes?
To have a new table for managing data binding mappings
What alternatives did you consider?
Repeating path could be improved/refined by checking if the field is a container and repeatable