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.

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

Show the default prompt as the first prompt in our settings #610

Merged
merged 5 commits into from
Nov 6, 2023

Conversation

dkotter
Copy link
Collaborator

@dkotter dkotter commented Nov 3, 2023

Description of the Change

Following on from #602, this PR ensures that our default prompt within ClassifAI always shows as the first prompt in our settings. This prompt can't be edited and can't be removed. This makes it easier for someone to go back to using the ClassifAI default prompt.

Important to note that to keep things simple, this ClassifAI default prompt is added and saved just like any other prompt. This does mean the prompt itself is saved in the database. In order to support situations where we may change the default prompt at a code-level, if this prompt is selected, instead of using the prompt text that is stored in the database, we use the prompt that is in the code.

The new flow will be as follows:

When first setting up a feature that uses prompts (like Title Generation), the only prompt you'll see is the default that is included in ClassifAI. The prompt fields are there but hidden (this allows us to save this as the default prompt):

Default prompt

If a custom prompt is needed, instead of editing the default prompt (which is how things landed in #602), you'll now need to click the Add new prompt button and then set a custom prompt in the fields that are added. In addition, to use the new prompt, you'll need to set that as the default:

New prompt added

You can add as many new prompts as needed, set whichever of those you want to be the default and delete any of those prompts. But you can't ever delete the ClassifAI default prompt but you can at any point set that as the default again.

How to test the Change

  1. Checkout the changes in this PR
  2. If you've previously tested Feat: multiple prompts #602, you may need to delete the existing prompt data (easiest is to delete the openai_chatgpt option)
  3. Ensure you have proper OpenAI credentials in place and at least one feature turned on (like excerpt generation)
  4. Without entering a new prompt, test that feature out and ensure it works
  5. Add a new prompt but don't set it as the default. Ensure things save and test the feature out again, ensuring it works
  6. Set this new prompt as the default and ensure it saves and the feature still works
  7. Can continue testing by adding more prompts across all the various features, ensuring you can change which one is the default and can delete prompts
  8. Ensure you can set the ClassifAI default as the default prompt again and that the feature works
  9. If desired, you can add some logging or debug statements in any of the features to ensure the prompt that gets used is the prompt you're setting as the default

Changelog Entry

Changed - Ensure the default prompts in ClassifAI show as the first prompt in our settings and that they can't be removed or edited.

Credits

Props @dkotter, @jeffpaul

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.

@dkotter dkotter added this to the 2.4.0 milestone Nov 3, 2023
@dkotter dkotter self-assigned this Nov 3, 2023
@dkotter dkotter requested review from jeffpaul and a team as code owners November 3, 2023 15:04
@dkotter dkotter requested a review from ravinderk November 3, 2023 15:04
@dkotter
Copy link
Collaborator Author

dkotter commented Nov 3, 2023

Note that there are some failing tests here due to these changes but wanted to get some feedback on if we think this is the right approach before spending time getting those fixed up (and adding some additional tests)

@ravinderk
Copy link
Contributor

@dkotter I exceed the daily query limit, so I cannot test this feature.

I have following questions -

  • Is it intentional to show Classifai default prompt as Default prompt while selecting it? I see correct text when reloading the page.
image

@dkotter
Copy link
Collaborator Author

dkotter commented Nov 3, 2023

Is it intentional to show Classifai default prompt as Default prompt while selecting it? I see correct text when reloading the page.

Yes, though we can update this if we think it's worth it. I didn't change any of the JS logic for that, just on the PHP side, so it only shows Classifai default prompt after loading or reloading the page.

Edit: as part of the changes requested below, I've actually changed this to just always show Default prompt, as we now show ClassifAI default prompt as part of the main text

@jeffpaul
Copy link
Member

jeffpaul commented Nov 3, 2023

Structurally I think this is good. As a future UI iteration, I'd like to see us collapse a good bit of the UI around the default (we don't need the full text field/area) so that it takes up less vertical space. Something more like this:

classifai-default-prompt

classifai-default-prompt-2

However, I'm fine with the current implementation as it achieves the same purpose but a future tweak could make the UI a bit simpler/cleaner.

…ag and hide the other fields. Remove a field description that isn't needed. Remove styling that is no longer needed.
@dkotter
Copy link
Collaborator Author

dkotter commented Nov 3, 2023

Structurally I think this is good. As a future UI iteration, I'd like to see us collapse a good bit of the UI around the default (we don't need the full text field/area) so that it takes up less vertical space. Something more like this:

Yeah, I like this and considered something similar before sticking with what we had. But it was easy enough to make this change so I've gone ahead and done that:

Default prompt only

One custom prompt

ravinderk
ravinderk previously approved these changes Nov 6, 2023
Copy link
Contributor

@ravinderk ravinderk left a comment

Choose a reason for hiding this comment

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

@dkotter I left a few minor comments which are not blockers. This pull request is working as expected.

includes/Classifai/Providers/OpenAI/ChatGPT.php Outdated Show resolved Hide resolved
<input type="hidden"
name="<?php echo esc_attr( $field_name_prefix . "[$field_index][default]" ); ?>"
value="<?php echo esc_attr( $prompt['default'] ?? '' ); ?>"
class="js-setting-field__default">
<input type="hidden"
name="<?php echo esc_attr( $field_name_prefix . "[$field_index][original]" ); ?>"
value="<?php echo esc_attr( $prompt['original'] ?? '' ); ?>">
Copy link
Contributor

Choose a reason for hiding this comment

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

@dkotter I suggest use the trinary short syntax ?: instead of the null coalescing operator??, because original value can not be null, right?

Copy link
Collaborator Author

@dkotter dkotter Nov 6, 2023

Choose a reason for hiding this comment

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

So short ternary's aren't allowed by our PHPCS config so we would need to change this to ! empty( $prompt['original'] ? $prompt['original'] : '' which I'm fine with changing that if we feel it's better. The goal here was to be defensive for a situation where this array value wasn't set (which should never happen) and using the null coalescing operator would catch that.

@dkotter dkotter merged commit e81fc1e into develop Nov 6, 2023
13 checks passed
@dkotter dkotter deleted the feature/show-default-prompt branch November 6, 2023 17:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants