Problem/Motivation

When the session manager was refactored to work with entities instead of IDs it looks like a subtle logic error was introduced. The getResults function call will now not check for temporary sessions if a Quiz object is passed in vs the original logic in resultOrTemp that checks for a temporary session if the quiz id isn't found in an active session. That's preventing people from accessing the final question feedback screen before the results page is reached, because getSession will always return null once the session has been moved into temporary storage. The submitEnd function in QuizQuestionFeedbackForm also appeared to break because it was passing the quiz_result entity instead of the ID to the redirect constructor.

Proposed resolution

This patch removes the !$quiz check from the temporary session if statement and changes the submitEnd function to use the ID again. It also removed a second parameter that the resultOrTemp function was passing to getResult that no longer existed. In my testing that seemed to fix everything.

CommentFileSizeAuthor
#17 3154985.patch6.64 KBdjdevin
#11 quiz_session-fixes-3154985-11.patch5.08 KBjacobbell84
#10 quiz_session-fixes-3154985-10.patch5.17 KBjacobbell84
#9 quiz_session-fixes-3154985-9.patch5.16 KBjacobbell84
#7 quiz_session-fixes-3154985-7.patch5.57 KBjacobbell84
#4 quiz_session-fixes-3154985-4.patch4.18 KBjacobbell84
quiz_session-fixes.patch1.62 KBjacobbell84

Issue fork quiz-3154985

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

    About issue forks
    Support from Acquia helps fund testing for Drupal Acquia logo

    Comments

    jacobbell84 created an issue. See original summary.

    Status: Needs review » Needs work

    The last submitted patch, quiz_session-fixes.patch, failed testing. View results
    - codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

    djdevin’s picture

    djdevin
    Location Philadelphia
    CreditAttribution: djdevin at Cadmium commented

    You can use the new issue queue fork system now too!

    jacobbell84’s picture

    jacobbell84 CreditAttribution: jacobbell84 commented
    FileSize
    quiz_session-fixes-3154985-4.patch4.18 KB

    Oh interesting, was just reading up a little on the issue fork system. So is the flow that I ask for push access and then can just commit to that vs having to make patches? That's pretty cool. I already had this patch ready, but I'll look into that next time.

    jacobbell84’s picture

    jacobbell84 CreditAttribution: jacobbell84 commented
    Status: Needs work » Needs review
    djdevin’s picture

    djdevin
    Location Philadelphia
    CreditAttribution: djdevin commented

    Thanks! Let's get a test on this. I don't think we have one for after the last question.

    jacobbell84’s picture

    jacobbell84 CreditAttribution: jacobbell84 commented
    FileSize
    quiz_session-fixes-3154985-7.patch5.57 KB

    Good idea, I changed the QuizFeedbackTest to run the same tests against the last question as it does the first question.

    Status: Needs review » Needs work

    The last submitted patch, 7: quiz_session-fixes-3154985-7.patch, failed testing. View results
    - codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

    jacobbell84’s picture

    jacobbell84 CreditAttribution: jacobbell84 commented
    FileSize
    quiz_session-fixes-3154985-9.patch5.16 KB

    Weird, updated test was failing because the second feedback page doesn't show feedback the same way the first one does.The more I was thinking about this though, we really just need to make sure we can access the page, we don't need to run the full tests again anyways. Trying a new patch with the simpler test.

    jacobbell84’s picture

    jacobbell84 CreditAttribution: jacobbell84 commented
    FileSize
    quiz_session-fixes-3154985-10.patch5.17 KB

    Attempting to fix the test

    jacobbell84’s picture

    jacobbell84 CreditAttribution: jacobbell84 commented
    FileSize
    quiz_session-fixes-3154985-11.patch5.08 KB

    One more try.

    jacobbell84’s picture

    jacobbell84 CreditAttribution: jacobbell84 commented
    Status: Needs work » Needs review
    djdevin’s picture

    djdevin
    Location Philadelphia
    CreditAttribution: djdevin commented

    This looks great, thanks. I want to take a look at the test because after you hit "Finish" on the last question of a quiz that also has per-question feedback, you should immediately see the question feedback, and then the overall quiz feedback.

    jacobbell84’s picture

    jacobbell84 CreditAttribution: jacobbell84 commented

    Sure, that works. It's weird, i downloaded the artifacts from the Jenkins build and based on the HTML output that's why it was failing....can't for the life of me figure out why that's happening though.

    afagioli’s picture

    afagioli
    Location Rome
    CreditAttribution: afagioli commented

    Confirming it works on latest quiz release.
    Thanks for sharing

    jacobbell84’s picture

    jacobbell84 CreditAttribution: jacobbell84 commented

    Hi @djdevin, have you had a chance to see why the tests for this ticket weren't acting correctly? I'm not very experienced with PHPUnit but I'd love to get this patch merged in.

    djdevin’s picture

    djdevin
    Location Philadelphia
    CreditAttribution: djdevin commented
    FileSize
    3154985.patch6.64 KB

    Sorry, got away from me. This works fine.

    I updated the test and removed the hard coded URLs just to make sure that the system redirects and forms work correctly during that final question feedback -> final -> question feedback -> quiz feedback flow.

    I tested this specific one already and it works but I'll merge it right after the full test run.

    • djdevin committed a09271d on 6.x authored by jacobbell84 
      Issue #3154985 by jacobbell84, djdevin: Session getResults logic error...
    djdevin’s picture

    djdevin
    Location Philadelphia
    CreditAttribution: djdevin commented
    Status: Needs review » Fixed

    Fixed, thanks

    jacobbell84’s picture

    jacobbell84 CreditAttribution: jacobbell84 commented

    Great, thank you!

    ugintl’s picture

    ugintl CreditAttribution: ugintl commented
    Status: Fixed » Reviewed & tested by the community

    #17 worked

    ugintl’s picture

    ugintl CreditAttribution: ugintl commented
    Status: Reviewed & tested by the community » Fixed

    Status: Fixed » Closed (fixed)

    Automatically closed - issue fixed for 2 weeks with no activity.