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.

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

Swik 2488 enable selection of questions for exam mode #1034

Merged
merged 22 commits into from
Oct 27, 2018

Conversation

dpaun
Copy link
Member

@dpaun dpaun commented Oct 16, 2018

This PR contains a basic version of the exam mode. Also, deck owners can now select which questions will be exam questions. Only these questions will be shown in the exam mode. They will not be shown to users without deck edit rights.
Modifications on the service side have already been merged.

@kprist kprist self-assigned this Oct 18, 2018
@kprist kprist requested a review from abijames October 18, 2018 12:49
@abijames
Copy link
Contributor

I have done a commit to fix accessibility
@dpaun please can you check that my aria-labels for correct / incorrect icons are right. Also if the result is a percentage, currently it is just showing a number.
@nk1g09 is also checking the branch

@abijames abijames requested a review from nk1g09 October 19, 2018 14:05
@nk1g09
Copy link
Contributor

nk1g09 commented Oct 19, 2018

So I have taken a look at this branch and I think there are two potential issues that I encountered. I'll put them in two comments.

The first is in relation to incorrect marking as shown in these screenshots below.
answers
score

Now this seems to be rather intermittent, but through multiple repeats I think it occurs if an answer is selected too quickly. The exam page loads immediately and the answers are instantly available to select, however if you click before everything has loaded in the background (not very obvious as it all seems to have loaded anyway) then the checkbox is marked but the selectedAnswer prop is not updated. So it is marked as incorrect even though it is correct.

Usually the full background load doesn't take more than about 5 seconds. But the first time I did it it seemed to take quite a while and I managed to answer the whole quiz before it had loaded.

Is there a simple way to either have a message come up or prevent people from clicking before it is ready?

@nk1g09
Copy link
Contributor

nk1g09 commented Oct 19, 2018

The second is in relation to questions on slides and deck forking, but it does impact on the exam mode.

When a user forks a deck, any questions that are contained on a slide (rather than the deck) are 'copied' to the new forked deck. However these questions are the same question id in both the original and the forked decks. Meaning that if the question is selected/ or deselected for exam mode in either the forked or original deck it also affects the other deck.

This also affects editing of the questions.

I know there was a suggestion to stop the addition of questions on slides but it was not done. Potentially this should be revisited?

@nk1g09
Copy link
Contributor

nk1g09 commented Oct 19, 2018

Additionally another thought - the edit exam questions button is visible both on the deck level and on the slide level (only if there are questions present on that slide though).

Is it preferable to only have it visible on the deck level? as that should be where the exam is being run from overall. It only requires a quick edit on line 103 of contentQuestionsPanel if so.

line 103 becomes:
let examQuestionsButton = (questions.length > 0 && this.props.ContentModulesStore.selector.stype === 'deck') ?

@dpaun
Copy link
Member Author

dpaun commented Oct 22, 2018

Thanks for the comments. I've made some improvements. Regarding the issue with forking, questions for slides are not copied, but they are shown in the forked deck because the slides in the forked deck are the same as in original deck (the ids are the same).

@nk1g09
Copy link
Contributor

nk1g09 commented Oct 22, 2018

Re forks: Ah okay I didn't realise they had the same slide ID as obviously they do not change when you edit the slide in the other fork. This is possibly further suggestion that the ability to add questions to slides should be removed?

@nk1g09
Copy link
Contributor

nk1g09 commented Oct 25, 2018

Thanks for the comments. I've made some improvements. Regarding the issue with forking, questions for slides are not copied, but they are shown in the forked deck because the slides in the forked deck are the same as in original deck (the ids are the same).

This edit doesn't seem to have fixed the problem at least under my testing. I can still click the checkboxes as soon as the page loads, but again it appears that when scoring, only the selections made after the page completes the load are counted.

Although that is on a second load... when I first go into the deck I am getting strange behaviour. Atleast on deck 4512. Then it seems to only have the clicks made before the page loads... But it doesn't always save the correct selected answers. Sometimes I will have selected the 3rd answer, but it thinks i selected the second one.

I'm going to have a look at creating a start button. Which will enable the checkboxes... which might solve all the clicking issues.

@dpaun
Copy link
Member Author

dpaun commented Oct 25, 2018

I'll take a look. Although, I'm still not able to reproduce this problem.

@nk1g09
Copy link
Contributor

nk1g09 commented Oct 25, 2018

Ah ha! I think I might have discovered part of the issue to do with different answers being selected.
In the ContentQuestionsStore line 94 it is updating the questions array, but this doesn't take into account whether or not the question is an exam question or not. As questions is the deck questions array and the question index it uses relates to the exam questions array.

nonexamquestion

So in this screenshot you can see that the index of the second question is 1 but the question at questions[1] is the second question in the deck which isn't an exam question. So then each answer after is shifted up one in the exam marking because it is associated with the next question down.

So either the questionstore needs to correct for non-exam questions, or more simply the exam components need to use the original questions index rather than the exam questions index for the selectExamAnswer action.
This can be achieved by making a few changes. One in the exampanel where the questions are pushed to the examQuestions and the another in ExamList when ExamItems are called. (adding another index as originalIndex) This index should then be passed through the components to the action.

Shows position of the exam questions in the full list of deck questions.
Necessary to ensure update of correct question answers.
@nk1g09
Copy link
Contributor

nk1g09 commented Oct 25, 2018

I have pushed new changes to add an originalQIndex which reflects the exam questions position in the full questions array.

This seems to fix the bugs related to the wrong answers being selected.

The other issues related to answers not being selected until the page has loaded appear now to only occur when a page is reloaded.

@dpaun
Copy link
Member Author

dpaun commented Oct 26, 2018

@nk1g09 , Great! Thanks for fixing a bug!

@kprist
Copy link
Member

kprist commented Oct 26, 2018

@dpaun should exam mode work here? I have some exam questions with deck 4949 on experimental but in exam mode none are presented.

@dpaun
Copy link
Member Author

dpaun commented Oct 26, 2018

@kprist , it seems that there is only one question, and it is not selected for the exam. https://questionservice.experimental.slidewiki.org/deck/4949/questions

@dpaun
Copy link
Member Author

dpaun commented Oct 26, 2018

oh. sorry, there are some slide questions, I'll take a look

@dpaun
Copy link
Member Author

dpaun commented Oct 26, 2018

Sorry about that, @kprist , there was a typo made by me.

@kprist
Copy link
Member

kprist commented Oct 26, 2018

I checked code and functionality, seems OK, expect the part about slides questions inclusion in exams affects all decks that share that slide, which is the case with forked decks. Will discuss this further with @dpaun.

I believe @nk1g09 has tested this extensively already, need some UI checks on @abijames part, please both of you add your reviews so I know that's the only remaining issue, thanks!

Copy link
Contributor

@nk1g09 nk1g09 left a comment

Choose a reason for hiding this comment

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

Following bug fixes this branch appears to function correctly. Excluding the discussed effect of questions on slides which are referenced in forked decks.

@kprist
Copy link
Member

kprist commented Oct 26, 2018

@abijames are you going to check this for UI issues etc ? It's fine by me.

@abijames
Copy link
Contributor

@kprist I have already checked the UI for the selecting questions for exams so happy for this to be merged.

@kprist kprist merged commit 0bef040 into master Oct 27, 2018
@kprist kprist deleted the SWIK-2488-Enable_selection_of_questions_for_exam_mode branch October 27, 2018 16:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants