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

Improve UI success/error while adding storages #38288

Merged
merged 2 commits into from
Jan 13, 2021

Conversation

AlexAndBear
Copy link

@AlexAndBear AlexAndBear commented Jan 12, 2021

Description

  • While adding or changing an external storage, a success/error notification will be shown
  • Added cursor->pointer to the status item, due a click results in a recheck

Related Issue

Motivation and Context

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

@update-docs
Copy link

update-docs bot commented Jan 12, 2021

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.

Copy link
Contributor

@JammingBen JammingBen left a comment

Choose a reason for hiding this comment

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

Looks good to me

Copy link
Contributor

@phil-davis phil-davis left a comment

Choose a reason for hiding this comment

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

A typo in the changelog text. Sad to have to run all that CI again just for this.

changelog/unreleased/38288 Outdated Show resolved Hide resolved
Co-authored-by: Phil Davis <phil@jankaritech.com>
@AlexAndBear
Copy link
Author

Can be merged after tech demo

@sonarcloud
Copy link

sonarcloud bot commented Jan 13, 2021

Kudos, SonarCloud Quality Gate passed!

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

No Coverage information No Coverage information
No Duplication information No Duplication information

@jvillafanez
Copy link
Member

I see a potential problem. Let's assume the following setup:

  1. SMB / WND with login credentials (shouldn't matter whether they're saved in session or in DB)
  2. Both ownCloud and the storage are connected to the windows' AD. This means that only the AD users will be able to access to the external storage. In particular, the admin won't be able to access

Normally, the connectivity check is done using the admin account. If the admin cannot access to the external storage, we assume there is a problem with the setup. However, in the setup above, the problem is that there is no admin in the AD. Using a valid AD account instead of the ownCloud's admin would show a green light instead.

I'm a bit worried that the error message will make more noise even though the setup itself could be fine. A softer message could be better, such as "The current ownCloud's user (in case there are multiple admins) can't access to the external storage. In some scenarios, this could be ok, but please, recheck the configuration"

@AlexAndBear
Copy link
Author

@jvillafanez Edgy case but a good objection, maybe we could show this 'soften message' only if there is an error with the SMB/WND configuration?

@jvillafanez
Copy link
Member

Technically, it can happen with any storage. I guess the setup above is kind of expected for SMB/WND, so this flaw is more visible there.
For the case of SFTP, for example, the same could also happen, although it's likely easier to create an account for the admin inside the SFTP storage

@AlexAndBear
Copy link
Author

@pmaier1 @cdamken @hodyroff @micbar
Please take @jvillafanez comment into account for the tech demo meeting

@AlexAndBear
Copy link
Author

@jvillafanez as discussed with PM we will still show this message but enter a hint in the docs

@AlexAndBear AlexAndBear merged commit 9f54065 into master Jan 13, 2021
@delete-merged-branch delete-merged-branch bot deleted the enterprise/issues/4302 branch January 13, 2021 11:54
@mmattel
Copy link
Contributor

mmattel commented Jan 17, 2021

as discussed with PM we will still show this message but enter a hint in the docs

@janackermann Please create a docs issue, else this will be gone and lost

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.

5 participants