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

        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