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

          Attachments

            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