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: 2.5
    • 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

          Attachments

            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:
                    Fix Release Date:
                    14/May/13