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:
      3
    • Rank:
      18593

      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

        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: