Implement editing of questions #5614

tjiang11 · 2018-09-01T07:50:14Z

No description provided.

codecov-io · 2018-09-01T09:35:42Z

Codecov Report

Merging #5614 into develop will decrease coverage by <.01%.
The diff coverage is 53.29%.

@@             Coverage Diff             @@
##           develop    #5614      +/-   ##
===========================================
- Coverage    45.48%   45.47%   -0.01%     
===========================================
  Files          517      518       +1     
  Lines        30372    30493     +121     
  Branches      4568     4576       +8     
===========================================
+ Hits         13814    13867      +53     
- Misses       16558    16626      +68

Impacted Files	Coverage Δ
...v/head/pages/creator_dashboard/CreatorDashboard.js	`18.66% <ø> (+0.06%)`	⬆️
...s/interactions/LogicProof/directives/LogicProof.js	`8.73% <0%> (ø)`	⬆️
...es/topic_editor/questions/QuestionsTabDirective.js	`0.52% <0%> (-0.13%)`	⬇️
...d/pages/question_editor/QuestionEditorDirective.js	`2.77% <0%> (-0.68%)`	⬇️
.../dev/head/domain/question/QuestionUpdateService.js	`100% <100%> (ø)`
...ev/head/domain/editor/undo_redo/UndoRedoService.js	`100% <100%> (ø)`	⬆️
...es/state_editor/StateInteractionEditorDirective.js	`33.72% <100%> (+0.39%)`	⬆️
...main/question/EditableQuestionBackendApiService.js	`74.41% <100%> (ø)`	⬆️
...ead/domain/exploration/InteractionObjectFactory.js	`75.92% <12.5%> (-11.04%)`	⬇️
...plates/dev/head/domain/state/StateObjectFactory.js	`73.33% <14.28%> (-17.98%)`	⬇️
... and 7 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d4a4df4...7a10a7b. Read the comment docs.

aks681

Did a quick pass and left some comments.

aks681 · 2018-09-02T05:49:44Z

core/domain/question_services.py

+
+    Returns:
+        list(QuestionSkillLinkModel). The list of question skill link
+        domain objects that are linked to the question ID or an empty list


I don't think the domain objects for QuestionSkillLink are being returned currently. Can you change the returned object to be a list of QuestionSkillLink domain objects instead of datastore objects?

In that case, I think the docstring should also be changed to QuestionSkillLink, instead of QuestionSkillLinkModel?

Pending comment?

aks681 · 2018-09-02T05:56:33Z

core/templates/dev/head/pages/question_editor/QuestionEditorDirective.js

@@ -25,7 +25,7 @@ oppia.directive('questionEditor', [
        getQuestionId: '&questionId',
        getMisconceptions: '&misconceptions',
        canEditQuestion: '&',
-        questionStateData: '='
+        question: '='


If you are doing this, might as well remove questionId above as well, and use the question object itself?
Also, did you change all instances of this directive to use this? I think this is used in creator_dashboard as well.

done, can you check if the instance in the creator dashboard still functions properly? I can't seem to figure out how to test it. (I activated ENABLE_NEW_STRUCTURES and ENABLE_GENERALIZED_FEEDBACK_THREADS in constants.js but don't see anything)

tjiang11 · 2018-09-04T18:03:36Z

Hi @aks681 , can you do another pass?

aks681

Did a full pass. Mostly minor comments.

core/domain/question_services.py

aks681 · 2018-09-08T14:02:56Z

core/domain/question_services.py

+
+    Returns:
+        list(QuestionSkillLinkModel). The list of question skill link
+        domain objects that are linked to the question ID or an empty list


In that case, I think the docstring should also be changed to QuestionSkillLink, instead of QuestionSkillLinkModel?

aks681 · 2018-09-08T18:58:28Z

core/storage/question/gae_models.py

+            ID doesn't exist.
+        """
+        return QuestionSkillLinkModel.query().filter(
+            cls.question_id == question_id).fetch()


Shouldn't cls.deleted == false be also checked?

aks681 · 2018-09-08T19:00:23Z

core/templates/dev/head/domain/question/QuestionUpdateService.js

+oppia.constant('CMD_UPDATE_QUESTION_PROPERTY', 'update_question_property');
+
+oppia.factory('QuestionUpdateService', [
+  'QuestionObjectFactory',


I think 2 or 3 dependencies can be declared in a line?

aks681 · 2018-09-08T19:01:45Z

core/templates/dev/head/domain/question/QuestionUpdateService.js

+      QUESTION_PROPERTY_LANGUAGE_CODE,
+      QUESTION_PROPERTY_QUESTION_STATE_DATA,
+      CMD_UPDATE_QUESTION_PROPERTY) {
+    var _changeCount = 0;


I don't see this being incremented in this file, only reset seems to be there. Isn't this available from QuestionUndoRedoService?

Yeah my bad, I forgot to remove those

aks681 · 2018-09-08T19:04:31Z

core/templates/dev/head/pages/topic_editor/questions/QuestionsTabDirective.js

+            EVENT_QUESTION_SUMMARIES_INITIALIZED,
+            QuestionUndoRedoService, UrlService,
+            UndoRedoService) {
+          $scope.QuestionUndoRedoService = QuestionUndoRedoService;


Is this used in the HTML file? If not, no need to have it in $scope.

It is used in the html file now

Ok, but even then it only uses that single function. So, just surface that function to scope instead of the whole service.

tjiang11 · 2018-09-09T21:00:21Z

Hi @aks681 , can you do another pass?

tjiang11 · 2018-10-28T19:32:28Z

I updated this to adapt to the recent UI changes. PTAL!

@aks681 @seanlip

aks681 · 2018-10-31T18:53:10Z

@tjiang11 Is there anything extra to do to test the question editor? I enabled the flag in constants.js. Went to Questions tab in topic editor, created a question and then clicked on it again, which opened the question editor with that question's details already filled.
But here, when I changed values and clicked Ok, I got console errors and orange pop ups saying.
http://localhost:8181/question_editor_handler/data/8U19JTCTGeFG not found. Do you know why?

tjiang11 · 2018-11-04T19:12:48Z

Hi @aks681 , the issue should be fixed, PTAL!

aks681 · 2018-11-09T19:28:53Z

UI LGTM! @seanlip can you do one last code review? I think it might be good to go.

seanlip · 2018-11-10T01:48:57Z

core/templates/dev/head/pages/state_editor/StateInteractionEditorDirective.js

@@ -98,10 +98,56 @@ oppia.directive('stateInteractionEditor', [
              interactionCustomizationArgs, false);
          };

+          var _updateInteractionPreviewAndAnswerChoices = function() {


Could you reuse the logic in StateEditorService.getAnswerChoices()? Better than duplicating.

seanlip · 2018-11-10T01:49:45Z

Just one last issue with code duplication (I think). Otherwise looks good, thanks!

oppiabot · 2018-11-19T07:34:03Z

Hi @tjiang11. Due to recent changes in the "develop" branch, this PR now has a merge conflict. Please follow this link if you need help resolving the conflict, so that the PR can be merged. Thanks!

oppiabot · 2018-11-29T08:35:47Z

Hi @tjiang11, I'm going to mark this PR as stale because it hasn't had any updates for 10 days. If no further activity occurs within 7 days, it will be automatically closed so that others can take up the issue.
If you are still working on this PR, please make a follow-up commit within 7 days (and submit it for review, if applicable). Please also let us know if you are stuck so we can help you!

…estion's interaction; Fix issue with not validating when no interaction is specified.

seanlip

Hi @tjiang11, I think you forgot to flip the flag back. Otherwise looks good, thanks!

seanlip · 2018-12-02T11:11:20Z

assets/constants.js

@@ -464,7 +464,7 @@ var constants = {
    "\\u001b", "\\u001c", "\\u001d", "\\u001e", "\\u001f"
  ],

-  "ENABLE_NEW_STRUCTURES": false,
+  "ENABLE_NEW_STRUCTURES": true,


This should be false, right?

aks681

Will merge as soon as the above comment is addressed.

aks681

Looks good, thanks!
Merging.

tjiang11 added 19 commits August 30, 2018 01:05

Save.

d9f2824

Save.

f0cb957

Save. UrlInterpolationError topic null.

9c8e702

Save.

c93f186

Modify backend to return associated skills when fetching a question.

f695dd6

save.

066faec

Saving content works.

07a11fb

Copy interaction.

2386f69

Apply QuestionUpdateService for all changes.

4360648

Circumvent race condition on initialization.

a68adef

Lint.

dd9d25f

Clone UndoRedoService for questions.

8a2c2bd

Fetch question summaries after edit question.

4782099

Create reusable function in QuestionEditorDirective.

a20b0ab

Lint.

4668f51

Add test.

fd91189

Fix test.

2f4f1cc

Lint.

3050886

Disable flag.

6d52d8c

tjiang11 requested a review from aks681 September 1, 2018 07:50

tjiang11 requested review from BenHenning and seanlip as code owners September 1, 2018 07:50

DubeySandeep assigned aks681 Sep 1, 2018

aks681 reviewed Sep 2, 2018

View reviewed changes

Review changes.

43807b1

aks681 reviewed Sep 8, 2018

View reviewed changes

Review changes.

7a4ff6f

oppiabot bot removed the PR: don't merge - HAS MERGE CONFLICTS label Oct 28, 2018

tjiang11 closed this Oct 28, 2018

tjiang11 reopened this Oct 28, 2018

Edit questions in modal.

f6f7e09

tjiang11 assigned aks681 Oct 28, 2018

Add blank commit message.

5722ca1

seanlip reviewed Nov 10, 2018

View reviewed changes

oppiabot bot added the PR: don't merge - HAS MERGE CONFLICTS label Nov 19, 2018

oppiabot bot added the stale label Nov 29, 2018

tjiang11 added 5 commits November 30, 2018 23:44

Remove duplicate function; Fix issue with not being able to edit a qu…

3b5983f

…estion's interaction; Fix issue with not validating when no interaction is specified.

Merge develop.

c4c9b93

Lint.

98227d0

Lint.

5abc2c8

Fix.

387a9cb

oppiabot bot removed stale PR: don't merge - HAS MERGE CONFLICTS labels Dec 1, 2018

seanlip reviewed Dec 2, 2018

View reviewed changes

aks681 suggested changes Dec 2, 2018

View reviewed changes

seanlip approved these changes Dec 2, 2018

View reviewed changes

Flip flag.

7a10a7b

aks681 approved these changes Dec 10, 2018

View reviewed changes

aks681 merged commit 4d615cf into develop Dec 10, 2018

aks681 deleted the edit-questions branch December 10, 2018 14:59

Implement editing of questions #5614

Implement editing of questions #5614

Conversation

tjiang11 commented Sep 1, 2018

codecov-io commented Sep 1, 2018 • edited Loading

Codecov Report

aks681 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tjiang11 commented Sep 4, 2018

aks681 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tjiang11 commented Sep 9, 2018

tjiang11 commented Oct 28, 2018

aks681 commented Oct 31, 2018 • edited Loading

tjiang11 commented Nov 4, 2018

aks681 commented Nov 9, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

seanlip commented Nov 10, 2018

oppiabot bot commented Nov 19, 2018

oppiabot bot commented Nov 29, 2018

seanlip left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aks681 left a comment

Choose a reason for hiding this comment

aks681 left a comment

Choose a reason for hiding this comment

codecov-io commented Sep 1, 2018 •

edited

Loading

aks681 commented Oct 31, 2018 •

edited

Loading