From 3184b43b3b8d182abb5be1940290f9f601190e2e Mon Sep 17 00:00:00 2001 From: Rostislav Wolny Date: Wed, 18 Jul 2018 15:32:58 +0200 Subject: [PATCH 1/3] Add status flag for cron deactivation Cron daemon, which was triggered by WordPress method, was deactivated by deletion from DB and it caused the lost of all log data about it. This commit changes the implementation so that the daemon is deactivated by changing status flag. [MAILPOET-1457] --- lib/Cron/CronHelper.php | 17 +++++++++---- lib/Cron/Daemon.php | 13 ++++++++-- lib/Cron/Supervisor.php | 4 +++- lib/Cron/Triggers/WordPress.php | 6 ++--- tests/unit/Cron/CronHelperTest.php | 28 ++++++++++++++++++++++ tests/unit/Cron/DaemonTest.php | 16 +++++++++++++ tests/unit/Cron/SupervisorTest.php | 10 ++++++++ tests/unit/Cron/Triggers/WordPressTest.php | 18 +++++++------- 8 files changed, 92 insertions(+), 20 deletions(-) diff --git a/lib/Cron/CronHelper.php b/lib/Cron/CronHelper.php index c215e42297..820798eedd 100644 --- a/lib/Cron/CronHelper.php +++ b/lib/Cron/CronHelper.php @@ -16,15 +16,18 @@ class CronHelper { const DAEMON_EXECUTION_TIMEOUT = 35; // seconds const DAEMON_REQUEST_TIMEOUT = 5; // seconds const DAEMON_SETTING = 'cron_daemon'; + const DAEMON_STATUS_ACTIVE = 'active'; + const DAEMON_STATUS_INACTIVE = 'inactive'; static function createDaemon($token) { - $daemon = array( + $daemon = [ 'token' => $token, + 'status' => self::DAEMON_STATUS_ACTIVE, 'run_accessed_at' => null, 'run_started_at' => null, 'run_completed_at' => null, 'last_error' => null, - ); + ]; self::saveDaemon($daemon); return $daemon; } @@ -45,8 +48,12 @@ class CronHelper { ); } - static function deleteDaemon() { - return Setting::deleteValue(self::DAEMON_SETTING); + static function deactivateDaemon($daemon) { + $daemon['status'] = self::DAEMON_STATUS_INACTIVE; + return Setting::setValue( + self::DAEMON_SETTING, + $daemon + ); } static function createToken() { @@ -89,7 +96,7 @@ class CronHelper { */ static function isDaemonAccessible() { $daemon = self::getDaemon(); - if(!$daemon || $daemon['run_accessed_at'] === null) { + if(!$daemon || !isset($daemon['run_accessed_at']) || $daemon['run_accessed_at'] === null) { return null; } if($daemon['run_accessed_at'] <= (int)$daemon['run_started_at']) { diff --git a/lib/Cron/Daemon.php b/lib/Cron/Daemon.php index 2e230bdc36..0b848c6345 100644 --- a/lib/Cron/Daemon.php +++ b/lib/Cron/Daemon.php @@ -75,7 +75,7 @@ class Daemon { } // after each execution, re-read daemon data in case it changed $daemon = CronHelper::getDaemon(); - if(!$daemon || $daemon['token'] !== $this->token) { + if($this->shouldTerminateExecution($daemon)) { return $this->terminateRequest(); } return $this->callSelf(); @@ -128,4 +128,13 @@ class Daemon { function terminateRequest($message = false) { die($message); } -} + + /** + * @return boolean + */ + private function shouldTerminateExecution(array $daemon = null) { + return !$daemon || + $daemon['token'] !== $this->token || + (isset($daemon['status']) && $daemon['status'] !== CronHelper::DAEMON_STATUS_ACTIVE); + } +} \ No newline at end of file diff --git a/lib/Cron/Supervisor.php b/lib/Cron/Supervisor.php index 4cb29edda0..dfbda9d04a 100644 --- a/lib/Cron/Supervisor.php +++ b/lib/Cron/Supervisor.php @@ -16,7 +16,9 @@ class Supervisor { $daemon = $this->daemon; $execution_timeout_exceeded = (time() - (int)$daemon['updated_at']) >= CronHelper::DAEMON_EXECUTION_TIMEOUT; - if($execution_timeout_exceeded) { + $daemon_is_inactive = + isset($daemon['status']) && $daemon['status'] === CronHelper::DAEMON_STATUS_INACTIVE; + if($execution_timeout_exceeded || $daemon_is_inactive) { CronHelper::restartDaemon($this->token); return $this->runDaemon(); } diff --git a/lib/Cron/Triggers/WordPress.php b/lib/Cron/Triggers/WordPress.php index 117919c23c..9bfc417db8 100644 --- a/lib/Cron/Triggers/WordPress.php +++ b/lib/Cron/Triggers/WordPress.php @@ -18,7 +18,7 @@ class WordPress { static function run() { return (self::checkExecutionRequirements()) ? MailPoet::run() : - self::cleanup(); + self::stop(); } static function checkExecutionRequirements() { @@ -60,10 +60,10 @@ class WordPress { ); } - static function cleanup() { + static function stop() { $cron_daemon = CronHelper::getDaemon(); if($cron_daemon) { - CronHelper::deleteDaemon(); + CronHelper::deactivateDaemon($cron_daemon); } } } diff --git a/tests/unit/Cron/CronHelperTest.php b/tests/unit/Cron/CronHelperTest.php index b19825c1d6..1b2c7c3e11 100644 --- a/tests/unit/Cron/CronHelperTest.php +++ b/tests/unit/Cron/CronHelperTest.php @@ -33,6 +33,7 @@ class CronHelperTest extends \MailPoetTest { expect($daemon)->equals( array( 'token' => $token, + 'status' => CronHelper::DAEMON_STATUS_ACTIVE, 'updated_at' => $time, 'run_accessed_at' => null, 'run_started_at' => null, @@ -50,6 +51,7 @@ class CronHelperTest extends \MailPoetTest { expect($daemon)->equals( array( 'token' => $token, + 'status' => CronHelper::DAEMON_STATUS_ACTIVE, 'updated_at' => $time, 'run_accessed_at' => null, 'run_started_at' => null, @@ -62,6 +64,7 @@ class CronHelperTest extends \MailPoetTest { function testItLoadsDaemon() { $daemon = array( 'token' => 'some_token', + 'status' => CronHelper::DAEMON_STATUS_ACTIVE, 'updated_at' => '12345678', 'run_accessed_at' => null, 'run_started_at' => null, @@ -79,6 +82,7 @@ class CronHelperTest extends \MailPoetTest { // when saving daemon, 'updated_at' value should change $daemon = array( 'token' => 'some_token', + 'status' => CronHelper::DAEMON_STATUS_ACTIVE, 'updated_at' => '12345678', 'run_accessed_at' => null, 'run_started_at' => null, @@ -98,6 +102,7 @@ class CronHelperTest extends \MailPoetTest { function testItUpdatesDaemonAccessedAt() { $daemon = [ 'token' => 'some_token', + 'status' => CronHelper::DAEMON_STATUS_ACTIVE, 'updated_at' => 12345678, 'run_accessed_at' => null, 'run_started_at' => null, @@ -131,6 +136,7 @@ class CronHelperTest extends \MailPoetTest { foreach($run_start_values as $run_start) { $daemon = [ 'token' => 'some_token', + 'status' => CronHelper::DAEMON_STATUS_ACTIVE, 'updated_at' => 12345678, 'run_accessed_at' => $time - 10, 'run_started_at' => $run_start, @@ -149,6 +155,7 @@ class CronHelperTest extends \MailPoetTest { $time = time(); $daemon = [ 'token' => 'some_token', + 'status' => CronHelper::DAEMON_STATUS_ACTIVE, 'updated_at' => 12345678, 'run_accessed_at' => $time - 5, 'run_started_at' => $time - 4, @@ -182,6 +189,7 @@ class CronHelperTest extends \MailPoetTest { foreach($test_inputs as $test_input) { $daemon = [ 'token' => 'some_token', + 'status' => CronHelper::DAEMON_STATUS_ACTIVE, 'updated_at' => 12345678, 'run_accessed_at' => $test_input['run_access'], 'run_started_at' => $test_input['run_start'], @@ -196,6 +204,26 @@ class CronHelperTest extends \MailPoetTest { } } + function testItDeactivatesDaemon() { + $daemon = [ + 'token' => 'some_token', + 'status' => CronHelper::DAEMON_STATUS_ACTIVE, + 'updated_at' => '12345678', + 'run_accessed_at' => null, + 'run_started_at' => null, + 'run_completed_at' => null, + 'last_error' => null, + ]; + Setting::setValue( + CronHelper::DAEMON_SETTING, + $daemon + ); + + CronHelper::deactivateDaemon($daemon); + $daemon = CronHelper::getDaemon(); + expect($daemon['status'])->equals(CronHelper::DAEMON_STATUS_INACTIVE); + } + function testItCreatesRandomToken() { // random token is a string of 5 characters $token1 = CronHelper::createToken(); diff --git a/tests/unit/Cron/DaemonTest.php b/tests/unit/Cron/DaemonTest.php index e48aa4c439..e64f43f004 100644 --- a/tests/unit/Cron/DaemonTest.php +++ b/tests/unit/Cron/DaemonTest.php @@ -157,6 +157,22 @@ class DaemonTest extends \MailPoetTest { $daemon->run(); } + function testItTerminatesExecutionWhenDaemonIsDeactivated() { + $daemon = Stub::make(new Daemon(true), [ + 'executeScheduleWorker' => null, + 'executeQueueWorker' => null, + 'pauseExecution' => null, + 'terminateRequest' => Expected::exactly(1) + ], $this); + $data = [ + 'token' => 123, + 'status' => CronHelper::DAEMON_STATUS_INACTIVE, + ]; + Setting::setValue(CronHelper::DAEMON_SETTING, $data); + $daemon->__construct($data); + $daemon->run(); + } + function testItUpdatesDaemonTokenDuringExecution() { $daemon = Stub::make(new Daemon(true), array( 'executeScheduleWorker' => null, diff --git a/tests/unit/Cron/SupervisorTest.php b/tests/unit/Cron/SupervisorTest.php index 47c74f5999..23bd8f6e5d 100644 --- a/tests/unit/Cron/SupervisorTest.php +++ b/tests/unit/Cron/SupervisorTest.php @@ -50,6 +50,16 @@ class SupervisorTest extends \MailPoetTest { $daemon = $supervisor->checkDaemon(); expect(is_int($daemon['updated_at']))->true(); expect($daemon['updated_at'])->notEquals($supervisor->daemon['updated_at']); + expect($daemon['status'])->equals(CronHelper::DAEMON_STATUS_ACTIVE); + } + + function testRestartsDaemonWhenItIsInactive() { + if(getenv('WP_TEST_ENABLE_NETWORK_TESTS') !== 'true') return; + $supervisor = new Supervisor(); + $supervisor->daemon['updated_at'] = time(); + $supervisor->daemon['status'] = CronHelper::DAEMON_STATUS_INACTIVE; + $daemon = $supervisor->checkDaemon(); + expect($daemon['status'])->equals(CronHelper::DAEMON_STATUS_ACTIVE); } function _after() { diff --git a/tests/unit/Cron/Triggers/WordPressTest.php b/tests/unit/Cron/Triggers/WordPressTest.php index d6330ad8c7..cdab7f486b 100644 --- a/tests/unit/Cron/Triggers/WordPressTest.php +++ b/tests/unit/Cron/Triggers/WordPressTest.php @@ -77,11 +77,11 @@ class WordPressTest extends \MailPoetTest { expect(WordPress::checkExecutionRequirements())->false(); } - function testItCanDeleteRunningDaemon() { - Setting::setValue(CronHelper::DAEMON_SETTING, true); - expect(Setting::getValue(CronHelper::DAEMON_SETTING))->notNull(); - WordPress::cleanup(); - expect(Setting::getValue(CronHelper::DAEMON_SETTING))->null(); + function testItCanDeactivateRunningDaemon() { + Setting::setValue(CronHelper::DAEMON_SETTING, ['status' => CronHelper::DAEMON_STATUS_ACTIVE]); + expect(Setting::getValue(CronHelper::DAEMON_SETTING)['status'])->equals(CronHelper::DAEMON_STATUS_ACTIVE); + WordPress::stop(); + expect(Setting::getValue(CronHelper::DAEMON_SETTING)['status'])->equals(CronHelper::DAEMON_STATUS_INACTIVE); } function testItRunsWhenExecutionRequirementsAreMet() { @@ -93,11 +93,11 @@ class WordPressTest extends \MailPoetTest { expect(Setting::getValue(CronHelper::DAEMON_SETTING))->notNull(); } - function testItDeletesCronDaemonWhenExecutionRequirementsAreNotMet() { - Setting::setValue(CronHelper::DAEMON_SETTING, true); - expect(Setting::getValue(CronHelper::DAEMON_SETTING))->notNull(); + function testItDeactivatesCronDaemonWhenExecutionRequirementsAreNotMet() { + Setting::setValue(CronHelper::DAEMON_SETTING, ['status' => CronHelper::DAEMON_STATUS_ACTIVE]); + expect(Setting::getValue(CronHelper::DAEMON_SETTING)['status'])->equals(CronHelper::DAEMON_STATUS_ACTIVE); WordPress::run(); - expect(Setting::getValue(CronHelper::DAEMON_SETTING))->null(); + expect(Setting::getValue(CronHelper::DAEMON_SETTING)['status'])->equals(CronHelper::DAEMON_STATUS_INACTIVE); } function _addMTAConfigAndLog($sent, $status = null) { From 5f6fe1a7e54e7b882ccd86199b3ccf7511cf2f9e Mon Sep 17 00:00:00 2001 From: Rostislav Wolny Date: Thu, 19 Jul 2018 16:50:22 +0200 Subject: [PATCH 2/3] Add cron status to admin help section [MAILPOET-1457] --- assets/js/src/common/print_boolean.jsx | 26 ++++++++++ assets/js/src/help/cron_status.jsx | 71 ++++++++++++++++++++++++++ assets/js/src/help/system_status.jsx | 2 + lib/Config/Menu.php | 16 +++--- views/help.html | 12 +++++ 5 files changed, 120 insertions(+), 7 deletions(-) create mode 100644 assets/js/src/common/print_boolean.jsx create mode 100644 assets/js/src/help/cron_status.jsx diff --git a/assets/js/src/common/print_boolean.jsx b/assets/js/src/common/print_boolean.jsx new file mode 100644 index 0000000000..fd4956b77a --- /dev/null +++ b/assets/js/src/common/print_boolean.jsx @@ -0,0 +1,26 @@ +import React from 'react'; +import MailPoet from 'mailpoet'; + +const PrintBoolean = props => ( + + {(props.children === true && props.truthy) || + (props.children === false && props.falsy) || + (props.unknown)} + +); + +PrintBoolean.propTypes = { + truthy: React.PropTypes.string, + falsy: React.PropTypes.string, + unknown: React.PropTypes.string, + children: React.PropTypes.bool, +}; + +PrintBoolean.defaultProps = { + truthy: MailPoet.I18n.t('yes'), + falsy: MailPoet.I18n.t('no'), + unknown: MailPoet.I18n.t('unknown'), + children: null, +}; + +module.exports = PrintBoolean; diff --git a/assets/js/src/help/cron_status.jsx b/assets/js/src/help/cron_status.jsx new file mode 100644 index 0000000000..80b268ab74 --- /dev/null +++ b/assets/js/src/help/cron_status.jsx @@ -0,0 +1,71 @@ +import MailPoet from 'mailpoet'; +import React from 'react'; +import PrintBoolean from 'common/print_boolean.jsx'; + +function renderStatusTableRow(title, value) { + return ( + + { title }{ value } + + ); +} + +const CronStatus = (props) => { + const status = props.status_data; + const activeStatusMapping = { + active: MailPoet.I18n.t('cronRunning'), + inactive: MailPoet.I18n.t('cronWaiting'), + }; + return ( +
+

{MailPoet.I18n.t('systemStatusCronStatusTitle')}

+ + + {renderStatusTableRow( + MailPoet.I18n.t('accessible'), + {status.accessible}) + } + {renderStatusTableRow( + MailPoet.I18n.t('status'), + activeStatusMapping[status.status] ? activeStatusMapping[status.status] : MailPoet.I18n.t('unknown')) + } + {renderStatusTableRow( + MailPoet.I18n.t('lastUpdated'), + status.updated_at ? MailPoet.Date.full(status.updated_at * 1000) : MailPoet.I18n.t('unknown')) + } + {renderStatusTableRow( + MailPoet.I18n.t('lastRunStarted'), + status.run_accessed_at ? MailPoet.Date.full(status.run_started_at * 1000) : MailPoet.I18n.t('unknown')) + } + {renderStatusTableRow( + MailPoet.I18n.t('lastRunCompleted'), + status.run_completed_at ? MailPoet.Date.full(status.run_completed_at * 1000) : MailPoet.I18n.t('unknown')) + } + {renderStatusTableRow(MailPoet.I18n.t('lastSeenError'), status.last_error || '-')} + +
+
+ ); +}; + +CronStatus.propTypes = { + status_data: React.PropTypes.shape({ + accessible: React.PropTypes.bool, + status: React.PropTypes.string, + updated_at: React.PropTypes.number, + run_accessed_at: React.PropTypes.number, + run_completed_at: React.PropTypes.number, + }).isRequired, +}; + +CronStatus.defaultProps = { + status_data: { + accessible: null, + status: null, + updated_at: null, + run_accessed_at: null, + run_completed_at: null, + }, +}; + +module.exports = CronStatus; diff --git a/assets/js/src/help/system_status.jsx b/assets/js/src/help/system_status.jsx index f4750aba56..7eb7f7b8cc 100644 --- a/assets/js/src/help/system_status.jsx +++ b/assets/js/src/help/system_status.jsx @@ -1,6 +1,7 @@ import MailPoet from 'mailpoet'; import React from 'react'; import ReactStringReplace from 'react-string-replace'; +import CronStatus from './cron_status.jsx'; import Tabs from './tabs.jsx'; function renderStatusMessage(status, error, link) { @@ -65,6 +66,7 @@ function SystemStatus() { {renderCronSection(systemStatusData)} {renderMSSSection(systemStatusData)} + ); } diff --git a/lib/Config/Menu.php b/lib/Config/Menu.php index fd2afa4b25..97f168d3f7 100644 --- a/lib/Config/Menu.php +++ b/lib/Config/Menu.php @@ -448,17 +448,19 @@ class Menu { function help() { $system_info_data = Beacon::getData(); - $system_status_data = array( - 'cron' => array( + $system_status_data = [ + 'cron' => [ 'url' => CronHelper::getCronUrl(CronDaemon::ACTION_PING), 'isReachable' => CronHelper::pingDaemon(true) - ), - 'mss' => array( + ], + 'mss' => [ 'enabled' => (Bridge::isMPSendingServiceEnabled()) ? - array('isReachable' => Bridge::pingBridge()) : + ['isReachable' => Bridge::pingBridge()] : false - ) - ); + ], + 'cronStatus' => CronHelper::getDaemon(), + ]; + $system_status_data['cronStatus']['accessible'] = CronHelper::isDaemonAccessible(); $this->displayPage( 'help.html', array( diff --git a/views/help.html b/views/help.html index 9f509a3e23..1d9c7b9f2f 100644 --- a/views/help.html +++ b/views/help.html @@ -33,6 +33,18 @@ 'knowledgeBaseButton': __('Visit our Knowledge Base for more articles'), 'systemInfoIntro': __('The information below is useful when you need to get in touch with our support. Just copy all the text below and paste it into a message to us.'), 'systemInfoDataError': __('Sorry, there was an error, please try again later.'), + 'systemStatusCronStatusTitle': __('Cron'), + 'lastUpdated': __('Last updated'), + 'lastRunStarted': __('Last run started'), + 'lastRunCompleted': __('Last run completed'), + 'lastSeenError': __('Last seen error'), + 'unknown': __('unknown'), + 'accessible': __('Accessible'), + 'status': __('Status'), + 'yes': __('yes'), + 'no': __('no'), + 'cronRunning': __('running'), + 'cronWaiting': __('waiting for the next run'), }) %> <% endblock %> From a8bfadf15787ea01767bcfb3a2172e0d073f6f87 Mon Sep 17 00:00:00 2001 From: Rostislav Wolny Date: Mon, 23 Jul 2018 16:26:54 +0200 Subject: [PATCH 3/3] Swap tabs in help section We decided to make the Knowledge Base tab the default one since it contains the information which are easiest to understand for an user. [MAILPOET-1457] --- assets/js/src/help/help.jsx | 4 ++-- assets/js/src/help/tabs.jsx | 12 ++++++------ 2 files changed, 8 insertions(+), 8 deletions(-) diff --git a/assets/js/src/help/help.jsx b/assets/js/src/help/help.jsx index b4a16411a1..9dba90fc45 100644 --- a/assets/js/src/help/help.jsx +++ b/assets/js/src/help/help.jsx @@ -21,11 +21,11 @@ if (container) { ReactDOM.render(( - + {/* Pages */} + - ), container); diff --git a/assets/js/src/help/tabs.jsx b/assets/js/src/help/tabs.jsx index 2db98f386e..6d258b6817 100644 --- a/assets/js/src/help/tabs.jsx +++ b/assets/js/src/help/tabs.jsx @@ -4,6 +4,11 @@ import classNames from 'classnames'; import MailPoet from 'mailpoet'; const tabs = [ + { + name: 'knowledgeBase', + label: MailPoet.I18n.t('tabKnowledgeBaseTitle'), + link: '/knowledgeBase', + }, { name: 'systemStatus', label: MailPoet.I18n.t('tabSystemStatusTitle'), @@ -14,11 +19,6 @@ const tabs = [ label: MailPoet.I18n.t('tabSystemInfoTitle'), link: '/systemInfo', }, - { - name: 'knowledgeBase', - label: MailPoet.I18n.t('tabKnowledgeBaseTitle'), - link: '/knowledgeBase', - }, ]; function Tabs(props) { @@ -45,6 +45,6 @@ function Tabs(props) { } Tabs.propTypes = { tab: React.PropTypes.string }; -Tabs.defaultProps = { tab: 'systemStatus' }; +Tabs.defaultProps = { tab: 'knowledgeBase' }; module.exports = Tabs;