ManageIQ
[WIP] Make supports default to false
Blocked:
- [x] #21493
Overview
It is a lot of work to declare the not supports, so we can default to unsupported.
When making the transition from our old system to our new system, we have been needing to declare quite a few supports_not. This PR is about removing the need fo that.
Pulled from @Fryguy in https://github.com/ManageIQ/manageiq/pull/21473 /cc @chessbyte
Before
the default features like :create were defaulted to unsupported
All other features needed to be declared
Features that were not declared would throw an exception
After
All features are defaulted to unsupported
Features that are not declared have supported?(:unknown_feature) == false
The supports_feature? methods are no longer declared for these. This will be a problem if we are using the longer form, methods but will be fine if we are using the shorter (i.e.: supports?(:feature)) form.
Implication
We can give it a reason, but the non reason seemed to make more sense here.
There were a few supports_not(:feature) that gave a reason why it was not supported but we want to just remove them and throw away the reason.
There is the possibility to default the response to some text if this works better in the user interface
With this change, there is less checking of the names of these features. So it is easier to not detect a typo since it no longer throws an error. It is similar to going from accessing a variable (where an error is thrown for an invalid variable) to accessing a value from a hash (where no error is thrown for accessing a missing key).
This change encourages us to transition from the long method to the short one. Which could be implemented using a class level hash lookup.
fixed merge
@chessbyte and @Fryguy I am not sure if I like deny by default or if I like this being explicit. But it definitely does simplify the code.
We'll definitely need to get away from supports_feature? if we want to reduce the number of methods and possibly convert from what it is now, opt-in and opt-out, to the simpler solution of just opt-in.
:+1: as long as we drop all of the supports_feature? methods (which I never liked anyway) this looks great
Do we really want to do this? This was an alternative to delegates as a way to cut down on so much boiler plate.
deleting supports_ calls are the basis for a lot
Conversion from supports_feature_name? to supports?(:feature_name) here: https://github.com/ManageIQ/manageiq/pull/21493
WIP: we need to convert the long name to the short name for this to work. Otherwise, we would be calling a bunch of methods that did not exist and things would blow up.
UNWIP: we have converted all supports calls to use `supports?(:feature)
Looks like we can merge this if we would like to go with this idea
we have converted all supports calls to use
supports?(:feature)
I am not so sure. Need to check callers in UI and API repos.
we have converted all supports calls to use
supports?(:feature)I am not so sure. Need to check callers in UI and API repos.
does look like with workflows and a few other spots we define supports but it doesn't look like we do this through the supports mixin.
Do you think it would be best to convert these across to use the supports infrastructure to make this more obvious?
Referring to places like https://github.com/ManageIQ/manageiq-ui-classic/search?q=supports_
There are 3 references in ui classic that I see (one of which is a spec stubs):
Both are there to bridges AvailabilityMixin and SupportsFeatureMixin:
https://github.com/ManageIQ/manageiq-ui-classic/blob/c1fa26b793e35c9e641ebff98504ca10d746016f/app/helpers/application_helper.rb#L207-L211
https://github.com/ManageIQ/manageiq-ui-classic/blob/5e3a5d7858224498f8fa09a01aaf268a4c00693c/app/helpers/application_helper/button/generic_feature_button_with_disable.rb#L7-L11
We also have a number of supports_*? methods that are not part of the SupportsFeatureMixin, especially around workflows.
on that node, there are also a number of validate_* methods that are not part of AvailabilityMixin
WIP:
I am concerned about https://github.com/kbrock/manageiq-ui-classic/blob/master/app/helpers/application_helper/button/generic_feature_button_with_disable.rb that depends upon the supports method being defined to know if it should use supports or availability.
Lets hold off on this until we have removed availability
This pull request has been automatically closed because it has not been updated for at least 3 months.
Feel free to reopen this pull request if these changes are still valid.
Thank you for all your contributions! More information about the ManageIQ triage process can be found in the triage process documentation.
This pull request has been automatically closed because it has not been updated for at least 3 months.
Feel free to reopen this pull request if these changes are still valid.
Thank you for all your contributions! More information about the ManageIQ triage process can be found in the triage process documentation.
issues were fixed by https://github.com/ManageIQ/manageiq-ui-classic/pull/8266 - it is no longer calling supports_*? So there is no reason for the WIP.