Comments

• 

  Plugins bot

  Mon, 3 Oct 2016, 3:23 PM

  Prechecker results: 301 errors/1 warnings
  https://integration.moodle.org/job/Precheck%20remote%20branch/27871/artifact/work/smurf.html
• 

  Daniel Thee Roperto

  Mon, 3 Oct 2016, 4:08 PM

  Hi. I will fix most of the issues, I am not sure why my moodle-plugin-ci check did not warn me before submitting.
  
  In any case, there are a few problems such as:
  
  https://git.in.moodle.com/mudrd8mz/moodle-plugins-snapshots/blob/63f61fb2ba21ff56c818249746a361409d34b6ea/auth/outage/tests/phpunit/local/controllers/infopage_test.php#L257
  
  That are related to tests, if I use ' $this->expectException() ' I will probably get an deprecated warning. Is there any way to allow annotations?
  
  I will comment again once I fixed most of the errors I can.
  
  Thank you,
  
  Daniel
• 

  David Mudrák

  Mon, 3 Oct 2016, 4:43 PM

  I can't remember now where I saw it, but IIRC it is preferred to use $this->setExpectedException('moodle_exception') over the annotation. In either case, please understand that these pre-checks are primarily here for you to double check they are really intended. We do not discriminate plugins just on these formal prechecks.

• 

  Daniel Thee Roperto

  Tue, 4 Oct 2016, 2:01 PM

  Hi David,
  
  Regarding the tests, you are right, $this->setExpectedException() is better and we can define which moment the exception should be thrown, which we cannot do with annotations.
  
  That said PHPUnit is deprecating the $this->setExpectedException() and suggesting us to use $this->expectsException() instead. We can do that in Moodle 32 (master) but not before that. I decided to use the annotation to cover both cases.
  
  I fixed them by creating a set_expected_exception which will adapt as needed, I just uploaded a new version with most of the issues fixed.
  
  I am expecting a few " Class ) is not documented " messages but I believe this to be an issue with moodlecheck -- I created an issue at:
  
  https://github.com/moodlehq/moodle-local_moodlecheck/issues/31
  
  If you believe the notation with ::class I used is not good, I can revert to strings. I just think the error message from moodlecheck is complaining about an undocumented class, not that we should not use this notation.
  
  Also, I expect a few invalid tag warning for unit tests, for example I use "@depends" to link test cases and a few "@SupressWarnings" for PHPMD in test cases (I don't see the why breaking testcases into more classes just to get PHPMD approval).
  
  I am waiting for the report, once it I will post here and you can guide me if I should make any further changes.
  
  Cheers,
  Daniel
• 

  Plugins bot

  Mon, 10 Oct 2016, 4:41 PM

  Prechecker results: 43 errors/14 warnings
  https://integration.moodle.org/job/Precheck%20remote%20branch/28123/artifact/work/smurf.html
• 

  Daniel Thee Roperto

  Thu, 13 Oct 2016, 7:34 AM

  Hi David, if its not done automatically, can you run again the checks in the version I submitted yesterday? Thanks.
• 

  Dan Marsden

  Fri, 14 Oct 2016, 8:11 AM

  Hi Daniel,
  
  thanks for working on this plugin, I can see it being really useful to manage scheduled outages in a more user-friendly manner. The code is documented nicely and follows Moodle coding guidelines well, also great to see the behat and phpunit tests. My one objection is the inclusion of the local_outage plugin within the code which doesn't seem to be required unless you are running an older version. The version uploaded here in the plugins db for newer versions definitely shouldn't include it, and the version for 2.8 should have it listed as a dependency in the version.php file. Once that is resolved I think it is ready for general release so that others can give further feedback on it.
  
• 

  Daniel Thee Roperto

  Fri, 14 Oct 2016, 10:27 AM

  Hi Dan,
  
  Thank you for the information, I've just submitted the changes as version 1.0.3.
  
  
  Cheers,
  
  Daniel
• 

  Plugins bot

  Tue, 25 Oct 2016, 5:47 AM

  Prechecker results: 0 errors/0 warnings
  https://integration.moodle.org/job/Precheck%20remote%20branch/28697/artifact/work/smurf.html
• 

  David Mudrák

  Tue, 25 Oct 2016, 5:52 AM

  Thanks Daniel for fixing the raised issues promptly, and thanks Dan for the approval review. I just checked and can see that the only remaining blocker with the local_outage presence has been fixed so I am happy to approve this now. You are cleared to land, welcome to the Plugins directory!

• 

  Daniel Thee Roperto

  Tue, 25 Oct 2016, 6:38 AM

  I am happy to hear that, thanks David and Dan!