Details

    • Type: Sub-task Sub-task
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 2.3.2, 2.4
    • Fix Version/s: 2.3.3, 2.4
    • Component/s: Web Services
    • Labels:
    • Rank:
      43551

      Description

      write PHPunit tests for user/externallib.php

        Issue Links

          Activity

          Hide
          Jérôme Mouneyrac added a comment -

          Attached github tmp work (one test done)

          Show
          Jérôme Mouneyrac added a comment - Attached github tmp work (one test done)
          Hide
          Jérôme Mouneyrac added a comment -

          Sent for peer-review to Rosie.

          Show
          Jérôme Mouneyrac added a comment - Sent for peer-review to Rosie.
          Hide
          Rossiani Wijaya added a comment -

          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

          Show
          Rossiani Wijaya added a comment - Hi Jerome, Some feedback for the patch: user/lib.php, 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 Remove line 33-40 (duplicate documentation) it might be better to remove the setUp() and place the global $CFG and require_once on top of the page. line 83 and line 148, might be good to add isset() check for $CFG->usetags. The reset looks good
          Hide
          Jérôme Mouneyrac added a comment -

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

          Show
          Jérôme Mouneyrac added a comment - Thanks Rosie (I did the change we talked about), submitting to integration.
          Hide
          Eloy Lafuente (stronk7) 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
          Eloy Lafuente (stronk7) 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
          Aparup Banerjee added a comment -

          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.

          Show
          Aparup Banerjee added a comment - 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.
          Hide
          CiBoT added a comment -

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

          Show
          CiBoT added a comment - Moving this reopened issue out from current integration. Please, re-submit it for integration once ready.
          Hide
          Jérôme Mouneyrac added a comment - - edited

          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.

          Show
          Jérôme Mouneyrac added a comment - - edited 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.
          Hide
          Dan Poltawski added a comment -

          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.

          Show
          Dan Poltawski added a comment - 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.
          Hide
          Jérôme Mouneyrac added a comment -

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

          Show
          Jérôme Mouneyrac added a comment - no problem I'll split in two commits, doing it now
          Hide
          Jérôme Mouneyrac added a comment -

          issue blocked by: MDL-35644

          Show
          Jérôme Mouneyrac added a comment - issue blocked by: MDL-35644
          Hide
          Dan Poltawski added a comment -

          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

          Show
          Dan Poltawski added a comment - 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
          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
          Eloy Lafuente (stronk7) added a comment -

          Integrated (23 & master), thanks!

          (note I've backported this to 23_STABLE)

          Show
          Eloy Lafuente (stronk7) added a comment - Integrated (23 & master), thanks! (note I've backported this to 23_STABLE)
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Tested while backporting. Passed.

          Show
          Eloy Lafuente (stronk7) added a comment - Tested while backporting. Passed.
          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:
              0 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved: