-
-
Notifications
You must be signed in to change notification settings - Fork 3.8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Enabling Learner for fill out answer details #7159
Conversation
Hi @seanlip, made the changes PTAL. I will upload the video soon |
Hi @seanlip uploaded the video too. PTAL Thanks! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, @anubhavsinha98! Just one more comment and I think it can be merged.
spyOn( | ||
mockAnswerClassificationService, | ||
'getMatchingClassificationResult').and.returnValue(acrof.createNew( | ||
oof.createNew('default', 'default_outcome', '', []), 2, 0, | ||
DEFAULT_OUTCOME_CLASSIFICATION)); | ||
mockAnswer = 'This is my answer'; | ||
mockInteractionRulesService = 'Sample interaction rules service'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be a service, not a string.
Why not just use one of the actual interaction rules services?
Also @nithusha21 PTAL as codeowner. Thanks! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Just one question
core/templates/dev/head/domain/statistics/LearnerAnswerDetailsBackendApiServiceSpec.ts
Show resolved
Hide resolved
Done @seanlip |
Apparently this needs a codeowner review from @brianrodri (not sure why this did not show up in GitHub before). @brianrodri could you PTAL? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
CODEOWNER LGTM
This PR includes the frontend changes, to enable the learner to fill out answer details when he is asked for it.
Explanation
Checklist
python scripts/pre_commit_linter.py
andbash scripts/run_frontend_tests.sh
.