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.

Sign up for GitHub

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

Jump to bottom

Allow to trigger a backup sync using the socket api #8437

Merged
merged 3 commits into from
Mar 1, 2021
Merged

Allow to trigger a backup sync using the socket api #8437

merged 3 commits into from
Mar 1, 2021

Conversation

TheOneRing
Copy link
Contributor

@TheOneRing TheOneRing commented Feb 15, 2021 •
edited
Loading

Test app output:

-> V2/LIST_ACCOUNTS
<- V2/ACCOUNTS:{"accounts":[{"id":"0","name":"admin@localhost"},{"id":"1","name":"[email protected]"}]}

-> V2/UPLOAD_FILES_FROM:{"account":{"id":"0","name":"admin@localhost"},"localPath":" "C:/user/hanna","pattern":".*\\.stl","remotePath":".snapshots/19 Feb 2021 13:31:05 +0000"}

<- V2/UPLOAD_FILES_RESULT:{"error":"","localPath":"C:/Users/hanna","syncedFiles":["Desktop/untitled2.stl"]}

@TheOneRing TheOneRing requested a review from dragotin February 15, 2021 15:13
@TheOneRing TheOneRing force-pushed the work/backup branch 3 times, most recently from 325b15e to dcda87f Compare February 19, 2021 13:59
@TheOneRing TheOneRing mentioned this pull request Feb 19, 2021
Merged
@TheOneRing TheOneRing changed the title [WIP] Allow to trigger a sync of a single folder using the socket api Allow to trigger a backup sync using the socket api Feb 19, 2021
@TheOneRing TheOneRing requested a review from dschmidt February 19, 2021 15:06
@TheOneRing TheOneRing marked this pull request as ready for review February 19, 2021 15:06
@TheOneRing
Copy link
Contributor Author

Is camel case in json a bad idea?

dragotin
dragotin approved these changes Feb 24, 2021
View reviewed changes
Copy link
Contributor

@dragotin dragotin left a comment

Choose a reason for hiding this comment

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

Awesome work!

src/gui/socketapi/socketuploadjob.cpp
_remotePath = args[QLatin1String("remotePath")].toString();
_pattern = args[QLatin1String("pattern")].toString();
// TODO: use uuid
const auto accname = args[QLatin1String("account")][QLatin1String("name")].toString();
Copy link
Contributor

Choose a reason for hiding this comment

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

Nitpick: QLatin1String -> QStringLiteral ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Json has an overload and as the string is not actually used, besides the lookup, QLatin1String should be slightly better 🙈

Copy link
Contributor Author

Choose a reason for hiding this comment

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

https://code.woboq.org/qt5/qtbase/src/corelib/text/qstring.h.html#QLatin1Literal

dschmidt
dschmidt reviewed Feb 24, 2021
View reviewed changes
Copy link
Member

@dschmidt dschmidt left a comment

Choose a reason for hiding this comment

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

What happens, when the remote path is not empty?

Should we define what should happen and possibly write a test for that? 🙈

src/gui/socketapi/socketapi.cpp
@@ -866,8 +882,7 @@ void SocketApi::sendSharingContextMenuOptions(const FileData &fileData, SocketLi
// If sharing is globally disabled, do not show any sharing entries.
// If there is no permission to share for this file, add a disabled entry saying so
if (isOnTheServer && !record._remotePerm.isNull() && !record._remotePerm.hasPermission(RemotePermissions::CanReshare)) {
listener->sendMessage(QLatin1String("MENU_ITEM:DISABLED:d:") + (!record.isDirectory()
? tr("Resharing this file is not allowed") : tr("Resharing this folder is not allowed")));
listener->sendMessage(QLatin1String("MENU_ITEM:DISABLED:d:") + (!record.isDirectory() ? tr("Resharing this file is not allowed") : tr("Resharing this folder is not allowed")));
Copy link
Member

Choose a reason for hiding this comment

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

This looks like a "Verschlimmbesserung".
maybe break it up into two real lines instead of a single statement.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Clang format does that for some reason and as I moved the file it was touched...

Copy link
Member

Choose a reason for hiding this comment

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

Well then move (!record.isDirectory() ? tr("Resharing this file is not allowed") : tr("Resharing this folder is not allowed")) to a variable?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I didn't change that line I can also try to revert the format change....

TheOneRing
TheOneRing commented Feb 25, 2021
View reviewed changes
src/gui/socketapi/socketuploadjob.cpp Outdated
connect(mkdir, &OCC::MkColJob::finishedWithoutError, _engine, &OCC::SyncEngine::startSync);
connect(mkdir, &OCC::MkColJob::finishedWithError, this, [this](QNetworkReply *reply) {
if (reply->error() == 202) {
// folder does exist, we could also assert here if we want to enforce a backup over a sync
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I could fail if the dir already exist as we don't really know what will happen in that case...

Copy link
Member

Choose a reason for hiding this comment

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

We don't know?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well I expect unintended things, but I guess it will mainly depend on the state of the remote and the local folder...

@dragotin dragotin requested a review from dschmidt February 25, 2021 21:16
@sonarqubecloud
Copy link

sonarqubecloud bot commented Mar 1, 2021

Kudos, SonarCloud Quality Gate passed!

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

0.0% 0.0% Coverage
0.0% 0.0% Duplication

@TheOneRing TheOneRing merged commit 0ded3a5 into master Mar 1, 2021
@delete-merged-branch delete-merged-branch bot deleted the work/backup branch March 1, 2021 13:39
@gabi18 gabi18 mentioned this pull request Apr 29, 2021
Closed
43 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dschmidt dschmidt dschmidt approved these changes

@dragotin dragotin dragotin approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@TheOneRing @dschmidt @dragotin