From b6771ef5fd1901fbf7c47957c2d99a507d592202 Mon Sep 17 00:00:00 2001 From: Nate Wright Date: Fri, 25 Aug 2023 14:12:36 +0100 Subject: [PATCH] pkp/pkp-lib#9253 Use null to associate announcements with the site --- .../announcements/PKPAnnouncementHandler.php | 30 +++++++---------- classes/announcement/AnnouncementTypeDAO.php | 30 ++++++++++------- classes/announcement/Collector.php | 20 ++++++++++- .../announcement/PKPAnnouncementForm.php | 11 ++----- .../listPanels/PKPAnnouncementsListPanel.php | 2 +- .../v3_5_0/I9253_SiteAnnouncements.php | 28 +++++----------- .../AnnouncementTypeGridHandler.php | 33 +++++-------------- .../form/AnnouncementTypeForm.php | 4 +-- pages/admin/AdminHandler.php | 5 +-- pages/announcement/AnnouncementHandler.php | 26 +++++++-------- pages/index/PKPIndexHandler.php | 20 ++++++----- schemas/announcement.json | 8 +++-- 12 files changed, 105 insertions(+), 112 deletions(-) diff --git a/api/v1/announcements/PKPAnnouncementHandler.php b/api/v1/announcements/PKPAnnouncementHandler.php index b4aa58f7578..76113c41e0f 100644 --- a/api/v1/announcements/PKPAnnouncementHandler.php +++ b/api/v1/announcements/PKPAnnouncementHandler.php @@ -22,6 +22,7 @@ use APP\facades\Repo; use Exception; use Illuminate\Support\Facades\Bus; +use PKP\announcement\Collector; use PKP\config\Config; use PKP\context\Context; use PKP\db\DAORegistry; @@ -133,7 +134,7 @@ public function get($slimRequest, $response, $args) } // The assocId in announcements should always point to the contextId - if ($announcement->getData('assocId') !== $this->getRequest()->getContext()->getId()) { + if ($announcement->getData('assocId') !== $this->getRequest()->getContext()?->getId()) { return $response->withStatus(404)->withJsonError('api.announcements.400.contextsNotMatched'); } @@ -174,7 +175,12 @@ public function getMany($slimRequest, $response, $args) } } - $collector->filterByContextIds([$this->getContextId()]); + if ($this->getRequest()->getContext()) { + $collector->filterByContextIds([$this->getRequest()->getContext()->getId()]); + } else { + $collector->withSiteAnnouncements(Collector::SITE_ONLY); + } + Hook::call('API::submissions::params', [$collector, $slimRequest]); @@ -199,11 +205,10 @@ public function add($slimRequest, $response, $args) { $request = $this->getRequest(); $context = $request->getContext(); - $contextId = $this->getContextId(); $params = $this->convertStringsToSchema(PKPSchemaService::SCHEMA_ANNOUNCEMENT, $slimRequest->getParsedBody()); $params['assocType'] = Application::get()->getContextAssocType(); - $params['assocId'] = $contextId; + $params['assocId'] = $context?->getId(); $primaryLocale = $context ? $context->getPrimaryLocale() : $request->getSite()->getPrimaryLocale(); $allowedLocales = $context ? $context->getSupportedFormLocales() : $request->getSite()->getSupportedLocales(); @@ -236,6 +241,7 @@ public function add($slimRequest, $response, $args) public function edit($slimRequest, $response, $args) { $request = $this->getRequest(); + $context = $request->getContext(); $announcement = Repo::announcement()->get((int) $args['announcementId']); @@ -248,7 +254,7 @@ public function edit($slimRequest, $response, $args) } // Don't allow to edit an announcement from one context from a different context's endpoint - if ($this->getContextId() !== $announcement->getData('assocId')) { + if ($context?->getId() !== $announcement->getData('assocId')) { return $response->withStatus(403)->withJsonError('api.announcements.400.contextsNotMatched'); } @@ -256,7 +262,6 @@ public function edit($slimRequest, $response, $args) $params['id'] = $announcement->getId(); $params['typeId'] ??= null; - $context = $request->getContext(); $primaryLocale = $context ? $context->getPrimaryLocale() : $request->getSite()->getPrimaryLocale(); $allowedLocales = $context ? $context->getSupportedFormLocales() : $request->getSite()->getSupportedLocales(); @@ -296,7 +301,7 @@ public function delete($slimRequest, $response, $args) } // Don't allow to delete an announcement from one context from a different context's endpoint - if ($this->getContextId() !== $announcement->getData('assocId')) { + if ($request->getContext()?->getId() !== $announcement->getData('assocId')) { return $response->withStatus(403)->withJsonError('api.announcements.400.contextsNotMatched'); } @@ -307,17 +312,6 @@ public function delete($slimRequest, $response, $args) return $response->withJson($announcementProps, 200); } - /** - * Get the context id or site-wide context id of the current request - */ - protected function getContextId(): int - { - $context = $this->getRequest()->getContext(); - return $context - ? $context->getId() - : Application::CONTEXT_ID_NONE; - } - /** * Modify the role assignments so that only * site admins have access diff --git a/classes/announcement/AnnouncementTypeDAO.php b/classes/announcement/AnnouncementTypeDAO.php index cc2fbba8876..e26c95c6320 100644 --- a/classes/announcement/AnnouncementTypeDAO.php +++ b/classes/announcement/AnnouncementTypeDAO.php @@ -35,8 +35,8 @@ public function newDataObject() /** * Retrieve an announcement type by announcement type ID. * - * @param int $typeId Announcement type ID - * @param int $contextId Optional context ID + * @param ?int $typeId Announcement type ID + * @param ?int $contextId Optional context ID * * @return AnnouncementType */ @@ -110,7 +110,11 @@ public function insertObject($announcementType) (context_id) VALUES (?)'), - [(int) $announcementType->getContextId()] + [ + $announcementType->getContextId() + ? (int) $announcementType->getContextId() + : null + ] ); $announcementType->setId($this->getInsertId()); $this->updateLocaleFields($announcementType); @@ -131,7 +135,7 @@ public function updateObject($announcementType) SET context_id = ? WHERE type_id = ?', [ - (int) $announcementType->getContextId(), + $announcementType->getContextId(), (int) $announcementType->getId() ] ); @@ -181,16 +185,20 @@ public function deleteByContextId($contextId) /** * Retrieve an array of announcement types matching a particular context ID. * - * @param int $contextId - * * @return \Generator Matching AnnouncementTypes */ - public function getByContextId($contextId) + public function getByContextId(?int $contextId) { - $result = $this->retrieve( - 'SELECT * FROM announcement_types WHERE context_id = ? ORDER BY type_id', - [(int) $contextId] - ); + if ($contextId) { + $result = $this->retrieve( + 'SELECT * FROM announcement_types WHERE context_id = ? ORDER BY type_id', + [$contextId] + ); + } else { + $result = $this->retrieve( + 'SELECT * FROM announcement_types WHERE context_id IS NULL ORDER BY type_id' + ); + } foreach ($result as $row) { yield $row->type_id => $this->_fromRow((array) $row); } diff --git a/classes/announcement/Collector.php b/classes/announcement/Collector.php index 2c264859088..f0ebb64445e 100644 --- a/classes/announcement/Collector.php +++ b/classes/announcement/Collector.php @@ -31,12 +31,15 @@ class Collector implements CollectorInterface public const ORDERBY_DATE_EXPIRE = 'date_expire'; public const ORDER_DIR_ASC = 'ASC'; public const ORDER_DIR_DESC = 'DESC'; + public const SITE_ONLY = 'site'; + public const SITE_AND_CONTEXTS = 'all'; public DAO $dao; public ?array $contextIds = null; public ?string $isActive = null; public ?string $searchPhrase = null; public ?array $typeIds = null; + public ?string $includeSite = null; public ?int $count = null; public ?int $offset = null; public string $orderBy = self::ORDERBY_DATE_POSTED; @@ -105,6 +108,15 @@ public function filterByTypeIds(array $typeIds): self return $this; } + /** + * Include site-level announcements in the results + */ + public function withSiteAnnouncements(?string $includeMethod = self::SITE_AND_CONTEXTS): self + { + $this->includeSite = $includeMethod; + return $this; + } + /** * Filter announcements by those matching a search query */ @@ -156,9 +168,15 @@ public function getQueryBuilder(): Builder $qb = DB::table($this->dao->table . ' as a') ->select(['a.*']); - if (isset($this->contextIds)) { + if (isset($this->contextIds) && $this->includeSite !== self::SITE_ONLY) { + $qb->where('a.assoc_type', Application::get()->getContextAssocType()); $qb->whereIn('a.assoc_id', $this->contextIds); + if ($this->includeSite === self::SITE_AND_CONTEXTS) { + $qb->orWhereNull('a.assoc_id'); + } + } elseif ($this->includeSite === self::SITE_ONLY) { $qb->where('a.assoc_type', Application::get()->getContextAssocType()); + $qb->whereNull('a.assoc_id'); } if (isset($this->typeIds)) { diff --git a/classes/components/forms/announcement/PKPAnnouncementForm.php b/classes/components/forms/announcement/PKPAnnouncementForm.php index 40797234b63..d6a8f158a3a 100644 --- a/classes/components/forms/announcement/PKPAnnouncementForm.php +++ b/classes/components/forms/announcement/PKPAnnouncementForm.php @@ -23,7 +23,6 @@ use PKP\components\forms\FormComponent; use PKP\context\Context; use PKP\db\DAORegistry; -use PKP\site\Site; define('FORM_ANNOUNCEMENT', 'announcement'); @@ -35,7 +34,7 @@ class PKPAnnouncementForm extends FormComponent /** @copydoc FormComponent::$method */ public $method = 'POST'; - public Context|Site $context; + public ?Context $context; /** * Constructor @@ -43,7 +42,7 @@ class PKPAnnouncementForm extends FormComponent * @param string $action URL to submit the form to * @param array $locales Supported locales */ - public function __construct($action, $locales, Context|Site $context) + public function __construct($action, $locales, ?Context $context = null) { $this->action = $action; $this->locales = $locales; @@ -99,11 +98,7 @@ protected function getAnnouncementTypeOptions(): array /** @var AnnouncementTypeDAO */ $announcementTypeDao = DAORegistry::getDAO('AnnouncementTypeDAO'); - $announcementTypes = $announcementTypeDao->getByContextId( - is_a($this->context, Context::class) - ? $this->context->getId() - : Application::CONTEXT_ID_NONE - ); + $announcementTypes = $announcementTypeDao->getByContextId($this->context?->getId()); $announcementTypeOptions = []; foreach ($announcementTypes as $announcementType) { diff --git a/classes/components/listPanels/PKPAnnouncementsListPanel.php b/classes/components/listPanels/PKPAnnouncementsListPanel.php index c0ccf41e90b..ab096a2aaa9 100644 --- a/classes/components/listPanels/PKPAnnouncementsListPanel.php +++ b/classes/components/listPanels/PKPAnnouncementsListPanel.php @@ -61,7 +61,7 @@ protected function getUrlBase(): string return $request->getDispatcher()->url( $request, Application::ROUTE_PAGE, - is_a($this->form->context, Context::class) + $this->form->context ? $request->getContext()->getPath() : 'index', 'announcement', diff --git a/classes/migration/upgrade/v3_5_0/I9253_SiteAnnouncements.php b/classes/migration/upgrade/v3_5_0/I9253_SiteAnnouncements.php index b243db7bd26..f56322e78bb 100644 --- a/classes/migration/upgrade/v3_5_0/I9253_SiteAnnouncements.php +++ b/classes/migration/upgrade/v3_5_0/I9253_SiteAnnouncements.php @@ -12,7 +12,6 @@ namespace PKP\migration\upgrade\v3_5_0; -use APP\core\Application; use Illuminate\Database\Schema\Blueprint; use Illuminate\Support\Facades\Schema; use PKP\migration\Migration; @@ -24,8 +23,11 @@ class I9253_SiteAnnouncements extends Migration */ public function up(): void { + Schema::table('announcements', function (Blueprint $table) { + $table->bigInteger('assoc_id')->nullable()->change(); + }); Schema::table('announcement_types', function (Blueprint $table) { - $table->dropForeign(['context_id']); + $table->bigInteger('context_id')->nullable()->change(); }); } @@ -34,23 +36,11 @@ public function up(): void */ public function down(): void { - $app = Application::getName(); - - $contextIdColumn = 'journal_id'; - $contextTable = 'journals'; - if ($app === 'omp') { - $contextIdColumn = 'press_id'; - $contextTable = 'presses'; - } elseif ($app === 'ops') { - $contextIdColumn = 'server_id'; - $contextTable = 'servers'; - } - - Schema::table('announcement_types', function (Blueprint $table) use ($contextIdColumn, $contextTable) { - $table - ->foreign('context_id') - ->references($contextIdColumn) - ->on($contextTable); + Schema::table('announcements', function (Blueprint $table) { + $table->bigInteger('assoc_id')->nullable(false)->change(); + }); + Schema::table('announcement_types', function (Blueprint $table) { + $table->bigInteger('context_id')->nullable(false)->change(); }); } } diff --git a/controllers/grid/announcements/AnnouncementTypeGridHandler.php b/controllers/grid/announcements/AnnouncementTypeGridHandler.php index edfea5dabdd..f13e034b4a2 100644 --- a/controllers/grid/announcements/AnnouncementTypeGridHandler.php +++ b/controllers/grid/announcements/AnnouncementTypeGridHandler.php @@ -60,12 +60,12 @@ public function __construct() */ public function authorize($request, &$args, $roleAssignments) { - $contextId = $this->getContextId(); + $context = $request->getContext(); - if ($contextId === Application::CONTEXT_ID_NONE) { - $this->addPolicy(new PKPSiteAccessPolicy($request, null, $roleAssignments)); - } else { + if ($context) { $this->addPolicy(new ContextAccessPolicy($request, $roleAssignments)); + } else { + $this->addPolicy(new PKPSiteAccessPolicy($request, null, $roleAssignments)); } $announcementTypeId = $request->getUserVar('announcementTypeId'); @@ -73,7 +73,7 @@ public function authorize($request, &$args, $roleAssignments) // Ensure announcement type is valid and for this context $announcementTypeDao = DAORegistry::getDAO('AnnouncementTypeDAO'); /** @var AnnouncementTypeDAO $announcementTypeDao */ $announcementType = $announcementTypeDao->getById($announcementTypeId); - if (!$announcementType || $announcementType->getContextId() != $contextId) { + if (!$announcementType || $announcementType->getContextId() != $context?->getId()) { return false; } } @@ -95,8 +95,6 @@ public function initialize($request, $args = null) // Set the no items row text $this->setEmptyRowText('manager.announcementTypes.noneCreated'); - $context = $request->getContext(); - // Columns $announcementTypeCellProvider = new AnnouncementTypeGridCellProvider(); $this->addColumn( @@ -134,7 +132,7 @@ public function initialize($request, $args = null) protected function loadData($request, $filter) { $announcementTypeDao = DAORegistry::getDAO('AnnouncementTypeDAO'); /** @var AnnouncementTypeDAO $announcementTypeDao */ - return $announcementTypeDao->getByContextId($this->getContextId()); + return $announcementTypeDao->getByContextId($request->getContext()?->getId()); } /** @@ -172,7 +170,7 @@ public function addAnnouncementType($args, $request) public function editAnnouncementType($args, $request) { $announcementTypeId = (int)$request->getUserVar('announcementTypeId'); - $announcementTypeForm = new AnnouncementTypeForm($this->getContextId(), $announcementTypeId); + $announcementTypeForm = new AnnouncementTypeForm($request->getContext()?->getId(), $announcementTypeId); $announcementTypeForm->initData(); return new JSONMessage(true, $announcementTypeForm->fetch($request)); @@ -192,7 +190,7 @@ public function updateAnnouncementType($args, $request) $announcementTypeId = $request->getUserVar('announcementTypeId'); // Form handling. - $announcementTypeForm = new AnnouncementTypeForm($this->getContextId(), $announcementTypeId); + $announcementTypeForm = new AnnouncementTypeForm($request->getContext()?->getId(), $announcementTypeId); $announcementTypeForm->readInputData(); if ($announcementTypeForm->validate()) { @@ -231,7 +229,7 @@ public function deleteAnnouncementType($args, $request) $announcementTypeId = (int) $request->getUserVar('announcementTypeId'); $announcementTypeDao = DAORegistry::getDAO('AnnouncementTypeDAO'); /** @var AnnouncementTypeDAO $announcementTypeDao */ - $announcementType = $announcementTypeDao->getById($announcementTypeId, $this->getContextId()); + $announcementType = $announcementTypeDao->getById($announcementTypeId, $request->getContext()?->getId()); if ($announcementType && $request->checkCSRF()) { $announcementTypeDao->deleteObject($announcementType); @@ -245,17 +243,4 @@ public function deleteAnnouncementType($args, $request) return new JSONMessage(false); } - - /** - * Get the id of the request context - * - * Returns CONTEXT_ID_NONE for site-level requests - */ - protected function getContextId(): int - { - $request = Application::get()->getRequest(); - return $request->getContext() - ? $request->getContext()->getId() - : Application::CONTEXT_ID_NONE; - } } diff --git a/controllers/grid/announcements/form/AnnouncementTypeForm.php b/controllers/grid/announcements/form/AnnouncementTypeForm.php index a190dd49539..909d70a3dc6 100644 --- a/controllers/grid/announcements/form/AnnouncementTypeForm.php +++ b/controllers/grid/announcements/form/AnnouncementTypeForm.php @@ -25,7 +25,7 @@ class AnnouncementTypeForm extends Form { - /** @var int Context ID */ + /** @var ?int Context ID or null for site announcement */ public $contextId; /** @var int The ID of the announcement type being edited */ @@ -34,7 +34,7 @@ class AnnouncementTypeForm extends Form /** * Constructor * - * @param int $contextId Context ID + * @param ?int $contextId Context ID or null for site announcement * @param int $typeId leave as default for new announcement type */ public function __construct($contextId, $typeId = null) diff --git a/pages/admin/AdminHandler.php b/pages/admin/AdminHandler.php index b5272db199d..39fe300ec8b 100644 --- a/pages/admin/AdminHandler.php +++ b/pages/admin/AdminHandler.php @@ -25,6 +25,7 @@ use Illuminate\Support\Facades\DB; use Illuminate\Support\Str; use PDO; +use PKP\announcement\Collector; use PKP\cache\CacheManager; use PKP\components\forms\highlight\HighlightForm; use PKP\components\listPanels\HighlightsListPanel; @@ -205,7 +206,7 @@ public function settings($args, $request) $siteStatisticsForm = new \PKP\components\forms\site\PKPSiteStatisticsForm($apiUrl, $locales, $site); $highlightsListPanel = $this->getHighlightsListPanel(); $announcementSettingsForm = new PKPAnnouncementSettingsForm($apiUrl, $locales, $site); - $announcementsForm = new PKPAnnouncementForm($announcementsApiUrl, $locales, $site); + $announcementsForm = new PKPAnnouncementForm($announcementsApiUrl, $locales); $announcementsListPanel = $this->getAnnouncementsListPanel($announcementsApiUrl, $announcementsForm); $templateMgr = TemplateManager::getManager($request); @@ -783,7 +784,7 @@ protected function getAnnouncementsListPanel(string $apiUrl, PKPAnnouncementForm { $collector = Repo::announcement() ->getCollector() - ->filterByContextIds([Application::CONTEXT_ID_NONE]); + ->withSiteAnnouncements(Collector::SITE_ONLY); $itemsMax = $collector->getCount(); $items = Repo::announcement()->getSchemaMap()->summarizeMany( diff --git a/pages/announcement/AnnouncementHandler.php b/pages/announcement/AnnouncementHandler.php index ce0b7e064b5..fe3f850ed5d 100644 --- a/pages/announcement/AnnouncementHandler.php +++ b/pages/announcement/AnnouncementHandler.php @@ -21,6 +21,7 @@ use APP\facades\Repo; use APP\handler\Handler; use APP\template\TemplateManager; +use PKP\announcement\Collector; use PKP\config\Config; use PKP\core\PKPRequest; use PKP\security\authorization\ContextRequiredPolicy; @@ -48,11 +49,17 @@ public function index($args, $request) $templateMgr->assign('announcementsIntroduction', $this->getAnnouncementsIntro($request)); // TODO the announcements list should support pagination - $announcements = Repo::announcement() + $collector = Repo::announcement() ->getCollector() - ->filterByContextIds([$this->getContextId($request)]) - ->filterByActive() - ->getMany(); + ->filterByActive(); + + if ($request->getContext()) { + $collector->filterByContextIds([$request->getContext()->getId()]); + } else { + $collector->withSiteAnnouncements(Collector::SITE_ONLY); + } + + $announcements = $collector->getMany(); $templateMgr->assign('announcements', $announcements->toArray()); $templateMgr->display('frontend/pages/announcements.tpl'); @@ -72,13 +79,12 @@ public function view($args, $request) $this->validate(); $this->setupTemplate($request); - $contextId = $this->getContextId($request); $announcementId = (int) array_shift($args); $announcement = Repo::announcement()->get($announcementId); if ( $announcement && $announcement->getAssocType() == Application::getContextAssocType() - && $announcement->getAssocId() == $contextId + && $announcement->getAssocId() == $request->getContext()?->getId() && ( $announcement->getDateExpire() == null || strtotime($announcement->getDateExpire()) > time() ) @@ -91,14 +97,6 @@ public function view($args, $request) $request->redirect(null, 'announcement'); } - protected function getContextId(Request $request): int - { - $context = $request->getContext(); - return $context - ? $context->getId() - : Application::CONTEXT_ID_NONE; - } - protected function isAnnouncementsEnabled(Request $request): bool { if (!Config::getVar('features', 'site_announcements') && !$request->getContext()) { diff --git a/pages/index/PKPIndexHandler.php b/pages/index/PKPIndexHandler.php index 184914e9101..69789903f89 100644 --- a/pages/index/PKPIndexHandler.php +++ b/pages/index/PKPIndexHandler.php @@ -16,11 +16,11 @@ namespace PKP\pages\index; -use APP\core\Application; use APP\facades\Repo; use APP\handler\Handler; use Illuminate\Support\LazyCollection; use PKP\config\Config; +use PKP\announcement\Collector; use PKP\context\Context; use PKP\site\Site; use PKP\template\PKPTemplateManager; @@ -40,16 +40,18 @@ protected function _setupAnnouncements(Context|Site $contextOrSite, $templateMgr $enableAnnouncements = $contextOrSite->getData('enableAnnouncements'); $numAnnouncementsHomepage = $contextOrSite->getData('numAnnouncementsHomepage'); if ($enableAnnouncements && $numAnnouncementsHomepage) { - $announcements = Repo::announcement() + $collector = Repo::announcement() ->getCollector() - ->filterByContextIds([ - is_a($contextOrSite, Context::class) - ? $contextOrSite->getId() - : Application::CONTEXT_ID_NONE - ]) ->filterByActive() - ->limit((int) $numAnnouncementsHomepage) - ->getMany(); + ->limit((int) $numAnnouncementsHomepage); + + if (is_a($contextOrSite, Context::class)) { + $collector->filterByContextIds([$contextOrSite->getId()]); + } else { + $collector->withSiteAnnouncements(Collector::SITE_ONLY); + } + + $announcements = $collector->getMany(); $templateMgr->assign([ 'announcements' => $announcements->toArray(), diff --git a/schemas/announcement.json b/schemas/announcement.json index b9119b5e40a..6a686b044ae 100644 --- a/schemas/announcement.json +++ b/schemas/announcement.json @@ -3,7 +3,6 @@ "description": "An announcement or news item.", "required": [ "assocType", - "assocId", "title" ], "properties": { @@ -16,8 +15,11 @@ }, "assocId": { "type": "integer", - "description": "The journal, press or preprint server ID.", - "apiSummary": true + "description": "The journal, press or preprint server ID. Null for site-level announcements.", + "apiSummary": true, + "validation": [ + "nullable" + ] }, "assocType": { "type": "integer",