Moodle
  1. Moodle
  2. MDL-29070

Clean enrol_imsenterprise following coding guidelines

    Details

    • Type: Task Task
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 2.0.4, 2.1.1, 2.2, 2.4.6, 2.5.2
    • Fix Version/s: 2.6
    • Component/s: Libraries
    • Labels:
    • Testing Instructions:
      Hide
      1. Download the attached IMS enterprise enrolment file and put it in your moodledata directory
      2. Go to Home / Site administration / Plugins / Enrolments
      3. Enable the IMS enterprise enrolment plugin
      4. In the settings, provide the path to the file you uploaded in File location
      5. Save settings
      6. Go to the link 'perform an IMS Enterprise import right now'
      7. VERIFY: the ims enrollment works ok (courses are created)
      8. VERIFY: no debugging is experienced
      9. Run the phpunit tests
      Show
      Download the attached IMS enterprise enrolment file and put it in your moodledata directory Go to Home / Site administration / Plugins / Enrolments Enable the IMS enterprise enrolment plugin In the settings, provide the path to the file you uploaded in File location Save settings Go to the link 'perform an IMS Enterprise import right now' VERIFY: the ims enrollment works ok (courses are created) VERIFY: no debugging is experienced Run the phpunit tests
    • Affected Branches:
      MOODLE_20_STABLE, MOODLE_21_STABLE, MOODLE_22_STABLE, MOODLE_24_STABLE, MOODLE_25_STABLE
    • Fixed Branches:
      MOODLE_26_STABLE
    • Pull Master Branch:
      MDL-29070-master
    • Story Points (Obsolete):
      3

      Description

      While reviewing MDL-28962 I detected that enrol/imsenterprise/lib.php is one of the worst files I've seen from the POV of Moodle coding guidelines (spaces, comments..., if/loop...).

      I think it would be great to perform a general cleanup for it. For your consideration if only dev or all 2.x stable branches too (note that, right now the file is 100% the same in 20, 21 and master). I'm sending this to STABLE backlog initially.

      Ciao

        Gliffy Diagrams

          Issue Links

            Activity

            Hide
            Dan Poltawski added a comment -

            Requesting peer review - my commits can be squashed, but i've kept them apart for review purposes.

            Show
            Dan Poltawski added a comment - Requesting peer review - my commits can be squashed, but i've kept them apart for review purposes.
            Hide
            Ankit Agarwal added a comment -

            Hi Dan,
            Can you please pull in the latest codechecker and re run it? you have a few "spaces around operators errors"

            Cheers

            Show
            Ankit Agarwal added a comment - Hi Dan, Can you please pull in the latest codechecker and re run it? you have a few "spaces around operators errors" Cheers
            Hide
            Dan Poltawski added a comment -

            Doh, thanks! Pushed fixes and rebased.

            Show
            Dan Poltawski added a comment - Doh, thanks! Pushed fixes and rebased.
            Hide
            Ankit Agarwal added a comment -

            Thanks for the update.

            1. Is it necessary to delete readme.txt?
            2. Please update the title of the issue to reflect that the whole plugin is cleaned not just lib.php
            3. imsenterprise_test.php is missing the Gpl doc block at the start of the page and also has some other operator space errors.
            4. upgrade.php has some errors in comment styles.
            5. should the line length limit of 132, needs to be followed in lang files?
            6. There are some moodlecheck errors. Please run the tool to identify the issues.

            Rest looks good.
            Cheers

            Show
            Ankit Agarwal added a comment - Thanks for the update. Is it necessary to delete readme.txt? Please update the title of the issue to reflect that the whole plugin is cleaned not just lib.php imsenterprise_test.php is missing the Gpl doc block at the start of the page and also has some other operator space errors. upgrade.php has some errors in comment styles. should the line length limit of 132, needs to be followed in lang files? There are some moodlecheck errors. Please run the tool to identify the issues. Rest looks good. Cheers
            Hide
            Michael de Raadt added a comment -

            Carrying over to the next sprint

            Show
            Michael de Raadt added a comment - Carrying over to the next sprint
            Hide
            moodle.com added a comment -

            Taking this out of sprint as it isn't a priority issue.

            Show
            moodle.com added a comment - Taking this out of sprint as it isn't a priority issue.
            Hide
            Dan Poltawski added a comment - - edited

            Hi Ankit,

            Thanks again for picking those up. I've added some more commits fixing the problems (note I plan to ask the integrator to squash them all down, but leaving like that for review). I've rebased on latest master too.

            1. Yes, I think that read me file was outdated and misleading, better none than misleading info.
            2. Done.
            3. I think you meant importnow.php - i've added now.
            4. Fixed.
            5. No, language files should be as long as the string and never concatenated.
            6. Fixed.

            Show
            Dan Poltawski added a comment - - edited Hi Ankit, Thanks again for picking those up. I've added some more commits fixing the problems (note I plan to ask the integrator to squash them all down, but leaving like that for review). I've rebased on latest master too. 1. Yes, I think that read me file was outdated and misleading, better none than misleading info. 2. Done. 3. I think you meant importnow.php - i've added now. 4. Fixed. 5. No, language files should be as long as the string and never concatenated. 6. Fixed.
            Hide
            Ankit Agarwal added a comment -

            Looks good Dan, +1

            Show
            Ankit Agarwal added a comment - Looks good Dan, +1
            Hide
            Dan Poltawski added a comment -

            Thanks Ankit.

            TO INTEGRATOR: Please squash the commits down, they are split for your reviewing ease. I know this is a questionable call at this late stage, but it will really help for some related issues to have a clean base to start from.

            Show
            Dan Poltawski added a comment - Thanks Ankit. TO INTEGRATOR: Please squash the commits down, they are split for your reviewing ease. I know this is a questionable call at this late stage, but it will really help for some related issues to have a clean base to start from.
            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
            Damyon Wiese added a comment -

            Integrated to master only. I squashed the commits on request (left one from each author).

            In reviewing (yay git diff -w) - the only real code changes I saw were to remove some unused variables and declaring functions as public/private which all seemed to be correct.

            There are 2 functions that were converted to static - but are always used via $this - IMO thats ugly but works in PHP. (get_recstatus and decode_timeframe).

            Show
            Damyon Wiese added a comment - Integrated to master only. I squashed the commits on request (left one from each author). In reviewing (yay git diff -w) - the only real code changes I saw were to remove some unused variables and declaring functions as public/private which all seemed to be correct. There are 2 functions that were converted to static - but are always used via $this - IMO thats ugly but works in PHP. (get_recstatus and decode_timeframe).
            Hide
            Damyon Wiese added a comment -

            Thanks Brian and Dan!

            Show
            Damyon Wiese added a comment - Thanks Brian and Dan!
            Hide
            Jason Fowler added a comment -

            All working Dan, thanks for the patch

            Show
            Jason Fowler added a comment - All working Dan, thanks for the patch
            Hide
            Eloy Lafuente (stronk7) added a comment -

            "Aequam memento rebus in arduis servare mentem"

            Many thanks for your hard work, this is now part of "Moodle, the LMS". Closing!

            Ciao

            Show
            Eloy Lafuente (stronk7) added a comment - "Aequam memento rebus in arduis servare mentem" Many thanks for your hard work, this is now part of "Moodle, the LMS". Closing! Ciao

              People

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

                Dates

                • Created:
                  Updated:
                  Resolved: