EBernhardson has submitted this change and it was merged. Change subject: Refactor repositories out of QueriesController ......................................................................
Refactor repositories out of QueriesController This class had quite a few repositories, and i was about to add one more. In a (perhaps misguided) attempt at the single responsibility principle pull all the repositories out of QueriesController into a class with less responsibilities, QueriesManager. Now QueriesController can handle the forms/templating/etc end of things, and QueriesManager can handle shuffling data back and forth from the database. Change-Id: Ia5fb71aedf3d13f5c9376394555d6ae9a2bf0ddc --- M src/RelevanceScoring/Controller/QueriesController.php A src/RelevanceScoring/QueriesManager.php M src/RelevanceScoring/RelevanceScoringProvider.php 3 files changed, 156 insertions(+), 87 deletions(-) Approvals: EBernhardson: Verified; Looks good to me, approved diff --git a/src/RelevanceScoring/Controller/QueriesController.php b/src/RelevanceScoring/Controller/QueriesController.php index fd2b822..c59fa92 100644 --- a/src/RelevanceScoring/Controller/QueriesController.php +++ b/src/RelevanceScoring/Controller/QueriesController.php @@ -8,11 +8,7 @@ use WikiMedia\OAuth\User; use WikiMedia\RelevanceScoring\Application; use WikiMedia\RelevanceScoring\Assert\MinimumSubmitted; -use WikiMedia\RelevanceScoring\Repository\QueriesRepository; -use WikiMedia\RelevanceScoring\Repository\ResultsRepository; -use WikiMedia\RelevanceScoring\Repository\ScoresRepository; -use WikiMedia\RelevanceScoring\Repository\ScoringQueueRepository; -use WikiMedia\RelevanceScoring\Repository\UsersRepository; +use WikiMedia\RelevanceScoring\QueriesManager; class QueriesController { @@ -24,14 +20,8 @@ private $twig; /** @var FormFactory */ private $formFactory; - /** @var QueriesRepository */ - private $queriesRepo; - /** @var ResultsRepository */ - private $resultsRepo; - /** @var ScoresRepository */ - private $scoresRepo; - /** @var UsersRepository */ - private $userRepo; + /** @var QueriesManager */ + private $queriesManager; /** @var string[] */ private $wikis; @@ -40,22 +30,14 @@ User $user, Twig_Environment $twig, FormFactory $formFactory, - QueriesRepository $queriesRepo, - ResultsRepository $resultsRepo, - ScoresRepository $scoresRepo, - ScoringQueueRepository $scoringQueueRepo, - UsersRepository $userRepo, + QueriesManager $queriesManager, array $wikis ) { $this->app = $app; $this->user = $user; $this->twig = $twig; $this->formFactory = $formFactory; - $this->queriesRepo = $queriesRepo; - $this->resultsRepo = $resultsRepo; - $this->scoresRepo = $scoresRepo; - $this->scoringQueueRepo = $scoringQueueRepo; - $this->userRepo = $userRepo; + $this->queriesManager = $queriesManager; $this->wikis = $wikis; } @@ -66,7 +48,7 @@ public function nextQuery(Request $request) { - $maybeId = $this->scoringQueueRepo->pop($this->user); + $maybeId = $this->queriesManager->nextQueryId(); $params = []; if ($request->query->get('saved')) { $params['saved'] = 1; @@ -82,7 +64,7 @@ public function skipQueryById(Request $request, $queryId) { - $maybeQuery = $this->queriesRepo->getQuery($queryId); + $maybeQuery = $this->queriesManager->getQuery($queryId); if ($maybeQuery->isEmpty()) { // @todo 404 throw new \Exception('Query not found'); @@ -95,8 +77,7 @@ // look into adding session based notifications to make it easier to // tell users about this. if ($form->isValid()) { - $this->queriesRepo->markQuerySkipped($this->user, $queryId); - $this->scoringQueueRepo->unassignUser($this->user); + $this->queriesManager->skipQuery($queryId); } return $this->app->redirect($this->app->path('next_query')); @@ -104,32 +85,27 @@ public function queryById(Request $request, $queryId) { - $maybeQuery = $this->queriesRepo->getQuery($queryId); + $maybeQuery = $this->queriesManager->getQuery($queryId); if ($maybeQuery->isEmpty()) { // @todo 404 throw new \Exception('Query not found'); } $query = $maybeQuery->get(); - if ( !$query['imported'] ) { + if (!$query['imported']) { return $this->twig->render('query_not_imported.twig', [ 'query' => $query, ]); } - $maybeResults = $this->resultsRepo->getQueryResults($queryId); + $maybeResults = $this->queriesManager->getQueryResults($queryId); if ($maybeResults->isEmpty()) { throw new \Exception('No results found for query'); } - $results = $this->shufflePreserveKeys( - $maybeResults->get(), - // user id is used to give each user a different order, - // but the same user gets same order each time. - $this->user->uid - ); // When encoded to json we will lose the ordering, so // add a key to identify the order $position = 0; + $results = $maybeResults->get(); foreach (array_keys($results) as $id) { $results[$id]['order'] = $position++; } @@ -138,8 +114,7 @@ $form->handleRequest($request); if ($form->isValid() && !$request->request->has('cards')) { - $this->scoresRepo->storeQueryScores($this->user, $queryId, $form->getData()); - $this->scoringQueueRepo->markScored($this->user, $queryId); + $this->queriesManager->saveScores($queryId, $form->getData()); return $this->app->redirect($this->app->path('next_query', ['saved' => 1])); } @@ -207,52 +182,10 @@ return $builder->getForm(); } - /** - * PHP's shuffle function loses the keys. So sort the keys - * and make a new array based on the order of sorted keys. - * Additionally php's shuffle is automatically seeded so we - * can't get the same order across requests. Fix that by using - * a local fisher yates implementation. - * - * @param array $array - * - * @return array - */ - private function shufflePreserveKeys(array $array, $seed) - { - $keys = $this->fisherYatesShuffle(array_keys($array), $seed); - $result = array(); - foreach ($keys as $key) { - $result[$key] = $array[$key]; - } - - return $result; - } - - /** - * @param array $array Must be numerically indexed starting - * from 0 with no gaps. - * @param int $seed - * - * @return array - */ - private function fisherYatesShuffle(array $array, $seed) - { - mt_srand($seed); - for ($i = count($array) - 1; $i > 0; --$i) { - $j = mt_rand(0, $i); - $tmp = $array[$i]; - $array[$i] = $array[$j]; - $array[$j] = $tmp; - } - - return $array; - } - private function chooseScoringTemplate(Request $request) { $fromQuery = $request->query->get('cards', null); - if ( $fromQuery === null ) { + if ($fromQuery === null) { $fromQuery = $request->request->get('cards', null); } @@ -263,7 +196,7 @@ : 'classic'; if ($interface !== $this->user->extra['scoringInterface']) { $this->user->extra['scoringInterface'] = $interface; - $this->userRepo->updateUser($this->user); + $this->queriesManager->updateUserStorage(); } } diff --git a/src/RelevanceScoring/QueriesManager.php b/src/RelevanceScoring/QueriesManager.php new file mode 100644 index 0000000..0ff747e --- /dev/null +++ b/src/RelevanceScoring/QueriesManager.php @@ -0,0 +1,129 @@ +<?php + +namespace WikiMedia\RelevanceScoring; + +use PlasmaConduit\option\Some; +use WikiMedia\OAuth\User; +use WikiMedia\RelevanceScoring\Repository\ResultsRepository; +use WikiMedia\RelevanceScoring\Repository\ScoresRepository; +use WikiMedia\RelevanceScoring\Repository\ScoringQueueRepository; +use WikiMedia\RelevanceScoring\Repository\QueriesRepository; +use WikiMedia\RelevanceScoring\Repository\UsersRepository; + +/** + * Handles the data read/write patterns used by the QueriesController. + */ +class QueriesManager +{ + /** @var User */ + private $user; + /** @var ResultsRepository */ + private $resultsRepository; + /** @var ScoresRepository */ + private $scoresRepo; + /** @var ScoringQueueRepository */ + private $scoringQueueRepo; + /** @var QueriesRepository */ + private $queriesRepo; + /** @var UsersRepository */ + private $usersRepo; + + public function __construct( + User $user, + QueriesRepository $queriesRepo, + ResultsRepository $resultsRepo, + ScoresRepository $scoresRepo, + ScoringQueueRepository $scoringQueueRepo, + UsersRepository $usersRepo + ) { + $this->user = $user; + $this->resultsRepo = $resultsRepo; + $this->scoresRepo = $scoresRepo; + $this->scoringQueueRepo = $scoringQueueRepo; + $this->queriesRepo = $queriesRepo; + $this->usersRepo = $usersRepo; + } + + public function nextQueryId() + { + return $this->scoringQueueRepo->pop($this->user); + } + + public function getQuery($queryId) + { + return $this->queriesRepo->getQuery($queryId); + } + + public function getQueryResults($queryId) + { + $maybeResults = $this->resultsRepo->getQueryResults($queryId); + if ($maybeResults->isEmpty()) { + return $maybeResults; + } + + $results = $this->shufflePreserveKeys( + $maybeResults->get(), + $this->user->uid + ); + + return new Some($results); + } + + public function skipQuery($queryId) + { + $this->queriesRepo->markQuerySkipped($this->user, $queryId); + $this->scoringQueueRepo->unassignUser($this->user); + } + + public function saveScores($queryId, array $scores) + { + $this->scoresRepo->storeQueryScores($this->user, $queryId, $scores); + $this->scoringQueueRepo->markScored($this->user, $queryId); + } + + public function updateUserStorage() + { + $this->userRepo->updateUser($this->user); + } + /** + * PHP's shuffle function loses the keys. So sort the keys + * and make a new array based on the order of sorted keys. + * Additionally php's shuffle is automatically seeded so we + * can't get the same order across requests. Fix that by using + * a local fisher yates implementation. + * + * @param array $array + * + * @return array + */ + private function shufflePreserveKeys(array $array, $seed) + { + $keys = $this->fisherYatesShuffle(array_keys($array), $seed); + $result = array(); + foreach ($keys as $key) { + $result[$key] = $array[$key]; + } + + return $result; + } + + /** + * @param array $array Must be numerically indexed starting + * from 0 with no gaps. + * @param int $seed + * + * @return array + */ + private function fisherYatesShuffle(array $array, $seed) + { + mt_srand($seed); + for ($i = count($array) - 1; $i > 0; --$i) { + $j = mt_rand(0, $i); + $tmp = $array[$i]; + $array[$i] = $array[$j]; + $array[$j] = $tmp; + } + + return $array; + } +} diff --git a/src/RelevanceScoring/RelevanceScoringProvider.php b/src/RelevanceScoring/RelevanceScoringProvider.php index b477b23..5421111 100644 --- a/src/RelevanceScoring/RelevanceScoringProvider.php +++ b/src/RelevanceScoring/RelevanceScoringProvider.php @@ -245,17 +245,24 @@ private function registerControllers(Application $app) { + // helper for queries controller + $app['search.queries_manager'] = function () use ($app) { + return new QueriesManager( + $app['session']->get('user'), + $app['search.repository.queries'], + $app['search.repository.results'], + $app['search.repository.scores'], + $app['search.repository.scoring_queue'], + $app['search.repository.users'] + ); + }; $app['search.controller.queries'] = function () use ($app) { return new QueriesController( $app, $app['session']->get('user'), $app['twig'], $app['form.factory'], - $app['search.repository.queries'], - $app['search.repository.results'], - $app['search.repository.scores'], - $app['search.repository.scoring_queue'], - $app['search.repository.users'], + $app['search.queries_manager'], $app['search.wikis'] ); }; -- To view, visit https://gerrit.wikimedia.org/r/318023 To unsubscribe, visit https://gerrit.wikimedia.org/r/settings Gerrit-MessageType: merged Gerrit-Change-Id: Ia5fb71aedf3d13f5c9376394555d6ae9a2bf0ddc Gerrit-PatchSet: 2 Gerrit-Project: wikimedia/discovery/discernatron Gerrit-Branch: master Gerrit-Owner: EBernhardson <ebernhard...@wikimedia.org> Gerrit-Reviewer: EBernhardson <ebernhard...@wikimedia.org> _______________________________________________ MediaWiki-commits mailing list MediaWiki-commits@lists.wikimedia.org https://lists.wikimedia.org/mailman/listinfo/mediawiki-commits