-
Notifications
You must be signed in to change notification settings - Fork 52
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
Conditional Display of Helper Text for AI Provider Account Creation #770
Conversation
b7d5c2b
to
6940eae
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left a few comments but also noting this only addresses the helper text for the IBM Watson Provider. Most of our other Providers also show similar helper text so probably best to tackle all of those in a single PR
'title' => [], | ||
], | ||
] | ||
'description' => $this->feature_instance->is_feature_enabled() ? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So the is_feature_enabled
method checks three things:
- Is the Provider configured properly?
- Does the current user have access to this Feature?
- Is the Feature turned on?
This is probably fine when someone is setting things up through the onboarding wizard but if changing things from the settings page, this means the helper text won't show if the Feature isn't turned on. This leads to the scenario where the only way to see the helper text is turning the Feature on and then saving the settings.
I think it may be better to use the is_configured
method, so we aren't running a user check or enabled check but just checking if the Provider is configured.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hello @dkotter.
Good point. However, instead of just checking if the feature is configured I preferred to check if the feature is configured with the specified provider. The problem was that when using just is_configured
the feature might have been configured with provider A and that would result in hiding the help texts of provider B for the same service. With my approach, the help texts of a provider are only hidden if the feature is configured with the specified provider.
Please let me know what you think.
Thank you
'class' => 'classifai-provider-field hidden provider-scope-' . static::ID, // Important to add this. | ||
] | ||
); | ||
if ( ! $this->feature_instance->is_feature_enabled() ) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we want to hide this toggle as someone may want to switch from using an API key to using a Username and Password, which this makes that impossible unless they remove credentials first or disable the Feature first.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point @dkotter, thank you. This should be fine now.
…ured" and always show the option to switch to authenticating with username and password
…ers when the feature is configured with the selected provider
@kmgalanakis thanks for the PR! Could you please fill out the PR template with description, changelog, and credits information so that we can properly review and merge this? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tested and works as expected 👍
Description of the Change
Closes #715
How to test the Change
Changelog Entry
Credits
Props @kmgalanakis
Checklist: