-
Notifications
You must be signed in to change notification settings - Fork 1
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
Fixed: Fix reversed section URL prefix #1024
Merged
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
drikusroor
force-pushed
the
fix/1020-fix-fetch-sections
branch
from
May 21, 2024 20:43
405a10e
to
0af4025
Compare
Alternatively, I could also simplify it and hard-code the localhost:8000 in the |
BeritJanssen
approved these changes
May 22, 2024
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice!
drikusroor
force-pushed
the
fix/1020-fix-fetch-sections
branch
from
May 22, 2024 08:59
3ef3397
to
f09d025
Compare
drikusroor
added a commit
that referenced
this pull request
May 27, 2024
…ll (#1034) * Fixed: Fix reversed section URL prefix (#1024) * fix: Prefix reversed section url with BASE_URL if present (cherry picked from commit b42f649) * chore: Add BASE_URL environment variable to production settings too (cherry picked from commit 0b4f6d8) * refactor: Fallback to "http://localhost:8000" even when Docker sets BASE_URL as an empty string (cherry picked from commit 0af4025) * refactor: Strip trailing slash from base url Co-authored-by: Berit <berit.janssen@gmail.com> (cherry picked from commit 3ef3397) * chore: Update package version to 2.1.0 * chore: Add release workflow for created releases
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR prefixes the section urls from the
absolute_url
method in the section'smodel.py
with an optionalBASE_URL
while removing the earlier usedMEDIA_ROOT
prefix from the fetch section actions in the frontend. This will make sure that the backend returns the correct section url while the frontend won't add a redundant/server
path prefix.There are two scenarios, which both require a slightly different configuration:
1. Local develop environment
As the frontend runs on
localhost:3000
and the backend onlocalhost:8000
, the returned section url needs to be prefixed withhttp://localhost:8000
in the backend, as theMEDIA_ROOT
(aka theAPI_ROOT
) doesn't do that anymore in the frontend. In Django's develop settings, theBASE_URL
setting is set tohttp://localhost:8000
by default. Theabsolute_url
method therefore returns"http://localhost:8000" + "/section/123/456" = http://localhost:8000/section/123/456
.2. Test / Acceptance / Production environments
As the frontend and the backend run on the same (sub) domain,
BASE_URL
needn't be configured. It therefore defaults to an empty string. Theabsolute_url
method will therefore return"" + "/section/123/456" = "/section/123/456"
. The frontend does not prefix this url with anything but fetches the path from the domain it is running on:https://(tst/acc/prd).amsterdammusiclab.nl/section/123/456
.How to test
Local develop environment
You do not need to set
BASE_URL
tohttp://localhost:8000
in your.env
file as it will already default to that value in Django's develop settings. Simply check if the audio and other files in the experiments are still working.Other environments
I've deployed this branch to the test environment using the workflow dispatch method (see here) and verified that the audio playback works for this matching pairs experiment: https://tst.amsterdammusiclab.nl/introductiono
Resolves #1020