Details

    • Type: Sub-task
    • Status: Closed
    • Priority: Minor
    • Resolution: Fixed
    • Affects Version/s: 2.0.3
    • Fix Version/s: 2.1
    • Component/s: Performance
    • Labels:
      None
    • Testing Instructions:
      Hide

      This is a performance issue - there should be no difference in the user interface and things should work as they did before.

      1. Log in as an admin
      2. Disable portoflios
      3. Browse to a forum, chat, data, assignment, and glossary.
      4. Ensure you see things correctly and that you don't see portfolio export buttons
      5. Enable portfolios
      6. Browse to a forum, chat, data, assignment, and glossary.
      7. Ensure you see things correctly and that you see portfolio export buttons in the correct places.
      Show
      This is a performance issue - there should be no difference in the user interface and things should work as they did before. Log in as an admin Disable portoflios Browse to a forum, chat, data, assignment, and glossary. Ensure you see things correctly and that you don't see portfolio export buttons Enable portfolios Browse to a forum, chat, data, assignment, and glossary. Ensure you see things correctly and that you see portfolio export buttons in the correct places.
    • Affected Branches:
      MOODLE_20_STABLE
    • Fixed Branches:
      MOODLE_21_STABLE
    • Pull Master Branch:
      wip-MDL-27810-master

      Gliffy Diagrams

        Attachments

          Activity

          Hide
          samhemelryk Sam Hemelryk added a comment -

          Looks like portfolios are being initialised regardless of being disabled (they are not displayed if disabled but still no need to initialise)

          Show
          samhemelryk Sam Hemelryk added a comment - Looks like portfolios are being initialised regardless of being disabled (they are not displayed if disabled but still no need to initialise)
          Hide
          rwijaya Rossiani Wijaya added a comment -

          Hi Sam,

          With portfolio is set to disable (note: works fine with portfolio is set to enable), I noticed the following error on the glossary page :

          Building portfolio add button while portfolios is disabled. This code can be optimised.
           
              * line 107 of /lib/portfoliolib.php: call to debugging()
              * line 1137 of /mod/glossary/lib.php: call to portfolio_add_button->__construct()
              * line 1205 of /mod/glossary/lib.php: call to glossary_print_entry_icons()
              * line 18 of /mod/glossary/formats/dictionary/dictionary_format.php: call to glossary_print_entry_lower_section()
              * line 926 of /mod/glossary/lib.php: call to glossary_show_entry_dictionary()
              * line 476 of /mod/glossary/view.php: call to glossary_print_entry()

          To re-produce the error:
          1. set 'portfolio' to disable in admin setting.
          2. create a item to glossary
          3. select 'browse by alphabet/date/author' tab.

          The above error will occurs.

          Show
          rwijaya Rossiani Wijaya added a comment - Hi Sam, With portfolio is set to disable (note: works fine with portfolio is set to enable), I noticed the following error on the glossary page : Building portfolio add button while portfolios is disabled. This code can be optimised.   * line 107 of /lib/portfoliolib.php: call to debugging() * line 1137 of /mod/glossary/lib.php: call to portfolio_add_button->__construct() * line 1205 of /mod/glossary/lib.php: call to glossary_print_entry_icons() * line 18 of /mod/glossary/formats/dictionary/dictionary_format.php: call to glossary_print_entry_lower_section() * line 926 of /mod/glossary/lib.php: call to glossary_show_entry_dictionary() * line 476 of /mod/glossary/view.php: call to glossary_print_entry() To re-produce the error: 1. set 'portfolio' to disable in admin setting. 2. create a item to glossary 3. select 'browse by alphabet/date/author' tab. The above error will occurs.
          Hide
          samhemelryk Sam Hemelryk added a comment -

          Thanks Rosie, there was a nesting bug on 1137 which I've now fixed and pushed up to github.

          Show
          samhemelryk Sam Hemelryk added a comment - Thanks Rosie, there was a nesting bug on 1137 which I've now fixed and pushed up to github.
          Hide
          rwijaya Rossiani Wijaya added a comment -

          Hi Sam,

          I found more errors with portfolio set to disabled:

          1. chat module: view past chat sessions.

          Building portfolio add button while portfolios is disabled. This code can be optimised.
           
              * line 107 of /lib/portfoliolib.php: call to debugging()
              * line 264 of /mod/chat/report.php: call to portfolio_add_button->__construct()

          2. chat module - list all sessions

          Building portfolio add button while portfolios is disabled. This code can be optimised.
           
              * line 107 of /lib/portfoliolib.php: call to debugging()
              * line 264 of /mod/chat/report.php: call to portfolio_add_button->__construct()
           
          Building portfolio add button while portfolios is disabled. This code can be optimised.
           
              * line 107 of /lib/portfoliolib.php: call to debugging()
              * line 241 of /mod/chat/report.php: call to portfolio_add_button->__construct()

          3. chat module: chat session (from list all session page, click on 'see this session')

          Building portfolio add button while portfolios is disabled. This code can be optimised.
           
              * line 107 of /lib/portfoliolib.php: call to debugging()
              * line 114 of /mod/chat/report.php: call to portfolio_add_button->__construct()

          4. Chad module: possible improvement on 'listing all sessions', add breaking line for 'export to porfolio' link. currently there is no break/space between 'see this session' and 'export to portfolio' links.

          Show
          rwijaya Rossiani Wijaya added a comment - Hi Sam, I found more errors with portfolio set to disabled: 1. chat module: view past chat sessions. Building portfolio add button while portfolios is disabled. This code can be optimised.   * line 107 of /lib/portfoliolib.php: call to debugging() * line 264 of /mod/chat/report.php: call to portfolio_add_button->__construct() 2. chat module - list all sessions Building portfolio add button while portfolios is disabled. This code can be optimised.   * line 107 of /lib/portfoliolib.php: call to debugging() * line 264 of /mod/chat/report.php: call to portfolio_add_button->__construct()   Building portfolio add button while portfolios is disabled. This code can be optimised.   * line 107 of /lib/portfoliolib.php: call to debugging() * line 241 of /mod/chat/report.php: call to portfolio_add_button->__construct() 3. chat module: chat session (from list all session page, click on 'see this session') Building portfolio add button while portfolios is disabled. This code can be optimised.   * line 107 of /lib/portfoliolib.php: call to debugging() * line 114 of /mod/chat/report.php: call to portfolio_add_button->__construct() 4. Chad module: possible improvement on 'listing all sessions', add breaking line for 'export to porfolio' link. currently there is no break/space between 'see this session' and 'export to portfolio' links.
          Hide
          samhemelryk Sam Hemelryk added a comment -

          Thanks for spotting those Rosie,
          There was one area that I had missed adding the check for portfolioenabled and one area where I hadn't added the required brackets for the check (leading to a naughty ||).

          I've put this up for integration now, Eloy this patch just adds $CFG->portfolioenabled checks to code generating portfolio buttons.
          It makes a big difference by default for teachers+ as when portfolios are disabled they are being initialised in several places still.

          Cheers
          Sam

          Show
          samhemelryk Sam Hemelryk added a comment - Thanks for spotting those Rosie, There was one area that I had missed adding the check for portfolioenabled and one area where I hadn't added the required brackets for the check (leading to a naughty ||). I've put this up for integration now, Eloy this patch just adds $CFG->portfolioenabled checks to code generating portfolio buttons. It makes a big difference by default for teachers+ as when portfolios are disabled they are being initialised in several places still. Cheers Sam
          Hide
          stronk7 Eloy Lafuente (stronk7) added a comment -

          Uhm.. reviewing uses of $button... I just found one places where I think the check is missing:

          • mod/assignment/lib.php, line 1896. There is one >set_callback_options() call not under the $CFG>enableportfolios "umbrella" check.

          All the rest seems ok, so I'm integrating this by adding the missing check.

          Ciao

          Show
          stronk7 Eloy Lafuente (stronk7) added a comment - Uhm.. reviewing uses of $button... I just found one places where I think the check is missing: mod/assignment/lib.php, line 1896. There is one >set_callback_options() call not under the $CFG >enableportfolios "umbrella" check. All the rest seems ok, so I'm integrating this by adding the missing check. Ciao
          Hide
          stronk7 Eloy Lafuente (stronk7) added a comment -

          Integrated, thanks!

          Show
          stronk7 Eloy Lafuente (stronk7) added a comment - Integrated, thanks!
          Hide
          dongsheng Dongsheng Cai added a comment -

          Passed, thanks.

          Show
          dongsheng Dongsheng Cai added a comment - Passed, thanks.
          Hide
          stronk7 Eloy Lafuente (stronk7) added a comment -

          All git & cvs servers have been updated with these cool changes, so closing, many thanks!

          Show
          stronk7 Eloy Lafuente (stronk7) added a comment - All git & cvs servers have been updated with these cool changes, so closing, many thanks!

            People

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

              Dates

              • Created:
                Updated:
                Resolved:
                Fix Release Date:
                1/Jul/11