Moodle
  1. Moodle
  2. MDL-30428

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

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 2.2
    • Fix Version/s: 2.2
    • Component/s: Chat, Portfolio API
    • 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
    • Rank:
      33085

      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.

        Issue Links

          Activity

          Hide
          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
          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
          Andrew Nicols added a comment -

          Screenshot displaying the bug

          Show
          Andrew Nicols added a comment - Screenshot displaying the bug
          Hide
          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
          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
          Andrew Nicols added a comment -

          Hi Michael,

          This is a fix for a QA issue.

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

          Hi Michael,

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

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

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

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

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

          Show
          Rajesh Taneja added a comment - @integrators: Please cherry pick this for 2.0 and 2.1 as well.
          Hide
          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
          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
          Aparup Banerjee added a comment -

          created MDL-30465 for stable integration.

          Show
          Aparup Banerjee added a comment - created MDL-30465 for stable integration.
          Hide
          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
          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: