Crm 18231 Create provision for testing environment variable #8132

pradpnayak · 2016-04-13T18:01:33Z

No description provided.

---------------------------------------- * CRM-18231: Create provision for testing environment variable https://issues.civicrm.org/jira/browse/CRM-18231

…ule job page ---------------------------------------- * CRM-18231: https://issues.civicrm.org/jira/browse/CRM-18231

… in civicrm.settings.php file ---------------------------------------- * CRM-18231: https://issues.civicrm.org/jira/browse/CRM-18231

…turned off ---------------------------------------- * CRM-18231: Create provision for testing environment variable https://issues.civicrm.org/jira/browse/CRM-18231

---------------------------------------- * CRM-18231: https://issues.civicrm.org/jira/browse/CRM-18231

---------------------------------------- * CRM-18231: Create provision for testing environment variable https://issues.civicrm.org/jira/browse/CRM-18231

eileenmcnaughton · 2016-04-14T04:09:28Z

test this please

pradpnayak · 2016-04-18T21:03:02Z

@eileenmcnaughton can you please review this for us?

eileenmcnaughton · 2016-04-19T10:16:05Z

Hi Pradeep, I had hoped to bring it into this months QA cycle but there was too much to get through. I checked with Monish because if you had put in a lot of work already this month but he wasn't aware of you having done that so I decided it makes more sense to push it to next month than for me to stay up late to get it done

eileenmcnaughton · 2016-04-19T10:17:38Z

CRM/Admin/Form/Setting/Debugging.php

+  public function postProcess() {
+    $params = $this->controller->exportValues($this->_name);
+
+    if ($params['isProductionEnvironment'] === FALSE) {


One thing I did notices is that this is happening in the form - which is not correct, it should work via the api too - take a look at how the logging setting works for how to not do it in the form layer.

@eileenmcnaughton I'm not quite sure what you are getting at here: isn't Civi::settings()->set() merely a wrapper for functionality also exposed via API wrapper? Or is your concern that the success message should be bubbled up from something the API calls too?

I mean - I wasn't expecting the call backs relevant to this setting to be doing any thing in the form layer. ie. this is setting a setting in the form post process hook - if you look at the logging setting, by contrast, disabling & enabling the field has a callback that does the needed, regardless where it is called from

eileenmcnaughton · 2016-04-19T10:32:29Z

NB - I don't have time to do a full QA but I think there is a lot to like here - it's well tested and I can see the idea is that if you want to test scheduled jobs etc you need to make sure it's set to production which definitely has merit.

One hesitation I have is that it feels like it should be isDevelopment or even environment to allow for the possibility of more options, but I'm waivering a bit on that. I think equating isProduction unset with isProduction = FALSE would be flakey on 4.6 but it might not be on 4.7.

A few things probably worth doing

sending an email on the dev list mentioning you have done this PR & inviting comment - since they will be affected
adding a few lines to the civicrm_settings.php as a template for enabling this
How will people know that the setting is enabled? Or not enabled? Should Dev sites have something showing?

xurizaemon · 2016-05-17T03:47:37Z

👍 to Eileen's comments above - both that I love the direction, and that isProduction feels prescriptive.

IMO isProduction is a true-false thing, and the aim here is to move towards more flexible methods around development workflow (not just production-or-not but dev-test-staging-production and back again). Drupal's Environment module has similarities, and that it ships with development/production/readonly as defaults while permitting other environments is IMO a good thing.

Sorry for coming into the discussion a month later but hadn't had this appear on my radar until now!

JoeMurray · 2016-05-20T19:38:20Z

CRM/Admin/Page/Job.php

@@ -142,6 +142,10 @@ public function run() {
   * @param null $action
   */
  public function browse($action = NULL) {
+    // check if non-prod mode is enabled.
+    if (Civi::settings()->get('isProductionEnvironment') === FALSE) {
+      CRM_Core_Session::setStatus(ts('Execution of scheduled jobs have been turned off.'), ts("Non-production Environment"), "warning", array('expires' => 0));


Just to be precise about things: when production environment is toggled off, execution of scheduled jobs is turned off by default, BUT parameters on jobs can override this. So please change message to 'Execution of scheduled jobs has been turned off by default since this is a non-production environment. You can override this for particular jobs by adding runInNonProductionEnvironment=TRUE as a parameter.'

JoeMurray · 2016-05-20T20:18:33Z

@eileenmcnaughton sorry I didn't see you comments till now. I think it is appropriate that when JMA didn't put in the volunteer time as promised in April that you didn't put in extra effort on this. Our bad. Would have been nice to see it included in list for May. @totten thank for putting on list for June.

I think the default value for whatever the new setting is called should be production. I considered increasing scope to put in staging/dev/test, but a) could not see any differences for our current use cases amongst the non-production environments, and b) didn't want to gold-plate especially since this not fully funded by client (http://www.projectmanagementlearning.com/what-is-gold-plating-in-project-management.html). I'd be happy to see this develop in the future, but feel it addresses something useful as it is. I see @xurizaemon also agrees, so now I'm on the fence about refactoring to support more values than just true false.

I'll send out a message to dev - great idea. I think there was a mention in response to @adixon but no call for comments.
Yes, we'll add some commented out text showing how to enable as well as explanation.
Umm, I was thinking the url would be the key, but I guess you're suggesting that when functionality is turned off on non-production environments then something dramatic should be displayed, beyond the values in the settings page and scheduled job page? I don't want it to be so irritating that people would want a setting to turn off the notice about the value of this setting...

JoeMurray · 2016-05-20T20:36:55Z

CRM/Admin/Form/Setting/Debugging.php

+
+    if ($params['isProductionEnvironment'] === FALSE) {
+      Civi::settings()->set('mailing_backend', array('outBound_option' => CRM_Mailing_Config::OUTBOUND_OPTION_DISABLED));
+      CRM_Core_Session::setStatus(ts('Outbound emails have been disabled. Scheduled jobs have been prevented from being executed.'), ts("Non-production environment set"), "success");


Please change message to ''Outbound emails have been disabled. Scheduled jobs will not run unless runInNonProductionEnvironment=TRUE is added as a parameter for a specific job"

eileenmcnaughton · 2016-06-03T21:16:11Z

Closing by request of @JoeMurray as spec is revised

pradpnayak added 8 commits April 13, 2016 21:38

CRM-18231, added settings for isProductionEnvironment

24ddcdd

---------------------------------------- * CRM-18231: Create provision for testing environment variable https://issues.civicrm.org/jira/browse/CRM-18231

CRM-18231, added form fields on debugging form

8d4f39a

---------------------------------------- * CRM-18231: Create provision for testing environment variable https://issues.civicrm.org/jira/browse/CRM-18231

CRM-18231, added status message on saving of debugging form and sched…

d2f82ad

…ule job page ---------------------------------------- * CRM-18231: https://issues.civicrm.org/jira/browse/CRM-18231

CRM-18231, added condition to check if isProductionEnvironment is set…

28b88db

… in civicrm.settings.php file ---------------------------------------- * CRM-18231: https://issues.civicrm.org/jira/browse/CRM-18231

CRM-18231, stop running schedule job when isProductionEnvironment is …

d792876

…turned off ---------------------------------------- * CRM-18231: Create provision for testing environment variable https://issues.civicrm.org/jira/browse/CRM-18231

CRM-18231, added CRM unit test

573643f

---------------------------------------- * CRM-18231: https://issues.civicrm.org/jira/browse/CRM-18231

CRM-18231, added api unit test

fb3e717

---------------------------------------- * CRM-18231: Create provision for testing environment variable https://issues.civicrm.org/jira/browse/CRM-18231

CRM-18231, added webtest

3346bef

---------------------------------------- * CRM-18231: Create provision for testing environment variable https://issues.civicrm.org/jira/browse/CRM-18231

civicrm-builder added the master label Apr 13, 2016

pradpnayak mentioned this pull request Apr 13, 2016

CRM-18231 Create provision for testing environment variable #7973

Closed

pradpnayak changed the title ~~Crm 18231 master~~ Crm 18231 Create provision for testing environment variable Apr 14, 2016

eileenmcnaughton reviewed Apr 19, 2016
View reviewed changes

totten mentioned this pull request May 6, 2016

Invitation: CiviCRM Release for June 2016 civicrm/release-management#2

Closed

JoeMurray reviewed May 20, 2016
View reviewed changes

eileenmcnaughton closed this Jun 3, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Crm 18231 Create provision for testing environment variable #8132

Crm 18231 Create provision for testing environment variable #8132

pradpnayak commented Apr 13, 2016

eileenmcnaughton commented Apr 14, 2016

pradpnayak commented Apr 18, 2016

eileenmcnaughton commented Apr 19, 2016

eileenmcnaughton Apr 19, 2016

JoeMurray May 20, 2016

eileenmcnaughton May 21, 2016

eileenmcnaughton commented Apr 19, 2016

xurizaemon commented May 17, 2016 •

edited

Loading

JoeMurray May 20, 2016

JoeMurray commented May 20, 2016

JoeMurray May 20, 2016

eileenmcnaughton commented Jun 3, 2016

Crm 18231 Create provision for testing environment variable #8132

Crm 18231 Create provision for testing environment variable #8132

Conversation

pradpnayak commented Apr 13, 2016

eileenmcnaughton commented Apr 14, 2016

pradpnayak commented Apr 18, 2016

eileenmcnaughton commented Apr 19, 2016

eileenmcnaughton Apr 19, 2016

Choose a reason for hiding this comment

JoeMurray May 20, 2016

Choose a reason for hiding this comment

eileenmcnaughton May 21, 2016

Choose a reason for hiding this comment

eileenmcnaughton commented Apr 19, 2016

xurizaemon commented May 17, 2016 • edited Loading

JoeMurray May 20, 2016

Choose a reason for hiding this comment

JoeMurray commented May 20, 2016

JoeMurray May 20, 2016

Choose a reason for hiding this comment

eileenmcnaughton commented Jun 3, 2016

xurizaemon commented May 17, 2016 •

edited

Loading