feat(collectives-publish) fix review findings

lib/ConfigLexicon.php

@@ -32,7 +32,7 @@ class ConfigLexicon implements ILexicon {
 	public const USER_FOLDER = 'user_folder';
 
 	public const PUBLISH_ENABLED = 'publish_enabled';
-	public const PUBLISH_ENABLED_DEFAULT = 'false';
+	public const PUBLISH_ENABLED_DEFAULT = false;
 	public const STATIC_SITE_MAX_SIZE = 'static_site_max_size';
 	public const STATIC_SITE_MAX_SIZE_DEFAULT = 100 * 1024 * 1024;
 	public const STATIC_SITE_GENERATOR_URL = 'static_site_generator_url';
@@ -88,9 +88,9 @@ public function getAppConfigs(): array {
 			),
 			new Entry(
 				key: self::PUBLISH_ENABLED,
-				type: ValueType::STRING,
+				type: ValueType::BOOL,
 				defaultRaw: self::PUBLISH_ENABLED_DEFAULT,
-				definition: 'Whether the publish feature is enabled (`true` or `false`)',
+				definition: 'Whether the publish feature is enabled',
 				lazy: false,
 			),
 			new Entry(

lib/Controller/StaticSiteController.php

@@ -172,7 +172,7 @@ private function handleArchiveErrorResponse(Closure $callback, array $context):
 			);
 		} catch (StaticSiteGeneratorException) {
 			throw new OCSException(
-				$this->l10n->t('The publish service rejected the website. Please contact your administrator.'),
+				$this->l10n->t('The publish service could not process the request. Please contact your administrator.'),
 				Http::STATUS_BAD_GATEWAY,
 			);
 		} catch (ServiceException $e) {

lib/Db/StaticSiteMapper.php

@@ -155,6 +155,26 @@ public function updateStatus(int $id, string $status): StaticSite {
 		return $this->update($staticSite);
 	}
 
+	/**
+	 * Reset status and update time to the values of the given static site as it was read before.
+	 *
+	 * Keeping the update time of an aborted publication allows the user to start the publish process again right away.
+	 *
+	 * @throws NotFoundException
+	 * @throws Exception
+	 */
+	public function restoreStatus(StaticSite $previous): StaticSite {
+		try {
+			$staticSite = $this->find($previous->getId());
+		} catch (DoesNotExistException|MultipleObjectsReturnedException $e) {
+			throw new NotFoundException('Static site not found', 0, $e);
+		}
+
+		$staticSite->setStatus($previous->getStatus());
+		$staticSite->setUpdatedAt($previous->getUpdatedAt());
+		return $this->update($staticSite);
+	}
+
 	/**
 	 * @throws NotFoundException
 	 * @throws Exception

lib/Listeners/BeforeTemplateRenderedListener.php

@@ -78,6 +78,6 @@ public function handle(Event $event): void {
 		$this->initialState->provideInitialState('is_guest', $isGuest);
 
 		// Provide admin settings as initial state
-		$this->initialState->provideInitialState('publish_enabled', $this->appConfig->getValueString('collectives', ConfigLexicon::PUBLISH_ENABLED, ConfigLexicon::PUBLISH_ENABLED_DEFAULT) === 'true');
+		$this->initialState->provideInitialState('publish_enabled', $this->appConfig->getValueBool('collectives', ConfigLexicon::PUBLISH_ENABLED, ConfigLexicon::PUBLISH_ENABLED_DEFAULT));
 	}
 }

lib/Service/StaticSiteService.php

@@ -60,13 +60,14 @@ public function __construct(
 	 * @param list<int> $pageIds IDs of the pages to publish, incl. the collective's root page
 	 *
 	 * @throws NotFoundException Collective or one of the pages not found
-	 * @throws NotPermittedException User is not allowed to edit the collective
+	 * @throws NotPermittedException Publish feature is disabled or user is not allowed to edit the collective
 	 * @throws UnprocessableEntityException No pages selected, invalid title or slug, slug already in use, path too long
 	 * @throws StaticSiteGeneratorException Generator not configured or didn't accept the build
 	 * @throws ServiceException Archive couldn't be provided
 	 * @throws DBException
 	 */
 	public function create(int $collectiveId, array $pageIds, string $userId, ?string $title = null, ?string $slug = null): StaticSite {
+		$this->assertPublishFeatureEnabled();
 		$pageIds = $this->normalizePageIds($pageIds);
 		$collective = $this->getEditableCollective($collectiveId, $userId);
 
@@ -109,13 +110,14 @@ public function create(int $collectiveId, array $pageIds, string $userId, ?strin
 	 * @param list<int> $pageIds IDs of the pages to publish, incl. the collective's root page
 	 *
 	 * @throws NotFoundException Collective, static site or one of the pages not found
-	 * @throws NotPermittedException User is not allowed to edit the collective
+	 * @throws NotPermittedException Publish feature is disabled or user is not allowed to edit the collective
 	 * @throws UnprocessableEntityException No pages selected, invalid title, publication in progress, path too long
 	 * @throws StaticSiteGeneratorException Generator not configured or didn't accept the build
 	 * @throws ServiceException Archive couldn't be provided
 	 * @throws DBException
 	 */
 	public function update(int $collectiveId, int $id, array $pageIds, string $title, string $userId): StaticSite {
+		$this->assertPublishFeatureEnabled();
 		$pageIds = $this->normalizePageIds($pageIds);
 		$this->getEditableCollective($collectiveId, $userId);
 
@@ -129,15 +131,14 @@ public function update(int $collectiveId, int $id, array $pageIds, string $title
 		$this->verifyPagesBelongToCollective($collectiveId, $pageIds, $userId);
 		$this->generatorClient->assertConfigured();
 
-		$previousStatus = $staticSite->getStatus();
 		if (!$this->staticSiteMapper->updateStatusIfUnchanged($staticSite, StaticSite::STATUS_PENDING)) {
 			throw new UnprocessableEntityException('Publication of static site is already in progress');
 		}
 
 		try {
 			$this->buildArchive($staticSite);
 		} catch (ServiceException $e) {
-			$this->restoreStatus($staticSite, $previousStatus);
+			$this->restoreStatus($staticSite);
 			throw $e;
 		}
 
@@ -160,6 +161,7 @@ public function delete(int $collectiveId, int $id, string $userId): void {
 		$this->getEditableCollective($collectiveId, $userId);
 
 		$staticSite = $this->staticSiteMapper->findByIdAndCollectiveId($id, $collectiveId);
+		// TODO(Publish): Send delete request to publish service once it provides an endpoint
 		$this->staticSiteMapper->delete($staticSite);
 		$this->deleteArchive($staticSite);
 	}
@@ -246,6 +248,7 @@ public function updateStatus(string $staticSiteId, string $status, array $data =
 	 * @throws ServiceException
 	 */
 	private function buildArchive(StaticSite $staticSite): void {
+		// TODO(Publish): Consider building the archive in a queued background job once the publish service reports the build result via callback
 		try {
 			$files = $this->contentCollector->collect($staticSite->getCollectiveId());
 			$this->validateSize($files);
@@ -309,9 +312,9 @@ private function updateSettingsAndStatus(StaticSite $staticSite, string $title,
 	/**
 	 * @throws DBException
 	 */
-	private function restoreStatus(StaticSite $staticSite, string $status): void {
+	private function restoreStatus(StaticSite $previous): void {
 		try {
-			$this->staticSiteMapper->updateStatus($staticSite->getId(), $status);
+			$this->staticSiteMapper->restoreStatus($previous);
 		} catch (NotFoundException) {
 			// Static site was deleted meanwhile, nothing to restore
 		}
@@ -348,6 +351,17 @@ private function normalizePageIds(array $pageIds): array {
 		return $pageIds;
 	}
 
+	/**
+	 * Only creating and updating static sites is blocked, so editors can still list and delete existing ones.
+	 *
+	 * @throws NotPermittedException
+	 */
+	private function assertPublishFeatureEnabled(): void {
+		if (!$this->appConfig->getValueBool('collectives', ConfigLexicon::PUBLISH_ENABLED, ConfigLexicon::PUBLISH_ENABLED_DEFAULT)) {
+			throw new NotPermittedException('Publish feature is disabled');
+		}
+	}
+
 	/**
 	 * @throws NotFoundException
 	 * @throws NotPermittedException

lib/Settings/Admin.php

@@ -29,7 +29,7 @@ public function __construct(
 	public function getForm(): TemplateResponse {
 		$parameters = [
 			'default_user_folder' => $this->appConfig->getValueString('collectives', ConfigLexicon::DEFAULT_USER_FOLDER, ''),
-			'publish_enabled' => $this->appConfig->getValueString('collectives', ConfigLexicon::PUBLISH_ENABLED, ConfigLexicon::PUBLISH_ENABLED_DEFAULT) === 'true',
+			'publish_enabled' => $this->appConfig->getValueBool('collectives', ConfigLexicon::PUBLISH_ENABLED, ConfigLexicon::PUBLISH_ENABLED_DEFAULT),
 			'static_site_generator_url' => $this->appConfig->getValueString('collectives', ConfigLexicon::STATIC_SITE_GENERATOR_URL),
 			// The token itself must not be rendered into the page, so a set token is replaced by a placeholder
 			'static_site_generator_token' => $this->appConfig->getValueString('collectives', ConfigLexicon::STATIC_SITE_GENERATOR_TOKEN) === '' ? '' : self::TOKEN_PLACEHOLDER,

playwright/e2e/collective-publish.spec.ts

@@ -104,6 +104,59 @@ test.describe('Collective publish', () => {
 		})
 	})
 
+	test('preselects pages created after the page list was loaded if the collective was never published', async ({ collective, navigation, page, user }) => {
+		await collective.openCollective()
+		// Created via API, so the page list in the browser doesn't contain it yet
+		await collective.createPage({ title: 'Page created meanwhile', user, page })
+
+		await navigation.clickCollectiveMenu(collective.data.name, 'Publish website')
+		const modal = page.getByRole('dialog')
+		await expect(modal.getByRole('checkbox', { name: 'Page created meanwhile' })).toBeChecked()
+		await expect(modal.getByRole('button', { name: 'Publish website' })).toBeEnabled()
+	})
+
+	test('restores the page selection of an already published website without deleted and new pages', async ({ collective, navigation, page, user }) => {
+		const publishedPage = await collective.createPage({ title: 'Published page', user, page })
+		const rootPageId = collective.getRootPage().data.id
+		const deletedPageId = 999999
+		const staticSite = {
+			id: 1,
+			title: 'Published website',
+			slug: 'published-website',
+			selectedPageIds: [rootPageId, publishedPage.data.id, deletedPageId],
+			publishedUrl: 'https://example.org/published-website',
+			status: 'published',
+		}
+		let sentPageIds: number[] = []
+		await page.route('**/static-sites', async (route) => {
+			const request = route.request()
+			if (request.method() === 'POST') {
+				// The publish service isn't part of this environment, only the sent page selection matters
+				sentPageIds = request.postDataJSON().pageIds
+				return route.fulfill({ json: { ocs: { meta: { status: 'ok', statuscode: 200, message: 'OK' }, data: staticSite } } })
+			}
+			if (request.method() !== 'GET') {
+				return route.fallback()
+			}
+			await route.fulfill({ json: { ocs: { meta: { status: 'ok', statuscode: 200, message: 'OK' }, data: [staticSite] } } })
+		})
+
+		await collective.openCollective()
+		await collective.createPage({ title: 'Page created after publication', user, page })
+
+		await navigation.clickCollectiveMenu(collective.data.name, 'Publish website')
+		const modal = page.getByRole('dialog')
+		await expect(modal.getByRole('textbox', { name: 'Website title' })).toHaveValue(staticSite.title)
+		await expect(modal.getByRole('textbox', { name: 'Web address' })).toHaveValue(staticSite.slug)
+		await expect(modal.getByRole('checkbox', { name: 'Published page' })).toBeChecked()
+		await expect(modal.getByRole('checkbox', { name: 'Page created after publication' })).not.toBeChecked()
+
+		await modal.getByRole('button', { name: 'Publish website' }).click()
+		await expect(modal).toHaveCount(0)
+		expect(sentPageIds).toHaveLength(2)
+		expect(sentPageIds).toEqual(expect.arrayContaining([rootPageId, publishedPage.data.id]))
+	})
+
 	test('keeps the modal open if the website files could not be provided', async ({ collective, navigation, page }) => {
 		// A failed archive build can't be provoked reliably, so the response is replaced
 		await page.route('**/static-sites', async (route) => {
@@ -132,7 +185,7 @@ test.describe('Collective publish', () => {
 			if (route.request().method() !== 'GET') {
 				return route.fallback()
 			}
-			await route.fulfill({ json: { ocs: { meta: { status: 'ok', statuscode: 200, message: 'OK' }, data: [{ id: 1, publishedUrl }] } } })
+			await route.fulfill({ json: { ocs: { meta: { status: 'ok', statuscode: 200, message: 'OK' }, data: [{ id: 1, title: 'Published website', slug: 'published-website', selectedPageIds: [], publishedUrl }] } } })
 		})
 
 		await collective.openCollective()

src/CollectivesApp.vue

@@ -162,9 +162,7 @@ export default {
 
 		loadAdminSettings() {
 			try {
-				const publishEnabledState = loadState('collectives', 'publish_enabled', true)
-				const isPublishEnabled = publishEnabledState === true || publishEnabledState === 'true'
-				this.setPublishFeatureEnabled(isPublishEnabled)
+				this.setPublishFeatureEnabled(loadState('collectives', 'publish_enabled', false))
 			} catch (e) {
 				console.error('Failed to load admin settings:', e)
 			}

src/components/Nav/CollectivePublishModal.vue

@@ -30,7 +30,8 @@
 					:helperText="t('collectives', 'Lowercase letters, numbers and single hyphens')"
 					:maxlength="slugMaxLength" />
 			</div>
-			<ul class="modal-publish__tree">
+			<NcLoadingIcon v-if="loadingPages" class="modal-publish__loading" :size="32" />
+			<ul v-else class="modal-publish__tree">
 				<li v-if="rootPage" class="modal-publish__collective-row">
 					<PublishTreeRow
 						:selected="isRootPageSelected"
@@ -57,7 +58,7 @@
 				<NcButton
 					variant="primary"
 					:loading="publishing"
-					:disabled="publishing || !isTitleValid || !isSlugValid"
+					:disabled="loadingPages || publishing || !isTitleValid || !isSlugValid"
 					@click="onPublishAsWebsite">
 					{{ t('collectives', 'Publish website') }}
 				</NcButton>
@@ -71,6 +72,7 @@ import { showError, showSuccess } from '@nextcloud/dialogs'
 import { t } from '@nextcloud/l10n'
 import { mapActions, mapState } from 'pinia'
 import NcButton from '@nextcloud/vue/components/NcButton'
+import NcLoadingIcon from '@nextcloud/vue/components/NcLoadingIcon'
 import NcModal from '@nextcloud/vue/components/NcModal'
 import NcNoteCard from '@nextcloud/vue/components/NcNoteCard'
 import NcTextField from '@nextcloud/vue/components/NcTextField'
@@ -90,6 +92,7 @@ export default {
 
 	components: {
 		NcButton,
+		NcLoadingIcon,
 		NcModal,
 		NcNoteCard,
 		NcTextField,
@@ -113,6 +116,7 @@ export default {
 		return {
 			selectedPageIds: new Set(),
 			expandedPageIds: new Set(),
+			loadingPages: true,
 			publishing: false,
 			title: this.collective.name,
 			slug: generateSlug(this.collective.name),
@@ -163,24 +167,33 @@ export default {
 
 	},
 
-	created() {
-		// Preselect the whole collective (all pages) when the modal is opened
-		this.selectedPageIds = new Set(this.allPageIds)
-		this.getStaticSites(this.collective.id)
-			.then(() => {
-				// Prefill with the settings of an already published static site, if any
-				const staticSite = this.staticSiteForCollective(this.collective.id)
-				if (staticSite) {
-					this.title = staticSite.title
-					this.slug = staticSite.slug
-					this.selectedPageIds = new Set(staticSite.selectedPageIds)
-				}
-			})
-			.catch(displayError('Could not load static sites for collective'))
+	async created() {
+		// The cached page list may be outdated or not loaded yet
+		await Promise.all([
+			this.getPagesForCollective(this.collective, false)
+				.catch(displayError('Could not fetch collective pages')),
+			this.getStaticSites(this.collective.id)
+				.catch(displayError('Could not load static sites for collective')),
+		])
+
+		// Prefill with the settings of an already published static site, if any,
+		// otherwise preselect the whole collective (all pages)
+		const staticSite = this.staticSiteForCollective(this.collective.id)
+		if (staticSite) {
+			this.title = staticSite.title
+			this.slug = staticSite.slug
+			// Pages may have been deleted since the last publication
+			const pageIds = new Set(this.allPageIds)
+			this.selectedPageIds = new Set(staticSite.selectedPageIds.filter((id) => pageIds.has(id)))
+		} else {
+			this.selectedPageIds = new Set(this.allPageIds)
+		}
+		this.loadingPages = false
 	},
 
 	methods: {
 		t,
+		...mapActions(usePagesStore, ['getPagesForCollective']),
 
 		...mapActions(useStaticSitesStore, ['getStaticSites', 'watchPublication']),
 
@@ -291,6 +304,10 @@ export default {
 		padding-block-end: 8px;
 	}
 
+	&__loading {
+		flex: 1 1 auto;
+	}
+
 	&__tree {
 		flex: 1 1 auto;
 		margin: 0;

src/views/AdminSettings.vue

@@ -19,7 +19,7 @@
 	</NcSettingsSection>
 	<NcSettingsSection
 		:name="t('collectives', 'Publish feature')"
-		:description="t('collectives', 'Enable or disable the publish feature for collectives')">
+		:description="t('collectives', 'Enable or disable the publish feature for collectives. Disabling it prevents new publications, already published websites are not removed.')">
 		<div class="publish-settings">
 			<NcCheckboxRadioSwitch
 				id="isPublishFeatureEnabled"

tests/Integration/features/staticSites.feature

@@ -4,8 +4,13 @@
 Feature: staticSites
 
   # TODO(Publish): Enable the commented steps and scenarios once the CI provides a stub of the publish service, publishing fails without it
-  Scenario: Publish a collective as website
+  Scenario: Fail to publish a website if publish website feature is disabled
     When user "jane" creates collective "BehatStaticSiteCollective"
+    And occ command "config:app:set collectives publish_enabled --value false --type boolean" is run
+    Then user "jane" fails to publish collective "BehatStaticSiteCollective" as website with slug "behat-static-site" with status 403
+
+  Scenario: Publish a collective as website
+    When occ command "config:app:set collectives publish_enabled --value true --type boolean" is run
     # And user "jane" publishes collective "BehatStaticSiteCollective" as website with slug "behat-static-site"
     # Then user "jane" sees website "behat-static-site" in collective "BehatStaticSiteCollective"
     # TODO(Publish): Download the archive and check its content once it's provided via public API
@@ -24,4 +29,5 @@ Feature: staticSites
   #   Then user "jane" doesn't see website "behat-static-site" in collective "BehatStaticSiteCollective"
 
   Scenario: Trash and delete collective
+    When occ command "config:app:delete collectives publish_enabled" is run
     Then user "jane" trashes and deletes collective "BehatStaticSiteCollective"