From 28bc4887ff7626190c2293fe307101132bfafa0f Mon Sep 17 00:00:00 2001 From: nfebe Date: Tue, 1 Apr 2025 19:55:56 +0100 Subject: [PATCH] refactor: Move pending actions to own component Moved the logic to create link shares out of the the `SharingEntryLink` component into `PendingActionsMixin.js` which is not used in the `SharingTab` and `SharingEntryLink` as well. --- .../src/components/SharingEntryLink.vue | 143 ++------------ apps/files_sharing/src/logger.ts | 11 -- ...ixin.ts => PendingActionsHandlersMixin.js} | 185 ++++++++++++++++-- .../src/views/SharingLinkList.vue | 67 ++----- apps/files_sharing/src/views/SharingTab.vue | 102 ++++++++-- 5 files changed, 298 insertions(+), 210 deletions(-) delete mode 100644 apps/files_sharing/src/logger.ts rename apps/files_sharing/src/mixins/{PendingActionsHandlersMixin.ts => PendingActionsHandlersMixin.js} (55%) diff --git a/apps/files_sharing/src/components/SharingEntryLink.vue b/apps/files_sharing/src/components/SharingEntryLink.vue index be53bd6d685..fb32bef395d 100644 --- a/apps/files_sharing/src/components/SharingEntryLink.vue +++ b/apps/files_sharing/src/components/SharingEntryLink.vue @@ -185,7 +185,8 @@ import PendingActions from './PendingActions.vue' import Share from '../models/Share.ts' import SharesMixin from '../mixins/SharesMixin.js' import ShareDetails from '../mixins/ShareDetails.js' -import logger from '../logger.ts' +import PendingActionsHandlersMixin from '../mixins/PendingActionsHandlersMixin.js' +import logger from '../services/logger.ts' export default { name: 'SharingEntryLink', @@ -210,7 +211,7 @@ export default { ShareExpiryTime, }, - mixins: [SharesMixin, ShareDetails], + mixins: [SharesMixin, ShareDetails, PendingActionsHandlersMixin], props: { canReshare: { @@ -312,135 +313,22 @@ export default { } return null }, - - passwordExpirationTime() { - if (this.share.passwordExpirationTime === null) { - return null - } - - const expirationTime = moment(this.share.passwordExpirationTime) - - if (expirationTime.diff(moment()) < 0) { - return false - } - - return expirationTime.fromNow() - }, - /** - * Is Talk enabled? + * Is the current share password protected ? * * @return {boolean} */ - isTalkEnabled() { - return OC.appswebroots.spreed !== undefined - }, - - /** - * Is it possible to protect the password by Talk? - * - * @return {boolean} - */ - isPasswordProtectedByTalkAvailable() { - return this.isPasswordProtected && this.isTalkEnabled - }, - - /** - * Is the current share password protected by Talk? - * - * @return {boolean} - */ - isPasswordProtectedByTalk: { + isPasswordProtected: { get() { - return this.share.sendPasswordByTalk + return this.config.enforcePasswordForPublicLink + || !!this.share.password }, async set(enabled) { - this.share.sendPasswordByTalk = enabled + // TODO: directly save after generation to make sure the share is always protected + Vue.set(this.share, 'password', enabled ? await GeneratePassword(true) : '') + Vue.set(this.share, 'newPassword', this.share.password) }, }, - - /** - * Is the current share an email share ? - * - * @return {boolean} - */ - isEmailShareType() { - return this.share - ? this.share.type === ShareType.Email - : false - }, - - canTogglePasswordProtectedByTalkAvailable() { - if (!this.isPasswordProtected) { - // Makes no sense - return false - } else if (this.isEmailShareType && !this.hasUnsavedPassword) { - // For email shares we need a new password in order to enable or - // disable - return false - } - - // Anything else should be fine - return true - }, - - /** - * Pending data. - * If the share still doesn't have an id, it is not synced - * Therefore this is still not valid and requires user input - * - * @return {boolean} - */ - pendingDataIsMissing() { - return this.pendingPassword || this.pendingEnforcedPassword || this.pendingDefaultExpirationDate || this.pendingEnforcedExpirationDate - }, - pendingPassword() { - return this.config.enableLinkPasswordByDefault && this.isPendingShare - }, - pendingEnforcedPassword() { - return this.config.enforcePasswordForPublicLink && this.isPendingShare - }, - pendingEnforcedExpirationDate() { - return this.config.isDefaultExpireDateEnforced && this.isPendingShare - }, - pendingDefaultExpirationDate() { - return (this.config.defaultExpirationDate instanceof Date || !isNaN(new Date(this.config.defaultExpirationDate).getTime())) && this.isPendingShare - }, - isPendingShare() { - return !!(this.share && !this.share.id) - }, - sharePolicyHasEnforcedProperties() { - return this.config.enforcePasswordForPublicLink || this.config.isDefaultExpireDateEnforced - }, - - enforcedPropertiesMissing() { - // Ensure share exist and the share policy has required properties - if (!this.sharePolicyHasEnforcedProperties) { - return false - } - - if (!this.share) { - // if no share, we can't tell if properties are missing or not so we assume properties are missing - return true - } - - // If share has ID, then this is an incoming link share created from the existing link share - // Hence assume required properties - if (this.share.id) { - return true - } - // Check if either password or expiration date is missing and enforced - const isPasswordMissing = this.config.enforcePasswordForPublicLink && !this.share.password - const isExpireDateMissing = this.config.isDefaultExpireDateEnforced && !this.share.expireDate - - return isPasswordMissing || isExpireDateMissing - }, - // if newPassword exists, but is empty, it means - // the user deleted the original password - hasUnsavedPassword() { - return this.share.newPassword !== undefined - }, - /** * Return the public share link * @@ -517,6 +405,17 @@ export default { }, methods: { + _handleBeforeAddShare(share, resolve) { + this._handleShareAdded(share, resolve) + }, + _handleShareAdded(share, resolve) { + this.$emit('add:share', share, resolve) + }, + + _handleShareUpdated(share, resolve) { + this.$emit('update:share', share, resolve) + }, + /** * Check if the share requires review * diff --git a/apps/files_sharing/src/logger.ts b/apps/files_sharing/src/logger.ts deleted file mode 100644 index 31490d814e8..00000000000 --- a/apps/files_sharing/src/logger.ts +++ /dev/null @@ -1,11 +0,0 @@ -/** - * SPDX-FileCopyrightText: 2020 Nextcloud GmbH and Nextcloud contributors - * SPDX-License-Identifier: AGPL-3.0-or-later - */ - -import { getLoggerBuilder } from '@nextcloud/logger' - -export default getLoggerBuilder() - .setApp('files_sharing') - .detectUser() - .build() \ No newline at end of file diff --git a/apps/files_sharing/src/mixins/PendingActionsHandlersMixin.ts b/apps/files_sharing/src/mixins/PendingActionsHandlersMixin.js similarity index 55% rename from apps/files_sharing/src/mixins/PendingActionsHandlersMixin.ts rename to apps/files_sharing/src/mixins/PendingActionsHandlersMixin.js index 99c16ca4005..66d715fc874 100644 --- a/apps/files_sharing/src/mixins/PendingActionsHandlersMixin.ts +++ b/apps/files_sharing/src/mixins/PendingActionsHandlersMixin.js @@ -3,37 +3,187 @@ * SPDX-License-Identifier: AGPL-3.0-or-later */ import { emit } from '@nextcloud/event-bus' +import moment from '@nextcloud/moment' import { ShareType } from '@nextcloud/sharing' -import GeneratePassword from '../utils/GeneratePassword' -import Share from '../models/Share' +import GeneratePassword from '../utils/GeneratePassword.ts' +import Share from '../models/Share.ts' import { showError, showSuccess } from '@nextcloud/dialogs' import { t } from '@nextcloud/l10n' -import SharesMixin from '../mixins/SharesMixin.js' +import SharesMixin from './SharesMixin.js' +import logger from '../services/logger.ts' +import Config from '../services/ConfigService.ts' +/** + * @mixin PendingActionsHandlersMixin + * + * This mixin provides the logic for handling the creation of new link shares, + * including showing a pending actions dialog and processing the share creation + * asynchronously. + * + * It follows a "template method" pattern. The main algorithm for creating a share + * is defined in `pushNewLinkShare`, but specific steps are delegated to the + * component that uses this mixin. + * + * IMPORTANT: Any component using this mixin MUST implement the following methods: + * + * - `_handleShareAdded(share, resolve)`: This method is called after a new share + * is successfully created. It is responsible for adding the new share to the + * component's state and updating the UI. + * + * - `_handleShareUpdated(share, resolve)`: This method is called when an existing + * share is updated (e.g., a new link share is created for a file that already + * had one). It should update the share in the component's state. + * + * The `resolve` function passed to both handlers MUST be called with the Vue + * component instance corresponding to the newly added/updated share. This instance + * is expected to have a `copyLink()` method, which will be called by `pushNewLinkShare` + * to copy the new share link to the clipboard. + */ export default { + data() { + return { + open: false, + config: new Config(), + shareCreationComplete: false, + pending: false, + loading: false, + defaultExpirationDateEnabled: false, + errors: {}, + logger, + } + }, computed: { + passwordExpirationTime() { + if (this.share.passwordExpirationTime === null) { + return null + } + + const expirationTime = moment(this.share.passwordExpirationTime) + + if (expirationTime.diff(moment()) < 0) { + return false + } + + return expirationTime.fromNow() + }, + /** - * Whether the share policy has enforced properties. + * Is Talk enabled? + * * @return {boolean} */ + isTalkEnabled() { + return OC.appswebroots.spreed !== undefined + }, + + /** + * Is it possible to protect the password by Talk? + * + * @return {boolean} + */ + isPasswordProtectedByTalkAvailable() { + return this.isPasswordProtected && this.isTalkEnabled + }, + + /** + * Is the current share password protected by Talk? + * + * @return {boolean} + */ + isPasswordProtectedByTalk: { + get() { + return this.share.sendPasswordByTalk + }, + async set(enabled) { + this.share.sendPasswordByTalk = enabled + }, + }, + + /** + * Is the current share an email share ? + * + * @return {boolean} + */ + isEmailShareType() { + return this.share + ? this.share.type === ShareType.Email + : false + }, + + canTogglePasswordProtectedByTalkAvailable() { + if (!this.isPasswordProtected) { + // Makes no sense + return false + } else if (this.isEmailShareType && !this.hasUnsavedPassword) { + // For email shares we need a new password in order to enable or + // disable + return false + } + + // Anything else should be fine + return true + }, + + /** + * Pending data. + * If the share still doesn't have an id, it is not synced + * Therefore this is still not valid and requires user input + * + * @return {boolean} + */ + pendingDataIsMissing() { + return this.pendingPassword || this.pendingEnforcedPassword || this.pendingDefaultExpirationDate || this.pendingEnforcedExpirationDate + }, + pendingPassword() { + return this.config.enableLinkPasswordByDefault && this.isPendingShare + }, + pendingEnforcedPassword() { + return this.config.enforcePasswordForPublicLink && this.isPendingShare + }, + pendingEnforcedExpirationDate() { + return this.config.isDefaultExpireDateEnforced && this.isPendingShare + }, + pendingDefaultExpirationDate() { + return (this.config.defaultExpirationDate instanceof Date || !isNaN(new Date(this.config.defaultExpirationDate).getTime())) && this.isPendingShare + }, + isPendingShare() { + return !!(this.share && !this.share.id) + }, sharePolicyHasEnforcedProperties() { return this.config.enforcePasswordForPublicLink || this.config.isDefaultExpireDateEnforced }, - /** - * Whether required properties are missing. - * @return {boolean} - */ enforcedPropertiesMissing() { - if (!this.sharePolicyHasEnforcedProperties) return false - if (!this.share) return true - if (this.share.id) return true + // Ensure share exist and the share policy has required properties + if (!this.sharePolicyHasEnforcedProperties) { + return false + } + if (!this.share) { + // if no share, we can't tell if properties are missing or not so we assume properties are missing + return true + } + + // If share has ID, then this is an incoming link share created from the existing link share + // Hence assume required properties + if (this.share.id) { + return true + } + // Check if either password or expiration date is missing and enforced const isPasswordMissing = this.config.enforcePasswordForPublicLink && !this.share.password const isExpireDateMissing = this.config.isDefaultExpireDateEnforced && !this.share.expireDate return isPasswordMissing || isExpireDateMissing }, + // if newPassword exists, but is empty, it means + // the user deleted the original password + hasUnsavedPassword() { + return this.share.newPassword !== undefined + }, + /** + * Whether the share policy has enforced properties. + * @return {boolean} + */ }, mixins: [SharesMixin], @@ -46,7 +196,6 @@ export default { * @return {boolean} */ shareRequiresReview(shareReviewComplete) { - // If a user clicks 'Create share' it means they have reviewed the share if (shareReviewComplete) { return false } @@ -58,6 +207,7 @@ export default { */ async onNewLinkShare(shareReviewComplete = false) { this.logger.debug('onNewLinkShare called (with this.share)', this.share) + this.logger.debug('onNewLinkShare shareReviewComplete', shareReviewComplete) if (this.loading) return const shareDefaults = { @@ -81,7 +231,7 @@ export default { const share = new Share(shareDefaults) const component = await new Promise((resolve) => { - this.$emit('add:share', share, resolve) + this._handleBeforeAddShare(share, resolve) }) this.open = false @@ -105,7 +255,6 @@ export default { return } } - const share = new Share(shareDefaults) await this.pushNewLinkShare(share) this.shareCreationComplete = true @@ -154,14 +303,14 @@ export default { let component if (update) { component = await new Promise(resolve => { - this.$emit('update:share', newShare, resolve) + this._handleShareUpdated(newShare, resolve) }) } else { // adding new share to the array and copying link to clipboard // using promise so that we can copy link in the same click function // and avoid firefox copy permissions issue component = await new Promise(resolve => { - this.$emit('add:share', newShare, resolve) + this._handleShareAdded(newShare, resolve) }) } @@ -183,7 +332,8 @@ export default { if (!message) { showError(t('files_sharing', 'Error while creating the share')) console.error(data) - return + // throw the original error to be caught by the caller + throw data } if (message.match(/password/i)) { @@ -234,6 +384,7 @@ export default { if (!this.shareCreationComplete) { this.$emit('remove:share', this.share) } + this.open = false }, }, } diff --git a/apps/files_sharing/src/views/SharingLinkList.vue b/apps/files_sharing/src/views/SharingLinkList.vue index 3dd6fdf317b..dd20033b33a 100644 --- a/apps/files_sharing/src/views/SharingLinkList.vue +++ b/apps/files_sharing/src/views/SharingLinkList.vue @@ -9,22 +9,26 @@ class="sharing-link-list"> + @add:share="(share, resolve) => $emit('add:share', share, resolve)" + @update:share="(share, resolve) => $emit('update:share', share, resolve)" + @open-sharing-details="openSharingDetails(share)" /> @@ -35,7 +39,6 @@ import { getCapabilities } from '@nextcloud/capabilities' import { t } from '@nextcloud/l10n' -import Share from '../models/Share.js' import SharingEntryLink from '../components/SharingEntryLink.vue' import ShareDetails from '../mixins/ShareDetails.js' import { ShareType } from '@nextcloud/sharing' @@ -69,6 +72,7 @@ export default { data() { return { canLinkShare: getCapabilities().files_sharing.public.enabled, + shareEntryRefs: {}, } }, @@ -94,48 +98,19 @@ export default { }, }, + beforeUpdate() { + // Clear refs before each update to ensure they are current + this.shareEntryRefs = {} + }, + methods: { t, - - /** - * Add a new share into the link shares list - * and return the newly created share component - * - * @param {Share} share the share to add to the array - * @param {Function} resolve a function to run after the share is added and its component initialized - */ - addShare(share, resolve) { - // eslint-disable-next-line vue/no-mutating-props - this.shares.push(share) - this.awaitForShare(share, resolve) - }, - - /** - * Await for next tick and render after the list updated - * Then resolve with the matched vue component of the - * provided share object - * - * @param {Share} share newly created share - * @param {Function} resolve a function to execute after - */ - awaitForShare(share, resolve) { - this.$nextTick(() => { - const newShare = this.$children.find(component => component.share === share) - if (newShare) { - resolve(newShare) - } - }) - }, - - /** - * Remove a share from the shares list - * - * @param {Share} share the share to remove - */ - removeShare(share) { - const index = this.shares.findIndex(item => item === share) - // eslint-disable-next-line vue/no-mutating-props - this.shares.splice(index, 1) + getShareEntryComponent(shareId) { + if (shareId) { + return this.shareEntryRefs[shareId] + } + // For the case when a new share is added and it's the first one (default entry was shown) + return this.$refs.defaultShareEntryRef }, }, } diff --git a/apps/files_sharing/src/views/SharingTab.vue b/apps/files_sharing/src/views/SharingTab.vue index d709084e83f..90057885372 100644 --- a/apps/files_sharing/src/views/SharingTab.vue +++ b/apps/files_sharing/src/views/SharingTab.vue @@ -16,7 +16,9 @@ class="sharingTab__content">