Uploaded image for project: 'Moodle'
  1. Moodle
  2. MDL-38564

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

    Details

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

      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.

        Gliffy Diagrams

          Issue Links

            Activity

            Hide
            ankit_frenz Ankit Agarwal added a comment -

            Requesting a review.
            Thanks

            Show
            ankit_frenz Ankit Agarwal added a comment - Requesting a review. Thanks
            Hide
            rajeshtaneja 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
            rajeshtaneja 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_frenz 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_frenz 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
            rajeshtaneja 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
            rajeshtaneja 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
            rajeshtaneja Rajesh Taneja added a comment -

            Also extra space in L0R3931 and L0R3904

            Show
            rajeshtaneja Rajesh Taneja added a comment - Also extra space in L0R3931 and L0R3904
            Hide
            ankit_frenz 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_frenz 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_frenz Ankit Agarwal added a comment -

            Submitting for integration.
            Thanks

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

            rebased.
            Thanks

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

            thanks, thats integrated into master.

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

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

            Show
            abgreeve Adrian Greeve added a comment - Unit tests were run and.. no errors were reported. Test passed.
            Hide
            damyon 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 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: