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

Enhancement/566 Confirm Classification before Save #609

Merged
merged 79 commits into from
Nov 21, 2023
Merged

Conversation

faisal-alvi
Copy link
Member

@faisal-alvi faisal-alvi commented Nov 2, 2023

Description of the Change

  • This is an enhancement PR that adds a pop-up when the classify post button is clicked and displays the terms coming from the API along with the existing terms. So users can add or remove terms and when they're expectations meet they can simply save it.
  • In the popup, the terms with the prefix "[AI]" stand for the terms that are suggested by the API and do not exist in the post at the moment.
  • Only those taxonomies will be displayed that are enabled from the settings.
  • If users do not open the classification modal/popup, we remind them on the pre-publish panel as a suggestion to classify the post.
  • The popup is also opened once the response is received.
  • Users will see a notice when no unique terms are received from AI or all the received terms are already saved.
  • Also bumps minimum WP version from 5.8 to 6.1.

Screenshots

image

image

image

Closes #566

How to test the Change

  • Click a "Classify Post" button from the right sidebar in a post with enough content and confirm the popup shows the terms from ClassfAI and also includes the existing terms.
  • Try to add/remove the terms and save and see if the final set of terms is applied to the post.
  • Reload the page and click the Publish button, you should see the same suggestion dropdowns in the Pre-Publish panel.

Changelog Entry

Added - Confirm Classification before Save
Change - Bump minimum WP version from 5.8 to 6.1

Credits

Props @jeffpaul @dkotter @faisal-alvi

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.

@faisal-alvi faisal-alvi added the type:enhancement New feature or request. label Nov 2, 2023
@faisal-alvi faisal-alvi added this to the 2.4.0 milestone Nov 2, 2023
@faisal-alvi faisal-alvi self-assigned this Nov 2, 2023
@faisal-alvi faisal-alvi requested review from dkotter, jeffpaul and a team as code owners November 2, 2023 14:55
@faisal-alvi
Copy link
Member Author

faisal-alvi commented Nov 2, 2023

@jeffpaul We have discussed on a call that we should display a message when there are no more unique terms returned from the ClassifAI but IMO, we should keep displaying the taxonomy dropdown(s) even if there are no terms or only the terms that are already exist In the post. So if there are no terms, users may want to add more terms, and because they are already updating the terms in the popup, why not give them the ability to add terms in the empty terms taxonomy? For now, I have kept the taxonomy dropdowns regardless of whether there are no AI terms or any existing terms. cc @dkotter

dkotter
dkotter previously approved these changes Nov 15, 2023
@dkotter dkotter requested a review from berkod November 15, 2023 16:07
berkod
berkod previously approved these changes Nov 16, 2023
@jeffpaul
Copy link
Member

@faisal-alvi one item that came up in UAT today was that when we changed the Taxonomy selected in the ClassifAI settings (e.g., changing Category from Watson Category to the WP native Category) that the modal displayed in the editor did not properly update to show the newly selected mapped taxonomy. So, please look to update to ensure the modal shows whichever taxonomies are selected from the settings.

@dkotter
Copy link
Collaborator

dkotter commented Nov 16, 2023

Adding a bit more to the above, did a little digging and seems one of the issues is these lines: https://github.com/10up/classifai/pull/609/files#diff-b387b736a54bc04206792307dd8c2dffb5b5c416d7189f0063c352b2e8ce2142R256-R259. Seems we only support the Watson custom taxonomies but we need to support all taxonomies, since we allow them to map the data returned from Watson to any registered taxonomy within the settings page.

I think the most common scenario is someone mapping those to the core category and tag taxonomies so we need to ensure those work. If I comment out those mentioned lines, I do then see inputs in the modal for those taxonomies but no terms show up, so I'm assuming something else here needs to be changed as well.

I guess one thing we may end up needing to figure out is how to address hierarchical taxonomies (like categories). Seems we may need to have UI in place for both hierarchical and non-hierarchical taxonomies, whereas now we really only support non-hierarchical.

@faisal-alvi faisal-alvi dismissed stale reviews from berkod and dkotter via 3b79afe November 17, 2023 13:32
@faisal-alvi
Copy link
Member Author

The popup was limited to the Watson taxonomies,

const watsonTaxonomies = [
'watson-category',
'watson-concept',
'watson-entity',
'watson-keyword',
];

I have updated it to cover any taxonomy selected in the ClassifAI settings.

I guess one thing we may end up needing to figure out is how to address hierarchical taxonomies (like categories). Seems we may need to have UI in place for both hierarchical and non-hierarchical taxonomies, whereas now we really only support non-hierarchical.

@dkotter This requires some discussion on UI. I think this should be handled later in the other issue/PR.

dkotter
dkotter previously approved these changes Nov 20, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type:enhancement New feature or request.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Adjust handling of Language Processing's flow for classifying posts
4 participants