search

steroid-team / app

Helping you organise your day with ease and privacy.
1 stars 2 forks source link

Feature/done tasks deletion #261

Closed VPellenc closed 3 years ago

VPellenc commented 3 years ago

Adds a button to delete all done tasks in the ItemViewFragment.

The feature is working but the related UI test isn't. I tried to do something similar from removeTaskWorks() but I feel like something is missing. I'm not sure I have the good approach.

VPellenc commented 3 years ago

Thanks for the review!

maybe you should put the button in another location (maybe top right corner?) because I think it is easily possible to miss-click with the add task button.

I could, but I'd like to have more opinions on this. Also remember we might want to add some more buttons in the top right corner and then the miss-click would become an issue again. There could also be the idea of having the button appear only when there are tasks to be deleted, which I didn't do (at least yet) in this PR, and in this case having the button on the bottom right would make more sense. Also I just thought about it but the best solution probably would be to be able to undo the deletion just after the button is tapped, but I think it would be harder to implement so that would probably be for another PR.

I don't know but maybe it is better to keep only CRUD methods in the database and keep the other stuff in the view model. For example, you can keep the removeAllDoneTask in the view model but it only calls removeTask from the database, so we don't need to add a new method in the database. What do you think ?

I thought about it, but I have a problem with that. I believe the database only allows to do one action at a time so then this way would be ping-ponging the database and the overall operation would take a long time to complete, if I understand well. Currently as there is no local cache the latency would be obvious to the eye, but even after the local cache is in I still think it is best to keep network operations to a minimum in latency.

theo-damiani commented 3 years ago

Thanks for the review!

maybe you should put the button in another location (maybe top right corner?) because I think it is easily possible to miss-click with the add task button.

I could, but I'd like to have more opinions on this. Also remember we might want to add some more buttons in the top right corner and then the miss-click would become an issue again. There could also be the idea of having the button appear only when there are tasks to be deleted, which I didn't do (at least yet) in this PR, and in this case having the button on the bottom right would make more sense. Also I just thought about it but the best solution probably would be to be able to undo the deletion just after the button is tapped, but I think it would be harder to implement so that would probably be for another PR.

Yes, don't worry it was just a small detail!

I don't know but maybe it is better to keep only CRUD methods in the database and keep the other stuff in the view model. For example, you can keep the removeAllDoneTask in the view model but it only calls removeTask from the database, so we don't need to add a new method in the database. What do you think ?

I thought about it, but I have a problem with that. I believe the database only allows to do one action at a time so then this way would be ping-ponging the database and the overall operation would take a long time to complete, if I understand well. Currently as there is no local cache the latency would be obvious to the eye, but even after the local cache is in I still think it is best to keep network operations to a minimum in latency.

Hooo okay, you are definitely right, it's better to keep a low latency!

sydneyhauke commented 3 years ago

Although there might be some missing method mocking in your 'removeDoneTasksWorks' test. Have you mocked every FirebaseDatabase method that could be called ?