search

dominikbraun / timetrace

A simple CLI for tracking your working time.
Apache License 2.0
668 stars 75 forks source link

`timetrace report` displays deleted records #161

Closed jwnpoh closed 2 years ago

jwnpoh commented 3 years ago

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.
dominikbraun commented 3 years ago

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 commented 3 years ago

@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 commented 3 years ago

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 commented 3 years ago

@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 commented 3 years ago

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 commented 3 years ago

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 commented 2 years ago

Should this issue be closed now, @dominikbraun?

dominikbraun commented 2 years ago

@jwnpoh Yes, thanks for noticing.