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

timetrace report displays deleted records #161

Closed
jwnpoh opened this issue Jul 7, 2021 · 8 comments
Closed

timetrace report displays deleted records #161

jwnpoh opened this issue Jul 7, 2021 · 8 comments
Labels
bug Something isn't working help wanted Extra attention is needed

Comments

@jwnpoh
Copy link
Contributor

jwnpoh commented Jul 7, 2021

Hello! I have observed that timetrace report currently lists records that have been deleted. Is this the intended behaviour? If it is not, then I would like to propose two fixes:

  1. delete project should also delete records with the associated project key.
  2. timetrace report should ignore record files that have the .bak suffix.
@jwnpoh jwnpoh changed the title Update delete project to also delete records 'timetrace report' displays deleted records Jul 7, 2021
@jwnpoh jwnpoh changed the title 'timetrace report' displays deleted records timetrace report displays deleted records Jul 7, 2021
@dominikbraun dominikbraun added bug Something isn't working help wanted Extra attention is needed labels Jul 7, 2021
@dominikbraun
Copy link
Owner

timetrace report should ignore record files that have the .bak suffix.

Actually, this should already be the case... 🤔 @KonstantinGasser Do you know something about this?

@KonstantinGasser
Copy link
Contributor

@dominikbraun the filter functions actually do not account for .bak files, since they where added in a later stage if I am correct. But fixing this should not be an issue and I will prepare a PR later tonight to fix the bug.

@dominikbraun
Copy link
Owner

dominikbraun commented Jul 7, 2021

Ok, so what about @jwnpoh implementing suggestion 1(*) and @KonstantinGasser implementing suggestion 2?

(*) In my opinion, deleting all records of a project when deleting the project should be opt-in, because this is a pretty dangerous operation. Maybe we could add a --delete-records flag to the delete project command that requires an additional confirmation.

@KonstantinGasser
Copy link
Contributor

@dominikbraun yes we can split that into two PRs, sounds good.

Regarding your second point - I agree that blindly deleting all records is not a good UX, a user should explicitly tell that all records should be deleted.

@jwnpoh
Copy link
Contributor Author

jwnpoh commented Jul 8, 2021

Maybe we could add a --delete-records flag to the delete project command that requires an additional confirmation.

Should the user input be in the form of a flag or a confirmation/option prompt? Or maybe both.

Printing a prompt if there is no `--delete-records' flag would take care of the possible user who may not be aware that records are not deleted by default, or that records are decoupled from projects. What do you think?

@dominikbraun
Copy link
Owner

Maybe we could add a --delete-records flag to the delete project command that requires an additional confirmation.

Should the user input be in the form of a flag or a confirmation/option prompt? Or maybe both.

I'd say both.

@jwnpoh
Copy link
Contributor Author

jwnpoh commented Aug 8, 2021

Should this issue be closed now, @dominikbraun?

@dominikbraun
Copy link
Owner

@jwnpoh Yes, thanks for noticing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working help wanted Extra attention is needed
Projects
None yet
Development

No branches or pull requests

3 participants