From cbe47a9dccb4c2dc6d1707dafa6d7566550ac2a4 Mon Sep 17 00:00:00 2001 From: James Cole Date: Sat, 7 Jul 2018 07:48:10 +0200 Subject: [PATCH] Code clean up in Handlers. --- .../Chart/Basic/ChartJsGenerator.php | 2 +- .../Report/Audit/MonthReportGenerator.php | 6 +- app/Handlers/Events/APIEventHandler.php | 36 +++--- app/Handlers/Events/AdminEventHandler.php | 35 +++--- app/Handlers/Events/AutomationHandler.php | 29 ++--- .../Events/StoredJournalEventHandler.php | 26 ++--- .../Events/UpdatedJournalEventHandler.php | 4 +- app/Handlers/Events/UserEventHandler.php | 76 ++++++------ .../Events/VersionCheckEventHandler.php | 110 ++++++++++++------ .../Controllers/Admin/UpdateController.php | 4 + app/Models/Role.php | 3 + app/Services/Github/Request/UpdateRequest.php | 2 +- 12 files changed, 184 insertions(+), 149 deletions(-) diff --git a/app/Generator/Chart/Basic/ChartJsGenerator.php b/app/Generator/Chart/Basic/ChartJsGenerator.php index ef66a2b7cf..a0d4cea278 100644 --- a/app/Generator/Chart/Basic/ChartJsGenerator.php +++ b/app/Generator/Chart/Basic/ChartJsGenerator.php @@ -90,7 +90,7 @@ class ChartJsGenerator implements GeneratorInterface if (isset($set['currency_symbol'])) { $currentSet['currency_symbol'] = $set['currency_symbol']; } - if(isset($set['backgroundColor'])) { + if (isset($set['backgroundColor'])) { $currentSet['backgroundColor'] = $set['backgroundColor']; } $chartData['datasets'][] = $currentSet; diff --git a/app/Generator/Report/Audit/MonthReportGenerator.php b/app/Generator/Report/Audit/MonthReportGenerator.php index c8029453e0..8f8c2be08d 100644 --- a/app/Generator/Report/Audit/MonthReportGenerator.php +++ b/app/Generator/Report/Audit/MonthReportGenerator.php @@ -216,9 +216,9 @@ class MonthReportGenerator implements ReportGeneratorInterface $transactionAmount = $transaction->transaction_foreign_amount; } - $newBalance = bcadd($startBalance, $transactionAmount); - $transaction->after = $newBalance; - $startBalance = $newBalance; + $newBalance = bcadd($startBalance, $transactionAmount); + $transaction->after = $newBalance; + $startBalance = $newBalance; } $return = [ diff --git a/app/Handlers/Events/APIEventHandler.php b/app/Handlers/Events/APIEventHandler.php index e73b151e86..5be569d2a2 100644 --- a/app/Handlers/Events/APIEventHandler.php +++ b/app/Handlers/Events/APIEventHandler.php @@ -39,6 +39,8 @@ use Session; class APIEventHandler { /** + * Respond to the creation of an access token. + * * @param AccessTokenCreated $event * * @return bool @@ -48,28 +50,24 @@ class APIEventHandler /** @var UserRepositoryInterface $repository */ $repository = app(UserRepositoryInterface::class); $user = $repository->findNull((int)$event->userId); - if (null === $user) { - Log::error('Access Token generated but no user associated.'); + if (null !== $user) { + $email = $user->email; + $ipAddress = Request::ip(); - return true; + Log::debug(sprintf('Now in APIEventHandler::accessTokenCreated. Email is %s, IP is %s', $email, $ipAddress)); + try { + Log::debug('Trying to send message...'); + Mail::to($email)->send(new AccessTokenCreatedMail($email, $ipAddress)); + // @codeCoverageIgnoreStart + } catch (Exception $e) { + Log::debug('Send message failed! :('); + Log::error($e->getMessage()); + Log::error($e->getTraceAsString()); + Session::flash('error', 'Possible email error: ' . $e->getMessage()); + } + Log::debug('If no error above this line, message was sent.'); } - $email = $user->email; - $ipAddress = Request::ip(); - - Log::debug(sprintf('Now in APIEventHandler::accessTokenCreated. Email is %s, IP is %s', $email, $ipAddress)); - try { - Log::debug('Trying to send message...'); - Mail::to($email)->send(new AccessTokenCreatedMail($email, $ipAddress)); - // @codeCoverageIgnoreStart - } catch (Exception $e) { - Log::debug('Send message failed! :('); - Log::error($e->getMessage()); - Log::error($e->getTraceAsString()); - Session::flash('error', 'Possible email error: ' . $e->getMessage()); - } - Log::debug('If no error above this line, message was sent.'); - // @codeCoverageIgnoreEnd return true; diff --git a/app/Handlers/Events/AdminEventHandler.php b/app/Handlers/Events/AdminEventHandler.php index 83225c7e53..1bc55af2ba 100644 --- a/app/Handlers/Events/AdminEventHandler.php +++ b/app/Handlers/Events/AdminEventHandler.php @@ -48,27 +48,24 @@ class AdminEventHandler $repository = app(UserRepositoryInterface::class); // is user even admin? - if (!$repository->hasRole($event->user, 'owner')) { - return true; + if ($repository->hasRole($event->user, 'owner')) { + $email = $event->user->email; + $ipAddress = $event->ipAddress; + + Log::debug(sprintf('Now in sendTestMessage event handler. Email is %s, IP is %s', $email, $ipAddress)); + try { + Log::debug('Trying to send message...'); + Mail::to($email)->send(new AdminTestMail($email, $ipAddress)); + // @codeCoverageIgnoreStart + } catch (Exception $e) { + Log::debug('Send message failed! :('); + Log::error($e->getMessage()); + Log::error($e->getTraceAsString()); + Session::flash('error', 'Possible email error: ' . $e->getMessage()); + } + Log::debug('If no error above this line, message was sent.'); } - - $email = $event->user->email; - $ipAddress = $event->ipAddress; - - Log::debug(sprintf('Now in sendTestMessage event handler. Email is %s, IP is %s', $email, $ipAddress)); - try { - Log::debug('Trying to send message...'); - Mail::to($email)->send(new AdminTestMail($email, $ipAddress)); - // @codeCoverageIgnoreStart - } catch (Exception $e) { - Log::debug('Send message failed! :('); - Log::error($e->getMessage()); - Log::error($e->getTraceAsString()); - Session::flash('error', 'Possible email error: ' . $e->getMessage()); - } - Log::debug('If no error above this line, message was sent.'); - // @codeCoverageIgnoreEnd return true; } diff --git a/app/Handlers/Events/AutomationHandler.php b/app/Handlers/Events/AutomationHandler.php index 3585e0ccf2..bc694b4108 100644 --- a/app/Handlers/Events/AutomationHandler.php +++ b/app/Handlers/Events/AutomationHandler.php @@ -37,6 +37,8 @@ class AutomationHandler { /** + * Respond to the creation of X journals. + * * @param RequestedReportOnJournals $event * * @return bool @@ -47,25 +49,16 @@ class AutomationHandler /** @var UserRepositoryInterface $repository */ $repository = app(UserRepositoryInterface::class); $user = $repository->findNull($event->userId); - if (null === $user) { - Log::debug('User is NULL'); - - return true; + if (null !== $user && 0 !== $event->journals->count()) { + try { + Log::debug('Trying to mail...'); + Mail::to($user->email)->send(new ReportNewJournalsMail($user->email, '127.0.0.1', $event->journals)); + // @codeCoverageIgnoreStart + } catch (Exception $e) { + Log::error($e->getMessage()); + } + Log::debug('Done!'); } - if ($event->journals->count() === 0) { - Log::debug('No journals.'); - - return true; - } - - try { - Log::debug('Trying to mail...'); - Mail::to($user->email)->send(new ReportNewJournalsMail($user->email, '127.0.0.1', $event->journals)); - // @codeCoverageIgnoreStart - } catch (Exception $e) { - Log::error($e->getMessage()); - } - Log::debug('Done!'); // @codeCoverageIgnoreEnd return true; diff --git a/app/Handlers/Events/StoredJournalEventHandler.php b/app/Handlers/Events/StoredJournalEventHandler.php index 43ca29745b..db3fcb41f2 100644 --- a/app/Handlers/Events/StoredJournalEventHandler.php +++ b/app/Handlers/Events/StoredJournalEventHandler.php @@ -25,35 +25,33 @@ namespace FireflyIII\Handlers\Events; use FireflyIII\Events\StoredTransactionJournal; use FireflyIII\Models\Rule; use FireflyIII\Models\RuleGroup; -use FireflyIII\Repositories\Journal\JournalRepositoryInterface as JRI; -use FireflyIII\Repositories\PiggyBank\PiggyBankRepositoryInterface as PRI; -use FireflyIII\Repositories\RuleGroup\RuleGroupRepositoryInterface as RGRI; +use FireflyIII\Repositories\Journal\JournalRepositoryInterface; +use FireflyIII\Repositories\PiggyBank\PiggyBankRepositoryInterface; +use FireflyIII\Repositories\RuleGroup\RuleGroupRepositoryInterface; use FireflyIII\TransactionRules\Processor; /** - * @codeCoverageIgnore - * * Class StoredJournalEventHandler */ class StoredJournalEventHandler { - /** @var JRI */ + /** @var JournalRepositoryInterface The journal repository. */ public $journalRepository; - /** @var PRI */ + /** @var PiggyBankRepositoryInterface The Piggy bank repository */ public $repository; - - /** @var RGRI */ + /** @var RuleGroupRepositoryInterface The rule group repository */ public $ruleGroupRepository; /** * StoredJournalEventHandler constructor. * - * @param PRI $repository - * @param JRI $journalRepository - * @param RGRI $ruleGroupRepository + * @param PiggyBankRepositoryInterface $repository + * @param JournalRepositoryInterface $journalRepository + * @param RuleGroupRepositoryInterface $ruleGroupRepository */ - public function __construct(PRI $repository, JRI $journalRepository, RGRI $ruleGroupRepository) - { + public function __construct( + PiggyBankRepositoryInterface $repository, JournalRepositoryInterface $journalRepository, RuleGroupRepositoryInterface $ruleGroupRepository + ) { $this->repository = $repository; $this->journalRepository = $journalRepository; $this->ruleGroupRepository = $ruleGroupRepository; diff --git a/app/Handlers/Events/UpdatedJournalEventHandler.php b/app/Handlers/Events/UpdatedJournalEventHandler.php index ac06e95908..f6308e08e3 100644 --- a/app/Handlers/Events/UpdatedJournalEventHandler.php +++ b/app/Handlers/Events/UpdatedJournalEventHandler.php @@ -29,13 +29,11 @@ use FireflyIII\Repositories\RuleGroup\RuleGroupRepositoryInterface; use FireflyIII\TransactionRules\Processor; /** - * @codeCoverageIgnore - * * Class UpdatedJournalEventHandler */ class UpdatedJournalEventHandler { - /** @var RuleGroupRepositoryInterface */ + /** @var RuleGroupRepositoryInterface The rule group repository */ public $repository; /** diff --git a/app/Handlers/Events/UserEventHandler.php b/app/Handlers/Events/UserEventHandler.php index 64ef4a83fc..530562ab60 100644 --- a/app/Handlers/Events/UserEventHandler.php +++ b/app/Handlers/Events/UserEventHandler.php @@ -18,6 +18,7 @@ * You should have received a copy of the GNU General Public License * along with Firefly III. If not, see . */ +/** @noinspection NullPointerExceptionInspection */ declare(strict_types=1); namespace FireflyIII\Handlers\Events; @@ -26,7 +27,6 @@ use Exception; use FireflyIII\Events\RegisteredUser; use FireflyIII\Events\RequestedNewPassword; use FireflyIII\Events\UserChangedEmail; -use FireflyIII\Factories\RoleFactory; use FireflyIII\Mail\ConfirmEmailChangeMail; use FireflyIII\Mail\RegisteredUser as RegisteredUserMail; use FireflyIII\Mail\RequestedNewPassword as RequestedNewPasswordMail; @@ -44,6 +44,7 @@ use Preferences; * This class responds to any events that have anything to do with the User object. * * The method name reflects what is being done. This is in the present tense. + * @SuppressWarnings(PHPMD.CouplingBetweenObjects) */ class UserEventHandler { @@ -68,6 +69,8 @@ class UserEventHandler } /** + * Fires to see if a user is admin. + * * @param Login $event * * @return bool @@ -81,34 +84,27 @@ class UserEventHandler $user = $event->user; $count = $repository->count(); - if ($count > 1) { - // if more than one user, do nothing. - Log::debug(sprintf('System has %d users, will not change users roles.', $count)); + // only act when there is 1 user in the system and he has no admin rights. + if (1 === $count && !$repository->hasRole($user, 'owner')) { + // user is the only user but does not have role "owner". + $role = $repository->getRole('owner'); + if (null === $role) { + // create role, does not exist. Very strange situation so let's raise a big fuss about it. + $role = $repository->createRole('owner', 'Site Owner', 'User runs this instance of FF3'); + Log::error('Could not find role "owner". This is weird.'); + } - return true; + Log::info(sprintf('Gave user #%d role #%d ("%s")', $user->id, $role->id, $role->name)); + // give user the role + $repository->attachRole($user, 'owner'); } - // user is only user but has admin role - if (1 === $count && $user->hasRole('owner')) { - Log::debug(sprintf('User #%d is only user but has role owner so all is well.', $user->id)); - - return true; - } - // user is the only user but does not have role "owner". - $role = $repository->getRole('owner'); - if (null === $role) { - // create role, does not exist. Very strange situation so let's raise a big fuss about it. - $role = $repository->createRole('owner', 'Site Owner', 'User runs this instance of FF3'); - Log::error('Could not find role "owner". This is weird.'); - } - - Log::info(sprintf('Gave user #%d role #%d ("%s")', $user->id, $role->id, $role->name)); - // give user the role - $repository->attachRole($user, 'owner'); return true; } /** + * Set the demo user back to English. + * * @param Login $event * * @return bool @@ -130,6 +126,8 @@ class UserEventHandler } /** + * Send email to confirm email change. + * * @param UserChangedEmail $event * * @return bool @@ -154,6 +152,8 @@ class UserEventHandler } /** + * Send email to be able to undo email change. + * * @param UserChangedEmail $event * * @return bool @@ -178,6 +178,8 @@ class UserEventHandler } /** + * Send a new password to the user. + * * @param RequestedNewPassword $event * * @return bool @@ -211,27 +213,25 @@ class UserEventHandler * * @return bool */ - public function sendRegistrationMail(RegisteredUser $event) + public function sendRegistrationMail(RegisteredUser $event): bool { $sendMail = env('SEND_REGISTRATION_MAIL', true); - if (!$sendMail) { - return true; // @codeCoverageIgnore - } - // get the email address - $email = $event->user->email; - $uri = route('index'); - $ipAddress = $event->ipAddress; + if ($sendMail) { + // get the email address + $email = $event->user->email; + $uri = route('index'); + $ipAddress = $event->ipAddress; - // send email. - try { - Mail::to($email)->send(new RegisteredUserMail($uri, $ipAddress)); - // @codeCoverageIgnoreStart - } catch (Exception $e) { - Log::error($e->getMessage()); + // send email. + try { + Mail::to($email)->send(new RegisteredUserMail($uri, $ipAddress)); + // @codeCoverageIgnoreStart + } catch (Exception $e) { + Log::error($e->getMessage()); + } + // @codeCoverageIgnoreEnd } - // @codeCoverageIgnoreEnd - return true; } } diff --git a/app/Handlers/Events/VersionCheckEventHandler.php b/app/Handlers/Events/VersionCheckEventHandler.php index 2b7b9a1f0f..b7276a7823 100644 --- a/app/Handlers/Events/VersionCheckEventHandler.php +++ b/app/Handlers/Events/VersionCheckEventHandler.php @@ -18,11 +18,13 @@ * You should have received a copy of the GNU General Public License * along with Firefly III. If not, see . */ - +/** @noinspection MultipleReturnStatementsInspection */ +/** @noinspection NullPointerExceptionInspection */ declare(strict_types=1); namespace FireflyIII\Handlers\Events; +use Carbon\Carbon; use FireflyConfig; use FireflyIII\Events\RequestedVersionCheckStatus; use FireflyIII\Exceptions\FireflyException; @@ -39,19 +41,23 @@ class VersionCheckEventHandler { /** + * Checks with GitHub to see if there is a new version. + * + * @SuppressWarnings(PHPMD.CyclomaticComplexity) + * @SuppressWarnings(PHPMD.NPathComplexity) + * @SuppressWarnings(PHPMD.ExcessiveMethodLength) * @param RequestedVersionCheckStatus $event */ public function checkForUpdates(RequestedVersionCheckStatus $event): void { // in Sandstorm, cannot check for updates: $sandstorm = 1 === (int)getenv('SANDSTORM'); - if ($sandstorm === true) { + if (true === $sandstorm) { return; // @codeCoverageIgnore } /** @var UserRepositoryInterface $repository */ $repository = app(UserRepositoryInterface::class); - /** @var User $user */ $user = $event->user; if (!$repository->hasRole($user, 'owner')) { @@ -80,43 +86,81 @@ class VersionCheckEventHandler return; } - $current = config('firefly.version'); - /** @var UpdateRequest $request */ - $request = app(UpdateRequest::class); - $check = -2; - $first = new Release(['id' => '0', 'title' => '0.2', 'updated' => '2017-01-01', 'content' => '']); - try { - $request->call(); - $releases = $request->getReleases(); - // first entry should be the latest entry: - /** @var Release $first */ - $first = reset($releases); - $check = version_compare($current, $first->getTitle()); - Log::debug(sprintf('Comparing %s with %s, result is %s', $current, $first->getTitle(), $check)); - FireflyConfig::set('last_update_check', time()); - } catch (FireflyException $e) { - Log::error(sprintf('Could not check for updates: %s', $e->getMessage())); - } - $string = 'no result: ' . $check; - if ($check === -2) { + $current = config('firefly.version'); + $latestRelease = $this->getLatestRelease(); + $versionCheck = $this->versionCheck($latestRelease); + $string = ''; + if ($versionCheck === -2) { $string = (string)trans('firefly.update_check_error'); } - if ($check === -1) { + if ($versionCheck === -1 && null !== $latestRelease) { // there is a new FF version! - $monthAndDayFormat = (string)trans('config.month_and_day'); - $string = (string)trans( - 'firefly.update_new_version_alert', - [ - 'your_version' => $current, - 'new_version' => $first->getTitle(), - 'date' => $first->getUpdated()->formatLocalized($monthAndDayFormat), - ] - ); + // has it been released for at least three days? + $today = new Carbon; + if ($today->diffInDays($latestRelease->getUpdated(), true) > 3) { + $monthAndDayFormat = (string)trans('config.month_and_day'); + $string = (string)trans( + 'firefly.update_new_version_alert', + [ + 'your_version' => $current, + 'new_version' => $latestRelease->getTitle(), + 'date' => $latestRelease->getUpdated()->formatLocalized($monthAndDayFormat), + ] + ); + } } - if ($check !== 0) { + if (0 !== $versionCheck && '' !== $string) { // flash info session()->flash('info', $string); } + FireflyConfig::set('last_update_check', time()); + } + + /** + * Get object for the latest release from GitHub. + * + * @return Release|null + */ + private function getLatestRelease(): ?Release + { + $return = null; + /** @var UpdateRequest $request */ + $request = app(UpdateRequest::class); + try { + $request->call(); + } catch (FireflyException $e) { + Log::error(sprintf('Could not check for updates: %s', $e->getMessage())); + } + + // get releases from array. + $releases = $request->getReleases(); + if (\count($releases) > 0) { + // first entry should be the latest entry: + /** @var Release $first */ + $first = reset($releases); + $return = $first; + } + + return $return; + } + + /** + * Compare version and store result. + * + * @param Release|null $release + * + * @return int + */ + private function versionCheck(Release $release = null): int + { + if (null === $release) { + return -2; + } + $current = (string)config('firefly.version'); + $check = version_compare($current, $release->getTitle()); + Log::debug(sprintf('Comparing %s with %s, result is %s', $current, $release->getTitle(), $check)); + + return $check; } } diff --git a/app/Http/Controllers/Admin/UpdateController.php b/app/Http/Controllers/Admin/UpdateController.php index 8a6b0b5494..3b4d442bde 100644 --- a/app/Http/Controllers/Admin/UpdateController.php +++ b/app/Http/Controllers/Admin/UpdateController.php @@ -133,6 +133,10 @@ class UpdateController extends Controller ] ); } + if ($today->diffInDays($first->getUpdated(), true) <= 3) { + // tell user their running the current version. + $string = (string)trans('firefly.update_current_version_alert', ['version' => $current]); + } } if ($check === 0) { // you are running the current version! diff --git a/app/Models/Role.php b/app/Models/Role.php index ee735d4605..c262631e60 100644 --- a/app/Models/Role.php +++ b/app/Models/Role.php @@ -28,6 +28,9 @@ use Illuminate\Database\Eloquent\Relations\BelongsToMany; /** * Class Role. + * + * @property int $id + * @property string $name */ class Role extends Model { diff --git a/app/Services/Github/Request/UpdateRequest.php b/app/Services/Github/Request/UpdateRequest.php index fd2d173049..d208e8c06c 100644 --- a/app/Services/Github/Request/UpdateRequest.php +++ b/app/Services/Github/Request/UpdateRequest.php @@ -54,7 +54,7 @@ class UpdateRequest implements GithubRequest throw new FireflyException(sprintf('Response error from Github: %s', $e->getMessage())); } - if ($res->getStatusCode() !== 200) { + if (200 !== $res->getStatusCode()) { throw new FireflyException(sprintf('Returned code %d, error: %s', $res->getStatusCode(), $res->getBody()->getContents())); }