Details

    • Type: Sub-task
    • Status: Closed
    • Priority: Minor
    • Resolution: Fixed
    • Affects Version/s: 2.4
    • Fix Version/s: 2.4
    • Component/s: Course
    • Labels:
    • Testing Instructions:
      Hide

      This issue re-factors code but has no functional changes. The testing should be aimed on making sure that no functionality is lost and no warning/errors are displayed (where they were not displayed before).

      Proper testing would be done by running all QA tests

      Quick test may be performed by making sure that course navigation looks right for different course formats

      Show
      This issue re-factors code but has no functional changes. The testing should be aimed on making sure that no functionality is lost and no warning/errors are displayed (where they were not displayed before). Proper testing would be done by running all QA tests Quick test may be performed by making sure that course navigation looks right for different course formats
    • Affected Branches:
      MOODLE_24_STABLE
    • Fixed Branches:
      MOODLE_24_STABLE
    • Pull Master Branch:
      wip-MDL-35263-master

      Description

      see parent issue and specification

        Gliffy Diagrams

          Issue Links

            Activity

            Hide
            poltawski Dan Poltawski added a comment -

            The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week.

            TIA and ciao

            Show
            poltawski Dan Poltawski added a comment - The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week. TIA and ciao
            Hide
            poltawski Dan Poltawski added a comment -

            Hi Marina,

            Looks good, though its a shame there are no unit tests with this.

            1. get_used_format and the docblock is a bit unclear to me. It is sort of like 'get_format_or_default' or somethign like that. I think at least explaining what its validating would be useful.
            2. The debugging notice in get_used_format should only be DEBUG_DEVELOPER (I expect)
            3. These look ominious // TODO filter only enabled // TODO get default format from config
            4. Does changing the course format setting cause rebuild_course_cache? If not, I guess that the instance cache is not reset. This could be more important with any MUC change as the cache might persist longer.
            5. I think that this will be breaking E_STRICT:

              $format = reset(array_keys(self::$instances[$courseid]));

            6. in format_base::instance() Seems like it'd be more robust to test if this was an integer and throw debugging if not.

              if (isset($courseorid->id)) {	
              	$courseid = (int)$courseorid->id;
              }

            7. (i'm sure this is probably existing problem, but) get_section_name should not concatinate with the lang string, insead we should pass it as a param to the get_string call

            Code checker issues:

            1. We should have a bug number for "@todo use MUC for caching of instances, limit the number of cached instances"
            2. Trailing whitespace errors in lib.php and formatlegacy.php.
            3. lib.php/formatlegacy.php: No scope modifier specified for function "get_section_name"
            4. lib.php: "require_once" must be immediately followed by an open parenthesis (x2)
            5. formatlegacy.php: No scope modifier specified for function "ajax_section_move"
            6. formatlegacy.php: No scope modifier specified for function "uses_sections"
            7. formatlegacy.php: Invalid inline phpdocs tag @see found (x2)
            Show
            poltawski Dan Poltawski added a comment - Hi Marina, Looks good, though its a shame there are no unit tests with this. get_used_format and the docblock is a bit unclear to me. It is sort of like 'get_format_or_default' or somethign like that. I think at least explaining what its validating would be useful. The debugging notice in get_used_format should only be DEBUG_DEVELOPER (I expect) These look ominious // TODO filter only enabled // TODO get default format from config Does changing the course format setting cause rebuild_course_cache? If not, I guess that the instance cache is not reset. This could be more important with any MUC change as the cache might persist longer. I think that this will be breaking E_STRICT: $format = reset(array_keys(self::$instances[$courseid])); in format_base::instance() Seems like it'd be more robust to test if this was an integer and throw debugging if not. if (isset($courseorid->id)) { $courseid = (int)$courseorid->id; } (i'm sure this is probably existing problem, but) get_section_name should not concatinate with the lang string, insead we should pass it as a param to the get_string call Code checker issues: We should have a bug number for "@todo use MUC for caching of instances, limit the number of cached instances" Trailing whitespace errors in lib.php and formatlegacy.php. lib.php/formatlegacy.php: No scope modifier specified for function "get_section_name" lib.php: "require_once" must be immediately followed by an open parenthesis (x2) formatlegacy.php: No scope modifier specified for function "ajax_section_move" formatlegacy.php: No scope modifier specified for function "uses_sections" formatlegacy.php: Invalid inline phpdocs tag @see found (x2)
            Hide
            marina Marina Glancy added a comment -

            Dan, thanks for review.
            I added commit.
            In 6 I used clean_param()
            In 7 there is nothing I can do, this is how it has been for ages
            everything else is corrected

            Show
            marina Marina Glancy added a comment - Dan, thanks for review. I added commit. In 6 I used clean_param() In 7 there is nothing I can do, this is how it has been for ages everything else is corrected
            Hide
            poltawski Dan Poltawski added a comment -

            I've integrated this now.

            Show
            poltawski Dan Poltawski added a comment - I've integrated this now.
            Hide
            poltawski Dan Poltawski added a comment -

            Hi Marina,

            This is failing unit tests:
            global_navigation_testcase::test_format_display_course_content
            coding_exception: Coding error detected, it must be fixed by a programmer: You have attempted to access a method that does not exist for the given object format_display_course_content (32767)

            /Users/Shared/Jenkins/Home/git_repositories/master/lib/tests/navigationlib_test.php:290
            /Users/Shared/Jenkins/Home/git_repositories/master/lib/tests/navigationlib_test.php:358
            /Users/Shared/Jenkins/Home/git_repositories/master/lib/tests/navigationlib_test.php:358
            /Users/Shared/Jenkins/Home/git_repositories/master/lib/phpunit/classes/basic_testcase.php:64

            Show
            poltawski Dan Poltawski added a comment - Hi Marina, This is failing unit tests: global_navigation_testcase::test_format_display_course_content coding_exception: Coding error detected, it must be fixed by a programmer: You have attempted to access a method that does not exist for the given object format_display_course_content (32767) /Users/Shared/Jenkins/Home/git_repositories/master/lib/tests/navigationlib_test.php:290 /Users/Shared/Jenkins/Home/git_repositories/master/lib/tests/navigationlib_test.php:358 /Users/Shared/Jenkins/Home/git_repositories/master/lib/tests/navigationlib_test.php:358 /Users/Shared/Jenkins/Home/git_repositories/master/lib/phpunit/classes/basic_testcase.php:64
            Hide
            marina Marina Glancy added a comment -

            Dan, I removed test test_format_display_course_content(), it fails because the function global_navigation::format_display_course_content() was removed

            Show
            marina Marina Glancy added a comment - Dan, I removed test test_format_display_course_content(), it fails because the function global_navigation::format_display_course_content() was removed
            Hide
            timb Tim Barker added a comment -

            Marina, It's unfeasible and impractical to run all QA tests on Wednesday. The QA tests have massive holes in coverage and are not a good regression test of Moodle - to fix that coverage and run the tests regularly, manually would take more resources than we have available to us. My hope is actually that we will build up sufficient automated unit and functional test coverage to prevent regressions. Did you provide any new unit tests with this issue?

            Raj, testing instructions are: Quick test may be performed by making sure that course navigation looks right for different course formats. Are these adequate for you?

            Show
            timb Tim Barker added a comment - Marina, It's unfeasible and impractical to run all QA tests on Wednesday. The QA tests have massive holes in coverage and are not a good regression test of Moodle - to fix that coverage and run the tests regularly, manually would take more resources than we have available to us. My hope is actually that we will build up sufficient automated unit and functional test coverage to prevent regressions. Did you provide any new unit tests with this issue? Raj, testing instructions are: Quick test may be performed by making sure that course navigation looks right for different course formats. Are these adequate for you?
            Hide
            marina Marina Glancy added a comment -

            Tim, I agree.
            Raj, please note that for social format I fixed navigation a little bit. It is supposed to be hidden all the time (and shown in the special block) but before my changes sometime the modules list appeared in the navigation under 'section 0' heading.

            Show
            marina Marina Glancy added a comment - Tim, I agree. Raj, please note that for social format I fixed navigation a little bit. It is supposed to be hidden all the time (and shown in the special block) but before my changes sometime the modules list appeared in the navigation under 'section 0' heading.
            Hide
            rajeshtaneja Rajesh Taneja added a comment -

            Thanks Tim and Marina,

            Can't see any change in Course navigation, except in Scrom and Social format (As mentioned by Marina).

            It make sense to hide links from navigation which used to get visible while viewing social forum and scrom.

            FYI:
            There are few things which we might want to consider here:

            1. On my home page, user can see pending notifications which let user to navigate and view activity in (social/scrom format) course
            2. If user knows activity link then he/she can view activity
              Just wondering if we have any existing issue to tackle this.
            Show
            rajeshtaneja Rajesh Taneja added a comment - Thanks Tim and Marina, Can't see any change in Course navigation, except in Scrom and Social format (As mentioned by Marina). It make sense to hide links from navigation which used to get visible while viewing social forum and scrom. FYI: There are few things which we might want to consider here: On my home page, user can see pending notifications which let user to navigate and view activity in (social/scrom format) course If user knows activity link then he/she can view activity Just wondering if we have any existing issue to tackle this.
            Hide
            marina Marina Glancy added a comment -

            Raj, hiding activities from navigation has nothing to do with hiding activities from user completely. In fact, they should not be hidden. Social format has it's own block with activities list.

            Show
            marina Marina Glancy added a comment - Raj, hiding activities from navigation has nothing to do with hiding activities from user completely. In fact, they should not be hidden. Social format has it's own block with activities list.
            Hide
            stronk7 Eloy Lafuente (stronk7) added a comment -

            Closing as fixed, many thanks for your awesome collaboration.

            Show
            stronk7 Eloy Lafuente (stronk7) added a comment - Closing as fixed, many thanks for your awesome collaboration.

              People

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

                Dates

                • Created:
                  Updated:
                  Resolved:
                  Fix Release Date:
                  3/Dec/12