openpsychotherapy / behavioral-activation

MIT License
1 stars 0 forks source link

Adds ability to remove activity. #82

Closed Neathan closed 3 years ago

Neathan commented 3 years ago

Comment: Adds the ability to remove activities. The plan was to also be able to modify the activity but then the issue of activities not being a single object made things harder.

Notes: The remove function added to storage removes every instance for the given day that matches the entry. This should not be a problem but the instance could occur where the exact same activity is registered on the same day and would be removed if either of them would be removed. This could be solved by requiring that the first index of the collection of activities is specified to the remove function and thus limiting the search enough to determine what collection should be removed.

When it comes the ability to modify an activity this would requires a similar solution as suggested above. However, in my opinion a more robust solution would be to change how we save activities. If activities contained the start and end time of each activity that would be easier to manage, data wise. This is however a much larger issue and should be handled on its own.

Neathan commented 3 years ago

It looks good for the most part, but I do think it is unacceptable that all of the same entry will be removed. I added some comments on how it could be fixed. Implementing proper typing would also be a plus.

I agree that the functionality is not completed, this was meant to be a draft. My intention was to open this PR for input on how this problem would be managed. Since this feature was functionally simple at the surface but required rather substantial storage implementations quite a few new quirks came up with how we currently have structured the data storage. I'm currently working on adding the missing functionality and i think i've come up with a solution to how activities should be modified. This will however once again increase the scope of the PR but it shouldn't be too bad.

I understand how you think but as a user it is weird to come to a editpage and not be able to save the changes. If the user only should be able to remove the activity a dialog like values is a much better solution.

I agree, the currently implementation is a weird experience for the user. My intentions with this PR was to add missing functionality to the registrator, not to specifically add the functionality to remove an activity. This was meant to be a draft, a mistake on my part, since the scope of the feature grew when i realised that the storage module did not have the required functionality. The remaining functionality will be added before the PR closes.

Neathan commented 3 years ago

This should now also solve #35, #36 and #72.

Neathan commented 3 years ago

You should now be able to both move and delete existing activities. I've implemented the changes suggested by Erik which made the remove function much simler and allowed for specific removal of a give activity. A modify function was also added to the storage module which combines the 'remove-' and 'addInterval' functions to move/modify a given activity.