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:

      Description

      write PHPunit tests for user/externallib.php

        Gliffy Diagrams

          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: