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

[fix/shortcut-save-file] #624

Merged
merged 2 commits into from
Feb 26, 2020
Merged

[fix/shortcut-save-file] #624

merged 2 commits into from
Feb 26, 2020

Conversation

hosy
Copy link
Collaborator

@hosy hosy commented Feb 21, 2020

Description

File could to be saved, when trying to save a file with Shortcuts and it already exists

Related Issue

#622

How Has This Been Tested?

Check Issue #622

Screenshots (if appropriate):

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have read the CONTRIBUTING document.
  • I have added tests to cover my changes.
  • All new and existing tests passed.

@hosy hosy requested a review from mneuwert February 21, 2020 08:08
@hosy hosy self-assigned this Feb 21, 2020
@hosy hosy added this to the 1.3.1 milestone Feb 21, 2020
@hosy hosy changed the base branch from master to release/1.3.1 February 25, 2020 09:30
@jesmrec
Copy link
Contributor

jesmrec commented Feb 25, 2020

The fix directly overwrites the existing file in the same path. This behaviour can be dangerous and lead to data loss. Should it be better to create a new file with the incremental number (same as uploading twice the same file)?

@hosy
Copy link
Collaborator Author

hosy commented Feb 25, 2020

@jesmrec yes, this is exactly what it should do, because as I user I want to write data to the same file.
If you do not agree, we can add a parameter Overwrite if already exists with YES/NO, to prevent overwriting, if it should not. What do you think?

@jesmrec
Copy link
Contributor

jesmrec commented Feb 25, 2020

It sounds good. So, the user can decide to overwrite the latest version or keep it by creating a new one.

@hosy
Copy link
Collaborator Author

hosy commented Feb 25, 2020

@jesmrec new parameter was added, please check again

@jesmrec
Copy link
Contributor

jesmrec commented Feb 26, 2020

Works properly when Overwrite existing file is enabled and disabled. And it is good to have that this is disabled by default, so the user has to decide explicitly if the file will be overwritten.

Approved.

@jesmrec jesmrec self-assigned this Feb 26, 2020
@jesmrec jesmrec added the Approved by QA Approved by QA label Feb 26, 2020
@hosy hosy requested a review from jesmrec February 26, 2020 15:37
@hosy hosy merged commit 7fb0668 into release/1.3.1 Feb 26, 2020
@delete-merged-branch delete-merged-branch bot deleted the fix/shortcut-save-file branch February 26, 2020 15:37
@jesmrec jesmrec added the Estimation - 2 (S) 2 points label Feb 28, 2020
@jesmrec jesmrec mentioned this pull request Mar 4, 2020
28 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants