Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(files_sharing): show proper share not found error message #46967

Merged
merged 1 commit into from
Aug 6, 2024

Conversation

skjnldsv
Copy link
Member

@skjnldsv skjnldsv commented Aug 1, 2024

When trying to access a share token which doesn't exists or is expired
https://dev.domain.com/s/oiqAxS8tzdPNBzN

Before After
2024-08-01_22-27 image

Not the best, but a bit cleaner as far as error page goes

@nextcloud/designers

@skjnldsv skjnldsv added this to the Nextcloud 30 milestone Aug 1, 2024
@skjnldsv skjnldsv requested review from artonge and a team August 1, 2024 20:32
@skjnldsv skjnldsv self-assigned this Aug 1, 2024
@skjnldsv skjnldsv requested review from nfebe, szaimen and susnux and removed request for a team August 1, 2024 20:32
Copy link
Contributor

@nfebe nfebe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🐘

apps/files_sharing/templates/sharenotfound.php Outdated Show resolved Hide resolved
Copy link
Contributor

@szaimen szaimen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks nice from the screenshots :)

@skjnldsv skjnldsv force-pushed the fix/share-not-found branch 2 times, most recently from 1799993 to a76aa8c Compare August 2, 2024 14:56
@skjnldsv skjnldsv added 4. to release Ready to be released and/or waiting for tests to finish and removed 3. to review Waiting for reviews labels Aug 2, 2024
@susnux
Copy link
Contributor

susnux commented Aug 2, 2024

We can also consider something like this:

diff --git a/core/templates/404.php b/core/templates/404.php
index d2dc10f9aa8..1f58da3ef9d 100644
--- a/core/templates/404.php
+++ b/core/templates/404.php
@@ -21,9 +21,13 @@ if (!isset($_)) {//standalone  page is not supported anymore - redirect to /
 	<?php print_unescaped($_['content']) ?>
 <?php else: ?>
 	<div class="body-login-container update">
-		<div class="icon-big icon-search"></div>
-		<h2><?php p($l->t('Page not found')); ?></h2>
-		<p class="infogroup"><?php p($l->t('The page could not be found on the server or you may not be allowed to view it.')); ?></p>
+		<div class="icon-big <?php p($_['iconClass'] ?? 'icon-search'); ?>"></div>
+		<h2>
+			<?php p($_['heading'] ?? $l->t('Page not found')); ?>
+		</h2>
+		<p class="infogroup">
+			<?php p($_['message'] ?? $l->t('The page could not be found on the server or you may not be allowed to view it.')); ?>
+		</p>
 		<p><a class="button primary" href="<?php p(\OC::$server->getURLGenerator()->linkTo('', 'index.php')) ?>">
 			<?php p($l->t('Back to %s', [$theme->getName()])); ?>
 		</a></p>
diff --git a/lib/public/AppFramework/Http/NotFoundResponse.php b/lib/public/AppFramework/Http/NotFoundResponse.php
index 9ebefe69be1..fa936a82976 100644
--- a/lib/public/AppFramework/Http/NotFoundResponse.php
+++ b/lib/public/AppFramework/Http/NotFoundResponse.php
@@ -12,18 +12,17 @@ use OCP\AppFramework\Http;
 /**
  * A generic 404 response showing an 404 error page as well to the end-user
  * @since 8.1.0
- * @template S of int
  * @template H of array<string, mixed>
- * @template-extends TemplateResponse<int, array<string, mixed>>
+ * @template-extends TemplateResponse<Http::STATUS_NOT_FOUND, array<string, mixed>>
  */
 class NotFoundResponse extends TemplateResponse {
 	/**
-	 * @param S $status
+	 * @param array{message?: string, headline?: string, iconClass?: string} $params
 	 * @param H $headers
 	 * @since 8.1.0
 	 */
-	public function __construct(int $status = Http::STATUS_NOT_FOUND, array $headers = []) {
-		parent::__construct('core', '404', [], 'guest', $status, $headers);
+	public function __construct(array $params = [], array $headers = []) {
+		parent::__construct('core', '404', $params, 'guest', Http::STATUS_NOT_FOUND, $headers);
 
 		$this->setContentSecurityPolicy(new ContentSecurityPolicy());
 	}

Meaning we could make the response customizable (heading, message and icon).

@skjnldsv
Copy link
Member Author

skjnldsv commented Aug 2, 2024

We can also consider something like this:

Meaning we could make the response customizable (heading, message and icon).

I also started doing this. But wanted to avoid changing any existing code during freeze 🧊

@nimishavijay
Copy link
Member

Nice! Indeed this is better :)

I would go for this person-cancel symbol instead.

Also, the subline is missing a full stop at the end :)

Copy link
Member

@AndyScherzinger AndyScherzinger left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 💙

Signed-off-by: skjnldsv <skjnldsv@protonmail.com>
@skjnldsv skjnldsv merged commit 42650f6 into master Aug 6, 2024
169 checks passed
@skjnldsv skjnldsv deleted the fix/share-not-found branch August 6, 2024 15:45
@Altahrim Altahrim mentioned this pull request Aug 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
4. to release Ready to be released and/or waiting for tests to finish bug feature: sharing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants