From 07f23b57d22d98c4072beba955ed4010091303c6 Mon Sep 17 00:00:00 2001 From: Greg Elin Date: Sun, 19 Dec 2021 11:31:51 -0600 Subject: [PATCH 1/6] [WIP] Store Questions in Module rather than individual records Migratate away from ModuleQuestions to just utilizing Module.spec.questions. This modification requires lots of changes. Long term advantage is easier to author/edit modules and questions because updates to questionnaire are just done inside of Module file rather than individual records. Much less coordination neeeded when deleting questions or modules. --- guidedmodules/_MQS_notes.txt | 48 ++++++++++++ guidedmodules/admin.py | 15 +++- guidedmodules/app_loading.py | 22 ++++-- .../0061_taskanswer_module_question_id.py | 28 +++++++ .../0062_alter_taskanswer_question.py | 19 +++++ guidedmodules/models.py | 37 +++++++++- guidedmodules/views.py | 73 +++++++++++++------ templates/question.html | 1 + 8 files changed, 211 insertions(+), 32 deletions(-) create mode 100644 guidedmodules/_MQS_notes.txt create mode 100644 guidedmodules/migrations/0061_taskanswer_module_question_id.py create mode 100644 guidedmodules/migrations/0062_alter_taskanswer_question.py diff --git a/guidedmodules/_MQS_notes.txt b/guidedmodules/_MQS_notes.txt new file mode 100644 index 000000000..f9faf1dbb --- /dev/null +++ b/guidedmodules/_MQS_notes.txt @@ -0,0 +1,48 @@ +MQS + +app_loading - storing questions, they are already in module.spec +show_question +update question from authoring tool +walk questions +impute questions +serialize +serialize to download + +TaskAnswer overhaul bc relationship would be on module.spec['question'].get_by_id + +key vs id might be related to reassigning the name of a module in app.yaml + +adding `key` and `definition_order` into Module.spec.questions + +when starting module and creating tasks and task questions - views.new_task + +taskanswer point back to question + +I want to start using module.questions_set as replacement for module.questions + +self.module.questions.get(key=question) needs to be self.module.questions_set.get_by_key(key=question) or get_by_id + +Lines that need to change: +guidedmodules.views: q = task.module.questions.get(id=request.POST.get("question")) +to change that file, I need to also send the module_question_id or question_key in the question form + + + def redirect_to(): + + # ============================================== + # TEMPORARY TODO: FIX + # TEMPORARY WHILE MOVE AWAY FROM ModuleQuestions + return "/projects" + # ============================================== + + next_q = get_next_question(q, task) + if next_q: + # Redirect to the next question. + return task.get_absolute_url_to_question(next_q) + f"?back_url={back_url}&previous=nquestion" + # Redirect to the module finished page because there are no more questions to answer. + return task.get_absolute_url() + f"/finished?back_url={back_url}&previous=nquestion" + + + +questions are already in Module.spec in module.spec['questions']; could leave them there. +But remeber to create as OrderedDict \ No newline at end of file diff --git a/guidedmodules/admin.py b/guidedmodules/admin.py index e5674e97e..975512c47 100644 --- a/guidedmodules/admin.py +++ b/guidedmodules/admin.py @@ -3,6 +3,7 @@ from django.utils.html import escape as escape_html from django_json_widget.widgets import JSONEditorWidget from jsonfield import JSONField +from django.db import models from .models import \ @@ -261,6 +262,14 @@ class ModuleAdmin(admin.ModelAdmin): } def app_(self, obj): return "{} [{}]".format(obj.app.appname, obj.app.id) if obj.app else "(not in an app)" +# class ModuleQuestionSetAdmin(admin.ModelAdmin): +# list_display = ('id', 'module') +# raw_id_fields = ('module',) +# readonly_fields = ("id",) +# formfield_overrides = { +# models.JSONField: {'widget': JSONEditorWidget}, +# } + class ModuleQuestionAdmin(admin.ModelAdmin): list_display = ('id', 'key', 'module') raw_id_fields = ('module', 'answer_type_module') @@ -293,11 +302,12 @@ def organization_and_project(self, obj): return obj.project.organization_and_title() class TaskAnswerAdmin(admin.ModelAdmin): - list_display = ('id', 'question', 'task', '_project', 'created') + list_display = ('id', 'question', 'module_question_id', 'task', '_project', 'created') raw_id_fields = ('task',) readonly_fields = ('id', 'task', 'question') - search_fields = ('task__project__organization__name', 'task__module__key') + search_fields = ('task__project__organization__name', 'task__module__key', 'module_question_id') fieldsets = [ + (None, {"fields": ('module_question_id',)}), (None, {"fields": ('task', 'question')}), (None, {"fields": ('notes',)}), (None, {"fields": ('extra',)}), @@ -334,6 +344,7 @@ class InstrumentationEventAdmin(admin.ModelAdmin): admin.site.register(AppVersion, AppVersionAdmin) admin.site.register(AppInput, AppInputAdmin) admin.site.register(Module, ModuleAdmin) +# admin.site.register(ModuleQuestionSet, ModuleQuestionSetAdmin) admin.site.register(ModuleQuestion, ModuleQuestionAdmin) admin.site.register(ModuleAsset, ModuleAssetAdmin) admin.site.register(Task, TaskAdmin) diff --git a/guidedmodules/app_loading.py b/guidedmodules/app_loading.py index a601f6f33..f2031cb14 100644 --- a/guidedmodules/app_loading.py +++ b/guidedmodules/app_loading.py @@ -226,8 +226,8 @@ def create_module(app, appinst, spec): def remove_questions(spec): spec = OrderedDict(spec) # clone - if "questions" in spec: - del spec["questions"] + # if "questions" in spec: + # del spec["questions"] return spec @@ -246,8 +246,16 @@ def update_module(m, spec, log_status): # Update its questions. qs = set() for i, question in enumerate(spec.get("questions", [])): + # print(f"adding question {i}, {question}") qs.add(update_question(m, i, question, log_status)) + # Update ModuleQuestionSet + question_set_json = spec.get("questions", []) + [q.update({"key": q["id"], "definition_order": i}) for i, q in enumerate(question_set_json,1)] + mqs, isnew = ModuleQuestionSet.objects.get_or_create( + module=m, question_set_json = spec.get("questions", []) + ) + # Delete removed questions (only happens if the Module is # not yet in use). for q in m.questions.all(): @@ -303,11 +311,13 @@ def is_module_changed(m, source, spec, module_id_map=None): # any string => Incompatible change - a new database record is needed. # If all other metadata is the same, then there are no changes. - if \ - json.dumps(m.spec, sort_keys=True) == json.dumps(remove_questions(spec), sort_keys=True) \ - and json.dumps([q.spec for q in m.get_questions()], sort_keys=True) \ - == json.dumps([q for q in spec.get("questions", [])], sort_keys=True): + if json.dumps(m.spec, sort_keys=True) == json.dumps(spec, sort_keys=True): return None + # if \ + # json.dumps(m.spec, sort_keys=True) == json.dumps(remove_questions(spec), sort_keys=True) \ + # and json.dumps([q.spec for q in m.get_questions()], sort_keys=True) \ + # == json.dumps([q for q in spec.get("questions", [])], sort_keys=True): + # return None # Now we're just checking if the change is compatible or not with # the existing database record. diff --git a/guidedmodules/migrations/0061_taskanswer_module_question_id.py b/guidedmodules/migrations/0061_taskanswer_module_question_id.py new file mode 100644 index 000000000..77370a0e2 --- /dev/null +++ b/guidedmodules/migrations/0061_taskanswer_module_question_id.py @@ -0,0 +1,28 @@ +# Generated by Django 3.2.5 on 2021-12-19 14:30 + +from django.db import migrations, models + +def forwards_func(apps, schema_editor): + # Update set module_question_id for existing TaskAnswer + # module_question_id is not a Django id, but is question yaml identifier string (e.g., key) + ta_model = apps.get_model("guidedmodules", "TaskAnswer") + db_alias = schema_editor.connection.alias + taskanswers = ta_model.objects.all() + for taskanswer in taskanswers: + taskanswer.module_question_id = taskanswer.question.spec['id'] + ta_model.objects.bulk_update(taskanswers, ['module_question_id'], 100) + +class Migration(migrations.Migration): + + dependencies = [ + ('guidedmodules', '0060_alter_moduleasset_updated'), + ] + + operations = [ + migrations.AddField( + model_name='taskanswer', + name='module_question_id', + field=models.CharField(blank=True, help_text='ID of the question', max_length=200, null=True), + ), + migrations.RunPython(forwards_func, migrations.RunPython.noop), + ] diff --git a/guidedmodules/migrations/0062_alter_taskanswer_question.py b/guidedmodules/migrations/0062_alter_taskanswer_question.py new file mode 100644 index 000000000..ec57dafd9 --- /dev/null +++ b/guidedmodules/migrations/0062_alter_taskanswer_question.py @@ -0,0 +1,19 @@ +# Generated by Django 3.2.5 on 2021-12-19 16:47 + +from django.db import migrations, models +import django.db.models.deletion + + +class Migration(migrations.Migration): + + dependencies = [ + ('guidedmodules', '0061_taskanswer_module_question_id'), + ] + + operations = [ + migrations.AlterField( + model_name='taskanswer', + name='question', + field=models.ForeignKey(help_text="The question (within the Task's Module) that this TaskAnswer is answering.", null=True, on_delete=django.db.models.deletion.PROTECT, to='guidedmodules.modulequestion'), + ), + ] diff --git a/guidedmodules/models.py b/guidedmodules/models.py index 9b410102a..c1445256d 100644 --- a/guidedmodules/models.py +++ b/guidedmodules/models.py @@ -15,6 +15,7 @@ from collections import OrderedDict import uuid +from django.db import models from api.base.models import BaseModel from siteapp.enums.assets import AssetTypeEnum from guidedmodules.enums.inputs import InputTypeEnum @@ -368,6 +369,20 @@ def spec_yaml(self): import rtyaml return rtyaml.dump(self.spec) + @property + def questions_set(self): + """Return list of questions from Module spec""" + + questions_set = self.spec.get("questions", []) + # Add in keys and definition order until we do this work on load + [q.update({"key": q["id"], "definition_order": i}) for i, q in enumerate(questions_set,1)] + return questions_set + + def get_question_by_id(self, q_id): + """Return question object by id of question""" + + return next((q for q in self.questions_set if q['id'] == q_id), None) + def get_questions(self): # Return the ModuleQuestions in definition order. return list(self.questions.order_by('definition_order')) @@ -567,6 +582,19 @@ def __repr__(self): return "" % (self.id, self.file.name, self.source) +# class ModuleQuestionSet(models.Model): +# module = models.ForeignKey(Module, related_name="question_set", on_delete=models.CASCADE, +# help_text="The Module that this ModuleQuestionSet is a part of.") +# question_set_json = models.JSONField(blank=True, null=True, help_text="JSON object representing all module questions") + +# def __str__(self): +# # For the admin. +# return "%s[%d].%s" % (self.module, self.module.id, 'ModuleQuestionSet') +# def __repr__(self): +# # For debugging. +# return "" % (self.id, self.module.module_name, self.module.lid) + + class ModuleQuestion(BaseModel): module = models.ForeignKey(Module, related_name="questions", on_delete=models.CASCADE, help_text="The Module that this ModuleQuestion is a part of.") @@ -1730,7 +1758,9 @@ def import_json_update(self, data, deserializer): class TaskAnswer(BaseModel): task = models.ForeignKey(Task, on_delete=models.CASCADE, related_name="answers", help_text="The Task that this TaskAnswer is a part of.") - question = models.ForeignKey(ModuleQuestion, on_delete=models.PROTECT, + module_question_id = models.CharField(max_length=200, unique=False, null=True, blank=True, help_text="ID of the question") + + question = models.ForeignKey(ModuleQuestion, null=True, on_delete=models.PROTECT, help_text="The question (within the Task's Module) that this TaskAnswer is answering.") notes = models.TextField(blank=True, help_text="Notes entered by editors working on this question.") @@ -1751,6 +1781,11 @@ def get_absolute_url(self): from urllib.parse import quote return self.task.get_absolute_url_to_question(self.question) + def get_module_question(self): + """Get question object from module.spec["questions"]""" + question_obj = self.task.module.get_question_by_id(self.module_question_id) + return question_obj + def get_current_answer(self): # The current answer is the one with the highest primary key. return self.answer_history \ diff --git a/guidedmodules/views.py b/guidedmodules/views.py index 344c124fe..a368807a4 100644 --- a/guidedmodules/views.py +++ b/guidedmodules/views.py @@ -213,6 +213,8 @@ def next_question(request, task, answered, *unused_args): return HttpResponseRedirect(task.get_absolute_url_to_question(q) + previous) def get_next_question(current_question, task): + + print(3,"========", current_question) # get context of questions in module answers = task.get_answers().with_extended_info() @@ -256,12 +258,34 @@ def save_answer(request, task, answered, context, __): return HttpResponseForbidden("Permission denied. {} does not have write privileges to task answer.".format(request.user.username)) # validate question - q = task.module.questions.get(id=request.POST.get("question")) + print(1,"==============req post question", request.POST.get("question")) + print(2, "============ request post keys", request.POST.keys()) + # q = task.module.questions.get(id=request.POST.get("question")) + question_key = request.POST.get("question_spec_id") + q = task.module.get_question_by_id(question_key) + # As part of migration away from ModuleQuestion, we need to (temporarily) + # recreate q.spec with all of data that was in ModuleQuestion.spec field + # to support the remaining code that is expecting q.spec in this operation. + # We can do this by stuffing all of q into q.spec + from copy import deepcopy + q.spec = deepcopy(q) + print("2.5", "==========", q) + # task.module.get_question_by_id + # store question/tasks for back button - back_url = task.get_absolute_url() + f"/question/{q.key}" + # back_url = task.get_absolute_url() + f"/question/{q.key}" + question_key = request.POST.get("question_spec_id") + back_url = task.get_absolute_url() + f"/question/{question_key}" # make a function that gets the URL to the next page def redirect_to(): + + # ============================================== + # TEMPORARY TODO: FIX + # TEMPORARY WHILE MOVE AWAY FROM ModuleQuestions + return "/projects" + # ============================================== + next_q = get_next_question(q, task) if next_q: # Redirect to the next question. @@ -344,7 +368,8 @@ def redirect_to(): # save answer - get the TaskAnswer instance first question, _ = TaskAnswer.objects.get_or_create( task=task, - question=q, + # question=q, + module_question_id=question_key, ) # fetch the task that answers this question @@ -424,26 +449,27 @@ def redirect_to(): # -------------------------- # How long was it since the question was initially viewed? That gives us # how long it took to answer the question. - i_task_question_view = InstrumentationEvent.objects\ - .filter(user=request.user, event_type="task-question-show", task=task, question=q)\ - .order_by('event_time')\ - .first() - i_event_value = (timezone.now() - i_task_question_view.event_time).total_seconds() \ - if i_task_question_view else None - # Save. - InstrumentationEvent.objects.create( - user=request.user, - event_type="task-question-" + instrumentation_event_type, - event_value=i_event_value, - module=task.module, - question=q, - project=task.project, - task=task, - answer=question, - extra={ - "answer_value": value, - } - ) + # COMMENT OUT INSTRUMENTATION WHILE MIGRATING AWAY FROM ModuleQuestion + # i_task_question_view = InstrumentationEvent.objects\ + # .filter(user=request.user, event_type="task-question-show", task=task, question=q)\ + # .order_by('event_time')\ + # .first() + # i_event_value = (timezone.now() - i_task_question_view.event_time).total_seconds() \ + # if i_task_question_view else None + # # Save. + # InstrumentationEvent.objects.create( + # user=request.user, + # event_type="task-question-" + instrumentation_event_type, + # event_value=i_event_value, + # module=task.module, + # question=q, + # project=task.project, + # task=task, + # answer=question, + # extra={ + # "answer_value": value, + # } + # ) # Process any actions from the question. # -------------------------------------- @@ -1070,6 +1096,7 @@ def render_markdown_field(field, output_format, **kwargs): context.update({ "header_col_active": "start" if (len(answered.as_dict()) == 0 and q.spec["type"] == "interstitial") else "questions", "q": q, + "q_spec_id": q.spec['id'], "title": title, "prompt": prompt, }) diff --git a/templates/question.html b/templates/question.html index 9cab23f90..8f7adbed3 100644 --- a/templates/question.html +++ b/templates/question.html @@ -169,6 +169,7 @@

{{title|safe}}

+ From 39688bd17780d5afe5ff4fe1274e260f9b4b85b2 Mon Sep 17 00:00:00 2001 From: Greg Elin Date: Sun, 19 Dec 2021 17:28:26 -0600 Subject: [PATCH 2/6] Working on getting answered questions --- guidedmodules/models.py | 27 ++++++++++++++++++++++++++- guidedmodules/views.py | 6 ++++-- 2 files changed, 30 insertions(+), 3 deletions(-) diff --git a/guidedmodules/models.py b/guidedmodules/models.py index c1445256d..0b8819407 100644 --- a/guidedmodules/models.py +++ b/guidedmodules/models.py @@ -772,6 +772,31 @@ def get_static_asset_image_data_url(self, asset_path, max_image_size): # ANSWERS + @staticmethod + def get_all_current_answer_records2(tasks): + # Efficiently get the current answer to every question of each of the tasks. + # + # Since we track the history of answers to each question, we need to get the most + # recent answer for each question. It's fastest if we pre-load the complete history + # of every question rather than making a separate database call for each question + # to find its most recent answer. See TaskAnswer.get_current_answer(). + # + # Return a generator that yields tuples of (Task, Module.spec['question']['question']?, TaskAnswerHistory). + # Among tuples for a particular Task, the tuples are in order of the questions + + ## TODO Migrating away from ModuleQuestions + # Should be able to replace most of the below with the following revised code + # for task in tasks: + # module = task.module + # for question in module.spec.['questions']: + # # Get the latest TaskAnswerHistory for the question, if there is one + # # [by n+1 filter]: TaskAnswerHistory.objects.filter(task and question_key) + # [by sorting through history] + # next(tah for tah blah, blah if tah.value=x) + # get answer + # yield (task, question, answer) + return (1, 1, 1) + @staticmethod def get_all_current_answer_records(tasks): # Efficiently get the current answer to every question of each of the tasks. @@ -794,6 +819,7 @@ def get_all_current_answer_records(tasks): tasks_ = {task.id: task for task in Task.objects.select_related('module', 'project').filter( id__in=history.values_list("taskanswer__task_id", flat=True))} + questions = {question.id: question for question in ModuleQuestion.objects.select_related('module').filter( id__in=history.values_list('taskanswer__question_id', flat=True))} @@ -801,7 +827,6 @@ def get_all_current_answer_records(tasks): for ansh in history: current_answers.setdefault( (tasks_.get(ansh.taskanswer.task_id), questions.get(ansh.taskanswer.question_id)), ansh) - # Batch load all of the ModuleQuestions. questions = ModuleQuestion.objects.prefetch_related('answer_type_module__questions').select_related('module') \ .filter(module__in={task.module for task in tasks}) \ diff --git a/guidedmodules/views.py b/guidedmodules/views.py index a368807a4..9acd4a146 100644 --- a/guidedmodules/views.py +++ b/guidedmodules/views.py @@ -227,6 +227,8 @@ def get_next_question(current_question, task): # go to the next one that is. If there are no subsequent questions to answer, go to the # first one that is answerable. answerable = list(answers.answerable) + print("5 ========== answerable", answerable) + # Avoid going to the current question as the computed available next question to answer if current_question in answerable: answerable.remove(current_question) @@ -269,7 +271,6 @@ def save_answer(request, task, answered, context, __): # We can do this by stuffing all of q into q.spec from copy import deepcopy q.spec = deepcopy(q) - print("2.5", "==========", q) # task.module.get_question_by_id # store question/tasks for back button @@ -280,13 +281,14 @@ def save_answer(request, task, answered, context, __): # make a function that gets the URL to the next page def redirect_to(): + next_q = get_next_question(q, task) + # ============================================== # TEMPORARY TODO: FIX # TEMPORARY WHILE MOVE AWAY FROM ModuleQuestions return "/projects" # ============================================== - next_q = get_next_question(q, task) if next_q: # Redirect to the next question. return task.get_absolute_url_to_question(next_q) + f"?back_url={back_url}&previous=nquestion" From 5e26024fd463ec9ead4690ec64d7840929da34d0 Mon Sep 17 00:00:00 2001 From: Greg Elin Date: Sun, 26 Dec 2021 07:14:56 -0600 Subject: [PATCH 3/6] Reorder fields in guidedmodules.py --- guidedmodules/models.py | 17 +---------------- 1 file changed, 1 insertion(+), 16 deletions(-) diff --git a/guidedmodules/models.py b/guidedmodules/models.py index 0b8819407..b4c32d986 100644 --- a/guidedmodules/models.py +++ b/guidedmodules/models.py @@ -582,19 +582,6 @@ def __repr__(self): return "" % (self.id, self.file.name, self.source) -# class ModuleQuestionSet(models.Model): -# module = models.ForeignKey(Module, related_name="question_set", on_delete=models.CASCADE, -# help_text="The Module that this ModuleQuestionSet is a part of.") -# question_set_json = models.JSONField(blank=True, null=True, help_text="JSON object representing all module questions") - -# def __str__(self): -# # For the admin. -# return "%s[%d].%s" % (self.module, self.module.id, 'ModuleQuestionSet') -# def __repr__(self): -# # For debugging. -# return "" % (self.id, self.module.module_name, self.module.lid) - - class ModuleQuestion(BaseModel): module = models.ForeignKey(Module, related_name="questions", on_delete=models.CASCADE, help_text="The Module that this ModuleQuestion is a part of.") @@ -1783,11 +1770,9 @@ def import_json_update(self, data, deserializer): class TaskAnswer(BaseModel): task = models.ForeignKey(Task, on_delete=models.CASCADE, related_name="answers", help_text="The Task that this TaskAnswer is a part of.") - module_question_id = models.CharField(max_length=200, unique=False, null=True, blank=True, help_text="ID of the question") - question = models.ForeignKey(ModuleQuestion, null=True, on_delete=models.PROTECT, help_text="The question (within the Task's Module) that this TaskAnswer is answering.") - + module_question_id = models.CharField(max_length=200, unique=False, null=True, blank=True, help_text="ID of the question") notes = models.TextField(blank=True, help_text="Notes entered by editors working on this question.") extra = JSONField(blank=True, help_text="Additional information stored with this object.") From de9ad43db25513d916d475004726c9695e148cba Mon Sep 17 00:00:00 2001 From: Greg Elin Date: Sun, 26 Dec 2021 09:22:59 -0600 Subject: [PATCH 4/6] Scaffolding for getting current answer --- guidedmodules/_MQS_notes.txt | 78 ++++++++++++++++++++++++++++++++++++ guidedmodules/app_loading.py | 7 ---- guidedmodules/models.py | 1 + guidedmodules/views.py | 70 ++++++++++++++++++-------------- 4 files changed, 120 insertions(+), 36 deletions(-) diff --git a/guidedmodules/_MQS_notes.txt b/guidedmodules/_MQS_notes.txt index f9faf1dbb..6b32ea471 100644 --- a/guidedmodules/_MQS_notes.txt +++ b/guidedmodules/_MQS_notes.txt @@ -1,5 +1,82 @@ MQS +=== +Migrate Questions from being individual records to bein array in Module. + +Benefits: + +- Easier authoring by either just changing one Module record, or copying one Module record and bumping version. + +Done +---- + +- File app_loading.py now retains the questions inside module.spec. When app is loaded, the question array still exists in Module. + +- Add guidedmodules.models.Module.questions_set to return questions from Module.spec. + +- Add guidedmodules.models.get_question_by_id to return question by id from Module.spec. + + +In Progress +----------- + + + +CONTINUE HERE > - Have views.show_question retrieve the current answer if exists + answer = taskq.get_current_answer() + + When Q does 'show_question', q value is passed in and q is type ModuleQuestion. But we are migrating away from ModuleQuestion. + So we need to pass in the module_question_id instead. we want to get the question from Module.spec.question and match on question id. + BUT, I don't understand how a whole question instance can be passed in a get URL?? BECAUSE of the @task_view decorator, dammit! + We need @task_view to not return ModuleQuestion: see + `question = get_object_or_404(ModuleQuestion, module=task.module, key=question_key)` + + +- Have guidedmodules.views.get_next_question get next question from Module.spec.question array instead of list of Module.question. + - The next question is determined as part of saving the answer. + +- Add get_all_current_answer_records2(tasks) to get all TaskAnswers efficiently + +- Editing a question saves in Module.spec.question. + + +TO DO +----- + +- (general) Use Module.questions_set everywhere instead of relationship Module.questions to get questions. + +- Editing question edits Module.spec.question + +- Deleting question (optionally) bumps app version and adds new module record (to protect old version). + +- Adding/Deleting modules bumps app version + +- Make sure TaskAnswers are related/can be found by data in Module.spec.question instead of by Django ORM relationship to ModuleQuestion. + +- Migrate Guidedmodules.models.ModuleQuestion back into guidedmodules.models.Module. Need to make sure any questionaires created witin application are now stored in Module.spec.question. + +- Have models.Module.serialize Module.spec.question. + Revise this section within code block: + ``` + spec["questions"] = [] + for i, q in enumerate(self.questions.order_by('definition_order')): + if i == 0 and q.key == "_introduction": + spec["introduction"] = {"format": "markdown", "template": q.spec["prompt"]} + continue + ... + ``` + +- Revise Module.questions_dependencies + Use Module.spec.questions + +- Delete guidedmodules.models.ModuleQuestion model. + +- Remove instrumentation? + + + +Areas +----- app_loading - storing questions, they are already in module.spec show_question update question from authoring tool @@ -23,6 +100,7 @@ I want to start using module.questions_set as replacement for module.questions self.module.questions.get(key=question) needs to be self.module.questions_set.get_by_key(key=question) or get_by_id Lines that need to change: +-------------------------- guidedmodules.views: q = task.module.questions.get(id=request.POST.get("question")) to change that file, I need to also send the module_question_id or question_key in the question form diff --git a/guidedmodules/app_loading.py b/guidedmodules/app_loading.py index f2031cb14..4ea14b6a9 100644 --- a/guidedmodules/app_loading.py +++ b/guidedmodules/app_loading.py @@ -249,13 +249,6 @@ def update_module(m, spec, log_status): # print(f"adding question {i}, {question}") qs.add(update_question(m, i, question, log_status)) - # Update ModuleQuestionSet - question_set_json = spec.get("questions", []) - [q.update({"key": q["id"], "definition_order": i}) for i, q in enumerate(question_set_json,1)] - mqs, isnew = ModuleQuestionSet.objects.get_or_create( - module=m, question_set_json = spec.get("questions", []) - ) - # Delete removed questions (only happens if the Module is # not yet in use). for q in m.questions.all(): diff --git a/guidedmodules/models.py b/guidedmodules/models.py index b4c32d986..7073aae2a 100644 --- a/guidedmodules/models.py +++ b/guidedmodules/models.py @@ -1955,6 +1955,7 @@ def are_files_same(): # Kick the Task and TaskAnswer's updated field and let the Task know that # its answers have changed. + print(10,"=========10 inside save_answer====") self.save(update_fields=[]) self.task.on_answer_changed() diff --git a/guidedmodules/views.py b/guidedmodules/views.py index 9acd4a146..d0f471e37 100644 --- a/guidedmodules/views.py +++ b/guidedmodules/views.py @@ -280,7 +280,7 @@ def save_answer(request, task, answered, context, __): # make a function that gets the URL to the next page def redirect_to(): - + return "" next_q = get_next_question(q, task) # ============================================== @@ -430,7 +430,8 @@ def redirect_to(): question.clear_answer(request.user) instrumentation_event_type = "clear" else: - # Save the answer. + # Save the answer. + print("4.5 ==================== save the answer:") had_answer = question.has_answer() if question.save_answer( value, answered_by_tasks, answered_by_file, @@ -746,6 +747,11 @@ def show_question(request, task, answered, context, q): authoring_tool_enabled = task.module.is_authoring_tool_enabled(request.user) can_upgrade_app = task.module.app.has_upgrade_priv(request.user) + print("16 =========== task:", task) + print("16 =========== answered:", answered) + print("16 =========== context:", context) + print("16 =========== q:", q) + is_answerable = (((q not in answered.unanswered) or (q in answered.can_answer)) and (q.key not in answered.was_imputed)) # TODO Create Guidedmodules settings model set in database whether to display_non_answerable # to allow allow access to imputed questions/ @@ -756,7 +762,9 @@ def show_question(request, task, answered, context, q): return HttpResponseRedirect(task.get_absolute_url()) # Is there a TaskAnswer for this yet? + print("17 =========== task=task, question=q, type(q)):", task, q, type(q)) taskq = TaskAnswer.objects.filter(task=task, question=q).first() + # taskq = TaskAnswer.objects.filter(task=task, module_question_id=q).first() # Get previous question for back button back_url = request.GET.get('back_url') @@ -767,8 +775,11 @@ def show_question(request, task, answered, context, q): # only here because the user is using the authoring tool, then there is no # real answer to load.) answer = None + print("19 =========== taskq and is_answerable:", taskq, is_answerable) + # taskq is None, needs to be something if taskq and is_answerable: answer = taskq.get_current_answer() + print("21 =========== taskq.get_current_answer():", answer) if answer and answer.cleared: # If the answer is cleared, treat as if it had not been answered. answer = None @@ -802,34 +813,34 @@ def show_question(request, task, answered, context, q): now-t.updated, )) - # Add instrumentation event. - # How many times has this question been shown? - i_prev_view = InstrumentationEvent.objects\ - .filter(user=request.user, event_type="task-question-show", task=task, question=q)\ - .order_by('-event_time')\ - .first() - # Save. - InstrumentationEvent.objects.create( - user=request.user, - event_type="task-question-show", - event_value=(i_prev_view.event_value+1) if i_prev_view else 1, - module=task.module, - question=q, - project=task.project, - task=task, - answer=taskq, - ) + # # Add instrumentation event. + # # How many times has this question been shown? + # i_prev_view = InstrumentationEvent.objects\ + # .filter(user=request.user, event_type="task-question-show", task=task, question=q)\ + # .order_by('-event_time')\ + # .first() + # # Save. + # InstrumentationEvent.objects.create( + # user=request.user, + # event_type="task-question-show", + # event_value=(i_prev_view.event_value+1) if i_prev_view else 1, + # module=task.module, + # question=q, + # project=task.project, + # task=task, + # answer=taskq, + # ) - # Indicate for the InstrumentQuestionPageLoadTimes middleware that this is - # a question page load. - request._instrument_page_load = { - "event_type": "task-question-request-duration", - "module": task.module, - "question": q, - "project": task.project, - "task": task, - "answer": taskq, - } + # # Indicate for the InstrumentQuestionPageLoadTimes middleware that this is + # # a question page load. + # request._instrument_page_load = { + # "event_type": "task-question-request-duration", + # "module": task.module, + # "question": q, + # "project": task.project, + # "task": task, + # "answer": taskq, + # } # Construct the page. def render_markdown_value(template, output_format, reference, **kwargs): @@ -858,6 +869,7 @@ def render_markdown_field(field, output_format, **kwargs): return render_markdown_value(template, output_format, field) # Get any existing answer for this question. + print(20, "========= get existing answer", answer) existing_answer = None if answer: existing_answer = answer.get_value() From 1b826ae990add3b16d1c1992fee3e419c60b0293 Mon Sep 17 00:00:00 2001 From: Greg Elin Date: Sun, 26 Dec 2021 15:47:58 -0600 Subject: [PATCH 5/6] Replacing ModuleQuestion in task_view and show_question --- guidedmodules/_MQS_notes.txt | 7 ++++ guidedmodules/models.py | 5 +++ guidedmodules/views.py | 63 ++++++++++++++++++++++++++++-------- 3 files changed, 61 insertions(+), 14 deletions(-) diff --git a/guidedmodules/_MQS_notes.txt b/guidedmodules/_MQS_notes.txt index 6b32ea471..e7356449b 100644 --- a/guidedmodules/_MQS_notes.txt +++ b/guidedmodules/_MQS_notes.txt @@ -31,6 +31,11 @@ CONTINUE HERE > - Have views.show_question retrieve the current answer if exists We need @task_view to not return ModuleQuestion: see `question = get_object_or_404(ModuleQuestion, module=task.module, key=question_key)` + to get correct answers see `get_all_current_answer_records` + + questions = {question.id: question for question in + ModuleQuestion.objects.select_related('module').filter( + id__in=history.values_list('taskanswer__question_id', flat=True))} - Have guidedmodules.views.get_next_question get next question from Module.spec.question array instead of list of Module.question. - The next question is determined as part of saving the answer. @@ -47,6 +52,8 @@ TO DO - Editing question edits Module.spec.question +- What type of object should module.get_question_by_id return? dictionary object? + - Deleting question (optionally) bumps app version and adds new module record (to protect old version). - Adding/Deleting modules bumps app version diff --git a/guidedmodules/models.py b/guidedmodules/models.py index 7073aae2a..2e4207349 100644 --- a/guidedmodules/models.py +++ b/guidedmodules/models.py @@ -807,14 +807,19 @@ def get_all_current_answer_records(tasks): Task.objects.select_related('module', 'project').filter( id__in=history.values_list("taskanswer__task_id", flat=True))} + # TODO: To migrate awy from ModuleQuestion, we need to use module.spec.question + # or module.get_questionby_id + # I think we can completely redo this process because we don't need to re-assemble questions questions = {question.id: question for question in ModuleQuestion.objects.select_related('module').filter( id__in=history.values_list('taskanswer__question_id', flat=True))} + # Got questions...now use qeustions to get answers in history. Then get questions again? for ansh in history: current_answers.setdefault( (tasks_.get(ansh.taskanswer.task_id), questions.get(ansh.taskanswer.question_id)), ansh) # Batch load all of the ModuleQuestions. + # TODO: with module.spec.questions we can just get quests array, instead? questions = ModuleQuestion.objects.prefetch_related('answer_type_module__questions').select_related('module') \ .filter(module__in={task.module for task in tasks}) \ .order_by("definition_order") diff --git a/guidedmodules/views.py b/guidedmodules/views.py index d0f471e37..8916841eb 100644 --- a/guidedmodules/views.py +++ b/guidedmodules/views.py @@ -125,8 +125,31 @@ def new_view_func(request, taskid, taskslug, pagepath, question_key, *args): # Is this page about a particular question? question = None if pagepath == "/question/": - question = get_object_or_404(ModuleQuestion, module=task.module, key=question_key) - taskans = TaskAnswer.objects.filter(task=task, question=question).first() + # OLD ModuleQuestion approach + # question = get_object_or_404(ModuleQuestion, module=task.module, key=question_key) + # taskans = TaskAnswer.objects.filter(task=task, question=question).first() + # NEW Module.spec.question approach + # Covert collections.OrderedDict to Dict + # As part of migrating from ModuleQuestion, get question from Module.spec.question + # question = json.loads(json.dumps(task.module.get_question_by_id(question_key))) + question_dict = json.loads(json.dumps(task.module.get_question_by_id(question_key))) + + # As part of migration away from ModuleQuestion, we need to (temporarily) + # recreate q.spec with all of data that was in ModuleQuestion.spec field + # to support the remaining code that is expecting q.spec in this operation. + # We can do this by stuffing all of q into q.spec + from copy import deepcopy + question_dict['spec'] = deepcopy(question_dict) + + # `question` is now a dictionary instead of a ModuleQuestion object and + # that fact may cause problems in views that use @task_view and expect `question` to be + # a ModuleQuestion object instead of a dictionary + # We get around this with SimpleNamespace to support dot notation on question object + from types import SimpleNamespace + question = SimpleNamespace(**question_dict) + taskans = TaskAnswer.objects.filter(task=task, module_question_id=question_key).first() + print("30 ============", taskans) + # TODO: is the above correct with `.first`? Why not get_or_404? # Does the user have read privs here? def read_priv(): @@ -763,8 +786,8 @@ def show_question(request, task, answered, context, q): # Is there a TaskAnswer for this yet? print("17 =========== task=task, question=q, type(q)):", task, q, type(q)) - taskq = TaskAnswer.objects.filter(task=task, question=q).first() - # taskq = TaskAnswer.objects.filter(task=task, module_question_id=q).first() + # taskq = TaskAnswer.objects.filter(task=task, question=q).first() + taskq = TaskAnswer.objects.filter(task=task, module_question_id=q).first() # Get previous question for back button back_url = request.GET.get('back_url') @@ -787,7 +810,9 @@ def show_question(request, task, answered, context, q): # For "module"-type questions, get the Module instance of the tasks that can # be an answer to this question, and get the existing Tasks that the user can # choose as an answer. - answer_module = q.answer_type_module + # answer_module = q.answer_type_module + answer_module = getattr(q,'answer_type_module', None) + # TODO : make above attr get one line answer_tasks = [] if answer_module: # The user can choose from any Task instances they have read permission on @@ -863,7 +888,8 @@ def render_markdown_value(template, output_format, reference, **kwargs): error = "

" + html.escape(error) + "

\n" return error def render_markdown_field(field, output_format, **kwargs): - template = q.spec.get(field) + # template = q.spec.get(field) + template = getattr(q, field, None) if not template: return None return render_markdown_value(template, output_format, field) @@ -893,7 +919,8 @@ def render_markdown_field(field, output_format, **kwargs): # What's the title/h1 of the page and the rest of the prompt? Render the # prompt field. If it starts with a paragraph, turn that paragraph into # the title. - title = q.spec["title"] + # title = q.spec["title"] # Used with ModuleQuest - delete + title = q.title prompt = render_markdown_field("prompt", "html") m = re.match(r"^

([\w\W]*?)

\s*", prompt) if m: @@ -904,7 +931,8 @@ def render_markdown_field(field, output_format, **kwargs): # Markdown into HTML for plain text fields. For longtext fields, turn it into # HTML because the WYSIWYG editor is initialized with HTML. default_answer = render_markdown_field("default", - "text" if q.spec["type"] != "longtext" else "html", + # "text" if q.spec["type"] != "longtext" else "html", + "text" if getattr(q, "type", None) != "longtext" else "html", demote_headings=False) ############################################################################### @@ -1069,10 +1097,12 @@ def render_markdown_field(field, output_format, **kwargs): # # We assume user has sufficient permission because user is answering question. # - if q.spec['type'] == "choice-from-data" or q.spec['type'] == "multiple-choice-from-data": + # if q.spec['type'] == "choice-from-data" or q.spec['type'] == "multiple-choice-from-data": + if getattr(q,'type', None) == "choice-from-data" or getattr(q,'type', None) == "multiple-choice-from-data": choices_from_data = [] choices_from_data_keys = {} - for action in q.spec['choices_from_data']: + # for action in q.spec['choices_from_data']: + for action in getattr(q, 'choices_from_data', None): a_obj, a_verb, a_filter = action['action'].split("/") # Process system actions for generating question option choices # ------------------------------------------------------------- @@ -1108,15 +1138,15 @@ def render_markdown_field(field, output_format, **kwargs): q.spec.update({"choices": choices_from_data}) context.update({ - "header_col_active": "start" if (len(answered.as_dict()) == 0 and q.spec["type"] == "interstitial") else "questions", + "header_col_active": "start" if (len(answered.as_dict()) == 0 and getattr(q,"type", None) == "interstitial") else "questions", "q": q, - "q_spec_id": q.spec['id'], + "q_spec_id": getattr(q, 'id', None), "title": title, "prompt": prompt, }) context.update({ "placeholder_answer": render_markdown_field("placeholder", "text") or "", # Render Jinja2 template but don't turn Markdown into HTML. - "example_answers": [render_markdown_value(ex.get("example", ""), "html", "example {}".format(i+1)) for i, ex in enumerate(q.spec.get("examples", []))], + "example_answers": [render_markdown_value(ex.get("example", ""), "html", "example {}".format(i+1)) for i, ex in enumerate(getattr(q, "examples", []))], "reference_text": render_markdown_field("reference_text", "html"), }) context.update({ @@ -1127,7 +1157,12 @@ def render_markdown_field(field, output_format, **kwargs): "answer": existing_answer, "answer_rendered": answer_rendered, "default_answer": default_answer, - "hidden_button_ids": q.module.app.modules.get(module_name="app").spec.get("hidden-buttons", []), + + # The next file for migrating away from ModuleQuestion could be a tough one. + # We want the module of the question, but such a relationship doesn't exist because + # question is inside module. do we have madule elsewhere? from Task? + # "hidden_button_ids": q.module.app.modules.get(module_name="app").spec.get("hidden-buttons", []), + "hidden_button_ids": task.module.app.modules.get(module_name="app").spec.get("hidden-buttons", []), }) context.update({ "can_review": task.has_review_priv(request.user), From 591b27befbf84e068437c6bb499c218dea1698ad Mon Sep 17 00:00:00 2001 From: Greg Elin Date: Mon, 27 Dec 2021 05:07:47 -0600 Subject: [PATCH 6/6] Add notes --- guidedmodules/_MQS_notes.txt | 6 +++--- guidedmodules/models.py | 9 +++++---- 2 files changed, 8 insertions(+), 7 deletions(-) diff --git a/guidedmodules/_MQS_notes.txt b/guidedmodules/_MQS_notes.txt index e7356449b..14c50af88 100644 --- a/guidedmodules/_MQS_notes.txt +++ b/guidedmodules/_MQS_notes.txt @@ -26,12 +26,12 @@ CONTINUE HERE > - Have views.show_question retrieve the current answer if exists answer = taskq.get_current_answer() When Q does 'show_question', q value is passed in and q is type ModuleQuestion. But we are migrating away from ModuleQuestion. - So we need to pass in the module_question_id instead. we want to get the question from Module.spec.question and match on question id. - BUT, I don't understand how a whole question instance can be passed in a get URL?? BECAUSE of the @task_view decorator, dammit! + The module_question_id must be passed instead to get the question from Module.spec.question by matching on the question id. + BUT, I don't understand how a whole question instance can be passed in a get URL? BECAUSE of the @task_view decorator, dammit! We need @task_view to not return ModuleQuestion: see `question = get_object_or_404(ModuleQuestion, module=task.module, key=question_key)` - to get correct answers see `get_all_current_answer_records` + to recover the current question answers the code calls `models.get_all_current_answer_records` questions = {question.id: question for question in ModuleQuestion.objects.select_related('module').filter( diff --git a/guidedmodules/models.py b/guidedmodules/models.py index 2e4207349..ea8385bf0 100644 --- a/guidedmodules/models.py +++ b/guidedmodules/models.py @@ -807,19 +807,20 @@ def get_all_current_answer_records(tasks): Task.objects.select_related('module', 'project').filter( id__in=history.values_list("taskanswer__task_id", flat=True))} - # TODO: To migrate awy from ModuleQuestion, we need to use module.spec.question + # TODO: To migrate away from ModuleQuestion, we need to use module.spec.question # or module.get_questionby_id # I think we can completely redo this process because we don't need to re-assemble questions + # from rows in database + # Below gets question for immediate module questions = {question.id: question for question in ModuleQuestion.objects.select_related('module').filter( id__in=history.values_list('taskanswer__question_id', flat=True))} - # Got questions...now use qeustions to get answers in history. Then get questions again? + print(50, "=============", questions) for ansh in history: current_answers.setdefault( (tasks_.get(ansh.taskanswer.task_id), questions.get(ansh.taskanswer.question_id)), ansh) - # Batch load all of the ModuleQuestions. - # TODO: with module.spec.questions we can just get quests array, instead? + # Batch load all of the ModuleQuestions for any question that has a module as an answer? questions = ModuleQuestion.objects.prefetch_related('answer_type_module__questions').select_related('module') \ .filter(module__in={task.module for task in tasks}) \ .order_by("definition_order")