Fix status check and saving of external storages #46859

danxuliu · 2024-07-30T01:17:27Z

This pull request fixes several issues with the status check in the external storage administration UI:

Not showing it automatically for user-global storages
Not removing it when the configuration of a storage is changed
Not updating the tooltip in some cases when the status is updated
Untranslated UI string

It also fixes some issues when saving an storage in the external storage administration UI:

Not being able to save several storages of the same type in a row
Saving __unmodified__ as the password if it was not changed, which messes the saved configuration

Please refer to the individual commit descriptions for further details.

How to test (scenario 1)

Install sshd in the system running the Nextcloud server (in Debian/Ubuntu, apt install openssh-server)
Add a user to the system (for example, useradd --create-home sftptest)
Start the sshd daemon (if systemctl is available, systemctl start sshd; otherwise, for example, in Docker, mkdir /run/sshd && /usr/sbin/sshd -D)
In Nextcloud, log in as an admin
Open the Administration settings
Open the external storage section
Add a new SFTP backend
Set authentication to Manually entered, store in database
Set host to 127.0.0.1
Enable the storage for all users
Save the storage
Open the external storage section under the Personal settings
Set the user and password for the storage
Save the storage
Reload the page

Result with this pull request

The status check is automatically shown for the storage. Moreover, if the storage is saved again without changes the status will succeed again

Result without this pull request

The status check is not automatically shown for the storage. Moreover, if the user saves the configuration again to trigger the status check (rather than just clicking on the status check) the status will fail (because the password is unmodified and obfuscated, and that obfuscated password is the one saved)

How to test (scenario 2)

Add a new external storage and fill it with the right configuration
Save it; the status check will appear with a successful result
Remove any of the fields, that should make the status check fail if triggered again

Result with this pull request

It is not possible to click on the status check (until the configuration is saved and the status check is shown again)

Result without this pull request

A click on the status check triggers a new check, which ends succeeding due to using the old configuration

How to test (scenario 3)

See steps in scenario 1 to setup SSH server
Add a new SFTP backend
Set host to 127.0.0.1
Set wrong credentials
Enable the storage for all users
Save the storage
Hover on the error status icon

Result with this pull request

Tooltip was updated with the error message (Exception: Login failed)

Result without this pull request

Tooltip is still the original message (Click to recheck the configuration)

How to test (scenario 4)

See steps in scenario 1 to setup SSH server
Add a new SFTP backend
Set host to 127.0.0.1
Set wrong credentials
Enable the storage for all users
Save the storage
Recheck the configuration by clicking on the status icon
Fix the credentials
Save the storage again
Hover on the success status icon

Result with this pull request

Tooltip was restored with the default message (Click to recheck the configuration)

Result without this pull request

Tooltip is still the error message set when rechecking it (Exception: Login failed)

How to test (scenario 5)

Add a new external storage and save it
Open the external storage settings in another tab and remove the storage in it
In the original tab, try to remove the storage

Result with this pull request

Tooltip was updated with the error message (Storage with ID "X" not found)

Result without this pull request

Tooltip is still the original message (Click to recheck the configuration)

How to test (scenario 6)

Add a new external storage and save it
Try to add another external storage of the same backend type

Result with this pull request

Add storage is selected in the dropdown, and selecting again the same backend type as before adds a new entry

Result without this pull request

The same backend type is still selected in the dropdown, and selecting it again does nothing

danxuliu · 2024-07-30T01:19:08Z

/backport to stable29

danxuliu · 2024-08-08T11:39:23Z

After a few rebases and job restarts CI is finally green 🎉 So I would recommend not to rebase again unless strictly needed ;-)

danxuliu · 2024-08-21T22:06:58Z

I removed the automatic backport for stable28, as it was going to have conflicts. I will backport it manually as needed.

Besides that, I added a little fix to keep the status width unmodified even if the internal span is removed (see scenario 2), and also added a fix to remove the tooltip from the default row used to select a backend and create a new mountpoint. As CI was going to run again anyway I also rebased everything on latest master.

danxuliu · 2024-08-21T22:08:26Z

/backport to stable30

Pytal · 2024-08-21T22:30:46Z

Adding @icewind1991 and @artonge as the files_external appowners :)

artonge

Looks good overall

When a storage is saved the status check can fail even if saving the storage succeeds. In those cases further details are provided in the status message of the storage, which is now set as the tooltip, similarly to how it is done when rechecking the storage. Signed-off-by: Daniel Calviño Sánchez <danxuliu@gmail.com>

When saving, updating and rechecking an storage fails (which is different to the soft-fail when the action itself succeeds but the status check does not) further details are provided in the error message of the response, which is now set as the tooltip. Signed-off-by: Daniel Calviño Sánchez <danxuliu@gmail.com>

If the status is updated but no explicit message is provided (for example, if the status check succeeded) the default tooltip (from the template) is now set to prevent a mismatch between the status and the tooltip (for example, if the configuration is fixed after a failed status check). Signed-off-by: Daniel Calviño Sánchez <danxuliu@gmail.com>

Signed-off-by: Daniel Calviño Sánchez <danxuliu@gmail.com>

As a new storage is added by selecting a backend the selected backend needs to be reset. Otherwise it is not possible to add another storage with the same backend. Signed-off-by: Daniel Calviño Sánchez <danxuliu@gmail.com>

As the external storage uses the Nextcloud server itself the number of workers of the PHP process running the Nextcloud server had to be increased. Otherwise if a request is sent for the external storage while handling a request from the integration tests a deadlock would occur. Signed-off-by: Daniel Calviño Sánchez <danxuliu@gmail.com>

When updating global storages and user storages a property is not updated by "StoragesService::updateStorage()" if the value matches the unmodified placeholder. However, userglobal storages are not updated through the "StoragesService"; as only the authentication mechanism is updated it is directly done with "saveBackendOptions()" in "IUserProvided" or "UserGlobalAuth". Due to this the unmodified placeholder value needs to be explicitly checked in those cases and replaced by the actual value (note that in this case it is not possible to just skip updating a specific property). Signed-off-by: Daniel Calviño Sánchez <danxuliu@gmail.com>

The row to add a new mount point is cloned when a new mountpoint is added, so it is expected that it includes a status span. However, it should not be displayed in that row, only in the cloned row when its status is updated. Signed-off-by: Daniel Calviño Sánchez <danxuliu@gmail.com>

skjnldsv · 2024-09-04T09:58:07Z

/backport to stable30

skjnldsv · 2024-09-04T10:03:37Z

/backport to stable30

skjnldsv · 2024-09-04T10:06:48Z

/backport to stable30

skjnldsv · 2024-09-04T10:11:21Z

/backport to stable30

skjnldsv · 2024-09-04T10:12:36Z

/backport to stable29

skjnldsv · 2024-09-04T10:12:39Z

/backport to stable28

danxuliu added bug 3. to review Waiting for reviews feature: external storage labels Jul 30, 2024

danxuliu added this to the Nextcloud 30 milestone Jul 30, 2024

backportbot bot added the backport-request label Jul 30, 2024

danxuliu force-pushed the fix-status-check-and-saving-of-external-storages branch from 76a8b73 to 2713e25 Compare July 30, 2024 02:08

danxuliu requested review from a team, Fenn-CS, Pytal and sorbaugh and removed request for a team July 30, 2024 03:49

This was referenced Jul 30, 2024

30.0.0 beta 2 #46870

Merged

30.0.0 beta 3 #46946

Merged

This was referenced Aug 5, 2024

30.0.0 beta 4 #47028

Merged

30.0.0 beta 5 #47100

Merged

danxuliu force-pushed the fix-status-check-and-saving-of-external-storages branch 2 times, most recently from b953fca to 7d8aa35 Compare August 7, 2024 18:35

danxuliu enabled auto-merge August 8, 2024 11:39

skjnldsv mentioned this pull request Aug 13, 2024

30.0.0 RC1 #47190

Merged

skjnldsv modified the milestones: Nextcloud 30, Nextcloud 31 Aug 14, 2024

danxuliu force-pushed the fix-status-check-and-saving-of-external-storages branch from 7d8aa35 to 2dca2d8 Compare August 21, 2024 22:04

Pytal requested review from icewind1991 and artonge August 21, 2024 22:30

artonge approved these changes Aug 22, 2024

View reviewed changes

danxuliu added 8 commits September 4, 2024 09:46

fix: Add missing translation for UI string

baaed7f

Signed-off-by: Daniel Calviño Sánchez <danxuliu@gmail.com>

fix: Reset selected backend when adding a new storage

8e5dba2

As a new storage is added by selecting a backend the selected backend needs to be reset. Otherwise it is not possible to add another storage with the same backend. Signed-off-by: Daniel Calviño Sánchez <danxuliu@gmail.com>

skjnldsv force-pushed the fix-status-check-and-saving-of-external-storages branch from 2dca2d8 to fa0862c Compare September 4, 2024 07:46

skjnldsv disabled auto-merge September 4, 2024 09:33

skjnldsv merged commit f1706df into master Sep 4, 2024
169 of 176 checks passed

skjnldsv deleted the fix-status-check-and-saving-of-external-storages branch September 4, 2024 09:33

This comment was marked as outdated.

Sign in to view

backportbot bot mentioned this pull request Sep 4, 2024

[stable30] Fix status check and saving of external storages #47733

Merged

2 tasks

backportbot bot removed the backport-request label Sep 4, 2024

backportbot bot added the backport-request label Sep 4, 2024

backportbot bot mentioned this pull request Sep 4, 2024

[stable29] Fix status check and saving of external storages #47734

Merged

2 tasks

backportbot bot removed the backport-request label Sep 4, 2024

backportbot bot mentioned this pull request Sep 4, 2024

[stable28] Fix status check and saving of external storages #47735

Merged

joshtrichards mentioned this pull request Sep 8, 2024

[Bug]: Adding multiple external storages with the same protocol is buggy [UI refinement] #43417

Closed

8 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix status check and saving of external storages #46859

Fix status check and saving of external storages #46859

danxuliu commented Jul 30, 2024 •

edited

Loading

danxuliu commented Jul 30, 2024

danxuliu commented Aug 8, 2024

danxuliu commented Aug 21, 2024

danxuliu commented Aug 21, 2024

Pytal commented Aug 21, 2024

artonge left a comment

This comment was marked as outdated.

This comment was marked as outdated.

skjnldsv commented Sep 4, 2024

This comment was marked as outdated.

skjnldsv commented Sep 4, 2024

This comment was marked as outdated.

skjnldsv commented Sep 4, 2024

This comment was marked as outdated.

skjnldsv commented Sep 4, 2024

skjnldsv commented Sep 4, 2024

skjnldsv commented Sep 4, 2024

Fix status check and saving of external storages #46859

Fix status check and saving of external storages #46859

Conversation

danxuliu commented Jul 30, 2024 • edited Loading

How to test (scenario 1)

Result with this pull request

Result without this pull request

How to test (scenario 2)

Result with this pull request

Result without this pull request

How to test (scenario 3)

Result with this pull request

Result without this pull request

How to test (scenario 4)

Result with this pull request

Result without this pull request

How to test (scenario 5)

Result with this pull request

Result without this pull request

How to test (scenario 6)

Result with this pull request

Result without this pull request

danxuliu commented Jul 30, 2024

danxuliu commented Aug 8, 2024

danxuliu commented Aug 21, 2024

danxuliu commented Aug 21, 2024

Pytal commented Aug 21, 2024

artonge left a comment

Choose a reason for hiding this comment

This comment was marked as outdated.

This comment was marked as outdated.

skjnldsv commented Sep 4, 2024

This comment was marked as outdated.

skjnldsv commented Sep 4, 2024

This comment was marked as outdated.

skjnldsv commented Sep 4, 2024

This comment was marked as outdated.

skjnldsv commented Sep 4, 2024

skjnldsv commented Sep 4, 2024

skjnldsv commented Sep 4, 2024

danxuliu commented Jul 30, 2024 •

edited

Loading