Uploaded image for project: 'Moodle'
  1. Moodle
  2. MDL-30428

Missing line break before "Export to portfolio" link when reviewing chat transcripts

    Details

    • Type: Bug
    • Status: Closed
    • Priority: Minor
    • Resolution: Fixed
    • Affects Version/s: 2.2
    • Fix Version/s: 2.2
    • Component/s: Chat, Portfolio
    • Labels:
    • Testing Instructions:
      Hide
      • Check the enableportfolios box in Settings > Site administration > Advanced features
      • Enable selected portfolio plugins in Settings > Site administration > Plugins > Portfolios > Manage portfolios
      • Create a new chat
      • Join the chat and make a post or two
      • Leave the chat
      • View the chat activity
      • Select 'View past chat sessions'
      • Choose 'List all session'
      Show
      Check the enableportfolios box in Settings > Site administration > Advanced features Enable selected portfolio plugins in Settings > Site administration > Plugins > Portfolios > Manage portfolios Create a new chat Join the chat and make a post or two Leave the chat View the chat activity Select 'View past chat sessions' Choose 'List all session'
    • Affected Branches:
      MOODLE_22_STABLE
    • Fixed Branches:
      MOODLE_22_STABLE
    • Pull from Repository:
    • Pull Master Branch:
      MDL-30428-master-1

      Description

      With portfolios enabled, viewing previous chat logs there's a <br/> missing between 'See this session' and 'Export to portfolio' (see screenshot)

      The 'See this session' is produced with an echo at mod/chat/report.php line 231
      The 'Export to portfolio' link is produced by a render function which only outputs if any portfolio plugins are actually enabled.

      As a result, it's not just a case of adding an

      echo html_writer::empty_tag('br');

      as if portfolios are enabled, but no portfolio plugins are, the newline would still be present.

        Gliffy Diagrams

          Issue Links

            Activity

            Hide
            dobedobedoh Andrew Nicols added a comment -

            $button->render() actually runs

            echo $this->to_html(...);

            As a result, I've changed mod/chat/report.php to store this value before outputting it so that we can test for content and conditionally at a br tag.

            Show
            dobedobedoh Andrew Nicols added a comment - $button->render() actually runs echo $this->to_html(...); As a result, I've changed mod/chat/report.php to store this value before outputting it so that we can test for content and conditionally at a br tag.
            Hide
            dobedobedoh Andrew Nicols added a comment -

            Screenshot displaying the bug

            Show
            dobedobedoh Andrew Nicols added a comment - Screenshot displaying the bug
            Hide
            salvetore Michael de Raadt added a comment -

            Thanks for sharing that. We're a little bogged down with QA testing and work for the upcoming release, but we will work our way through pending peer reviews as soon as we can.

            Show
            salvetore Michael de Raadt added a comment - Thanks for sharing that. We're a little bogged down with QA testing and work for the upcoming release, but we will work our way through pending peer reviews as soon as we can.
            Hide
            dobedobedoh Andrew Nicols added a comment -

            Hi Michael,

            This is a fix for a QA issue.

            Show
            dobedobedoh Andrew Nicols added a comment - Hi Michael, This is a fix for a QA issue.
            Hide
            dobedobedoh Andrew Nicols added a comment - - edited

            Hi Michael,

            This is a fix for the linked QA issue - MDLQA-1245.

            Show
            dobedobedoh Andrew Nicols added a comment - - edited Hi Michael, This is a fix for the linked QA issue - MDLQA-1245 .
            Hide
            rajeshtaneja Rajesh Taneja added a comment -

            Thanks for providing spot-on patch, Andrew..
            Will push this for integration review.

            Show
            rajeshtaneja Rajesh Taneja added a comment - Thanks for providing spot-on patch, Andrew.. Will push this for integration review.
            Hide
            rajeshtaneja Rajesh Taneja added a comment -

            @integrators:
            Please cherry pick this for 2.0 and 2.1 as well.

            Show
            rajeshtaneja Rajesh Taneja added a comment - @integrators: Please cherry pick this for 2.0 and 2.1 as well.
            Hide
            nebgor Aparup Banerjee added a comment -

            Thanks, this has been integrated into master under continuous integration for MDLQA issues.

            This has tested fine for me in 2.2 - please test the MDLQA.

            I'm creating another issue backporting this into 2.x stable branches for the next weekly stable integration/testing.

            Show
            nebgor Aparup Banerjee added a comment - Thanks, this has been integrated into master under continuous integration for MDLQA issues. This has tested fine for me in 2.2 - please test the MDLQA. I'm creating another issue backporting this into 2.x stable branches for the next weekly stable integration/testing.
            Hide
            nebgor Aparup Banerjee added a comment -

            created MDL-30465 for stable integration.

            Show
            nebgor Aparup Banerjee added a comment - created MDL-30465 for stable integration.
            Hide
            stronk7 Eloy Lafuente (stronk7) added a comment -

            And this has landed upstream, just on time for the upcoming new releases week. Thanks for it!

            Ciao

            Show
            stronk7 Eloy Lafuente (stronk7) added a comment - And this has landed upstream, just on time for the upcoming new releases week. Thanks for it! Ciao

              People

              • Votes:
                0 Vote for this issue
                Watchers:
                1 Start watching this issue

                Dates

                • Created:
                  Updated:
                  Resolved:
                  Fix Release Date:
                  5/Dec/11