Details

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

      Description

      see parent issue and specification

        Issue Links

          Activity

          Hide
          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
          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
          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
          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 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 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
          Dan Poltawski added a comment -

          I've integrated this now.

          Show
          Dan Poltawski added a comment - I've integrated this now.
          Hide
          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
          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 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 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
          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
          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 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 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
          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
          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 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 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
          Eloy Lafuente (stronk7) added a comment -

          Closing as fixed, many thanks for your awesome collaboration.

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