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.

Sign up for GitHub

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

Jump to bottom

Feat/432 add new filters to provide more granular control #459

Merged
merged 14 commits into from
Jul 17, 2023
Merged

Feat/432 add new filters to provide more granular control #459

merged 14 commits into from
Jul 17, 2023

Conversation

bjorn2404
Copy link
Member

@bjorn2404 bjorn2404 commented May 20, 2023 •
edited
Loading

Description of the Change

Added filters to provide more granular control over new OpenAI features as outlined in #432

Closes #432

How to test the Change

Changelog Entry

Fixed comment typo in includes/Classifai/Providers/Provider.php
Added classifai_chatgpt_allowed_image_roles filter to allow ChatGPT image role settings to be overridden
Added classifai_openai_dalle_allowed_image_roles filter to allow DALL·E image role settings to be overridden
Added classifai_openai_dalle_user_can_{$feature} filter to allow granular control for DALL·E image generation
Added classifai_chatgpt_user_can_{$feature} filter to allow granular control for ChatGPT title and excerpt generation

Credits

Props @bjorn2404

Checklist:

  • I agree to follow this project's Code of Conduct.
  • I have updated the documentation accordingly.
  • I have added tests to cover my change.
  • All new and existing tests pass.

@bjorn2404
Copy link
Member Author

@dkotter Wanted to double check on your intention from #432. You had mentioned to update ChatGPT.php and DallE.php, which appears to just output the settings in the admin. If more of an override filter is wanted I was thinking of just adding one to the bottom of generate_image_permissions_check() within includes/Classifai/Services/ImageProcessing.php. Apologies if you had something else in mind - first time looking at this plugin and just interpreting everything. Thanks!

@jeffpaul jeffpaul added this to the 2.3.0 milestone May 20, 2023
@dkotter
Copy link
Collaborator

dkotter commented May 22, 2023

@bjorn2404 Thanks for the questions!

If more of an override filter is wanted I was thinking of just adding one to the bottom of generate_image_permissions_check() within includes/Classifai/Services/ImageProcessing.php

So this would work for image generation but only if that is triggered via the REST API. This should be true for most use cases, someone in theory could directly use the methods within the DallE class. I personally would recommend adding a filter to the generate_image_callback within the DallE class instead, though I think some refactoring will be needed there to make this work.

In essence what I'm thinking is it's very likely that a client will want more fine-tune control over which users can use these features (since each request costs them money). As an example, they may have a single admin and two editors that should have access but none of the other admins or editors (or other account types). While we have settings in place to limit access based on role, that wouldn't help in this situation.

So having some sort of override filter that allows you to have that level of control could be very useful and is something we do in a few other places, like within the OCR class or Read class (though to be fair, these are more about allowing you to process items that you may not otherwise and less about user permissions).

I like the idea of introducing a new method to check if a user has access to the feature, that by default can have the checks we do right now (like checking the user role matches what is in the settings) but then has a filter around the final value, allowing someone total control on who can use the feature (this is just one potential approach though).

For ChatGPT, we have both a generate_excerpt and generate_titles methods that would be nice to support with this feature, as well as the generate_image_callback for DallE.

@bjorn2404
Copy link
Member Author

@dkotter Thanks for the info! Does this look closer now to what you were thinking? I've added a user_can_access method to both classes where the setting name is passed through to the filter dynamically. Naming is maybe a little odd but I thought it would be nice to pass the setting name along as part of the feature filter name - ex: classifai_chatgpt_user_can_enable_excerpt.

Also, not sure if it's worth keeping the previous allowed_image_roles filters I added. Let me know if you think there would be of any value. Otherwise, I will update to remove them.

dkotter
dkotter requested changes Jun 23, 2023
View reviewed changes
includes/Classifai/Providers/OpenAI/ChatGPT.php Outdated
* Filter the allowed WordPress roles for ChatGTP
*
* @since x.x.x
* @hook classifai_chatgpt_allowed_image_roles
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this filter should have image in it, I'm assuming this was probably just a copy/paste error

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dkotter The fix for this is now ready for review. Thanks!

cc @jeffpaul

includes/Classifai/Providers/OpenAI/ChatGPT.php Outdated
Comment on lines 76 to 79
if (
! empty( $settings ) &&
isset( $settings['authenticated'] ) &&
false !== $settings['authenticated'] &&
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would suggest we remove the authentication checks here.

  1. Seems to go against what this function says it does (checking if a user can access)
  2. And we recently introduced an is_configured method that does this

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dkotter, The fix for this is now ready for review. Could you point me in the right direction for the is_configured
method? This needs to be added as well. Thanks!

cc @jeffpaul

includes/Classifai/Providers/OpenAI/ChatGPT.php Outdated
Comment on lines 80 to 81
isset( $settings[ $feature ] ) &&
'no' !== $settings[ $feature ]
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Similar to the above, not sure I'd put this check here, was this checks if a feature is enabled, not if the user has access to the feature. I think I'd recommend removing this and instead adding our role checking in this method (which currently that hasn't been moved). So if we aren't authenticated or if the feature isn't turned on, no one will have access. But if both of those are true, we can then run this method to determine if an individual user should have access (by default, just checking if their role is allowed but developers can hook in and change access on a per user basis if needed).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dkotter, The fix for this is now ready for review.

cc @jeffpaul

includes/Classifai/Providers/OpenAI/ChatGPT.php Outdated
@@ -87,7 +124,7 @@ public function enqueue_editor_assets() {

if (
( ! empty( $excerpt_roles ) && empty( array_diff( $user_roles, $excerpt_roles ) ) )
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As mentioned above, I'd move this check within the new user_can_access method

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dkotter, The fix for this is now ready for review.

cc @jeffpaul

includes/Classifai/Providers/OpenAI/ChatGPT.php Outdated
@@ -87,7 +124,7 @@ public function enqueue_editor_assets() {

if (
( ! empty( $excerpt_roles ) && empty( array_diff( $user_roles, $excerpt_roles ) ) )
&& ( isset( $settings['enable_excerpt'] ) && 1 === (int) $settings['enable_excerpt'] )
&& $this->user_can_access( 'enable_excerpt', $excerpt_roles )
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As mentioned above, I'd leave this enabled check alone rather than moving it into the new method

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dkotter, The fix for this is now ready for review.

cc @jeffpaul

includes/Classifai/Providers/OpenAI/ChatGPT.php Outdated
@@ -423,7 +473,7 @@ public function generate_excerpt( int $post_id = 0 ) {

// These checks (and the one above) happen in the REST permission_callback,
// but we run them again here in case this method is called directly.
if ( empty( $settings ) || ( isset( $settings['authenticated'] ) && false === $settings['authenticated'] ) || ( isset( $settings['enable_excerpt'] ) && 'no' === $settings['enable_excerpt'] ) ) {
if ( ! $this->user_can_access( 'enable_excerpt' ) ) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As mentioned above, if we remove auth checks and enabled checks from the user_can_access method, I think these lines can stay as they are

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dkotter, The fix for this is now ready for review.

cc @jeffpaul

includes/Classifai/Providers/OpenAI/DallE.php Outdated
&& ( ! empty( $roles ) && empty( array_diff( $user_roles, $roles ) ) )
&& ( isset( $settings['enable_image_gen'] ) && 1 === (int) $settings['enable_image_gen'] )
) {
&& ( isset( $settings[ $feature ] ) && 1 === (int) $settings[ $feature ] ) ) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Similar here, I'd move this enabled check out of this method

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dkotter Could you point me in the right direction for the is_configured
method?

cc @jeffpaul

@dkotter
Copy link
Collaborator

dkotter commented Jun 23, 2023

In addition to some of the above comments, we have user role checking that happens in our REST endpoints which will need to be handled (check out the LanguageProcessing.php and ImageProcessing.php Services)

@jeffpaul jeffpaul requested a review from dkotter July 14, 2023 19:22
@dkotter dkotter marked this pull request as ready for review July 17, 2023 22:08
@dkotter dkotter requested review from a team and jeffpaul as code owners July 17, 2023 22:08
@dkotter dkotter merged commit 8371f26 into develop Jul 17, 2023
@dkotter dkotter deleted the feat/432-Add-new-filters-to-provide-more-granular-control branch July 17, 2023 22:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@phpbits phpbits phpbits left review comments

@dkotter dkotter dkotter approved these changes

@jeffpaul jeffpaul Awaiting requested review from jeffpaul jeffpaul is a code owner

Assignees

@bjorn2404 bjorn2404

@phpbits phpbits

Labels
None yet
Projects
None yet
Milestone
2.3.0
Development

Successfully merging this pull request may close these issues.

Add new filters to provide more granular control over new OpenAI features
4 participants
@bjorn2404 @dkotter @phpbits @jeffpaul