Moodle
  1. Moodle
  2. MDL-38564

Rewrite and move functions from course/recent.php and add unit tests

    Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 2.4.3, 2.5
    • Fix Version/s: None
    • Component/s: Reports
    • Labels:
      None
    • Rank:
      48584

      Description

      Course/recent.php should not contain functions. All functions should be moved to course/lib.php and proper unit tests should be added for the same
      Also array_key_exists shouldn't be used for properties and objects.

        Issue Links

          Activity

          Hide
          Ankit Agarwal added a comment -

          Requesting a review.
          Thanks

          Show
          Ankit Agarwal added a comment - Requesting a review. Thanks
          Hide
          Rajesh Taneja added a comment -

          Thanks for fixing this Ankit,

          Few things you might want to consider:

          1. L0R3897 - oder should be order
          2. L0R3900 - Might reconsider renaming params from $a and $b to somthing meaningful
          3. L0R3906 and L0R3933 comment should start with uppercase letter.
          4. Similarly testcase can have better variable names.
          5. In if logic you are checking for array_key_exists(), but passing stdClass to it. I know it was like this but I think you should reconsider this.
          Show
          Rajesh Taneja added a comment - Thanks for fixing this Ankit, Few things you might want to consider: L0R3897 - oder should be order L0R3900 - Might reconsider renaming params from $a and $b to somthing meaningful L0R3906 and L0R3933 comment should start with uppercase letter. Similarly testcase can have better variable names. In if logic you are checking for array_key_exists(), but passing stdClass to it. I know it was like this but I think you should reconsider this.
          Hide
          Ankit Agarwal added a comment -

          I have updated the description/summary of the issue to bring rewriting/format fixes into the scope of issue.
          Patch updated. Fixed all things mentioned.
          I have still left the naming $a, $b, as it seems okay for me to use such naming for this use case as the objects can be anything. (Not necessarily activities).
          Requesting another review.
          Thanks

          Show
          Ankit Agarwal added a comment - I have updated the description/summary of the issue to bring rewriting/format fixes into the scope of issue. Patch updated. Fixed all things mentioned. I have still left the naming $a, $b, as it seems okay for me to use such naming for this use case as the objects can be anything. (Not necessarily activities). Requesting another review. Thanks
          Hide
          Rajesh Taneja added a comment -

          Hello Ankit,

          Do you expect timestamp to be NULL ? if not then it will be nice to keep it simple and use isset($a->timestamp)

          Rest all seems fine. Feel free to push it for integration if you feel otherwise.

          Show
          Rajesh Taneja added a comment - Hello Ankit, Do you expect timestamp to be NULL ? if not then it will be nice to keep it simple and use isset($a->timestamp) Rest all seems fine. Feel free to push it for integration if you feel otherwise.
          Hide
          Rajesh Taneja added a comment -

          Also extra space in L0R3931 and L0R3904

          Show
          Rajesh Taneja added a comment - Also extra space in L0R3931 and L0R3904
          Hide
          Ankit Agarwal added a comment -

          Hi Raj,
          As we discussed it "shouldn't" be normally null, but anyway property_exists seems best alternative imo.
          There are no spaces at L0R3931 and L0R3904, are you sure the correct lines are being mentioned?

          Show
          Ankit Agarwal added a comment - Hi Raj, As we discussed it "shouldn't" be normally null, but anyway property_exists seems best alternative imo. There are no spaces at L0R3931 and L0R3904, are you sure the correct lines are being mentioned?
          Hide
          Ankit Agarwal added a comment -

          Submitting for integration.
          Thanks

          Show
          Ankit Agarwal added a comment - Submitting for integration. Thanks
          Hide
          Damyon Wiese 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.

          Thanks!

          Show
          Damyon Wiese 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. Thanks!
          Hide
          Ankit Agarwal added a comment -

          rebased.
          Thanks

          Show
          Ankit Agarwal added a comment - rebased. Thanks
          Hide
          Aparup Banerjee added a comment -

          thanks, thats integrated into master.

          Show
          Aparup Banerjee added a comment - thanks, thats integrated into master.
          Hide
          Adrian Greeve added a comment -

          Unit tests were run and..
          no errors were reported.
          Test passed.

          Show
          Adrian Greeve added a comment - Unit tests were run and.. no errors were reported. Test passed.
          Hide
          Damyon Wiese added a comment -

          Thanks for your hard work. This issue has been integrated upstream and is now available via git (and in some hours, via mirrors and downloads).

          Show
          Damyon Wiese added a comment - Thanks for your hard work. This issue has been integrated upstream and is now available via git (and in some hours, via mirrors and downloads).

            People

            • Votes:
              0 Vote for this issue
              Watchers:
              3 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved: