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

Static content URL in feedback templates is not absolute in the production setting #75

Open
atilante opened this issue Jun 11, 2020 · 2 comments
Labels
area: user interface effort: hours The issue seems to be small and should be doable in hours or few days. experience: beginner required knowledge estimate requester: Aalto teacher The issue is raised by a teacher from Aalto University status: requires a priority using this label to flag that need EDIT decision ASAP type: bug This is a bug.

Comments

@atilante
Copy link

In short: the variable course.static_url which can be used when rendering a custom feedback template is broken on production server. When I run the course in local development setting, it is correct: course.static_url = "http://localhost:8080/static/default/". When the same template is rendered on production setting, it is: course.static_url = "/static/CS-A1143_2020/", but it should be: "https://grader.cs.hut.fi/static/CS-A1143_2020/".

Longer explanation: I am developing JSAV exercises for Data Structures and Algorithms Y course. There is a need to use a feedback template which is rendered on mooc-grader. The feedback template should refer by URL to CSS and JavaScript files from the static directory of the course, for example, https://grader.cs.hut.fi/static/CS-A1141_2020/OpenDSA/ . Mooc-grader does not know its domain (grader.cs.hut.fi, localhost:8080 in development), because it is running inside a Docker container. The current solution that I know has these hardcoded domains in the Django template or Django view of the exercise. The view or template tries to detect whether it is running in a local development or in a production environment. Reference from A+ manual: https://github.com/apluslms/course-templates/blob/master/exercises/ajax_exercise/template.html From that perspective, it would be nice to have variable similar to course.static_url configured at the container startup so that it is set correctly ("http://localhost:8080/static/<course_key>" in local, "https://grader.cs.hut.fi:8080/static/<course_and_instance_key>" in production).

This feature would assist the work on stop using custom Django views on A+ courses (for example: https://version.aalto.fi/gitlab/course/traky/blob/2020/exercises/jsav/views/iframe.py ), as this is a potential security risk.

@raphendyr
Copy link
Contributor

Apparently static_url is already passed to the exercise renderind context, but that has not been documented: 6de29f3

In addition, for local containers a hack was implemented in 0aae89a (the reason why static_url seems correct in local content).

MOOC-Grader could configure that value also in production, though MOOC-Grader can't be sure where the static file is really hosted. Thus, I would proceed implementing this in the build pipeline, where we choose where static files are uploaded.

For now, easiest would be adding environment substitutions to the build.sh which is also going to be supported by apluslms/compile-rst container with roman. The end of the build.sh would then look something like this:

url=${STATIC_CONTENT_HOST:-{{ course.static_url }}}
for template in template.html; do
  mkdir -p "_build/templates/${template%/*}"
  sed "s#@@STATIC_CONTENT_URL@@#${host%%#*}#g" \
    "$template" > "_build/templates/$template"
done
make touchrst html

In exercise config yaml, one would then use _build/templates/exercises/ex1_foo.html in place of exercises/ex1_foo.html. Sadly, for this to work on production #57 should be fixed. Alternatively, STATIC_URL_HOST_INJECT should be defined.

That said, I would avoid making absolute value in static_url a part of public API, because it most likely would make future design way harder.

@markkuriekkinen markkuriekkinen added priority: low requester: Aalto teacher The issue is raised by a teacher from Aalto University type: bug This is a bug. labels Jun 18, 2020
@markkuriekkinen markkuriekkinen changed the title Static content URL is not absolute in production setting Static content URL in feedback templates is not absolute in the production setting Mar 30, 2022
@markkuriekkinen
Copy link
Contributor

#57 fixed the course build environment. There is an environment variable for the course build that contains the absolute static URL. However, this issue is about rendering feedback templates at the end of grading submissions.

It would be simple to make the template variable static_url absolute or add a new variable with absolute value. The server setting STATIC_URL_HOST_INJECT seems like it already creates an absolute URL and we could test that again. Nowadays, the course static files are hosted in the Git Manager service, not MOOC-Grader.

MOOC-Grader could configure that value also in production, though MOOC-Grader can't be sure where the static file is really hosted.

I think that the template variable static_url may point to the default location, which used to be in the static files of the MOOC-Grader course and since A+ v1.12, it is in the Git Manager course instance. If someone uses static files hosted elsewhere, they just don't use this variable. We probably won't ever have any complex and more flexible build pipeline with support for multiple static file host locations.

Note: template variables static_url and course.static_url might have different values. I didn't double-check this yet. It looks like static_url is the one that should be used.


#57:

The new gitmanager service in A+ v1.12 defines STATIC_CONTENT_HOST.

https://github.com/apluslms/gitmanager/blob/912a7fceff8b232445bbe6cb6d5791f2f4daccf3/builder/builder.py#L112

In RST courses, the build env variable STATIC_CONTENT_HOST should be used in the course's conf.py:

static_host = os.environ.get('STATIC_CONTENT_HOST', 'http://localhost:8080/static/default')

@markkuriekkinen markkuriekkinen added effort: hours The issue seems to be small and should be doable in hours or few days. experience: beginner required knowledge estimate status: requires a priority using this label to flag that need EDIT decision ASAP area: user interface and removed priority: low labels Mar 30, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: user interface effort: hours The issue seems to be small and should be doable in hours or few days. experience: beginner required knowledge estimate requester: Aalto teacher The issue is raised by a teacher from Aalto University status: requires a priority using this label to flag that need EDIT decision ASAP type: bug This is a bug.
Projects
Status: No status
Development

No branches or pull requests

3 participants