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

new route to help img cropping for already uploaded image (to skip CO… #820

Merged
merged 4 commits into from
Nov 14, 2023

Conversation

frimpongopoku
Copy link
Contributor

@frimpongopoku frimpongopoku commented Oct 25, 2023

Works with frontend PR: massenergize/frontend-admin#1108

  • Created a new route that reads the content of an image in our S3 bucket and returns the base64 string of the image. Which is need for cropping on the F.E.

Note

To be able to implement cropping, we need to be able to read the image data. Doing that with a just-selected image from a user's device is straight forward. But for an image that has been uploaded, you need to be able to load the image via the link and try to read it's content. Doing that with the image links we have gives a cross-origin error (CORS). That is why I had to make a route that reads the image data and sends it to the frontend to be able to continue.

Things would have been easier and more straight forward if we could simply read the data from the image that is loaded. And I think that could probably be done with adding some new policies to our image buckets on AWS to looses the grip on accepted origins, but that might open us up to our images being used in various places we might not like... so as you can see the PR, I went with the B.E route which works nicely. 🤣 🤣 👌🏾

Copy link
Contributor

@abdullai-t abdullai-t left a comment

Choose a reason for hiding this comment

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

LGTM

@BradHN1
Copy link
Contributor

BradHN1 commented Oct 26, 2023

Unit tests for new route?

@frimpongopoku
Copy link
Contributor Author

Unit tests for new route?

Incoming 🚗

@frimpongopoku
Copy link
Contributor Author

Unit tests for new route?

  • Added the test for the read_image route

Copy link
Contributor

@BradHN1 BradHN1 left a comment

Choose a reason for hiding this comment

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

LGTM
I see a bunch of media library tests have been commented out. Do theuy need replaciing

@frimpongopoku
Copy link
Contributor Author

LGTM I see a bunch of media library tests have been commented out. Do they need replacing

They have already been replaced in new PRs. I will clear them out soon.

Copy link
Contributor

@BradHN1 BradHN1 left a comment

Choose a reason for hiding this comment

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

LGTM - test in dev

@BradHN1 BradHN1 merged commit 17b37b0 into development Nov 14, 2023
1 check failed
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