From cebe8bfa4edd34bb10945371c3e61acdef782102 Mon Sep 17 00:00:00 2001 From: Isuru Fernando Date: Sat, 28 Sep 2019 23:49:37 -0500 Subject: [PATCH 01/19] Add forms validation and view all --- course/constants.py | 2 + course/forms.py | 172 +++++++++++++++++++++++++++++ course/templates/course/forms.html | 19 ++++ course/validation.py | 111 +++++++++++++++++++ relate/urls.py | 16 ++- 5 files changed, 319 insertions(+), 1 deletion(-) create mode 100644 course/forms.py create mode 100644 course/templates/course/forms.html diff --git a/course/constants.py b/course/constants.py index 514998f6c..36e671c17 100644 --- a/course/constants.py +++ b/course/constants.py @@ -38,6 +38,8 @@ COURSE_ID_REGEX = "(?P[-a-zA-Z0-9]+)" EVENT_KIND_REGEX = "(?P[_a-z0-9]+)" FLOW_ID_REGEX = "(?P[-_a-zA-Z0-9]+)" +FORM_ID_REGEX = "(?P[-_a-zA-Z0-9]+)" +FORM_FIELD_ID_REGEX = "(?P[-_a-zA-Z0-9]+)" GRADING_OPP_ID_REGEX = "(?P[-_a-zA-Z0-9]+)" # FIXME : Support page hierarchy. Add '/' here, fix validation code. STATICPAGE_PATH_REGEX = r"(?P[-\w]+)" diff --git a/course/forms.py b/course/forms.py new file mode 100644 index 000000000..51efa9b8c --- /dev/null +++ b/course/forms.py @@ -0,0 +1,172 @@ +# -*- coding: utf-8 -*- + +from __future__ import division + +__copyright__ = "Copyright (C) 2014 Andreas Kloeckner" + +__license__ = """ +Permission is hereby granted, free of charge, to any person obtaining a copy +of this software and associated documentation files (the "Software"), to deal +in the Software without restriction, including without limitation the rights +to use, copy, modify, merge, publish, distribute, sublicense, and/or sell +copies of the Software, and to permit persons to whom the Software is +furnished to do so, subject to the following conditions: + +The above copyright notice and this permission notice shall be included in +all copies or substantial portions of the Software. + +THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR +IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, +FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE +AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER +LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, +OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN +THE SOFTWARE. +""" + +from typing import cast, Tuple +import os + +import django.forms as forms +from django.utils.safestring import mark_safe +from django.contrib import messages # noqa +from django.core.exceptions import PermissionDenied, ObjectDoesNotExist +from django.utils.translation import ugettext, ugettext_lazy as _ +from django import http # noqa + +from crispy_forms.layout import Submit + +from course.utils import course_view, render_course_page + +from course.constants import participation_permission as pperm +from course.utils import ( # noqa + CoursePageContext) +from course.content import FlowPageDesc, get_course_repo, get_repo_blob, get_raw_yaml_from_repo + +# {{{ for mypy + +if False: + from typing import Text, Optional, Any, Iterable, Dict # noqa + +# }}} + +# {{{ sandbox session key prefix + +PAGE_SESSION_KEY_PREFIX = "cf_validated_sandbox_page" +ANSWER_DATA_SESSION_KEY_PREFIX = "cf_page_sandbox_answer_data" +PAGE_DATA_SESSION_KEY_PREFIX = "cf_page_sandbox_page_data" + +# }}} + + +class CreateFlowForm(forms.Form): + # prevents form submission with codemirror's empty textarea + use_required_attribute = False + + def __init__(self, initial_text, + language_mode, interaction_mode, help_text, *args, **kwargs): + # type: (Text, Text, Text, Text, *Any, **Any) -> None + super(SandboxForm, self).__init__(*args, **kwargs) + + from crispy_forms.helper import FormHelper + self.helper = FormHelper() + + from course.utils import get_codemirror_widget + cm_widget, cm_help_text = get_codemirror_widget( + language_mode=language_mode, + interaction_mode=interaction_mode) + + self.fields["content"] = forms.CharField( + required=False, + initial=initial_text, + widget=cm_widget, + help_text=mark_safe( + help_text + + " " + + ugettext("Press Alt/Cmd+(Shift+)P to preview.") + + " " + + cm_help_text), + label=_("Content")) + + # 'strip' attribute was added to CharField in Django 1.9 + # with 'True' as default value. + self.fields["content"].strip = False + + self.helper.add_input( + Submit("preview", _("Preview"), accesskey="p"), + ) + self.helper.add_input( + Submit("clear", _("Clear"), css_class="btn-default"), + ) + + +def list_form_names(repo, commit_sha): + # type: (Repo_ish, bytes) -> List[Text] + form_names = [] + try: + form_tree = get_repo_blob(repo, "forms", commit_sha) + except ObjectDoesNotExist: + # That's OK--no forms yet. + pass + else: + for entry in form_tree.items(): + if entry.path.endswith(b".yml"): + form_names.append(entry.path[:-4].decode("utf-8")) + + return sorted(form_names) + + +def get_all_forms(repo, commit_sha): + form_names = list_form_names(repo, commit_sha) + forms = [] + for name in form_names: + contents = get_raw_yaml_from_repo(repo, "forms/%s.yml" % name, commit_sha) + contents["name"] = name + forms.append(contents) + + return forms + + +def validate_form(form): + pass + + +@course_view +def view_all_forms(pctx): + if not pctx.has_permission(pperm.use_markup_sandbox): + raise PermissionDenied() + + forms = get_all_forms(pctx.repo, pctx.course_commit_sha) + + return render_course_page(pctx, "course/forms.html", { + "forms": forms, + }) + + +@course_view +def view_form(pctx, form_id): + if not pctx.has_permission(pperm.use_markup_sandbox): + raise PermissionDenied() + + def make_form(data=None): + help_text = (ugettext("Enter " + "RELATE markup.")) + return CreateFlowForm( + None, "markdown", request.user.editor_mode, + help_text, + data) + + request = pctx.request + + if request.method == "POST": + form = make_form(request.POST) + else: + form = make_form() + + return render_course_page(pctx, "course/sandbox-markup.html", { + "form": form, + "preview_text": preview_text, + }) + + diff --git a/course/templates/course/forms.html b/course/templates/course/forms.html new file mode 100644 index 000000000..c3ce71a8b --- /dev/null +++ b/course/templates/course/forms.html @@ -0,0 +1,19 @@ +{% extends "course/course-base.html" %} +{% load i18n %} + +{% load crispy_forms_tags %} +{% load static %} + +{%block header_extra %} + +{% endblock %} + +{% block content %} +

{% trans "Templates" %}

+ +{% endblock %} + diff --git a/course/validation.py b/course/validation.py index c0a646348..039b4c182 100644 --- a/course/validation.py +++ b/course/validation.py @@ -1157,6 +1157,77 @@ def validate_flow_desc(vctx, location, flow_desc): # }}} +# {{{ form validation + +def validate_form_desc(vctx, location, form_desc): + validate_struct( + vctx, + location, + form_desc, + required_attrs=[ + ("title", str), + ("description", "markup"), + ("type", str), + ("fields", list), + ("access_roles", list), + ("template", str), + ], + allowed_attrs=[], + ) + + for j, role in enumerate(form_desc.access_roles): + validate_role( + vctx, + "%s, role %d" % (location, j+1), + role) + + for j, field in enumerate(form_desc.fields): + validate_form_field( + vctx, + "%s, field %d" % (location, j+1), + field) + + # {{{ check field id uniqueness + + field_ids = set() + + for field in form_desc.fields: + if field.id in field_ids: + raise ValidationError( + string_concat("%(location)s: ", + _("form field id '%(field_id)s' not unique")) + % {'location': location, 'field_id': field.id}) + + field_ids.add(field.id) + + # }}} + + +def validate_form_field(vctx, location, field_desc): + validate_struct( + vctx, + location, + field_desc, + required_attrs=[ + ("id", str), + ("type", str), + ], + allowed_attrs=[ + ("values", list), + ("default", (str, int, float, bool)), + ], + ) + + from course.constants import FORM_FIELD_ID_REGEX + + if field_desc.type not in ["Text", "Integer", "Float", "Choice"]: + raise ValidationError( + string_concat("%(location)s: ", + _("form field type '%(field_type)s' not recognized")) + % {'location': location, 'field_type': field_desc.type}) + +# }}} + # {{{ calendar validation @@ -1424,6 +1495,21 @@ def validate_flow_id(vctx, location, flow_id): % location) +def validate_form_id(vctx, location, form_id): + # type: (ValidationContext, Text, Text) -> None + + from course.constants import FORM_ID_REGEX + match = re.match("^" + FORM_ID_REGEX + "$", form_id) + if match is None: + raise ValidationError( + string_concat("%s: ", + _("invalid form name. " + "Form names may only contain (roman) " + "letters, numbers, " + "dashes and underscores.")) + % location) + + def validate_static_page_name(vctx, location, page_name): # type: (ValidationContext, Text, Text) -> None @@ -1591,6 +1677,31 @@ def validate_course_content(repo, course_file, events_file, # }}} + # {{{ forms + + try: + forms_tree = get_repo_blob(repo, "forms", validate_sha) + except ObjectDoesNotExist: + # That's OK--no forms yet. + pass + else: + for entry in forms_tree.items(): + entry_path = entry.path.decode("utf-8") + if not entry_path.endswith(".yml"): + continue + + tmpl_id = entry_path[:-4] + location = entry_path + validate_form_id(vctx, location, tmpl_id) + + location = "forms/%s" % entry_path + form_desc = get_yaml_from_repo_safely(repo, location, + commit_sha=validate_sha) + + validate_form_desc(vctx, location, form_desc) + + # }}} + return vctx.warnings diff --git a/relate/urls.py b/relate/urls.py index 4b63ecdda..c64549727 100644 --- a/relate/urls.py +++ b/relate/urls.py @@ -27,7 +27,8 @@ from django.conf.urls import include, url from django.contrib import admin from django.conf import settings -from course.constants import COURSE_ID_REGEX, FLOW_ID_REGEX, STATICPAGE_PATH_REGEX +from course.constants import (COURSE_ID_REGEX, FLOW_ID_REGEX, STATICPAGE_PATH_REGEX, + FORM_ID_REGEX) import course.auth import course.views @@ -41,6 +42,7 @@ import course.analytics import course.exam import course.api +import course.forms urlpatterns = [ url(r"^login/$", @@ -156,6 +158,18 @@ course.sandbox.view_page_sandbox, name="relate-view_page_sandbox"), + url(r"^course" + "/" + COURSE_ID_REGEX + + "/forms/list", + course.forms.view_all_forms, + name="relate-view_all_forms"), + + url(r"^course" + "/" + COURSE_ID_REGEX + + "/forms/view/" + FORM_ID_REGEX, + course.forms.view_form, + name="relate-view_form"), + url("^purge-pageview-data/$", course.flow.purge_page_view_data, name="relate-purge_page_view_data"), From 3fce19e9d888f4e28f7ac799debc59640e7b58d9 Mon Sep 17 00:00:00 2001 From: Isuru Fernando Date: Sun, 29 Sep 2019 21:03:52 -0500 Subject: [PATCH 02/19] Done with validation --- course/forms.py | 231 +++++++++++++++++++++++------- course/templates/course/form.html | 34 +++++ course/validation.py | 17 ++- 3 files changed, 226 insertions(+), 56 deletions(-) create mode 100644 course/templates/course/form.html diff --git a/course/forms.py b/course/forms.py index 51efa9b8c..aad82b485 100644 --- a/course/forms.py +++ b/course/forms.py @@ -2,7 +2,7 @@ from __future__ import division -__copyright__ = "Copyright (C) 2014 Andreas Kloeckner" +__copyright__ = "Copyright (C) 2019 Isuru Fernando" __license__ = """ Permission is hereby granted, free of charge, to any person obtaining a copy @@ -26,6 +26,9 @@ from typing import cast, Tuple import os +import uuid +import textwrap +import yaml import django.forms as forms from django.utils.safestring import mark_safe @@ -41,7 +44,8 @@ from course.constants import participation_permission as pperm from course.utils import ( # noqa CoursePageContext) -from course.content import FlowPageDesc, get_course_repo, get_repo_blob, get_raw_yaml_from_repo +from course.content import FlowPageDesc, get_course_repo, get_repo_blob, get_yaml_from_repo, expand_yaml_macros +from relate.utils import dict_to_struct, Struct # {{{ for mypy @@ -59,45 +63,109 @@ # }}} -class CreateFlowForm(forms.Form): +class CreateForm(forms.Form): # prevents form submission with codemirror's empty textarea use_required_attribute = False - def __init__(self, initial_text, - language_mode, interaction_mode, help_text, *args, **kwargs): - # type: (Text, Text, Text, Text, *Any, **Any) -> None - super(SandboxForm, self).__init__(*args, **kwargs) + def __init__(self, form_fields): + super(CreateForm, self).__init__() from crispy_forms.helper import FormHelper self.helper = FormHelper() - from course.utils import get_codemirror_widget - cm_widget, cm_help_text = get_codemirror_widget( - language_mode=language_mode, - interaction_mode=interaction_mode) - - self.fields["content"] = forms.CharField( - required=False, - initial=initial_text, - widget=cm_widget, - help_text=mark_safe( - help_text - + " " - + ugettext("Press Alt/Cmd+(Shift+)P to preview.") - + " " - + cm_help_text), - label=_("Content")) - - # 'strip' attribute was added to CharField in Django 1.9 - # with 'True' as default value. - self.fields["content"].strip = False + self.form_fields = form_fields + self.id = str(uuid.uuid1()).replace("-", "") + + from django.utils.timezone import now + self.created_time = now().strftime("%Y-%m-%d %H:%M") + + for field in form_fields: + field_data = dict(required=True, + initial=field.value, + label=field.label) + if field.type == "Choice": + self.fields[field.id] = forms.ChoiceField( + choices=[(c, c) for c in field.choices], + **field_data) + elif field.type == "Text": + self.fields[field.id] = forms.CharField( + **field_data) + elif field.type == "Integer": + self.fields[field.id] = forms.IntegerField( + **field_data) + + if field.id == "template_in": + self.template_in = field.value + if field.id == "template_out": + file_out = field.value.rsplit(".", 1) + self.template_out = file_out[0] + "_" + self.id + if len(file_out) > 1: + self.template_out += "." + file_out[-1] + if field.id == "announce": + self.announce = str(field.value).lower() == "true" self.helper.add_input( - Submit("preview", _("Preview"), accesskey="p"), + Submit("submit", _("Submit"), accesskey="p"), ) self.helper.add_input( - Submit("clear", _("Clear"), css_class="btn-default"), + Submit("reset", _("Reset"), css_class="btn-default"), ) + self.helper.add_input( + Submit("validate", _("Validate"), css_class="btn-default"), + ) + + + def get_jinja_text(self): + text = textwrap.dedent(""" + {{% with id="{id}", + """).format(id=self.id) + for field in self.form_fields: + text += " {field_name}=\"{field_value}\",\n".format(field_name=field.id, field_value=field.value) + text += " created_time=\"{created_time}\" %}}".format(created_time=self.created_time) + text += textwrap.dedent(""" + {{% include "{template_in}" %}} + {{% endwith %}} + """).format(template_in=self.template_in) + return text, self.template_out + + +def process_value(field): + if field.type == "Integer": + try: + field.value = int(field.value) + except ValueError: + pass + elif field.type == "Float": + try: + field.value = float(field.value) + except ValueError: + pass + + +def process_form_fields(form_fields, data): + if "reset" in data: + data = {} + for field in form_fields: + if not hasattr(field, "label"): + field.label = field.id + if not hasattr(field, "help"): + field.help = field.label + + if field.id in data: + field.value = data[field.id] + + if field.type == "Choice": + choices = [] + for value in field.choices: + if value.startswith("~DEFAULT~"): + v = value[9:].strip() + choices.append(v) + if not hasattr(field, "value"): + field.value = v + else: + choices.append(value) + field.choices = choices + process_value(field) def list_form_names(repo, commit_sha): @@ -116,24 +184,24 @@ def list_form_names(repo, commit_sha): return sorted(form_names) +def get_form(repo, form_name, commit_sha): + contents = get_yaml_from_repo(repo, "forms/%s.yml" % form_name, commit_sha) + contents.name = form_name + return contents + + def get_all_forms(repo, commit_sha): form_names = list_form_names(repo, commit_sha) forms = [] for name in form_names: - contents = get_raw_yaml_from_repo(repo, "forms/%s.yml" % name, commit_sha) - contents["name"] = name + contents = get_form(repo, name, commit_sha) forms.append(contents) - return forms -def validate_form(form): - pass - - @course_view def view_all_forms(pctx): - if not pctx.has_permission(pperm.use_markup_sandbox): + if not pctx.has_permission(pperm.update_content): raise PermissionDenied() forms = get_all_forms(pctx.repo, pctx.course_commit_sha) @@ -145,28 +213,87 @@ def view_all_forms(pctx): @course_view def view_form(pctx, form_id): - if not pctx.has_permission(pperm.use_markup_sandbox): + if not pctx.has_permission(pperm.update_content): raise PermissionDenied() - def make_form(data=None): - help_text = (ugettext("Enter " - "RELATE markup.")) - return CreateFlowForm( - None, "markdown", request.user.editor_mode, - help_text, - data) + form_info = get_form(pctx.repo, form_id, pctx.course_commit_sha) + + from course.enrollment import get_participation_role_identifiers + roles = get_participation_role_identifiers( + pctx.course, pctx.participation) + + if not any(role in form_info.access_roles for role in roles): + raise PermissionDenied() + + def back_to_form(form, form_info, page_errors=None, page_warnings=None): + return render_course_page(pctx, "course/form.html", { + "form": form, + "description": form_info.description, + "title": form_info.title, + "page_errors": page_errors, + "page_warnings": page_warnings, + }) request = pctx.request - if request.method == "POST": - form = make_form(request.POST) + if request.method != "POST": + process_form_fields(form_info.fields, {}) + form = CreateForm(form_info.fields) + return back_to_form(form, form_info) + + process_form_fields(form_info.fields, request.POST) + form = CreateForm(form_info.fields) + + if "clear" in request.POST: + return back_to_form(form, form_info) + + new_page_source, file_out = form.get_jinja_text() + page_warnings = None + page_errors = None + try: + print(new_page_source) + new_page_source = expand_yaml_macros( + pctx.repo, pctx.course_commit_sha, new_page_source) + print(new_page_source) + + yaml_data = yaml.safe_load(new_page_source) # type: ignore + page_desc = dict_to_struct(yaml_data) + + if not isinstance(page_desc, Struct): + raise ValidationError("Provided page source code is not " + "a dictionary. Do you need to remove a leading " + "list marker ('-') or some stray indentation?") + + from course.validation import validate_flow_desc, ValidationContext + vctx = ValidationContext( + repo=pctx.repo, + commit_sha=pctx.course_commit_sha) + + validate_flow_desc(vctx, "form", page_desc) + + page_warnings = vctx.warnings + + except Exception: + import sys + tp, e, _ = sys.exc_info() + + page_errors = ( + ugettext("Page failed to load/validate") + + ": " + + "%(err_type)s: %(err_str)s" % { + "err_type": tp.__name__, "err_str": e}) # type: ignore + + return back_to_form(form, form_info, page_errors, page_warnings) else: - form = make_form() + # Yay, it did validate. + pass + + if "validate" in request.POST: + return back_to_form(form, form_info, page_errors, page_warnings) - return render_course_page(pctx, "course/sandbox-markup.html", { + return render_course_page(pctx, "course/form.html", { "form": form, - "preview_text": preview_text, + "description": form_info.description, + "title": form_info.title, }) - diff --git a/course/templates/course/form.html b/course/templates/course/form.html new file mode 100644 index 000000000..aeccf138d --- /dev/null +++ b/course/templates/course/form.html @@ -0,0 +1,34 @@ +{% extends "course/course-base.html" %} +{% load i18n %} + +{% load crispy_forms_tags %} +{% load static %} + +{% block content %} +

{{ title }}

+ {{ description }} + + {% if page_errors %} +
+ + {{ page_errors | safe }} +
+ {% endif %} + + {% if page_warnings %} +
+ + {% blocktrans trimmed %} Warnings were encountered when validating the page: {% endblocktrans %} + +
    + {% for w in page_warnings %} +
  • {{ w.location }}: {{ w.text }}
    • + {% endfor %} +
+
+ {% endif %} +
+ {% crispy form %} +
+{% endblock %} + diff --git a/course/validation.py b/course/validation.py index 039b4c182..6a2d74c96 100644 --- a/course/validation.py +++ b/course/validation.py @@ -1170,7 +1170,6 @@ def validate_form_desc(vctx, location, form_desc): ("type", str), ("fields", list), ("access_roles", list), - ("template", str), ], allowed_attrs=[], ) @@ -1202,6 +1201,15 @@ def validate_form_desc(vctx, location, form_desc): # }}} + # Check required fields + + for req_field in ["template_in", "template_out", "announce"]: + if req_field not in field_ids: + raise ValidationError( + string_concat("%(location)s: ", + _("required form field id '%(field_id)s' not found")) + % {'location': location, 'field_id': req_field}) + def validate_form_field(vctx, location, field_desc): validate_struct( @@ -1213,14 +1221,15 @@ def validate_form_field(vctx, location, field_desc): ("type", str), ], allowed_attrs=[ - ("values", list), - ("default", (str, int, float, bool)), + ("choices", list), + ("value", (str, int, float, bool)), + ("label", str), ], ) from course.constants import FORM_FIELD_ID_REGEX - if field_desc.type not in ["Text", "Integer", "Float", "Choice"]: + if field_desc.type not in ["Text", "Integer", "Float", "Choice", "Hidden"]: raise ValidationError( string_concat("%(location)s: ", _("form field type '%(field_type)s' not recognized")) From c435a2a95aedf4697e895c2bb98b75b7a0bb3877 Mon Sep 17 00:00:00 2001 From: Isuru Fernando Date: Sun, 29 Sep 2019 22:00:57 -0500 Subject: [PATCH 03/19] Fix created_time --- course/forms.py | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/course/forms.py b/course/forms.py index aad82b485..d79acb094 100644 --- a/course/forms.py +++ b/course/forms.py @@ -77,7 +77,7 @@ def __init__(self, form_fields): self.id = str(uuid.uuid1()).replace("-", "") from django.utils.timezone import now - self.created_time = now().strftime("%Y-%m-%d %H:%M") + self.created_time = now().strftime("%Y-%m-%d @ %H:%M") for field in form_fields: field_data = dict(required=True, @@ -251,10 +251,8 @@ def back_to_form(form, form_info, page_errors=None, page_warnings=None): page_warnings = None page_errors = None try: - print(new_page_source) new_page_source = expand_yaml_macros( pctx.repo, pctx.course_commit_sha, new_page_source) - print(new_page_source) yaml_data = yaml.safe_load(new_page_source) # type: ignore page_desc = dict_to_struct(yaml_data) From e9616d572df2c7d2ecf0d38a0a0c53ff7ff6e734 Mon Sep 17 00:00:00 2001 From: Isuru Fernando Date: Mon, 30 Sep 2019 00:41:38 -0500 Subject: [PATCH 04/19] Save new file and announce --- course/forms.py | 89 +++++++++++++++++++++++++++++++++++++------- course/validation.py | 2 +- relate/urls.py | 5 ++- 3 files changed, 80 insertions(+), 16 deletions(-) diff --git a/course/forms.py b/course/forms.py index d79acb094..a7bb88e3b 100644 --- a/course/forms.py +++ b/course/forms.py @@ -36,6 +36,7 @@ from django.core.exceptions import PermissionDenied, ObjectDoesNotExist from django.utils.translation import ugettext, ugettext_lazy as _ from django import http # noqa +from django.utils.timezone import now from crispy_forms.layout import Submit @@ -46,6 +47,7 @@ CoursePageContext) from course.content import FlowPageDesc, get_course_repo, get_repo_blob, get_yaml_from_repo, expand_yaml_macros from relate.utils import dict_to_struct, Struct +from course.versioning import run_course_update_command # {{{ for mypy @@ -76,8 +78,7 @@ def __init__(self, form_fields): self.form_fields = form_fields self.id = str(uuid.uuid1()).replace("-", "") - from django.utils.timezone import now - self.created_time = now().strftime("%Y-%m-%d @ %H:%M") + self.created_time = now() for field in form_fields: field_data = dict(required=True, @@ -116,12 +117,10 @@ def __init__(self, form_fields): def get_jinja_text(self): - text = textwrap.dedent(""" - {{% with id="{id}", - """).format(id=self.id) + text = "{{% with id=\"{id}\",\n".format(id=self.id) for field in self.form_fields: text += " {field_name}=\"{field_value}\",\n".format(field_name=field.id, field_value=field.value) - text += " created_time=\"{created_time}\" %}}".format(created_time=self.created_time) + text += " created_time=\"{created_time}\" %}}".format(created_time=self.created_time.strftime("%Y-%m-%d @ %H:%M")) text += textwrap.dedent(""" {{% include "{template_in}" %}} {{% endwith %}} @@ -247,12 +246,12 @@ def back_to_form(form, form_info, page_errors=None, page_warnings=None): if "clear" in request.POST: return back_to_form(form, form_info) - new_page_source, file_out = form.get_jinja_text() + page_source, file_out = form.get_jinja_text() page_warnings = None page_errors = None try: new_page_source = expand_yaml_macros( - pctx.repo, pctx.course_commit_sha, new_page_source) + pctx.repo, pctx.course_commit_sha, page_source) yaml_data = yaml.safe_load(new_page_source) # type: ignore page_desc = dict_to_struct(yaml_data) @@ -289,9 +288,73 @@ def back_to_form(form, form_info, page_errors=None, page_warnings=None): if "validate" in request.POST: return back_to_form(form, form_info, page_errors, page_warnings) - return render_course_page(pctx, "course/form.html", { - "form": form, - "description": form_info.description, - "title": form_info.title, - }) + course = pctx.course + content_repo = pctx.repo + + from course.content import SubdirRepoWrapper + if isinstance(content_repo, SubdirRepoWrapper): + repo = content_repo.repo + else: + repo = content_repo + + repo_head = repo[b"HEAD"] + repo_contents = [(entry.path, entry.sha, entry.mode) for entry in repo.object_store.iter_tree_contents(repo_head.tree)] + for entry in repo_contents: + if entry[0].decode("utf-8") == file_out: + page_errors = (ugettext("Target file already exists") + + ": " + file_out) + return back_to_form(form, form_info, page_errors, page_warnings) + + + from dulwich.objects import Blob, Commit + from dulwich.index import commit_tree + from time import time + + # Create a blob (file) and save in object store + blob = Blob.from_string(page_source.encode("utf-8")) + repo.object_store.add_object(blob) + + # Create a tree with the contents from HEAD and new file + repo_contents.append((file_out.encode("utf-8"), blob.id, 0o100644)) + tree_id = commit_tree(repo.object_store, repo_contents) + + user = pctx.participation.user + committer = "{} <{}>".format(user.username, user.email).encode("utf-8") + message = "Create page {} with form {}".format(file_out, form_id).encode("utf-8") + + # Create a commit with the tree and parent as HEAD. + commit = Commit() + commit.tree = tree_id + commit.parents = [repo_head.id] + commit.author = commit.committer = committer + commit.commit_time = commit.author_time = int(now().timestamp()) + commit.commit_timezone = commit.author_timezone = 0 + commit.encoding = b"UTF-8" + commit.message = message + repo.object_store.add_object(commit) + + if repo[b"HEAD"] != repo_head: + page_errors = (ugettext("Repo updated by somebody else. Try again.") + + ": " + file_out) + return back_to_form(form, form_info, page_errors, page_warnings) + + run_course_update_command( + request, repo, content_repo, pctx, "update", commit.id, True, + False) + + for message in messages.get_messages(request): + if message.level == messages.ERROR: + return back_to_form(form, form_info) + + if form_info.type == "flow" and hasattr(form, "announce") and form.announce: + from course.models import InstantFlowRequest + from datetime import timedelta + ifr = InstantFlowRequest() + ifr.course = course + ifr.flow_id = file_out.rsplit("/", 1)[-1][:-4] + ifr.start_time = form.created_time + ifr.end_time = form.created_time + timedelta(minutes=20) + ifr.save() + + return back_to_form(form, form_info) diff --git a/course/validation.py b/course/validation.py index 6a2d74c96..d2122e544 100644 --- a/course/validation.py +++ b/course/validation.py @@ -1203,7 +1203,7 @@ def validate_form_desc(vctx, location, form_desc): # Check required fields - for req_field in ["template_in", "template_out", "announce"]: + for req_field in ["template_in", "template_out"]: if req_field not in field_ids: raise ValidationError( string_concat("%(location)s: ", diff --git a/relate/urls.py b/relate/urls.py index c64549727..14859e4a9 100644 --- a/relate/urls.py +++ b/relate/urls.py @@ -160,13 +160,14 @@ url(r"^course" "/" + COURSE_ID_REGEX - + "/forms/list", + + "/forms/$", course.forms.view_all_forms, name="relate-view_all_forms"), url(r"^course" "/" + COURSE_ID_REGEX - + "/forms/view/" + FORM_ID_REGEX, + + "/forms/" + FORM_ID_REGEX + + "/$", course.forms.view_form, name="relate-view_form"), From 2436ff39291818ef56bfdf81f98c4dc8db85db9d Mon Sep 17 00:00:00 2001 From: Isuru Fernando Date: Mon, 30 Sep 2019 01:38:48 -0500 Subject: [PATCH 05/19] Validate only once --- course/forms.py | 126 +++++++++++------------ course/templates/course/course-base.html | 1 + course/templates/course/forms.html | 2 +- 3 files changed, 61 insertions(+), 68 deletions(-) diff --git a/course/forms.py b/course/forms.py index a7bb88e3b..c823e89db 100644 --- a/course/forms.py +++ b/course/forms.py @@ -46,7 +46,7 @@ from course.utils import ( # noqa CoursePageContext) from course.content import FlowPageDesc, get_course_repo, get_repo_blob, get_yaml_from_repo, expand_yaml_macros -from relate.utils import dict_to_struct, Struct +from relate.utils import dict_to_struct, Struct, string_concat from course.versioning import run_course_update_command # {{{ for mypy @@ -56,15 +56,6 @@ # }}} -# {{{ sandbox session key prefix - -PAGE_SESSION_KEY_PREFIX = "cf_validated_sandbox_page" -ANSWER_DATA_SESSION_KEY_PREFIX = "cf_page_sandbox_answer_data" -PAGE_DATA_SESSION_KEY_PREFIX = "cf_page_sandbox_page_data" - -# }}} - - class CreateForm(forms.Form): # prevents form submission with codemirror's empty textarea use_required_attribute = False @@ -224,13 +215,11 @@ def view_form(pctx, form_id): if not any(role in form_info.access_roles for role in roles): raise PermissionDenied() - def back_to_form(form, form_info, page_errors=None, page_warnings=None): + def back_to_form(form, form_info): return render_course_page(pctx, "course/form.html", { "form": form, "description": form_info.description, "title": form_info.title, - "page_errors": page_errors, - "page_warnings": page_warnings, }) request = pctx.request @@ -247,46 +236,8 @@ def back_to_form(form, form_info, page_errors=None, page_warnings=None): return back_to_form(form, form_info) page_source, file_out = form.get_jinja_text() - page_warnings = None - page_errors = None - try: - new_page_source = expand_yaml_macros( - pctx.repo, pctx.course_commit_sha, page_source) - - yaml_data = yaml.safe_load(new_page_source) # type: ignore - page_desc = dict_to_struct(yaml_data) - - if not isinstance(page_desc, Struct): - raise ValidationError("Provided page source code is not " - "a dictionary. Do you need to remove a leading " - "list marker ('-') or some stray indentation?") - - from course.validation import validate_flow_desc, ValidationContext - vctx = ValidationContext( - repo=pctx.repo, - commit_sha=pctx.course_commit_sha) - validate_flow_desc(vctx, "form", page_desc) - - page_warnings = vctx.warnings - - except Exception: - import sys - tp, e, _ = sys.exc_info() - - page_errors = ( - ugettext("Page failed to load/validate") - + ": " - + "%(err_type)s: %(err_str)s" % { - "err_type": tp.__name__, "err_str": e}) # type: ignore - - return back_to_form(form, form_info, page_errors, page_warnings) - else: - # Yay, it did validate. - pass - - if "validate" in request.POST: - return back_to_form(form, form_info, page_errors, page_warnings) + # {{{ Check if file already exists course = pctx.course content_repo = pctx.repo @@ -304,17 +255,17 @@ def back_to_form(form, form_info, page_errors=None, page_warnings=None): page_errors = (ugettext("Target file already exists") + ": " + file_out) return back_to_form(form, form_info, page_errors, page_warnings) + # }}} - - from dulwich.objects import Blob, Commit - from dulwich.index import commit_tree - from time import time - - # Create a blob (file) and save in object store + # {{{ Create a blob (file) and save in object store + from dulwich.objects import Blob blob = Blob.from_string(page_source.encode("utf-8")) repo.object_store.add_object(blob) - # Create a tree with the contents from HEAD and new file + # }}} + + # {{{ Create a tree with the contents from HEAD and new file + from dulwich.index import commit_tree repo_contents.append((file_out.encode("utf-8"), blob.id, 0o100644)) tree_id = commit_tree(repo.object_store, repo_contents) @@ -322,7 +273,10 @@ def back_to_form(form, form_info, page_errors=None, page_warnings=None): committer = "{} <{}>".format(user.username, user.email).encode("utf-8") message = "Create page {} with form {}".format(file_out, form_id).encode("utf-8") - # Create a commit with the tree and parent as HEAD. + # }}} + + # {{{ Create a commit with the tree and parent as HEAD. + from dulwich.objects import Commit commit = Commit() commit.tree = tree_id commit.parents = [repo_head.id] @@ -333,18 +287,55 @@ def back_to_form(form, form_info, page_errors=None, page_warnings=None): commit.message = message repo.object_store.add_object(commit) + # }}} + + # {{{ validate + + from course.validation import validate_course_content, ValidationError + try: + warnings = validate_course_content( + content_repo, course.course_file, course.events_file, + commit.id, course=course) + except ValidationError as e: + messages.add_message(request, messages.ERROR, + _("Course content did not validate successfully: '%s' " + "Update not applied.") % str(e)) + return back_to_form(form, form_info) + else: + if not warnings: + messages.add_message(request, messages.SUCCESS, + _("Course content validated successfully.")) + else: + messages.add_message(request, messages.WARNING, + string_concat( + _("Course content validated OK, with warnings: "), + "
    %s
") + % ("".join( + "
  • %(location)s: %(warningtext)s
    • " + % {'location': w.location, 'warningtext': w.text} + for w in warnings))) + # }}} + + if "validate" in request.POST: + return back_to_form(form, form_info) + + if pctx.participation.preview_git_commit_sha is not None: + messages.add_message(request, messages.ERROR, + _("Cannot apply update while previewing. ")) + return back_to_form(form, form_info) + if repo[b"HEAD"] != repo_head: page_errors = (ugettext("Repo updated by somebody else. Try again.") + ": " + file_out) - return back_to_form(form, form_info, page_errors, page_warnings) + return back_to_form(form, form_info) - run_course_update_command( - request, repo, content_repo, pctx, "update", commit.id, True, - False) + repo[b"HEAD"] = commit.id + course.active_git_commit_sha = commit.id.decode() + course.save() + messages.add_message(request, messages.SUCCESS, + _("Update applied. ")) - for message in messages.get_messages(request): - if message.level == messages.ERROR: - return back_to_form(form, form_info) + # {{{ Create InstantFlow if form_info.type == "flow" and hasattr(form, "announce") and form.announce: from course.models import InstantFlowRequest @@ -355,6 +346,7 @@ def back_to_form(form, form_info, page_errors=None, page_warnings=None): ifr.start_time = form.created_time ifr.end_time = form.created_time + timedelta(minutes=20) ifr.save() + # }}} return back_to_form(form, form_info) diff --git a/course/templates/course/course-base.html b/course/templates/course/course-base.html index 4d206bbfc..1a62df2d1 100644 --- a/course/templates/course/course-base.html +++ b/course/templates/course/course-base.html @@ -119,6 +119,7 @@ {% if pperm.use_markup_sandbox %}
  • {% trans "Markup sandbox" %}
    • {% endif %} +
  • {% trans "Forms" %}
    • {% if pperm.test_flow %} diff --git a/course/templates/course/forms.html b/course/templates/course/forms.html index c3ce71a8b..594370d1a 100644 --- a/course/templates/course/forms.html +++ b/course/templates/course/forms.html @@ -9,7 +9,7 @@ {% endblock %} {% block content %} -

    {% trans "Templates" %}

    +

    {% trans "Forms" %}

      {% for form in forms %}
    • {{ form.title }}
      • From 58ee0680367ef41016aabbf622c1cd803ceb1b00 Mon Sep 17 00:00:00 2001 From: Isuru Fernando Date: Mon, 30 Sep 2019 02:03:53 -0500 Subject: [PATCH 06/19] Use local time --- course/forms.py | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/course/forms.py b/course/forms.py index c823e89db..c01db2930 100644 --- a/course/forms.py +++ b/course/forms.py @@ -108,10 +108,13 @@ def __init__(self, form_fields): def get_jinja_text(self): + from relate.utils import as_local_time + created_time = as_local_time(self.created_time).strftime("%Y-%m-%d @ %H:%M") + text = "{{% with id=\"{id}\",\n".format(id=self.id) for field in self.form_fields: text += " {field_name}=\"{field_value}\",\n".format(field_name=field.id, field_value=field.value) - text += " created_time=\"{created_time}\" %}}".format(created_time=self.created_time.strftime("%Y-%m-%d @ %H:%M")) + text += " created_time=\"{created_time}\" %}}".format(created_time=created_time) text += textwrap.dedent(""" {{% include "{template_in}" %}} {{% endwith %}} From 5b705f61174815c33789e97f6425b658fe9ea8db Mon Sep 17 00:00:00 2001 From: Isuru Fernando Date: Mon, 30 Sep 2019 13:25:35 -0500 Subject: [PATCH 07/19] Use time for id --- course/forms.py | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/course/forms.py b/course/forms.py index c01db2930..4e233a182 100644 --- a/course/forms.py +++ b/course/forms.py @@ -46,7 +46,7 @@ from course.utils import ( # noqa CoursePageContext) from course.content import FlowPageDesc, get_course_repo, get_repo_blob, get_yaml_from_repo, expand_yaml_macros -from relate.utils import dict_to_struct, Struct, string_concat +from relate.utils import dict_to_struct, Struct, string_concat, as_local_time from course.versioning import run_course_update_command # {{{ for mypy @@ -67,9 +67,8 @@ def __init__(self, form_fields): self.helper = FormHelper() self.form_fields = form_fields - self.id = str(uuid.uuid1()).replace("-", "") - self.created_time = now() + self.id = as_local_time(self.created_time).strftime("%Y%m%d_%H%M%S_%f") for field in form_fields: field_data = dict(required=True, @@ -108,7 +107,6 @@ def __init__(self, form_fields): def get_jinja_text(self): - from relate.utils import as_local_time created_time = as_local_time(self.created_time).strftime("%Y-%m-%d @ %H:%M") text = "{{% with id=\"{id}\",\n".format(id=self.id) From edced1d85850d9efaba118b52483cca3f009aa51 Mon Sep 17 00:00:00 2001 From: Isuru Fernando Date: Sat, 12 Oct 2019 20:41:55 -0500 Subject: [PATCH 08/19] Add docs --- doc/flow.rst | 154 +++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 154 insertions(+) diff --git a/doc/flow.rst b/doc/flow.rst index fa26f0e0a..dc12cf1f7 100644 --- a/doc/flow.rst +++ b/doc/flow.rst @@ -789,6 +789,160 @@ Life cycle .. autoclass:: flow_session_expiration_mode + +Templated Flows and Forms +------------------------- + +Forms provide a web interface for creating flows from a templated flow. +A templated flow is a flow with an extension `.jinja` that has one or more +undefined jinja variables used in it. These undefined jinja variables +are filled using a form submitted by an admin user. When an admin user fills +the fields in the web form, values from the fields are substituted into the +undefined jinja variables in the templated flow and a new flow is created. +This is useful for creating flows without access to the git repository and +also provides a way to create a flow in the during a lecture. + +For example, an instructor wants to create a multiple choice question in the +middle of the lecture and have students submit an answer within 10 minutes of +the announcement similar to an i-clicker. Then, a templated flow looks like +the following: + + + title: "{{ title }}" + + description: | + # RELATE Instant Flow + + rules: + start: + - + if_before: end_of_class + if_has_role: [student, ta, instructor] + if_has_fewer_sessions_than: 1 + may_start_new_session: True + may_list_existing_sessions: True + + - + may_start_new_session: False + may_list_existing_sessions: True + + access: + - + permissions: [view, submit_answer, end_session, see_correctness, see_answer_after_submission] + + grade_identifier: instant_quiz_{{ id }} + grade_aggregation_strategy: use_latest + + grading: + - + if_completed_before: "{{ created_time }} + {{ duration }} minutes" + credit_percent: 100 + + - + credit_percent: 0 + + pages: + - + type: ChoiceQuestion + id: instant_{{ id }} + title: {{ title }} + shuffle: True + prompt: | + + {{ description }} + + choices: + - {{ choice1 }} + - {{ choice2 }} + - {{ choice3 }} + +The jinja variables `title, description, choice1, choice2, choice3, duration, +created_time, id` are undefined and needs to be created by the form. +REALTE will fill in `created_time` and `id` and the others need to be fields +in the form. A form must have the fields `template_in`, `template_out` +which correspond to the name of the templted flow and actual flow respectively. +`template_out` will get the timestamp appended to make the file name unique. +A special field `announce` will create an Instant Flow request which will make +the flow visible to all visitors to the course page. + +An example form description to create a form that an admin user can submit via +the web interface is as follows. Field types are one of +`Text, Integer, Float, Choice, Hidden`. Hidden fields are not shown to the admin +user, but their default values are substituted in the templated flow. + +title: "Create an instant flow with one multiple choice question" + + description: | + Instructions on filling out this form + type: flow + + access_roles: [ta, instructor] + + fields: + - id: title + type: Text + value: "InClass quiz" + label: "Title" + + - id: description + type: Text + value: "" + label: "Question" + + - id: duration + type: Integer + value: "20" + label: "Duration in minutes for the flow" + + - id: choice1 + type: Text + value: "~CORRECT~ (a)" + label: "Correct choice" + + - id: choice2 + type: Text + value: "(b)" + label: "Incorrect choice" + + - id: choice3 + type: Text + value: "(c)" + label: "Incorrect choice" + + - id: template_in + type: Hidden + value: "flows/instant_flow.jinja" + + - id: template_out + type: Hidden + value: "flows/instant_flow.yml" + + - id: announce + type: Choice + choices: + - ~DEFAULT~ True + - False + label: "Announce to the class" + + +When the admin user submits the form, a flow will be created using the +values the admin user filled in the web form like the following: + + {% with id="20190930_022148_311577", + title="InClass quiz", + description="What's 1 + 1?", + duration="20", + choice1="~CORRECT~ 2", + choice2="1", + choice3="3", + template_in="flows/instant_flow.jinja", + template_out="flows/instant_flow.yml", + announce="True", + created_time="2019-09-30 @ 02:21" %} + {% include "flows/instant_flow.jinja" %} + {% endwith %} + + Sample Rule Sets ---------------- From f698507aa25d6b833174ced9cca51494a191666f Mon Sep 17 00:00:00 2001 From: Isuru Fernando Date: Wed, 16 Oct 2019 22:11:07 -0500 Subject: [PATCH 09/19] Fix already existing tests --- .../test_validate_course_content.py | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) diff --git a/tests/test_validation/test_validate_course_content.py b/tests/test_validation/test_validate_course_content.py index 6a561b50e..b4bf02cf9 100644 --- a/tests/test_validation/test_validate_course_content.py +++ b/tests/test_validation/test_validate_course_content.py @@ -244,6 +244,10 @@ def get_repo_blob_side_effect(repo, full_name, commit_sha, allow_tree=True): tree.add(staticpage2_location.encode(), stat.S_IFREG, b"a static page") return tree + if full_name == "forms": + tree = Tree() + tree.add(b"not_a_form", stat.S_IFREG, b"not a form") + return tree if full_name == "": return Tree() @@ -267,6 +271,10 @@ def get_repo_blob_side_effect1(repo, full_name, commit_sha, allow_tree=True): tree.add(staticpage2_location.encode(), stat.S_IFREG, b"a static page") return tree + if full_name == "forms": + tree = Tree() + tree.add(b"not_a_form", stat.S_IFREG, b"not a form") + return tree if full_name == "": return Tree() @@ -285,6 +293,10 @@ def get_repo_blob_side_effect2(repo, full_name, commit_sha, allow_tree=True): tree.add(staticpage2_location.encode(), stat.S_IFREG, b"a static page") return tree + if full_name == "forms": + tree = Tree() + tree.add(b"not_a_form", stat.S_IFREG, b"not a form") + return tree if full_name == "": return Tree() @@ -301,6 +313,10 @@ def get_repo_blob_side_effect3(repo, full_name, commit_sha, allow_tree=True): return tree if full_name == "staticpages": raise ObjectDoesNotExist() + if full_name == "forms": + tree = Tree() + tree.add(b"not_a_form", stat.S_IFREG, b"not a form") + return tree if full_name == "": return Tree() From 4c4f30eb97b3d52a623571600b3a24ad3bda59ad Mon Sep 17 00:00:00 2001 From: Isuru Fernando Date: Wed, 16 Oct 2019 22:11:19 -0500 Subject: [PATCH 10/19] Fix formatting --- course/forms.py | 37 +++++++++++++++++-------------------- course/validation.py | 15 +++++++++++++-- 2 files changed, 30 insertions(+), 22 deletions(-) diff --git a/course/forms.py b/course/forms.py index 4e233a182..25ed2b09b 100644 --- a/course/forms.py +++ b/course/forms.py @@ -24,17 +24,12 @@ THE SOFTWARE. """ -from typing import cast, Tuple -import os -import uuid import textwrap -import yaml import django.forms as forms -from django.utils.safestring import mark_safe from django.contrib import messages # noqa from django.core.exceptions import PermissionDenied, ObjectDoesNotExist -from django.utils.translation import ugettext, ugettext_lazy as _ +from django.utils.translation import ugettext_lazy as _ from django import http # noqa from django.utils.timezone import now @@ -45,17 +40,18 @@ from course.constants import participation_permission as pperm from course.utils import ( # noqa CoursePageContext) -from course.content import FlowPageDesc, get_course_repo, get_repo_blob, get_yaml_from_repo, expand_yaml_macros -from relate.utils import dict_to_struct, Struct, string_concat, as_local_time -from course.versioning import run_course_update_command +from course.content import get_repo_blob, get_yaml_from_repo +from relate.utils import string_concat, as_local_time # {{{ for mypy if False: - from typing import Text, Optional, Any, Iterable, Dict # noqa + from typing import Text, Optional, Any, Iterable, Dict, List # noqa + from relate.utils import Repo_ish # noqa # }}} + class CreateForm(forms.Form): # prevents form submission with codemirror's empty textarea use_required_attribute = False @@ -105,14 +101,15 @@ def __init__(self, form_fields): Submit("validate", _("Validate"), css_class="btn-default"), ) - def get_jinja_text(self): created_time = as_local_time(self.created_time).strftime("%Y-%m-%d @ %H:%M") text = "{{% with id=\"{id}\",\n".format(id=self.id) for field in self.form_fields: - text += " {field_name}=\"{field_value}\",\n".format(field_name=field.id, field_value=field.value) - text += " created_time=\"{created_time}\" %}}".format(created_time=created_time) + text += " {field_name}=\"{field_value}\",\n".format( + field_name=field.id, field_value=field.value) + text += " created_time=\"{created_time}\" %}}".format( + created_time=created_time) text += textwrap.dedent(""" {{% include "{template_in}" %}} {{% endwith %}} @@ -250,12 +247,13 @@ def back_to_form(form, form_info): repo = content_repo repo_head = repo[b"HEAD"] - repo_contents = [(entry.path, entry.sha, entry.mode) for entry in repo.object_store.iter_tree_contents(repo_head.tree)] + repo_contents = [(entry.path, entry.sha, entry.mode) for entry in + repo.object_store.iter_tree_contents(repo_head.tree)] for entry in repo_contents: if entry[0].decode("utf-8") == file_out: - page_errors = (ugettext("Target file already exists") - + ": " + file_out) - return back_to_form(form, form_info, page_errors, page_warnings) + messages.add_message(request, messages.ERROR, + _("Target file: '%s' already exists ") % file_out) + return back_to_form(form, form_info) # }}} # {{{ Create a blob (file) and save in object store @@ -326,8 +324,8 @@ def back_to_form(form, form_info): return back_to_form(form, form_info) if repo[b"HEAD"] != repo_head: - page_errors = (ugettext("Repo updated by somebody else. Try again.") - + ": " + file_out) + messages.add_message(request, messages.ERROR, + _("Repo updated by somebody else. Try again.")) return back_to_form(form, form_info) repo[b"HEAD"] = commit.id @@ -350,4 +348,3 @@ def back_to_form(form, form_info): # }}} return back_to_form(form, form_info) - diff --git a/course/validation.py b/course/validation.py index d2122e544..ce8b17d72 100644 --- a/course/validation.py +++ b/course/validation.py @@ -1159,6 +1159,7 @@ def validate_flow_desc(vctx, location, flow_desc): # {{{ form validation + def validate_form_desc(vctx, location, form_desc): validate_struct( vctx, @@ -1227,14 +1228,24 @@ def validate_form_field(vctx, location, field_desc): ], ) - from course.constants import FORM_FIELD_ID_REGEX - if field_desc.type not in ["Text", "Integer", "Float", "Choice", "Hidden"]: raise ValidationError( string_concat("%(location)s: ", _("form field type '%(field_type)s' not recognized")) % {'location': location, 'field_type': field_desc.type}) + from course.constants import FORM_FIELD_ID_REGEX + + match = re.match("^" + FORM_FIELD_ID_REGEX + "$", field_desc.id) + if match is None: + raise ValidationError( + string_concat("%s: ", + _("invalid form field id. " + "Form field id may only contain (roman) " + "letters, numbers, " + "dashes and underscores.")) + % location) + # }}} From 731a1855cf9428bd8115c32da5cd40313984d11c Mon Sep 17 00:00:00 2001 From: Isuru Fernando Date: Thu, 17 Oct 2019 02:57:31 -0500 Subject: [PATCH 11/19] Test validation --- .../test_validate_course_content.py | 21 +++ .../test_validation/test_validation_tools.py | 121 ++++++++++++++++++ 2 files changed, 142 insertions(+) diff --git a/tests/test_validation/test_validate_course_content.py b/tests/test_validation/test_validate_course_content.py index b4bf02cf9..39f18f87f 100644 --- a/tests/test_validation/test_validate_course_content.py +++ b/tests/test_validation/test_validate_course_content.py @@ -139,6 +139,11 @@ staticpage2_id = "spage2" staticpage2_desc = mock.MagicMock() +form1_path = "forms/form1.yml" +form1_location = "form1.yml" +form1_id = "form1" +form1_desc = mock.MagicMock() + flow1_path = "flows/flow1.yml" flow1_location = "flow1.yml" flow1_id = "flow1" @@ -203,6 +208,9 @@ def get_yaml_from_repo_safely_side_effect(repo, full_name, commit_sha): if full_name == staticpage2_path: return staticpage2_desc + if full_name == form1_path: + return form1_desc + return get_yaml_from_repo_safely(repo, full_name, commit_sha) @@ -223,6 +231,9 @@ def get_yaml_from_repo_safely_with_duplicate_grade_identifier_side_effect( if full_name == staticpage2_path: return staticpage2_desc + if full_name == form1_path: + return form1_desc + return get_yaml_from_repo_safely(repo, full_name, commit_sha) @@ -247,6 +258,7 @@ def get_repo_blob_side_effect(repo, full_name, commit_sha, allow_tree=True): if full_name == "forms": tree = Tree() tree.add(b"not_a_form", stat.S_IFREG, b"not a form") + tree.add(form1_location.encode(), stat.S_IFREG, b"a form") return tree if full_name == "": return Tree() @@ -343,6 +355,11 @@ def setUp(self): self.mock_validate_staticpage_desc = fake_validate_staticpage_desc.start() self.addCleanup(fake_validate_staticpage_desc.stop) + fake_validate_form_desc = mock.patch( + "course.validation.validate_form_desc") + self.mock_validate_form_desc = fake_validate_form_desc.start() + self.addCleanup(fake_validate_form_desc.stop) + fake_get_yaml_from_repo = mock.patch( "course.content.get_yaml_from_repo") self.mock_get_yaml_from_repo = fake_get_yaml_from_repo.start() @@ -419,6 +436,10 @@ def test_course_none(self): self.assertSetEqual(expected_validate_staticpage_desc_call_args, args_set) + # make sure validate_form_desc was called with expected args + self.assertEqual(self.mock_validate_form_desc.call_args_list[0][0][1:], + (form1_path, form1_desc)) + # validate_calendar_desc_struct is called self.assertEqual(self.mock_validate_calendar_desc_struct.call_count, 1) diff --git a/tests/test_validation/test_validation_tools.py b/tests/test_validation/test_validation_tools.py index c858b5767..4bc856f67 100644 --- a/tests/test_validation/test_validation_tools.py +++ b/tests/test_validation/test_validation_tools.py @@ -2872,6 +2872,127 @@ def test_fail(self): self.assertIn(expected_error_msg, str(cm.exception)) +class ValidateFormIdTest(ValidationTestMixin, unittest.TestCase): + # test validation.validate_form_id + + def test_success(self): + flow_id = "abc-def" + validation.validate_form_id(vctx, location, flow_id) + flow_id = "abc_def1" + validation.validate_form_id(vctx, location, flow_id) + + def test_fail(self): + expected_error_msg = ( + "invalid form name. Form names may only contain (roman) " + "letters, numbers, dashes and underscores.") + + flow_id = "abc def" + with self.assertRaises(ValidationError) as cm: + validation.validate_form_id(vctx, location, flow_id) + self.assertIn(expected_error_msg, str(cm.exception)) + + flow_id = "abc/def" + with self.assertRaises(ValidationError) as cm: + validation.validate_form_id(vctx, location, flow_id) + self.assertIn(expected_error_msg, str(cm.exception)) + + +class ValidateFormFieldTest(ValidationTestMixin, unittest.TestCase): + # test validation.validate_form_field + + def get_updated_form_field(self, **kwargs): + field_desc = {"id": "my_page_id", + "type": "Text"} + field_desc.update(kwargs) + return dict_to_struct(field_desc) + + def test_success(self): + validation.validate_form_field(vctx, location, + self.get_updated_form_field(id="abc")) + + def test_invalid_form_field_id(self): + expected_error_msg = ( + "invalid form field id. Form field id may only contain (roman) " + "letters, numbers, dashes and underscores.") + with self.assertRaises(ValidationError) as cm: + validation.validate_form_field(vctx, location, + self.get_updated_form_field(id="abc def")) + self.assertIn(expected_error_msg, str(cm.exception)) + + def test_invalid_form_field_type(self): + expected_error_msg = ( + "some_where: form field type 'qwe' not recognized") + with self.assertRaises(ValidationError) as cm: + validation.validate_form_field(vctx, location, + self.get_updated_form_field(type="qwe")) + self.assertIn(expected_error_msg, str(cm.exception)) + + +class ValidateFormTest(ValidationTestMixin, unittest.TestCase): + # test validation.validate_form_desc + + def setUp(self): + super(ValidateFormTest, self).setUp() + patch = mock.patch("course.validation.validate_role") + self.mock_validate_role = patch.start() + self.addCleanup(patch.stop) + + patch = mock.patch("course.validation.validate_form_field") + self.mock_validate_form_field = patch.start() + self.addCleanup(patch.stop) + + def get_updated_form_desc(self, **kwargs): + form_desc = { + "title": "title", + "description": "description", + "type": "flow", + "access_roles": ["ta", "ta2"], + "fields": [ + dict_to_struct({"id": "template_in", "type": "Text"}), + dict_to_struct({"id": "template_out", "type": "Text"}), + ], + } + form_desc.update(kwargs) + return dict_to_struct(form_desc) + + def test_validate_role_called(self): + validation.validate_form_desc(vctx, location, + self.get_updated_form_desc(access_roles=[])) + self.assertEqual(self.mock_validate_role.call_count, 0) + self.assertEqual(self.mock_validate_form_field.call_count, 2) + + validation.validate_form_desc(vctx, location, + self.get_updated_form_desc()) + self.assertEqual(self.mock_validate_role.call_count, 2) + + def test_field_id_unique(self): + expected_error_msg = ("some_where: form field id 'template_in' not unique") + fields = [ + dict_to_struct({"id": "template_in", "type": "Text"}), + dict_to_struct({"id": "template_in", "type": "Text"}), + dict_to_struct({"id": "template_out", "type": "Text"}), + ] + with self.assertRaises(ValidationError) as cm: + validation.validate_form_desc(vctx, location, + self.get_updated_form_desc(fields=fields)) + self.assertIn(expected_error_msg, str(cm.exception)) + + def test_field_required(self): + fields = [ + dict_to_struct({"id": "template_in", "type": "Text"}), + dict_to_struct({"id": "template_out", "type": "Text"}), + ] + + for field_name in ["template_in", "template_out"]: + expected_error_msg = ( + "some_where: required form field id '%s' not found" % field_name) + test_fields = [field for field in fields if field.id != field_name] + with self.assertRaises(ValidationError) as cm: + validation.validate_form_desc(vctx, location, + self.get_updated_form_desc(fields=test_fields)) + self.assertIn(expected_error_msg, str(cm.exception)) + + class ValidateStaticPageNameTest(ValidationTestMixin, unittest.TestCase): # test validation.validate_static_page_name From 3e1e15215de6db47f6c1dfa84e429e2e8f6c9f2b Mon Sep 17 00:00:00 2001 From: Isuru Fernando Date: Thu, 17 Oct 2019 11:10:23 -0500 Subject: [PATCH 12/19] Add tests for process_form_fields --- course/content.py | 11 ++++++-- course/forms.py | 25 +++------------- tests/test_forms.py | 69 +++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 81 insertions(+), 24 deletions(-) create mode 100644 tests/test_forms.py diff --git a/course/content.py b/course/content.py index b6978593f..9b9385592 100644 --- a/course/content.py +++ b/course/content.py @@ -1552,11 +1552,11 @@ def is_commit_sha_valid(repo, commit_sha): return sha.encode() -def list_flow_ids(repo, commit_sha): - # type: (Repo_ish, bytes) -> List[Text] +def list_dir_yaml_ids(repo, commit_sha, dir_name): + # type: (Repo_ish, bytes, Text) -> List[Text] flow_ids = [] try: - flows_tree = get_repo_blob(repo, "flows", commit_sha) + flows_tree = get_repo_blob(repo, dir_name, commit_sha) except ObjectDoesNotExist: # That's OK--no flows yet. pass @@ -1567,4 +1567,9 @@ def list_flow_ids(repo, commit_sha): return sorted(flow_ids) + +def list_flow_ids(repo, commit_sha): + # type: (Repo_ish, bytes) -> List[Text] + return list_dir_yaml_ids(repo, commit_sha, "flows") + # vim: foldmethod=marker diff --git a/course/forms.py b/course/forms.py index 25ed2b09b..a349b57f1 100644 --- a/course/forms.py +++ b/course/forms.py @@ -28,7 +28,7 @@ import django.forms as forms from django.contrib import messages # noqa -from django.core.exceptions import PermissionDenied, ObjectDoesNotExist +from django.core.exceptions import PermissionDenied from django.utils.translation import ugettext_lazy as _ from django import http # noqa from django.utils.timezone import now @@ -40,7 +40,7 @@ from course.constants import participation_permission as pperm from course.utils import ( # noqa CoursePageContext) -from course.content import get_repo_blob, get_yaml_from_repo +from course.content import get_yaml_from_repo from relate.utils import string_concat, as_local_time # {{{ for mypy @@ -136,8 +136,6 @@ def process_form_fields(form_fields, data): for field in form_fields: if not hasattr(field, "label"): field.label = field.id - if not hasattr(field, "help"): - field.help = field.label if field.id in data: field.value = data[field.id] @@ -156,22 +154,6 @@ def process_form_fields(form_fields, data): process_value(field) -def list_form_names(repo, commit_sha): - # type: (Repo_ish, bytes) -> List[Text] - form_names = [] - try: - form_tree = get_repo_blob(repo, "forms", commit_sha) - except ObjectDoesNotExist: - # That's OK--no forms yet. - pass - else: - for entry in form_tree.items(): - if entry.path.endswith(b".yml"): - form_names.append(entry.path[:-4].decode("utf-8")) - - return sorted(form_names) - - def get_form(repo, form_name, commit_sha): contents = get_yaml_from_repo(repo, "forms/%s.yml" % form_name, commit_sha) contents.name = form_name @@ -179,7 +161,8 @@ def get_form(repo, form_name, commit_sha): def get_all_forms(repo, commit_sha): - form_names = list_form_names(repo, commit_sha) + from course.content import list_dir_yaml_ids + form_names = list_dir_yaml_ids(repo, commit_sha, "forms") forms = [] for name in form_names: contents = get_form(repo, name, commit_sha) diff --git a/tests/test_forms.py b/tests/test_forms.py new file mode 100644 index 000000000..cc8f67cf6 --- /dev/null +++ b/tests/test_forms.py @@ -0,0 +1,69 @@ +from __future__ import division + +__copyright__ = "Copyright (C) 2019 Isuru Fernando" + +__license__ = """ +Permission is hereby granted, free of charge, to any person obtaining a copy +of this software and associated documentation files (the "Software"), to deal +in the Software without restriction, including without limitation the rights +to use, copy, modify, merge, publish, distribute, sublicense, and/or sell +copies of the Software, and to permit persons to whom the Software is +furnished to do so, subject to the following conditions: + +The above copyright notice and this permission notice shall be included in +all copies or substantial portions of the Software. + +THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR +IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, +FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE +AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER +LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, +OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN +THE SOFTWARE. +""" + +from django.test import TestCase + +from course.forms import process_form_fields +from relate.utils import dict_to_struct + + +class CreateFormTest(TestCase): + + def test_fields_label(self): + fields = [ + dict_to_struct({"id": "template_in", "type": "Text"}), + dict_to_struct({"id": "template_out", "type": "Text", "label": "label"}), + ] + process_form_fields(fields, {}) + self.assertEqual(fields[0].label, "template_in") + self.assertEqual(fields[1].label, "label") + + def test_fields_value(self): + fields = [ + dict_to_struct({"id": "template_in", "type": "Text", "value": "spam"}), + dict_to_struct({"id": "template_out", "type": "Choice", + "choices": ["choice1", "~DEFAULT~ choice2"]}), + ] + process_form_fields(fields, {}) + self.assertEqual(fields[0].value, "spam") + self.assertEqual(fields[1].value, "choice2") + self.assertEqual(fields[1].choices, ["choice1", "choice2"]) + + def test_fields_assign_data(self): + fields = [ + dict_to_struct({"id": "template_in", "type": "Text", "value": "spam"}), + dict_to_struct({"id": "template_out", "type": "Choice", + "choices": ["choice1", "~DEFAULT~ choice2"]}), + dict_to_struct({"id": "field0", "type": "Integer", "value": 2}), + dict_to_struct({"id": "field1", "type": "Float", "value": 2.5}), + ] + process_form_fields(fields, {"template_in": "eggs", + "template_out": "choice1", + "field0": "1", + "field1": "1.5", + }) + self.assertEqual(fields[0].value, "eggs") + self.assertEqual(fields[1].value, "choice1") + self.assertEqual(fields[2].value, 1) + self.assertEqual(fields[3].value, 1.5) From 677fe05ecaf38f63f8a31e0243583539026d6c64 Mon Sep 17 00:00:00 2001 From: Isuru Fernando Date: Thu, 17 Oct 2019 18:00:50 -0500 Subject: [PATCH 13/19] tests for process_value --- course/forms.py | 19 ++++--- course/validation.py | 53 +++++++++++++++++++ tests/test_forms.py | 22 ++++++++ .../test_validation/test_validation_tools.py | 26 ++++++++- 4 files changed, 111 insertions(+), 9 deletions(-) diff --git a/course/forms.py b/course/forms.py index a349b57f1..c1f987e75 100644 --- a/course/forms.py +++ b/course/forms.py @@ -41,6 +41,7 @@ from course.utils import ( # noqa CoursePageContext) from course.content import get_yaml_from_repo +from course.validation import ValidationError from relate.utils import string_concat, as_local_time # {{{ for mypy @@ -118,16 +119,18 @@ def get_jinja_text(self): def process_value(field): - if field.type == "Integer": - try: + try: + if field.type == "Integer": field.value = int(field.value) - except ValueError: - pass - elif field.type == "Float": - try: + elif field.type == "Float": field.value = float(field.value) - except ValueError: - pass + except ValueError: + # This condition is impossible if the user uses the web UI + raise ValidationError( + _("form field '%(id)s' value '%(field_value)s' is" + " not a '%(field_type)s'.") % {'field_value': field.value, + 'field_type': field.type, + 'id': field.id}) def process_form_fields(form_fields, data): diff --git a/course/validation.py b/course/validation.py index ce8b17d72..08308e1be 100644 --- a/course/validation.py +++ b/course/validation.py @@ -1246,6 +1246,59 @@ def validate_form_field(vctx, location, field_desc): "dashes and underscores.")) % location) + if field_desc.type != "Choice": + required_types = { + "Integer": int, + "Float": float, + } + value_types = required_types.get(field_desc.type, + (str, int, float, bool)) + validate_struct( + vctx, + location, + field_desc, + required_attrs=[ + ("id", str), + ("type", str), + ("value", value_types), + ], + allowed_attrs=[ + ("label", str), + ], + ) + else: + validate_struct( + vctx, + location, + field_desc, + required_attrs=[ + ("id", str), + ("type", str), + ("choices", list), + ], + allowed_attrs=[ + ("label", str), + ], + ) + found_default = 0 + for choice in field_desc.choices: + if choice.startswith("~DEFAULT~"): + found_default += 1 + if found_default == 0: + raise ValidationError( + string_concat("%(location)s: ", + _("form field '%(id)s' of type '%(field_type)s' requires" + " a default value.")) + % {'location': location, 'field_type': field_desc.type, + 'id': field_desc.id}) + if found_default > 1: + raise ValidationError( + string_concat("%(location)s: ", + _("form field '%(id)s' of type '%(field_type)s' requires" + " only one default value.")) + % {'location': location, 'field_type': field_desc.type, + 'id': field_desc.id}) + # }}} diff --git a/tests/test_forms.py b/tests/test_forms.py index cc8f67cf6..78ad75852 100644 --- a/tests/test_forms.py +++ b/tests/test_forms.py @@ -25,11 +25,17 @@ from django.test import TestCase from course.forms import process_form_fields +from course.validation import ValidationError from relate.utils import dict_to_struct class CreateFormTest(TestCase): + required_fields = [ + dict_to_struct({"id": "template_in", "type": "Text", "value": "spam"}), + dict_to_struct({"id": "template_out", "type": "Text", "value": "spam"}), + ] + def test_fields_label(self): fields = [ dict_to_struct({"id": "template_in", "type": "Text"}), @@ -67,3 +73,19 @@ def test_fields_assign_data(self): self.assertEqual(fields[1].value, "choice1") self.assertEqual(fields[2].value, 1) self.assertEqual(fields[3].value, 1.5) + + def test_invalid_data(self): + fields = [ + dict_to_struct({"id": "template_in", "type": "Text", "value": "spam"}), + dict_to_struct({"id": "template_out", "type": "Text", "value": "spam"}), + dict_to_struct({"id": "field0", "type": "Integer", "value": 2}), + ] + + expected_error_msg = ( + "form field 'field0' value 'a' is not a 'Integer'.") + with self.assertRaises(ValidationError) as cm: + process_form_fields(fields, {"template_in": "eggs", + "template_out": "choice1", + "field0": "a", + }) + self.assertIn(expected_error_msg, str(cm.exception)) diff --git a/tests/test_validation/test_validation_tools.py b/tests/test_validation/test_validation_tools.py index 4bc856f67..c65b29f75 100644 --- a/tests/test_validation/test_validation_tools.py +++ b/tests/test_validation/test_validation_tools.py @@ -2902,7 +2902,8 @@ class ValidateFormFieldTest(ValidationTestMixin, unittest.TestCase): def get_updated_form_field(self, **kwargs): field_desc = {"id": "my_page_id", - "type": "Text"} + "type": "Text", + "value": "foo"} field_desc.update(kwargs) return dict_to_struct(field_desc) @@ -2927,6 +2928,29 @@ def test_invalid_form_field_type(self): self.get_updated_form_field(type="qwe")) self.assertIn(expected_error_msg, str(cm.exception)) + def test_invalid_form_field_choice(self): + field_desc = {"id": "my_page_id", + "type": "Choice", + "choices": ["foo"]} + field_desc = dict_to_struct(field_desc) + expected_error_msg = ( + "form field 'my_page_id' of type 'Choice' requires" + " a default value.") + with self.assertRaises(ValidationError) as cm: + validation.validate_form_field(vctx, location, field_desc) + self.assertIn(expected_error_msg, str(cm.exception)) + + field_desc.choices = ["~DEFAULT~ a", "~DEFAULT~ b", "c"] + expected_error_msg = ( + "form field 'my_page_id' of type 'Choice' requires" + " only one default value.") + with self.assertRaises(ValidationError) as cm: + validation.validate_form_field(vctx, location, field_desc) + self.assertIn(expected_error_msg, str(cm.exception)) + + field_desc.choices = ["~DEFAULT~ a", "b"] + validation.validate_form_field(vctx, location, field_desc) + class ValidateFormTest(ValidationTestMixin, unittest.TestCase): # test validation.validate_form_desc From 507455d99378793b832ee89ff056e33d5c638aa9 Mon Sep 17 00:00:00 2001 From: Isuru Fernando Date: Thu, 17 Oct 2019 22:30:58 -0500 Subject: [PATCH 14/19] Test CreateForm --- course/forms.py | 5 +++++ tests/test_forms.py | 39 +++++++++++++++++++++++++++++++++------ 2 files changed, 38 insertions(+), 6 deletions(-) diff --git a/course/forms.py b/course/forms.py index c1f987e75..bd5f13784 100644 --- a/course/forms.py +++ b/course/forms.py @@ -81,6 +81,11 @@ def __init__(self, form_fields): elif field.type == "Integer": self.fields[field.id] = forms.IntegerField( **field_data) + elif field.type == "Float": + self.fields[field.id] = forms.FloatField( + **field_data) + else: + assert field.type == "Hidden" if field.id == "template_in": self.template_in = field.value diff --git a/tests/test_forms.py b/tests/test_forms.py index 78ad75852..bad062051 100644 --- a/tests/test_forms.py +++ b/tests/test_forms.py @@ -23,19 +23,15 @@ """ from django.test import TestCase +import django.forms as forms -from course.forms import process_form_fields +from course.forms import process_form_fields, CreateForm from course.validation import ValidationError from relate.utils import dict_to_struct class CreateFormTest(TestCase): - required_fields = [ - dict_to_struct({"id": "template_in", "type": "Text", "value": "spam"}), - dict_to_struct({"id": "template_out", "type": "Text", "value": "spam"}), - ] - def test_fields_label(self): fields = [ dict_to_struct({"id": "template_in", "type": "Text"}), @@ -56,6 +52,15 @@ def test_fields_value(self): self.assertEqual(fields[1].value, "choice2") self.assertEqual(fields[1].choices, ["choice1", "choice2"]) + def test_reset(self): + fields = [ + dict_to_struct({"id": "template_in", "type": "Text", "value": "spam"}), + dict_to_struct({"id": "template_out", "type": "Text", "value": "eggs"}), + ] + process_form_fields(fields, {"reset": True, "template_in": "eggs"}) + self.assertEqual(fields[0].value, "spam") + self.assertEqual(fields[1].value, "eggs") + def test_fields_assign_data(self): fields = [ dict_to_struct({"id": "template_in", "type": "Text", "value": "spam"}), @@ -69,6 +74,7 @@ def test_fields_assign_data(self): "field0": "1", "field1": "1.5", }) + _ = CreateForm(fields) self.assertEqual(fields[0].value, "eggs") self.assertEqual(fields[1].value, "choice1") self.assertEqual(fields[2].value, 1) @@ -89,3 +95,24 @@ def test_invalid_data(self): "field0": "a", }) self.assertIn(expected_error_msg, str(cm.exception)) + + + def test_create_form(self): + fields = [ + dict_to_struct({"id": "template_in", "type": "Text", "value": "spam"}), + dict_to_struct({"id": "template_out", "type": "Text", + "value": "out.yml"}), + dict_to_struct({"id": "field0", "type": "Integer", "value": 2}), + dict_to_struct({"id": "field1", "type": "Float", "value": 2.5}), + dict_to_struct({"id": "field2", "type": "Choice", + "choices": ["choice1", "~DEFAULT~ choice2"]}), + dict_to_struct({"id": "field3", "type": "Hidden", "value": 2}), + ] + process_form_fields(fields, {}) + form = CreateForm(fields) + for field in ["field0", "field1", "field2", "template_in", "template_out"]: + self.assertIn(field, form.fields) + self.assertNotIn("field3", form.fields) + # Check that template_out has id appended + self.assertEqual(form.template_out, "out_{}.yml".format(form.id)) + self.assertIn(form.id, form.get_jinja_text()[0]) From 2966c3c987381ec116c58c37a8cd792e77b7c17e Mon Sep 17 00:00:00 2001 From: Isuru Fernando Date: Thu, 17 Oct 2019 22:38:59 -0500 Subject: [PATCH 15/19] Check field types --- tests/test_forms.py | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/tests/test_forms.py b/tests/test_forms.py index bad062051..61d17e082 100644 --- a/tests/test_forms.py +++ b/tests/test_forms.py @@ -96,7 +96,6 @@ def test_invalid_data(self): }) self.assertIn(expected_error_msg, str(cm.exception)) - def test_create_form(self): fields = [ dict_to_struct({"id": "template_in", "type": "Text", "value": "spam"}), @@ -110,8 +109,14 @@ def test_create_form(self): ] process_form_fields(fields, {}) form = CreateForm(fields) - for field in ["field0", "field1", "field2", "template_in", "template_out"]: + for field, ftype in [("field0", forms.IntegerField), + ("field1", forms.FloatField), + ("field2", forms.ChoiceField), + ("template_in", forms.CharField), + ("template_out", forms.CharField)]: self.assertIn(field, form.fields) + self.assertIsInstance(form.fields[field], ftype) + self.assertNotIn("field3", form.fields) # Check that template_out has id appended self.assertEqual(form.template_out, "out_{}.yml".format(form.id)) From 84d29e5f688f5596f3bb6fee4e88f06e259f2781 Mon Sep 17 00:00:00 2001 From: Isuru Fernando Date: Sat, 19 Oct 2019 14:19:55 -0500 Subject: [PATCH 16/19] Update docs --- course/forms.py | 1 + course/validation.py | 2 +- doc/flow.rst | 28 +++++++++++++++------------- 3 files changed, 17 insertions(+), 14 deletions(-) diff --git a/course/forms.py b/course/forms.py index bd5f13784..7ce823e48 100644 --- a/course/forms.py +++ b/course/forms.py @@ -151,6 +151,7 @@ def process_form_fields(form_fields, data): if field.type == "Choice": choices = [] for value in field.choices: + value = str(value) if value.startswith("~DEFAULT~"): v = value[9:].strip() choices.append(v) diff --git a/course/validation.py b/course/validation.py index 08308e1be..5c033efe7 100644 --- a/course/validation.py +++ b/course/validation.py @@ -1282,7 +1282,7 @@ def validate_form_field(vctx, location, field_desc): ) found_default = 0 for choice in field_desc.choices: - if choice.startswith("~DEFAULT~"): + if str(choice).startswith("~DEFAULT~"): found_default += 1 if found_default == 0: raise ValidationError( diff --git a/doc/flow.rst b/doc/flow.rst index dc12cf1f7..02fefecb3 100644 --- a/doc/flow.rst +++ b/doc/flow.rst @@ -805,8 +805,7 @@ also provides a way to create a flow in the during a lecture. For example, an instructor wants to create a multiple choice question in the middle of the lecture and have students submit an answer within 10 minutes of the announcement similar to an i-clicker. Then, a templated flow looks like -the following: - +the following:: title: "{{ title }}" @@ -852,10 +851,11 @@ the following: {{ description }} choices: - - {{ choice1 }} + - ~CORRECT~ {{ choice1 }} - {{ choice2 }} - {{ choice3 }} + The jinja variables `title, description, choice1, choice2, choice3, duration, created_time, id` are undefined and needs to be created by the form. REALTE will fill in `created_time` and `id` and the others need to be fields @@ -865,12 +865,14 @@ which correspond to the name of the templted flow and actual flow respectively. A special field `announce` will create an Instant Flow request which will make the flow visible to all visitors to the course page. -An example form description to create a form that an admin user can submit via -the web interface is as follows. Field types are one of -`Text, Integer, Float, Choice, Hidden`. Hidden fields are not shown to the admin -user, but their default values are substituted in the templated flow. +A form description file will create a form in the web interface for an admin +user and upon submitting a new flow will be created. +The form in the web interface can have several types of field. +Field types are one of `Text, Integer, Float, Choice, Hidden`. Hidden +fields are not shown to the admin user, but their default values are +substituted in the templated flow. An example form description is as follows:: -title: "Create an instant flow with one multiple choice question" + title: "Create an instant flow with one multiple choice question" description: | Instructions on filling out this form @@ -891,23 +893,23 @@ title: "Create an instant flow with one multiple choice question" - id: duration type: Integer - value: "20" + value: 20 label: "Duration in minutes for the flow" - id: choice1 type: Text - value: "~CORRECT~ (a)" + value: "(a)" label: "Correct choice" - id: choice2 type: Text value: "(b)" - label: "Incorrect choice" + label: "An Incorrect choice" - id: choice3 type: Text value: "(c)" - label: "Incorrect choice" + label: "Another incorrect choice" - id: template_in type: Hidden @@ -926,7 +928,7 @@ title: "Create an instant flow with one multiple choice question" When the admin user submits the form, a flow will be created using the -values the admin user filled in the web form like the following: +values the admin user filled in the web form like the following:: {% with id="20190930_022148_311577", title="InClass quiz", From 2e0c09c0ec38656bb81d56509799e101dad72f18 Mon Sep 17 00:00:00 2001 From: Isuru Fernando Date: Sat, 19 Oct 2019 14:41:20 -0500 Subject: [PATCH 17/19] Test view permission --- tests/base_test_mixins.py | 13 +++++++++++++ tests/test_forms.py | 16 ++++++++++++++++ 2 files changed, 29 insertions(+) diff --git a/tests/base_test_mixins.py b/tests/base_test_mixins.py index 669125d50..03ddd3407 100644 --- a/tests/base_test_mixins.py +++ b/tests/base_test_mixins.py @@ -847,6 +847,19 @@ def get_edit_course_url(cls, course_identifier=None): course_identifier or cls.get_default_course_identifier()) return cls.get_course_view_url("relate-edit_course", course_identifier) + @classmethod + def get_view_all_forms_url(cls, course_identifier=None): + course_identifier = ( + course_identifier or cls.get_default_course_identifier()) + return cls.get_course_view_url("relate-view_all_forms", course_identifier) + + @classmethod + def get_view_form_url(cls, form_id, course_identifier=None): + course_identifier = ( + course_identifier or cls.get_default_course_identifier()) + return cls.get_course_view_url("relate-view_form", course_identifier, + form_id) + @classmethod def post_edit_course(cls, data, course=None): course = course or cls.get_default_course() diff --git a/tests/test_forms.py b/tests/test_forms.py index 61d17e082..219b81970 100644 --- a/tests/test_forms.py +++ b/tests/test_forms.py @@ -29,6 +29,8 @@ from course.validation import ValidationError from relate.utils import dict_to_struct +from tests.base_test_mixins import SingleCourseTestMixin, MockAddMessageMixing + class CreateFormTest(TestCase): @@ -121,3 +123,17 @@ def test_create_form(self): # Check that template_out has id appended self.assertEqual(form.template_out, "out_{}.yml".format(form.id)) self.assertIn(form.id, form.get_jinja_text()[0]) + + +class ViewAllFormsTest(SingleCourseTestMixin, MockAddMessageMixing, TestCase): + + def test_student_no_form_access(self): + with self.temporarily_switch_to_user(self.student_participation.user): + print(self.get_course_page_url()) + resp = self.c.get(self.get_view_all_forms_url()) + self.assertEqual(resp.status_code, 403) + + def test_instructor_forms_access(self): + with self.temporarily_switch_to_user(self.instructor_participation.user): + resp = self.c.get(self.get_view_all_forms_url()) + self.assertEqual(resp.status_code, 200) From 706e7ccdc611b3e21246117f63796b33566c0c05 Mon Sep 17 00:00:00 2001 From: Isuru Fernando Date: Sun, 27 Oct 2019 18:17:08 -0500 Subject: [PATCH 18/19] Add tests for permission --- course/constants.py | 1 + course/forms.py | 4 +- .../0114_add_use_forms_permission.py | 34 +++++++++++ tests/base_test_mixins.py | 4 +- tests/test_forms.py | 59 +++++++++++++++++-- 5 files changed, 94 insertions(+), 8 deletions(-) create mode 100644 course/migrations/0114_add_use_forms_permission.py diff --git a/course/constants.py b/course/constants.py index 36e671c17..c6197d959 100644 --- a/course/constants.py +++ b/course/constants.py @@ -130,6 +130,7 @@ class participation_permission: # noqa use_git_endpoint = "use_git_endpoint" use_markup_sandbox = "use_markup_sandbox" use_page_sandbox = "use_page_sandbox" + use_forms = "use_forms" test_flow = "test_flow" edit_events = "edit_events" diff --git a/course/forms.py b/course/forms.py index 7ce823e48..d357006f4 100644 --- a/course/forms.py +++ b/course/forms.py @@ -181,7 +181,7 @@ def get_all_forms(repo, commit_sha): @course_view def view_all_forms(pctx): - if not pctx.has_permission(pperm.update_content): + if not pctx.has_permission(pperm.use_forms): raise PermissionDenied() forms = get_all_forms(pctx.repo, pctx.course_commit_sha) @@ -193,7 +193,7 @@ def view_all_forms(pctx): @course_view def view_form(pctx, form_id): - if not pctx.has_permission(pperm.update_content): + if not pctx.has_permission(pperm.use_forms): raise PermissionDenied() form_info = get_form(pctx.repo, form_id, pctx.course_commit_sha) diff --git a/course/migrations/0114_add_use_forms_permission.py b/course/migrations/0114_add_use_forms_permission.py new file mode 100644 index 000000000..3ecec7be4 --- /dev/null +++ b/course/migrations/0114_add_use_forms_permission.py @@ -0,0 +1,34 @@ +# -*- coding: utf-8 -*- + +from __future__ import unicode_literals + +from django.db import migrations + +def add_use_forms_permission(apps, schema_editor): + from course.constants import participation_permission as pperm + + ParticipationRolePermission = apps.get_model("course", "ParticipationRolePermission") # noqa + + roles_pks = ( + ParticipationRolePermission.objects.filter( + permission=pperm.edit_course) + .values_list("role", flat=True) + ) + + if roles_pks.count(): + for pk in roles_pks: + ParticipationRolePermission.objects.get_or_create( + role_id=pk, + permission=pperm.use_forms + ) + + +class Migration(migrations.Migration): + + dependencies = [ + ('course', '0113_merge_20190919_1408'), + ] + + operations = [ + migrations.RunPython(add_use_forms_permission) + ] diff --git a/tests/base_test_mixins.py b/tests/base_test_mixins.py index 03ddd3407..f19cf613d 100644 --- a/tests/base_test_mixins.py +++ b/tests/base_test_mixins.py @@ -823,10 +823,10 @@ def create_course(cls, create_course_kwargs, raise_error=True): assert Course.objects.count() == existing_course_count + 1 @classmethod - def get_course_view_url(cls, view_name, course_identifier=None): + def get_course_view_url(cls, view_name, course_identifier=None, *args): course_identifier = ( course_identifier or cls.get_default_course_identifier()) - return reverse(view_name, args=[course_identifier]) + return reverse(view_name, args=[course_identifier] + list(args)) @classmethod def get_course_calender_url(cls, course_identifier=None): diff --git a/tests/test_forms.py b/tests/test_forms.py index 219b81970..723ef7103 100644 --- a/tests/test_forms.py +++ b/tests/test_forms.py @@ -27,7 +27,10 @@ from course.forms import process_form_fields, CreateForm from course.validation import ValidationError +from course.constants import participation_permission as pperm from relate.utils import dict_to_struct +from tests import factories +from course.models import ParticipationRolePermission, ParticipationRole from tests.base_test_mixins import SingleCourseTestMixin, MockAddMessageMixing @@ -125,15 +128,63 @@ def test_create_form(self): self.assertIn(form.id, form.get_jinja_text()[0]) -class ViewAllFormsTest(SingleCourseTestMixin, MockAddMessageMixing, TestCase): +class FormsBase(SingleCourseTestMixin, MockAddMessageMixing, TestCase): + + initial_commit_sha = "f3e9d31a61714e759a6ea12b900b173accb753f5" + form_title = b"Create an instant flow with one multiple choice question" + + def get_user_with_no_forms(self): + # This user has no form with access, but has access to viewing the + # forms list. + limited_instructor = factories.UserFactory() + limited_instructor_role = factories.ParticipationRoleFactory( + course=self.course, + identifier="limited_instructor" + ) + participation = factories.ParticipationFactory( + course=self.course, + user=limited_instructor) + participation.roles.set([limited_instructor_role]) + ParticipationRolePermission(role=limited_instructor_role, + permission=pperm.use_forms).save() + return limited_instructor + + +class ViewAllFormsTest(FormsBase): def test_student_no_form_access(self): with self.temporarily_switch_to_user(self.student_participation.user): - print(self.get_course_page_url()) resp = self.c.get(self.get_view_all_forms_url()) self.assertEqual(resp.status_code, 403) - def test_instructor_forms_access(self): - with self.temporarily_switch_to_user(self.instructor_participation.user): + def test_use_forms_permission(self): + with self.temporarily_switch_to_user(self.get_user_with_no_forms()): resp = self.c.get(self.get_view_all_forms_url()) self.assertEqual(resp.status_code, 200) + self.assertIn(self.form_title, resp.content) + + +class ViewFormTest(FormsBase): + + def test_student_no_form_access(self): + with self.temporarily_switch_to_user(self.student_participation.user): + resp = self.c.get(self.get_view_form_url(form_id="instant")) + self.assertEqual(resp.status_code, 403) + + def test_user_with_no_forms(self): + with self.temporarily_switch_to_user(self.get_user_with_no_forms()): + resp = self.c.get(self.get_view_form_url(form_id="instant")) + self.assertEqual(resp.status_code, 403) + + def get_instructor_with_perm(self): + role = ParticipationRole.objects.filter( + identifier="instructor", + ).first() + ParticipationRolePermission(role=role, + permission=pperm.use_forms).save() + return self.instructor_participation.user + + def test_instructor_form_access(self): + with self.temporarily_switch_to_user(self.get_instructor_with_perm()): + resp = self.c.get(self.get_view_form_url(form_id="instant")) + self.assertEqual(resp.status_code, 200) From 4dafc6434b6449f6c38c340afac58f43786d3d0b Mon Sep 17 00:00:00 2001 From: Isuru Fernando Date: Sun, 27 Oct 2019 20:10:02 -0500 Subject: [PATCH 19/19] Fix reset and test --- course/forms.py | 2 +- tests/test_forms.py | 9 +++++++++ 2 files changed, 10 insertions(+), 1 deletion(-) diff --git a/course/forms.py b/course/forms.py index d357006f4..d551dea36 100644 --- a/course/forms.py +++ b/course/forms.py @@ -222,7 +222,7 @@ def back_to_form(form, form_info): process_form_fields(form_info.fields, request.POST) form = CreateForm(form_info.fields) - if "clear" in request.POST: + if "reset" in request.POST: return back_to_form(form, form_info) page_source, file_out = form.get_jinja_text() diff --git a/tests/test_forms.py b/tests/test_forms.py index 723ef7103..a85d51490 100644 --- a/tests/test_forms.py +++ b/tests/test_forms.py @@ -188,3 +188,12 @@ def test_instructor_form_access(self): with self.temporarily_switch_to_user(self.get_instructor_with_perm()): resp = self.c.get(self.get_view_form_url(form_id="instant")) self.assertEqual(resp.status_code, 200) + + def test_form_reset(self): + with self.temporarily_switch_to_user(self.get_instructor_with_perm()): + from time import time + new_duration = int(time()) + data = {"reset": "", "duration": new_duration} + resp = self.c.post(self.get_view_form_url(form_id="instant"), data=data) + self.assertEqual(resp.status_code, 200) + self.assertNotIn(str(new_duration), resp.content.decode("utf-8"))