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

Process Existing Images with Computer Vision #135

Merged
merged 19 commits into from
Feb 26, 2020

Conversation

ryanwelcher
Copy link
Contributor

@ryanwelcher ryanwelcher commented Sep 20, 2019

Description of the Change

This PR introduces two features:

  1. The ability to process an image from the associated attachment page.
  2. The ability to process any image via the Media Modal.

Benefits

These features will allow users to process existing images via the admin.

Checklist:

  • I have read the CONTRIBUTING document.
  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • All new and existing tests passed.

Applicable Issues

#75

@ryanwelcher ryanwelcher self-assigned this Sep 20, 2019
@ryanwelcher
Copy link
Contributor Author

Meta box added that is tied into the post save. There are opt-in and are not tied to the automatic checkbox options in the settings.

Image without scanned data:

no-scanned-data

Image with scanned data:
with-scanned-data

@jeffpaul jeffpaul added this to the 1.4.0 milestone Sep 20, 2019
@jeffpaul jeffpaul added the type:enhancement New feature or request. label Sep 20, 2019
@helen helen modified the milestones: 1.4.0, 1.5.0 Sep 24, 2019
@ryanwelcher ryanwelcher marked this pull request as ready for review February 7, 2020 19:22
@ryanwelcher
Copy link
Contributor Author

@jeffpaul I've implemented the buttons and associated endpoints to allow scanning in the media modal.

Screenshot:
scanning_buttons

Copy link
Contributor

@dinhtungdu dinhtungdu left a comment

Choose a reason for hiding this comment

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

Rescan button, rescan checkbox and generate checkbox work as expected, only some minor bugs remain.

includes/Classifai/Providers/Azure/ComputerVision.php Outdated Show resolved Hide resolved
src/js/media.js Outdated Show resolved Hide resolved
src/js/media.js Outdated Show resolved Hide resolved
src/js/media.js Outdated Show resolved Hide resolved
includes/Classifai/Providers/Azure/ComputerVision.php Outdated Show resolved Hide resolved
includes/Classifai/Services/ImageProcessing.php Outdated Show resolved Hide resolved
includes/Classifai/Providers/Azure/ComputerVision.php Outdated Show resolved Hide resolved
@ryanwelcher
Copy link
Contributor Author

@dinhtungdu thanks for the review! I've addressed the issues you pointed out and just had one question for you.

@ryanwelcher
Copy link
Contributor Author

@dinhtungdu this is ready for review. Thanks!

src/js/media.js Outdated Show resolved Hide resolved
Adds logic to display correct CTA on the attachement edit screen.
@ryanwelcher
Copy link
Contributor Author

@dinhtungdu I've added a couple of changes. This is ready for review.

dinhtungdu
dinhtungdu previously approved these changes Feb 18, 2020
Copy link
Contributor

@dinhtungdu dinhtungdu left a comment

Choose a reason for hiding this comment

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

Awesome @ryanwelcher, let get this merged 🎉

dinhtungdu
dinhtungdu previously approved these changes Feb 18, 2020
Copy link
Contributor

@dinhtungdu dinhtungdu left a comment

Choose a reason for hiding this comment

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

👍

@jeffpaul
Copy link
Member

@ryanwelcher noting there are a couple merge conflicts to resolve before we get this merged in, you able to handle those conflicts?

@ryanwelcher
Copy link
Contributor Author

@dinhtungdu I resolved the merge conflicts and pushed. Can you review so I can merge, please?

Copy link
Contributor

@dinhtungdu dinhtungdu left a comment

Choose a reason for hiding this comment

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

🎉

@dinhtungdu
Copy link
Contributor

dinhtungdu commented Feb 25, 2020

I found a case when the API credentials are wrong or expired, the spinner will run forever without any feedback to users.

In the media detail page, check Generate caption and hit Update (with wrong/expired API credentials), no feedback about the error after saving too.

But due to our milestone, I think we can ship this PR and address the mentioned issue separately.

@jeffpaul jeffpaul mentioned this pull request Feb 26, 2020
19 tasks
@ryanwelcher ryanwelcher merged commit 1d60931 into develop Feb 26, 2020
@jeffpaul jeffpaul deleted the feature/computer-vision-process-existing-images branch March 5, 2020 01:04
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.

4 participants