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

Feature/server forbidden filenames #6965

Merged
merged 13 commits into from
Aug 21, 2024
Merged

Conversation

claucambra
Copy link
Collaborator

@claucambra claucambra commented Aug 5, 2024

Screenshot 2024-08-20 at 23 25 46

Screenshot 2024-08-20 at 23 27 16

@claucambra claucambra self-assigned this Aug 5, 2024
@AndyScherzinger AndyScherzinger added enhancement enhancement of a already implemented feature/code 🍂 2024-Autumn labels Aug 5, 2024
@claucambra claucambra force-pushed the feature/server-forbidden-filenames branch from 7d3b09b to 35180c7 Compare August 6, 2024 08:56
@tobiasKaminsky
Copy link
Member

From my limited c++ understanding, this means that you exclude those wrong files/folders from sync?
But we need to have an notification entry

  • stating that this file/folder is not allowed
  • with a valid error message
  • with an option to rename

@claucambra
Copy link
Collaborator Author

From my limited c++ understanding, this means that you exclude those wrong files/folders from sync? But we need to have an notification entry

  • stating that this file/folder is not allowed
  • with a valid error message
  • with an option to rename

We already have existing handling for this from other implementations of file blacklisting on the server. CSYNC_FILE_EXCLUDE_SERVER_BLACKLISTED exclusion types lead to: item->_errorString = tr("The filename is blacklisted on the server."); slightly further down this method

The result is a warning activity in the tray

@tobiasKaminsky
Copy link
Member

The result is a warning activity in the tray

If those capabilities are enabled, I think this should be an error, instead of a warning.
What do you think? :)

@claucambra
Copy link
Collaborator Author

The result is a warning activity in the tray

If those capabilities are enabled, I think this should be an error, instead of a warning. What do you think? :)

I don't have an issue with it, @mgallien would this have any ramifications?

@mgallien
Copy link
Collaborator

The result is a warning activity in the tray

If those capabilities are enabled, I think this should be an error, instead of a warning. What do you think? :)

I don't have an issue with it, @mgallien would this have any ramifications?

@claucambra I am testing this PR
having had a look at the changes, I fear that will be unusable by an user
how can they fix the warnings that this will generate for them ?
will they get an easily reachable way to rename and/or delete the unwanted files or folders ?

@mgallien
Copy link
Collaborator

@tobiasKaminsky @claucambra
I did test with 4 files having a forbidden file extension
I get this
image
if I click on the first line (the warning activity), nothing happen
are we sure we want to merge the current state ?

@claucambra
Copy link
Collaborator Author

The result is a warning activity in the tray

If those capabilities are enabled, I think this should be an error, instead of a warning. What do you think? :)

I don't have an issue with it, @mgallien would this have any ramifications?

@claucambra I am testing this PR having had a look at the changes, I fear that will be unusable by an user how can they fix the warnings that this will generate for them ? will they get an easily reachable way to rename and/or delete the unwanted files or folders ?

I will add a rename button to these activities, this way the warnings are actionable for the users

@tobiasKaminsky
Copy link
Member

This is way better.
Can you also add the specific error message by server, not the generic "file is blacklisted"?
And add a "rename" button, otherwise no one knows that they need to click on it to start rename dialog.

Same for the dialog: there we should also show the correct error message, as this is misleading right now:
image

--> com1 is a reserved filename.

@tobiasKaminsky
Copy link
Member

image

Rename button looks good ✔
Correct error message, and message in dialog pending : )

@claucambra
Copy link
Collaborator Author

Okay, will go ahead and modify the texts

@tobiasKaminsky
Copy link
Member

Okay, will go ahead and modify the texts

they need to be dynamic from server

@claucambra
Copy link
Collaborator Author

Okay, will go ahead and modify the texts

they need to be dynamic from server

@tobiasKaminsky @mgallien This is the current state:

Screenshot 2024-08-20 at 23 25 46

Screenshot 2024-08-20 at 23 27 16

The messages are dynamic depending on the forbidden characters and the reason(s) for the upload failure

@mgallien mgallien force-pushed the feature/server-forbidden-filenames branch from 83bb1ed to abb1f89 Compare August 21, 2024 07:51
Copy link

sonarcloud bot commented Aug 21, 2024

Quality Gate Failed Quality Gate failed

Failed conditions
2 Security Hotspots
50.6% Coverage on New Code (required ≥ 80%)
44 New Code Smells (required ≤ 0)
B Maintainability Rating on New Code (required ≥ A)

See analysis details on SonarCloud

Catch issues before they fail your Quality Gate with our IDE extension SonarLint

@nextcloud-desktop-bot
Copy link

AppImage file: nextcloud-PR-6965-abb1f89119bd25e887cf8a8b769423c07d0fbf95-x86_64.AppImage

To test this change/fix you can simply download above AppImage file and test it.

Please make sure to quit your existing Nextcloud app and backup your data.

@mgallien mgallien merged commit dfe9ada into master Aug 21, 2024
12 of 15 checks passed
@mgallien mgallien deleted the feature/server-forbidden-filenames branch August 21, 2024 09:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement enhancement of a already implemented feature/code hotspot: filename handling Filenames - invalid, portable, blacklisting, etc. 🍂 2024-Autumn
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants