From 5fdcf94ebe0ab1a36b5685697191663ecd3487b7 Mon Sep 17 00:00:00 2001 From: Vitalii Bezsheiko Date: Mon, 9 Sep 2024 14:58:04 +0300 Subject: [PATCH] pkp/pkp-lib#10328 Refactor announcement controller and re-add Repository --- .../PKPBackendSubmissionsController.php | 17 --- .../PKPAnnouncementController.php | 69 +++++------ classes/announcement/Announcement.php | 87 +++++++++++++- classes/announcement/Repository.php | 108 ++++++++++++++++++ .../StoreTemporaryFileException.php | 11 ++ classes/facades/Repo.php | 6 + pages/admin/AdminHandler.php | 3 +- pages/management/ManagementHandler.php | 10 +- 8 files changed, 242 insertions(+), 69 deletions(-) create mode 100644 classes/announcement/Repository.php diff --git a/api/v1/_submissions/PKPBackendSubmissionsController.php b/api/v1/_submissions/PKPBackendSubmissionsController.php index 30923a3a84f..56e2d4c70c4 100644 --- a/api/v1/_submissions/PKPBackendSubmissionsController.php +++ b/api/v1/_submissions/PKPBackendSubmissionsController.php @@ -24,7 +24,6 @@ use Illuminate\Http\Request; use Illuminate\Http\Response; use Illuminate\Support\Facades\Route; -use PKP\announcement\Announcement; use PKP\API\v1\submissions\AnonymizeData; use PKP\config\Config; use PKP\core\PKPBaseController; @@ -142,15 +141,6 @@ public function getGroupRoutes(): void Role::ROLE_ID_REVIEWER, ]) ]); - - // Endpoint for testing new User Model; TODO remove before merging - Route::get('testUserModel', $this->getTestUsers(...)) - ->name('_submission.getTestUsers') - ->middleware([ - self::roleAuthorizer([ - Role::ROLE_ID_MANAGER, - ]) - ]); } } @@ -518,11 +508,4 @@ protected function canAccessAllSubmissions(): bool $userRoles = $this->getAuthorizedContextObject(Application::ASSOC_TYPE_USER_ROLES); return !empty(array_intersect([Role::ROLE_ID_SITE_ADMIN, Role::ROLE_ID_MANAGER], $userRoles)); } - - public function getTestUsers(Request $illuminateRequest): JsonResponse - { - $announcement = Announcement::find(1); - - return response()->json($announcement->datePosted, Response::HTTP_OK); - } } diff --git a/api/v1/announcements/PKPAnnouncementController.php b/api/v1/announcements/PKPAnnouncementController.php index 9398fb3756d..7132c1e89e3 100644 --- a/api/v1/announcements/PKPAnnouncementController.php +++ b/api/v1/announcements/PKPAnnouncementController.php @@ -25,9 +25,10 @@ use Illuminate\Http\Response; use Illuminate\Support\Facades\Bus; use Illuminate\Support\Facades\Route; -use PKP\announcement\Collector; +use PKP\announcement\Announcement; use PKP\context\Context; use PKP\core\exceptions\StoreTemporaryFileException; +use PKP\core\PKPApplication; use PKP\core\PKPBaseController; use PKP\core\PKPRequest; use PKP\db\DAORegistry; @@ -124,7 +125,7 @@ public function authorize(PKPRequest $request, array &$args, array $roleAssignme */ public function get(Request $illuminateRequest): JsonResponse { - $announcement = Repo::announcement()->get((int) $illuminateRequest->route('announcementId')); + $announcement = Announcement::find((int) $illuminateRequest->route('announcementId')); if (!$announcement) { return response()->json([ @@ -133,7 +134,7 @@ public function get(Request $illuminateRequest): JsonResponse } // The assocId in announcements should always point to the contextId - if ($announcement->getData('assocId') !== $this->getRequest()->getContext()?->getId()) { + if ($announcement->getAttribute('assocId') !== $this->getRequest()->getContext()?->getId()) { return response()->json([ 'error' => __('api.announcements.400.contextsNotMatched') ], Response::HTTP_BAD_REQUEST); @@ -149,43 +150,38 @@ public function get(Request $illuminateRequest): JsonResponse */ public function getMany(Request $illuminateRequest): JsonResponse { - $collector = Repo::announcement()->getCollector() - ->limit(self::DEFAULT_COUNT) - ->offset(0); + $announcements = Announcement::limit(self::DEFAULT_COUNT)->offset(0); foreach ($illuminateRequest->query() as $param => $val) { switch ($param) { case 'typeIds': - $collector->filterByTypeIds( + $announcements->withTypeIds( array_map('intval', paramToArray($val)) ); break; case 'count': - $collector->limit(min((int) $val, self::MAX_COUNT)); + $announcements->limit(min((int) $val, self::MAX_COUNT)); break; case 'offset': - $collector->offset((int) $val); + $announcements->offset((int) $val); break; case 'searchPhrase': - $collector->searchPhrase($val); + $announcements->withSearchPhrase($val); break; } } if ($this->getRequest()->getContext()) { - $collector->filterByContextIds([$this->getRequest()->getContext()->getId()]); + $announcements->withContextIds([$this->getRequest()->getContext()->getId()]); } else { - $collector->withSiteAnnouncements(Collector::SITE_ONLY); + $announcements->withContextIds([PKPApplication::SITE_CONTEXT_ID]); } - - Hook::run('API::announcements::params', [$collector, $illuminateRequest]); - - $announcements = $collector->getMany(); + Hook::run('API::announcements::params', [$announcements, $illuminateRequest]); return response()->json([ - 'itemsMax' => $collector->getCount(), - 'items' => Repo::announcement()->getSchemaMap()->summarizeMany($announcements)->values(), + 'itemsMax' => $announcements->count(), + 'items' => Repo::announcement()->getSchemaMap()->summarizeMany($announcements->get())->values(), ], Response::HTTP_OK); } @@ -209,27 +205,22 @@ public function add(Request $illuminateRequest): JsonResponse return response()->json($errors, Response::HTTP_BAD_REQUEST); } - $announcement = Repo::announcement()->newDataObject($params); - try { - $announcementId = Repo::announcement()->add($announcement); + $announcement = Announcement::create($params); } catch (StoreTemporaryFileException $e) { - $announcementId = $e->dataObject->getId(); + $announcementId = $e->getDataObjectId(); if ($announcementId) { - $announcement = Repo::announcement()->get($announcementId); - Repo::announcement()->delete($announcement); + Announcement::destroy([$announcementId]); } return response()->json([ 'image' => [__('api.400.errorUploadingImage')] ], Response::HTTP_BAD_REQUEST); } - $announcement = Repo::announcement()->get($announcementId); - $sendEmail = (bool) filter_var($params['sendEmail'], FILTER_VALIDATE_BOOLEAN); if ($context) { - $this->notifyUsers($request, $context, $announcementId, $sendEmail); + $this->notifyUsers($request, $context, $announcement->getKey(), $sendEmail); } return response()->json(Repo::announcement()->getSchemaMap()->map($announcement), Response::HTTP_OK); @@ -243,7 +234,8 @@ public function edit(Request $illuminateRequest): JsonResponse $request = $this->getRequest(); $context = $request->getContext(); - $announcement = Repo::announcement()->get((int) $illuminateRequest->route('announcementId')); + /** @var Announcement $announcement */ + $announcement = Announcement::find((int) $illuminateRequest->route('announcementId')); if (!$announcement) { return response()->json([ @@ -251,19 +243,19 @@ public function edit(Request $illuminateRequest): JsonResponse ], Response::HTTP_NOT_FOUND); } - if ($announcement->getData('assocType') !== Application::get()->getContextAssocType()) { + if ($announcement->getAttribute('assocType') !== Application::get()->getContextAssocType()) { throw new Exception('Announcement has an assocType that did not match the context.'); } // Don't allow to edit an announcement from one context from a different context's endpoint - if ($request->getContext()?->getId() !== $announcement->getData('assocId')) { + if ($request->getContext()?->getId() !== $announcement->getAttribute('assocId')) { return response()->json([ 'error' => __('api.announcements.400.contextsNotMatched') ], Response::HTTP_FORBIDDEN); } $params = $this->convertStringsToSchema(PKPSchemaService::SCHEMA_ANNOUNCEMENT, $illuminateRequest->input()); - $params['id'] = $announcement->getId(); + $params['id'] = $announcement->getKey(); $params['typeId'] ??= null; $primaryLocale = $context ? $context->getPrimaryLocale() : $request->getSite()->getPrimaryLocale(); @@ -275,15 +267,15 @@ public function edit(Request $illuminateRequest): JsonResponse } try { - Repo::announcement()->edit($announcement, $params); + $announcement->update($params); } catch (StoreTemporaryFileException $e) { - Repo::announcement()->delete($announcement); + $announcement->delete(); // TODO do we really need to delete an announcement if the image upload fails? return response()->json([ 'image' => [__('api.400.errorUploadingImage')] ], Response::HTTP_BAD_REQUEST); } - $announcement = Repo::announcement()->get($announcement->getId()); + $announcement = Announcement::find($announcement->getKey()); return response()->json(Repo::announcement()->getSchemaMap()->map($announcement), Response::HTTP_OK); } @@ -295,7 +287,8 @@ public function delete(Request $illuminateRequest): JsonResponse { $request = $this->getRequest(); - $announcement = Repo::announcement()->get((int) $illuminateRequest->route('announcementId')); + /** @var Announcement $announcement */ + $announcement = Announcement::find((int) $illuminateRequest->route('announcementId')); if (!$announcement) { return response()->json([ @@ -303,12 +296,12 @@ public function delete(Request $illuminateRequest): JsonResponse ], Response::HTTP_NOT_FOUND); } - if ($announcement->getData('assocType') !== Application::get()->getContextAssocType()) { + if ($announcement->getAttribute('assocType') !== Application::get()->getContextAssocType()) { throw new Exception('Announcement has an assocType that did not match the context.'); } // Don't allow to delete an announcement from one context from a different context's endpoint - if ($request->getContext()?->getId() !== $announcement->getData('assocId')) { + if ($request->getContext()?->getId() !== $announcement->getAttribute('assocId')) { return response()->json([ 'error' => __('api.announcements.400.contextsNotMatched') ], Response::HTTP_FORBIDDEN); @@ -316,7 +309,7 @@ public function delete(Request $illuminateRequest): JsonResponse $announcementProps = Repo::announcement()->getSchemaMap()->map($announcement); - Repo::announcement()->delete($announcement); + $announcement->delete(); return response()->json($announcementProps, Response::HTTP_OK); } diff --git a/classes/announcement/Announcement.php b/classes/announcement/Announcement.php index 502b6df0697..a8f200940da 100644 --- a/classes/announcement/Announcement.php +++ b/classes/announcement/Announcement.php @@ -16,11 +16,14 @@ use APP\core\Application; use APP\file\PublicFileManager; use Eloquence\Behaviours\HasCamelCasing; +use Illuminate\Database\Eloquent\Builder as EloquentBuilder; use Illuminate\Database\Eloquent\Casts\Attribute; use Illuminate\Database\Eloquent\Model; +use Illuminate\Support\Facades\DB; use PKP\context\Context; use PKP\core\Core; use PKP\core\exceptions\StoreTemporaryFileException; +use PKP\core\PKPApplication; use PKP\core\PKPString; use PKP\core\traits\ModelWithSettings; use PKP\db\DAORegistry; @@ -30,6 +33,10 @@ use PKP\plugins\Hook; use PKP\services\PKPSchemaService; +/** + * @method static \Illuminate\Database\Eloquent\Builder withContextIds(array $contextIds) accepts valid context IDs or PKPApplication::SITE_CONTEXT_ID as an array values + * @method static \Illuminate\Database\Eloquent\Builder withTypeIds(array $typeIds) filters results by announcement type IDs + */ class Announcement extends Model { use HasCamelCasing; @@ -44,6 +51,13 @@ class Announcement extends Model public const UPDATED_AT = null; protected string $settingsTable = 'announcement_settings'; + /** + * The attributes that are mass assignable. + * + * @var array + */ + protected $fillable = ['assocType', 'assocId', 'typeId', 'title', 'image', 'description', 'descriptionShort']; + /** * @inheritDoc */ @@ -122,14 +136,75 @@ public function save(array $options = []) } /** - * Get the base URL for announcement file uploads + * Filter by context IDs, accepts PKPApplication::SITE_CONTEXT_ID as a parameter array value if include site wide + */ + protected function scopeWithContextIds(EloquentBuilder $builder, array $contextIds): EloquentBuilder + { + $filteredIds = []; + $siteWide = false; + foreach ($contextIds as $contextId) { + if ($contextId == PKPApplication::SITE_CONTEXT_ID) { + $siteWide = true; + continue; + } + + $filteredIds[] = $contextId; + } + + return $builder->where('assoc_type', Application::get()->getContextAssocType()) + ->whereIn('assoc_id', $filteredIds) + ->when($siteWide, fn (EloquentBuilder $builder) => $builder->orWhereNull('assoc_id')); + } + + /** + * Filter by announcement type IDs */ - public static function getFileUploadBaseUrl(?Context $context = null): string + protected function scopeWithTypeIds(EloquentBuilder $builder, array $typeIds): EloquentBuilder { - return join('/', [ - Application::get()->getRequest()->getPublicFilesUrl($context), - static::IMAGE_SUBDIRECTORY, - ]); + return $builder->whereIn('type_id', $typeIds); + } + + /** + * + * @param string $date Optionally filter announcements by those + * not expired until $date (YYYY-MM-DD) + */ + protected function scopeWithActiveByDate(EloquentBuilder $builder, string $date = ''): EloquentBuilder + { + return $builder->where('a.date_expire', '>', empty($date) ? Core::getCurrentDate() : $date) + ->orWhereNull('a.date_expire'); + } + + /** + * Filter announcements by those matching a search query + */ + protected function scopeWithSearchPhrase(EloquentBuilder $builder, string $searchPhrase): EloquentBuilder + { + $words = explode(' ', $searchPhrase); + if (!count($words)) { + return $builder; + } + + return $builder->whereIn('announcement_id', function ($builder) use ($words) { + $builder->select('announcement_id')->from($this->getSettingsTable()); + foreach ($words as $word) { + $word = strtolower(addcslashes($word, '%_')); + $builder->where(function ($builder) use ($word) { + $builder->where(function ($builder) use ($word) { + $builder->where('setting_name', 'title'); + $builder->where(DB::raw('lower(setting_value)'), 'LIKE', "%{$word}%"); + }) + ->orWhere(function ($builder) use ($word) { + $builder->where('setting_name', 'descriptionShort'); + $builder->where(DB::raw('lower(setting_value)'), 'LIKE', "%{$word}%"); + }) + ->orWhere(function ($builder) use ($word) { + $builder->where('setting_name', 'description'); + $builder->where(DB::raw('lower(setting_value)'), 'LIKE', "%{$word}%"); + }); + }); + } + }); } /** diff --git a/classes/announcement/Repository.php b/classes/announcement/Repository.php new file mode 100644 index 00000000000..487014d9a12 --- /dev/null +++ b/classes/announcement/Repository.php @@ -0,0 +1,108 @@ + $schemaService */ + protected PKPSchemaService $schemaService; + + public function __construct(Request $request, PKPSchemaService $schemaService) + { + $this->request = $request; + $this->schemaService = $schemaService; + } + + /** + * Validate properties for an announcement + * + * Perform validation checks on data used to add or edit an announcement. + * + * @param array $props A key/value array with the new data to validate + * @param array $allowedLocales The context's supported locales + * @param string $primaryLocale The context's primary locale + * + * @return array A key/value array with validation errors. Empty if no errors + * + * @hook Announcement::validate [[&$errors, $object, $props, $allowedLocales, $primaryLocale]] + */ + public function validate(?Announcement $object, array $props, array $allowedLocales, string $primaryLocale): array + { + $schema = $object->getSchemaName(); + + $validator = ValidatorFactory::make( + $props, + $this->schemaService->getValidationRules($schema, $allowedLocales), + [ + 'dateExpire.date_format' => __('stats.dateRange.invalidDate'), + ] + ); + + // Check required fields + ValidatorFactory::required( + $validator, + $object, + $this->schemaService->getRequiredProps($schema), + $this->schemaService->getMultilingualProps($schema), + $allowedLocales, + $primaryLocale + ); + + // Check for input from disallowed locales + ValidatorFactory::allowedLocales($validator, $this->schemaService->getMultilingualProps($schema), $allowedLocales); + + $errors = []; + + if ($validator->fails()) { + $errors = $this->schemaService->formatValidationErrors($validator->errors()); + } + + Hook::call('Announcement::validate', [&$errors, $object, $props, $allowedLocales, $primaryLocale]); + + return $errors; + } + + /** + * Get an instance of the map class for mapping + * announcements to their schema + */ + public function getSchemaMap(): maps\Schema + { + return app('maps')->withExtensions($this->schemaMap); + } + + /** + * Get the base URL for announcement file uploads + */ + public static function getFileUploadBaseUrl(?Context $context = null): string + { + return join('/', [ + Application::get()->getRequest()->getPublicFilesUrl($context), + Announcement::IMAGE_SUBDIRECTORY, + ]); + } +} diff --git a/classes/core/exceptions/StoreTemporaryFileException.php b/classes/core/exceptions/StoreTemporaryFileException.php index e1604ce6f40..8c7ec2c0433 100644 --- a/classes/core/exceptions/StoreTemporaryFileException.php +++ b/classes/core/exceptions/StoreTemporaryFileException.php @@ -38,4 +38,15 @@ public function __construct(public TemporaryFile $temporaryFile, public string $ } parent::__construct($message); } + + public function getDataObjectId(): ?int + { + if (!isset($this->dataObject)) { + return null; + } + + return is_a(get_class($this->dataObject), DataObject::class) ? + $this->dataObject->getId() : + $this->dataObject->getKey(); + } } diff --git a/classes/facades/Repo.php b/classes/facades/Repo.php index 16781a09daf..cbfb448f7d1 100644 --- a/classes/facades/Repo.php +++ b/classes/facades/Repo.php @@ -24,6 +24,7 @@ namespace PKP\facades; +use PKP\announcement\Repository as AnnouncementRepository; use PKP\author\Repository as AuthorRepository; use PKP\category\Repository as CategoryRepository; use PKP\decision\Repository as DecisionRepository; @@ -44,6 +45,11 @@ class Repo { + public static function announcement(): AnnouncementRepository + { + return app(AnnouncementRepository::class); + } + public static function author(): AuthorRepository { return app(AuthorRepository::class); diff --git a/pages/admin/AdminHandler.php b/pages/admin/AdminHandler.php index 5d3f35fdb9c..7f3c4bd321f 100644 --- a/pages/admin/AdminHandler.php +++ b/pages/admin/AdminHandler.php @@ -25,7 +25,6 @@ use Illuminate\Support\Facades\DB; use Illuminate\Support\Str; use PDO; -use PKP\announcement\Announcement; use PKP\announcement\Collector; use PKP\components\forms\announcement\PKPAnnouncementForm; use PKP\components\forms\context\PKPAnnouncementSettingsForm; @@ -215,7 +214,7 @@ public function settings($args, $request) $siteStatisticsForm = new PKPSiteStatisticsForm($apiUrl, $locales, $site); $highlightsListPanel = $this->getHighlightsListPanel(); $announcementSettingsForm = new PKPAnnouncementSettingsForm($apiUrl, $locales, $site); - $announcementsForm = new PKPAnnouncementForm($announcementsApiUrl, $locales, Announcement::getFileUploadBaseUrl(), $temporaryFileApiUrl); + $announcementsForm = new PKPAnnouncementForm($announcementsApiUrl, $locales, Repo::Announcement()->getFileUploadBaseUrl(), $temporaryFileApiUrl); $announcementsListPanel = $this->getAnnouncementsListPanel($announcementsApiUrl, $announcementsForm); $templateMgr = TemplateManager::getManager($request); diff --git a/pages/management/ManagementHandler.php b/pages/management/ManagementHandler.php index 49142a5386b..85c60a392d8 100644 --- a/pages/management/ManagementHandler.php +++ b/pages/management/ManagementHandler.php @@ -392,18 +392,16 @@ public function announcements($args, $request) $announcementForm = new PKPAnnouncementForm( $apiUrl, $locales, - Announcement::getFileUploadBaseUrl($context), + Repo::announcement()->getFileUploadBaseUrl($context), $this->getTemporaryFileApiUrl($context), $request->getContext() ); - $collector = Repo::announcement() - ->getCollector() - ->filterByContextIds([$request->getContext()->getId()]); + $announcementBuilder = Announcement::withContextIds([$request->getContext()->getId()]); - $itemsMax = $collector->getCount(); + $itemsMax = $announcementBuilder->count(); $items = Repo::announcement()->getSchemaMap()->summarizeMany( - $collector->limit(30)->getMany() + $announcementBuilder->limit(30)->get() ); $announcementsListPanel = new \PKP\components\listPanels\PKPAnnouncementsListPanel(