Skip to content

Make studio file uploads site aware - #482

Merged
OmarIthawi merged 1 commit into
appsembler/tahoe/developfrom
omar/upload-url
Nov 21, 2019
Merged

Make studio file uploads site aware#482
OmarIthawi merged 1 commit into
appsembler/tahoe/developfrom
omar/upload-url

Conversation

@OmarIthawi

@OmarIthawi OmarIthawi commented Nov 20, 2019

Copy link
Copy Markdown

This makes the assets API site-aware.

The assets API is what the File Page Upload uses.

This fixes: RED-147 - Itential: Make Files & Uploads web links microsite-aware.

@OmarIthawi OmarIthawi changed the title (WIP) Make studio file uploads site aware Make studio file uploads site aware Nov 20, 2019
@OmarIthawi
OmarIthawi marked this pull request as ready for review November 20, 2019 19:43

@johnbaldwin johnbaldwin left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

LGTM!

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Cool!

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

@OmarIthawi Do we need to check that asset_url has a leading slash? I assume asset_url is the resource path from the root?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Great question @johnbaldwin!

Yes, I think we do need it. I want to avoid the http->https redirect which Open edX enforces by default. The double slashes ensure that at least when a secure site uses the URL, it'll work.

The leading slashes break the link for when including the files in an on-disk HTML file (file://) but we don't care about this case.

Some annoyance is found when using $ curl:

$ curl //courses.edx.org/robots.txt
curl: (3) <url> malformed

I do like to have the leading slashes, but I'm open to removing them.

I've also renamed base to be domain so it's more clear, and yes asset_url looks like the following: /asset-v1:Red+Red+Red2020+type@asset+block@2017-Scrum-Guide-US.pdf

@OmarIthawi

Copy link
Copy Markdown
Author

Thanks @johnbaldwin! I've answered your comments. I'll merge for now. Let me know if this would need a follow up work!

@OmarIthawi
OmarIthawi merged commit 4cd6397 into appsembler/tahoe/develop Nov 21, 2019
@OmarIthawi
OmarIthawi deleted the omar/upload-url branch November 21, 2019 07:37
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.

2 participants