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

Added "Continue" link to error page (#40909) #40910

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

mijarena
Copy link

@mijarena mijarena commented Aug 7, 2023

Description

Related Issue

Motivation and Context

User can quickly return to the starting page when encountering an error.

How Has This Been Tested?

  • test environment:
  • test case 1:
  • test case 2:
  • ...

Screenshots (if appropriate):

image

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Database schema changes (next release will require increase of minor version instead of patch)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Technical debt
  • Tests only (no source changes)

Checklist:

  • Code changes
  • Unit tests added
  • Acceptance tests added
  • Documentation ticket raised:
  • Changelog item, see TEMPLATE

User can quickly return to the starting page when encountering an error.
@update-docs
Copy link

update-docs bot commented Aug 7, 2023

Thanks for opening this pull request! The maintainers of this repository would appreciate it if you would create a changelog item based on your changes.

@CLAassistant
Copy link

CLAassistant commented Aug 7, 2023

CLA assistant check
All committers have signed the CLA.

Copy link
Contributor

@jnweiger jnweiger left a comment

Choose a reason for hiding this comment

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

href="/" is not always correct.

There are two config variables that need to be taken into account:

/**
 * Define the Web base URL
 * This key is necessary for the navigation item to the new ownCloud Web UI and for redirecting
 * public and private links.
 */
'web.baseUrl' => '',
/**
 * Define clean URLs without `/index.php`
 * This parameter will be written as `RewriteBase` on update and installation of
 * ownCloud to your `.htaccess` file. While this value is often simply the URL
 * path of the ownCloud installation it cannot be set automatically properly in
 * every scenario and needs thus some manual configuration.
 *
 * In a standard Apache setup this usually equals the folder that ownCloud is
 * accessible at. So if ownCloud is accessible via `https://mycloud.org/owncloud`
 * the correct value would most likely be `/owncloud`. If ownCloud is running
 * under `https://mycloud.org/` then it would be `/`.
 *
 * Note that the above rule is not valid in every case, as there are some rare setup
 * cases where this may not apply. However, to avoid any update problems this
 * configuration value is explicitly opt-in.
 *
 * After setting this value run `{occ-command-example-prefix} maintenance:update:htaccess`. Now, when the
 * following conditions are met ownCloud URLs won't contain `index.php`:
 *
 * - `mod_rewrite` is installed
 * - `mod_env` is installed
 */
'htaccess.RewriteBase' => '/',

ownCloud may also be installed like this:

https://oc10130a1-20230807.jw-qa.owncloud.works/mammamia/owncloud/login
config.php: 'htaccess.RewriteBase' => '/mammamia/owncloud',

https://oc1012-index-20230807.jw-qa.owncloud.works/index.php
No RewriteBase in config.php

https://oc10122-both-20230807.jw-qa.owncloud.works/big-bang/index.php
apache.conf: Alias /big-bang "/var/www/owncloud/" without a RewriteBase in config.php

https://oc10130a1-web-app-20230807.jw-qa.owncloud.works/index.php
... and what happens with the new web UI? (Optional, but it should not break...)

@mijarena mijarena requested review from jnweiger and IljaN August 7, 2023 14:07
@sonarcloud
Copy link

sonarcloud bot commented Aug 7, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

100.0% 100.0% Coverage
0.0% 0.0% Duplication

Now not only at the error page (template error.php) but also at the 403 page (template 403.php)

IL10N is not working for "Continue", but as part of the message is in english anyway, this is a fix for later.
@mijarena
Copy link
Author

Fixed on 403 page, too

Copy link
Contributor

@jnweiger jnweiger left a comment

Choose a reason for hiding this comment

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

Work in all my test cases.

  • admin permission, wrong file id
  • index.php less and other
  • / and subfolder install.

@jnweiger
Copy link
Contributor

jnweiger commented Aug 11, 2023

Sidenote: The string "Logged in user must be an admin" seems to be untranslated.
Can we make that eligible for translation too?

Copy link
Member

@IljaN IljaN left a comment

Choose a reason for hiding this comment

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

@mijarena
Copy link
Author

I made this to be working correctly So it could be an accepted feature for the next release. I planed to polish code, style and translation next during beta.

Copy link
Member

@jvillafanez jvillafanez left a comment

Choose a reason for hiding this comment

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

Not sure if it's easy to add some basic unit tests.

Comment on lines +152 to +153
$template->assign('continue_text', "Continue");
$template->assign('base_url', \OC::$server->getURLGenerator()->getAbsoluteURL("/"));
Copy link
Member

Choose a reason for hiding this comment

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

This shouldn't be needed. Variable assignment for the template should be already done in the foreach loop above.

'error',
[
"errors" => [$param],
"base_url" => \OC::$server->getURLGenerator()->getAbsoluteURL("/"),
Copy link
Member

Choose a reason for hiding this comment

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

Is it possible to inject the URLGenerator?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants