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

Use a URL object in OpenInAppResponse #139

Merged
merged 3 commits into from
Aug 12, 2021

Conversation

ishank011
Copy link
Contributor

@ishank011 ishank011 commented Aug 3, 2021

This PR has a breaking change. Previously, we returned a string as the URL as part of OpenInAppResponse. Now, we return a complete URL object which also specifies the method to be used to call the URL, the headers, and the form parameters to be specified. Related cs3org/reva#1923

cc @wkloucek

@labkode labkode requested a review from wkloucek August 4, 2021 06:18
map<string, string> form_parameters = 3;
// OPTIONAL.
// The headers to be added to the request.
map<string, string> headers = 4;
Copy link
Contributor

Choose a reason for hiding this comment

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

I read up on forms a little bit. The GET method on a form submit will add the form fields to the query parameters in the URL whereas the POST method will put them in the body (https://developer.mozilla.org/en-US/docs/Learn/Forms/Sending_and_retrieving_form_data).

But in neither case there is an explicit option to set Headers. How would it be used then?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is possible to pass headers to iframes, albeit not in a straightforward manner https://stackoverflow.com/a/60571237. The apps which we're considering don't need any to be set but it would be a good functionality to have IMO.

@ishank011
Copy link
Contributor Author

I've added a couple of fields to ProviderInfo to store the icon URI and a desktop_only parameter based on this requirement. Now we return the complete provider object instead of just the name so the clients can use the icon field and we can filter these out in the HTTP layer based on the request's user-agent.

@glpatcern
Copy link
Member

As discussed, I'm merging this given that we tested the whole chain and we are ready to merge cs3org/reva#1968 as well.

@glpatcern glpatcern merged commit cf97f94 into cs3org:main Aug 12, 2021
@ishank011 ishank011 deleted the open-in-app-url branch August 12, 2021 14:20
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