-
Notifications
You must be signed in to change notification settings - Fork 793
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
Properly fetch shares for files with spaces/other special characters in filenames #5884
Conversation
Signed-off-by: Claudio Cambra <[email protected]>
…eManager Signed-off-by: Claudio Cambra <[email protected]>
…tEncoding Signed-off-by: Claudio Cambra <[email protected]>
AppImage file: nextcloud-PR-5884-48f1dfaef5afbce2dba6fcf99e56e5895ae77d1c-x86_64.AppImage |
Signed-off-by: Claudio Cambra <[email protected]>
Kudos, SonarCloud Quality Gate passed! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@claucambra besides one small comment, I've tested and it seems there is some issue with the path:
newShare = parseLinkShare(data); | ||
} else if (Share::isShareTypeUserGroupEmailRoomOrRemote(static_cast <Share::ShareType>(shareType))) { | ||
newShare = parseUserGroupShare(data); | ||
shares.append(parseLinkShare(data)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@claucambra I don't see a good reason for this refactoring, besides removing space after static_cast
you are basically doing the same but introducing more code for review
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
while fixing the issue @claucambra would it be possible to write a test for the new method you're adding?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The main reason for this change was to eliminate the likelihood of appending a null newShare
to shares
, and making the code a little shorter -- it doesn't seem like we need the newShare
here.
As for the space I think this was auto-done by clang-format but I can re-add
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking into the other points now
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@claucambra No, I am fine with removing the space. I can see your point about newShare
being not null
, but there is an else statement so it should never be null, that said, you can just add a check if (newShare)
prior to adding it if that makes sense
Ensure percent encoding is used. PR also includes some other minor fixes