Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement editing of questions #5614

Merged
merged 46 commits into from
Dec 10, 2018
Merged

Implement editing of questions #5614

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
46 commits
Select commit Hold shift + click to select a range
d9f2824
Save.
tjiang11 Aug 30, 2018
f0cb957
Save.
tjiang11 Aug 30, 2018
9c8e702
Save. UrlInterpolationError topic null.
tjiang11 Aug 30, 2018
c93f186
Save.
tjiang11 Aug 30, 2018
f695dd6
Modify backend to return associated skills when fetching a question.
tjiang11 Aug 30, 2018
066faec
save.
tjiang11 Aug 30, 2018
07a11fb
Saving content works.
tjiang11 Aug 30, 2018
2386f69
Copy interaction.
tjiang11 Aug 30, 2018
4360648
Apply QuestionUpdateService for all changes.
tjiang11 Aug 30, 2018
a68adef
Circumvent race condition on initialization.
tjiang11 Aug 30, 2018
dd9d25f
Lint.
tjiang11 Aug 30, 2018
8a2c2bd
Clone UndoRedoService for questions.
tjiang11 Aug 30, 2018
4782099
Fetch question summaries after edit question.
tjiang11 Aug 30, 2018
a20b0ab
Create reusable function in QuestionEditorDirective.
tjiang11 Aug 30, 2018
4668f51
Lint.
tjiang11 Aug 30, 2018
fd91189
Add test.
tjiang11 Sep 1, 2018
2f4f1cc
Fix test.
tjiang11 Sep 1, 2018
3050886
Lint.
tjiang11 Sep 1, 2018
6d52d8c
Disable flag.
tjiang11 Sep 1, 2018
43807b1
Review changes.
tjiang11 Sep 3, 2018
7a4ff6f
Review changes.
tjiang11 Sep 9, 2018
41798c6
Review comments.
tjiang11 Sep 13, 2018
c496793
Lint.
tjiang11 Sep 13, 2018
fbdaa83
Review changes.
tjiang11 Sep 23, 2018
732be38
Deduplicate UndoRedoService.
tjiang11 Sep 23, 2018
7756f6d
Move updateFunction into QuestionUpdateService.
tjiang11 Sep 23, 2018
17ca89f
Fix test.
tjiang11 Oct 14, 2018
5785784
Lint.
tjiang11 Oct 14, 2018
13254ea
Update documentation.
tjiang11 Oct 15, 2018
fc4981d
Add tests for get_models_by_question_id
tjiang11 Oct 15, 2018
9ef2940
Update setQuestionStateData.
tjiang11 Oct 15, 2018
5861a5a
Fix test.
tjiang11 Oct 15, 2018
4ccea9c
Fix test.
tjiang11 Oct 15, 2018
0c589eb
Merge develop.
tjiang11 Oct 15, 2018
c0ad966
Lint.
tjiang11 Oct 15, 2018
9896d99
Add test.
tjiang11 Oct 25, 2018
4d3c329
Merge develop.
tjiang11 Oct 28, 2018
d37ab38
Lint.
tjiang11 Oct 28, 2018
f6f7e09
Edit questions in modal.
tjiang11 Oct 28, 2018
5722ca1
Add blank commit message.
tjiang11 Nov 4, 2018
3b5983f
Remove duplicate function; Fix issue with not being able to edit a qu…
tjiang11 Dec 1, 2018
c4c9b93
Merge develop.
tjiang11 Dec 1, 2018
98227d0
Lint.
tjiang11 Dec 1, 2018
5abc2c8
Lint.
tjiang11 Dec 1, 2018
387a9cb
Fix.
tjiang11 Dec 1, 2018
7a10a7b
Flip flag.
tjiang11 Dec 7, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
Saving content works.
  • Loading branch information
@tjiang11
tjiang11 committed Aug 30, 2018
commit 07a11fb024f0acb707e9ac37bfc13bbacfa57a36
2 changes: 1 addition & 1 deletion core/controllers/question_editor.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -160,7 +160,7 @@ def put(self, question_id):

question_dict = question_services.get_question_by_id(
question_id).to_dict()
return self.render_json({
self.render_json({
'question_dict': question_dict
})

Expand Down
5 changes: 3 additions & 2 deletions core/domain/question_services.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -330,15 +330,16 @@ def apply_change_list(question_id, change_list):
Question. The resulting question domain object.
"""
question = get_question_by_id(question_id)
print "\n\n\na"
try:
for change in change_list:
if change.cmd == question_domain.CMD_UPDATE_QUESTION_PROPERTY:
if (change.property_name ==
question_domain.QUESTION_PROPERTY_LANGUAGE_CODE):
question.update_language_code(change.new_value)
elif (change.cmd ==
elif (change.property_name ==
question_domain.QUESTION_PROPERTY_QUESTION_STATE_DATA):
question.update_question_data(change.new_value)
question.update_question_state_data(change.new_value)

return question

Expand Down
2 changes: 1 addition & 1 deletion core/templates/dev/head/domain/question/EditableQuestionBackendApiService.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -85,7 +85,7 @@ oppia.factory('EditableQuestionBackendApiService', [
var putData = {
version: questionVersion,
commit_message: commitMessage,
change_dicts: changeList
change_list: changeList
};
$http.put(editableQuestionDataUrl, putData).then(function(response) {
// The returned data is an updated question dict.
Expand Down
4 changes: 4 additions & 0 deletions core/templates/dev/head/domain/question/QuestionObjectFactory.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,10 @@ oppia.factory('QuestionObjectFactory', [
return this._stateData;
};

Question.prototype.setStateData = function(stateData) {
this._stateData.copy(stateData);
};

Question.prototype.getLanguageCode = function() {
return this._languageCode;
};
Expand Down
5 changes: 2 additions & 3 deletions core/templates/dev/head/domain/question/QuestionUpdateService.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -75,10 +75,9 @@ oppia.factory('QuestionUpdateService', [
setQuestionStateData: function(question, oldStateData, newStateData) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is old state data being passed? Shouldn't that be taken from the passed in question object?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not really sure how else to do it, since the state data is bound to scope. I spent a few hours debugging this but couldn't find a different way. If you want I can just file an issue for it.

_applyPropertyChange(
question, QUESTION_PROPERTY_QUESTION_STATE_DATA,
newStateData, oldStateData,
newStateData.toBackendDict(), oldStateData.toBackendDict(),
function(changeDict, question) {
var stateData = _getNewPropertyValueFromChangeDict(changeDict);
question.setStateData(stateData);
question.setStateData(newStateData);
}, function(changeDict, question) {
question.setStateData(oldStateData);
});
Expand Down
9 changes: 9 additions & 0 deletions core/templates/dev/head/domain/state/StateObjectFactory.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,15 @@ oppia.factory('StateObjectFactory', [
};
};

State.prototype.copy = function(otherState) {
this.name = otherState.name;
this.classifierModelId = otherState.classifierModelId;
this.content = angular.copy(otherState.content);
this.interaction = angular.copy(otherState.interaction);
this.paramChanges = angular.copy(otherState.paramChanges);
this.contentIdsToAudioTranslations = angular.copy(otherState.contentIdsToAudioTranslations);
};

State.createDefaultState = function(newStateName) {
var newStateTemplate = angular.copy(constants.NEW_STATE_TEMPLATE);
var newState = this.createFromBackendDict(newStateName, {
Expand Down
15 changes: 8 additions & 7 deletions core/templates/dev/head/pages/question_editor/QuestionEditorDirective.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@ oppia.directive('questionEditor', [
getQuestionId: '&questionId',
getMisconceptions: '&misconceptions',
canEditQuestion: '&',
questionStateData: '='
question: '='
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

},
templateUrl: UrlInterpolationService.getDirectiveTemplateUrl(
'/pages/question_editor/question_editor_directive.html'),
Expand Down Expand Up @@ -83,7 +83,8 @@ oppia.directive('questionEditor', [
StateEditorService.setCorrectnessFeedbackEnabled(true);
StateEditorService.setInQuestionMode(true);
SolutionValidityService.init(['question']);
var stateData = $scope.questionStateData;
console.log($scope.question);
var stateData = $scope.question.getStateData();
stateData.interaction.defaultOutcome.setDestination(null);
if (stateData) {
$rootScope.$broadcast('stateEditorInitialized', stateData);
Expand All @@ -100,8 +101,8 @@ oppia.directive('questionEditor', [
$scope.saveStateContent = function(displayedValue) {
// Show the interaction when the text content is saved, even if no
// content is entered.
var oldQuestionStateData = angular.copy($scope.questionStateData);
$scope.questionStateData.content = angular.copy(displayedValue);
var oldQuestionStateData = angular.copy($scope.question.getStateData());
$scope.question._stateData.content = angular.copy(displayedValue);
QuestionUpdateService.setQuestionStateData(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah ok, I see the reason behind the 3 parameters in update question, but I think logically, the question object itself should be updated only in QuestionUpdateService. So, try to call the QuestionUpdateService before the update is done.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I already tried that, it didn't work. I can file an issue?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's fine, since the question is bound to scope, we are effectively updating the scope values and the actual question object separately, right?
But since, the questionData field of question is also bound to scope, won't doing updateFunction() change the question object also? So effectively the setQuestionStateData() function only adds a change object, right? Since question was already updated.

Also, yeah, I don't think we can do it similar to other editors as question editor does not have a separate page for itself, nor a state service, so maybe this is fine? What do you think?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just to give a third-party perspective on this: I'm kind of confused, reading the code. It looks like stateData is being modified in updateFunction() and then it's being modified again here (from the name setQuestionStateData)?

Should setQuestionStateData be part of updateFunction instead? If you need to reference oldQuestionStateData you can probably do currying (with bind() or something). But as it stands currently, I'm kinda confused about what's going on here.

(Happy to chat if useful.)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, I updated it so that there's not as much redundancy, take a look?

$scope.question,
oldQuestionStateData,
Expand All @@ -114,10 +115,10 @@ oppia.directive('questionEditor', [
};

$scope.saveInteractionAnswerGroups = function(newAnswerGroups) {
console.log(angular.copy($scope.questionStateData));
console.log(angular.copy($scope.question.getStateData()));
StateEditorService.setInteractionAnswerGroups(
angular.copy(newAnswerGroups));
console.log(angular.copy($scope.questionStateData));
console.log(angular.copy($scope.question.getStateData()));
};

$scope.saveInteractionDefaultOutcome = function(newOutcome) {
Expand All @@ -141,7 +142,7 @@ oppia.directive('questionEditor', [
};

$scope.saveContentIdsToAudioTranslations = function(displayedValue) {
$scope.questionStateData.contentIdsToAudioTranslations =
$scope.question._stateData.contentIdsToAudioTranslations =
angular.copy(displayedValue);
};

Expand Down
1 change: 1 addition & 0 deletions core/templates/dev/head/pages/state_editor/StateEditorDirective.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,6 +64,7 @@ oppia.directive('stateEditor', [
};

$scope.reinitializeEditor = function() {
console.log($scope.stateData);
$rootScope.$broadcast('stateEditorInitialized', $scope.stateData);
};

Expand Down
1 change: 1 addition & 0 deletions core/templates/dev/head/pages/topic_editor/questions/QuestionsTabDirective.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -78,6 +78,7 @@ oppia.directive('questionsTab', [
});
} else {
if (UndoRedoService.hasChanges()) {
console.log(UndoRedoService.getCommittableChangeList());
$scope.questionIsBeingSaved = true;
EditableQuestionBackendApiService.updateQuestion(
$scope.questionId, $scope.question.getVersion(), 'test',
Expand Down
4 changes: 2 additions & 2 deletions core/templates/dev/head/pages/topic_editor/questions/questions_tab_directive.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -83,7 +83,7 @@ <h3 class="form-heading" ng-if="questionSuggestionThreads.length > 0"> Questions
</div>
<question-editor question-id="questionId"
misconceptions="misconceptions"
question-state-data="questionStateData"
question="question"
can-edit-question="canEditQuestion">
</question-editor>
</div>
Expand All @@ -92,7 +92,7 @@ <h3 class="form-heading" ng-if="questionSuggestionThreads.length > 0"> Questions

<question-editor question-id="activeQuestion.getId()"
misconceptions="emptyMisconceptionsList"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know this was there before itself, but does QuestionsTabDirective have this variable?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added it to QuestionsTabDirective

question-state-data="activeQuestion.getStateData()"
question="activeQuestion"
can-edit-question="false">
</question-editor>

Expand Down