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

EZP-28851: Added credentials to the request checking if selected item can be considered as an image #357

Merged
merged 1 commit into from
Feb 28, 2018

Conversation

adamwojs
Copy link
Member

@adamwojs adamwojs commented Feb 26, 2018

Question Answer
Tickets https://jira.ez.no/browse/EZP-28851
Bug fix? yes
New feature? no
BC breaks? no
Tests pass? yes
Doc needed? no
License GPL-2.0

Added credentials to the request which checks if selected item can be considered as an image. Actual change: https://github.com/ezsystems/ezplatform-admin-ui/pull/357/files#diff-5ed292618b1d024126a31fe3cdd78983R20

Checklist:

  • Coding standards ($ composer fix-cs)
  • Ready for Code Review

@adamwojs
Copy link
Member Author

Q: Does regenerated *.js.map files should be included in PR?

@@ -17,6 +17,7 @@ export default class EzEmbedImageButton extends EzEmbedDiscoverContentButton {
const request = new Request(selection.item.ContentInfo.Content.ContentType._href, {
method: 'GET',
headers: {'Accept': 'application/vnd.ez.api.ContentType+json'},
Copy link
Member Author

Choose a reason for hiding this comment

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

Should we also include X-Siteaccess and X-CSRF-Token headers?

Copy link
Contributor

@andrerom andrerom Feb 26, 2018

Choose a reason for hiding this comment

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

X-CSRF-Token

not needed for GET

X-Siteaccess

Yes, it is used in the ez-embed so should be used here to.

@sunpietro Should we abstract fetch a bit to avoid these kind of issues happening?

Copy link
Contributor

@andrerom andrerom Feb 26, 2018

Choose a reason for hiding this comment

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

Or should we rather set mode: 'same-origin' instead of having to set credentials: 'same-origin' all over?

The reason why Cookies are missing is the mode: 'cors' afaik.
In which cases will we send REST request to another domain in these cases?

Ref: https://developer.mozilla.org/en-US/docs/Web/API/Request/mode

@micszo
Copy link
Member

micszo commented Feb 28, 2018

That's a huge patch. 😮

@micszo
Copy link
Member

micszo commented Feb 28, 2018

Should I worry about Storm complaining?
screen shot 2018-02-28 at 12 14 36

@sunpietro
Copy link
Contributor

@micszo @adamwojs I believe it can be smaller. This PR should contain only an updated map related only to a changed JS file: ez-embedimage.js (I'm writing the filename from memory).
Other maps can be safely removed.

@andrerom
Copy link
Contributor

@adamwojs can you ommit the non relevant map updates?

@adamwojs
Copy link
Member Author

@andrerom @sunpietro Done.

Copy link
Member

@micszo micszo left a comment

Choose a reason for hiding this comment

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

Retest OK on eZ Platform v2.0.2 with patch. 👍

@micszo micszo removed their assignment Feb 28, 2018
@lserwatka lserwatka merged commit 9473ff9 into 1.0 Feb 28, 2018
@andrerom andrerom deleted the ezp_28851 branch February 28, 2018 13:16
@lserwatka
Copy link
Member

@adamwojs please merge this up to master

@adamwojs
Copy link
Member Author

@lserwatka done.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

6 participants