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

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)

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

I've integrated this now.

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

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

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?

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.

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.

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.

Closing as fixed, many thanks for your awesome collaboration.