PHPunit test: user/externallib.php

Attached github tmp work (one test done)

Sent for peer-review to Rosie.

Hi Jerome,

Some feedback for the patch:
user/lib.php,

1. I don't think

    if (!$cannotviewdescription) 

   should be part of $user->description and $hiddenfields check. the code will performed as long as you are an admin, regardless the value of $cannotviewdescription.

user/tests/externallib_test.php

1. Remove line 33-40 (duplicate documentation)
2. it might be better to remove the setUp() and place the global $CFG and require_once on top of the page.
3. line 83 and line 148, might be good to add isset() check for $CFG->usetags.

The reset looks good

Thanks Rosie (I did the change we talked about), submitting to integration.

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 Jerome,
@user/lib.php line 338 : there is a slight change of logic it seems.

if (isset($user->description) &&  
    (!isset($hiddenfields['description']) or $isadmin)) {

  if (!$cannotviewdescription) {

is If(a && (!b or c) .. && !d) where the last && is due to the 2nd if statement .. compare that to

if (isset($user->description) &&	
    ((!isset($hiddenfields['description']) && !$cannotviewdescription) or $isadmin)

is if(a && (!b && !d) or c) ..

so few things:

• the commit for this bit of change needs to be in a different commit which explains the change.
• the change itself needs to be commented about in this MDL or maybe another dependant MDL so any one can look up the reason for this change.
• i suspect this change is part of fixes you saw were needed during writing this phpunit test. so just needs explanation. we can integrate both fixes as dependant on each other if needed.
• lastly, we can summarise the long 'if' into a var like $ishungry = $notatefood && $pastlunchhour as per our coding style for better readability

reopening for further look and commit tidying.

Moving this reopened issue out from current integration. Please, re-submit it for integration once ready.

yes it's a fix, sorry for not mentioning the reason. The reason is that admins can see the user description in any case (except if the field is not requested).

I thought about making a different commit for this, but I didn't think it was deserving two issues (double peer-reviews/double testing/integration waiting for a different issue/...). But now I regret it as you are second person to stop this issue for the same reason

Let me know if you really want another commit. I think you will have accepted it first if I had told you the reason in a comment, so I resend it to integration.

Hi Jerome,

It'd be good to split the commits in two, I don't have a problem with the fix using the same issue, but for the benefit of people looking at the change history I think it should be split and explained in a commit log.

In fact, shouldn't this logic fix be backported? In which case the logic fix should be applied to all issues and thus perhaps should be a new issue.

no problem I'll split in two commits, doing it now

issue blocked by: MDL-35644

The integration of this issue has been delayed to next week because the integration period is over (Monday, Tuesday) and testing must happen on Wednesday.
This change to a more rigid timeframe on each integration/testing cycle aims to produce a better and clear separation and organization of tasks for everybody.
This is a bulk-automated message, so if you want to blame somebody/thing/where, don't do it here (use git instead) :-D :-P
Apologizes for the inconvenient, this will be integrated next week. Thanks for your collaboration & ciao

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

Integrated (23 & master), thanks!

(note I've backported this to 23_STABLE)

Tested while backporting. Passed.

Closing as fixed, many thanks for your awesome collaboration.