Details

    • Type: Sub-task Sub-task
    • Status: Closed
    • Priority: Minor 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
    • Rank:
      17373

      Activity

      Hide
      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
      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
      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
      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
      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
      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
      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
      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
      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
      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
      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
      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
      Eloy Lafuente (stronk7) added a comment -

      Integrated, thanks!

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

      Passed, thanks.

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

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

      Show
      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: