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

Add Observation Deletion Functionality. #746

Open
wants to merge 2 commits into
base: dev
Choose a base branch
from

Conversation

davner
Copy link
Contributor

@davner davner commented Nov 16, 2023

  • Implement unified method for deleting observations and associated data products.
  • Resolve bug preventing deletion of thumbnails.
  • Introduce new template for observation deletion process.

This PR introduces a new feature for deleting observations and their associated data products, responding to user feedback within our project. It also addresses a oversight made by my previous PR that prevented the deletion of thumbnails for a dataproduct, ensuring a more comprehensive cleanup process.

A new template has been added to streamline the observation deletion process. Tests are added and confirm that observations and related data products are thoroughly deleted. Additionally, a deletion button has been integrated into the observation detail view for user convenience.

I believe these enhancements could benefit the TOMToolkit community. However, I understand and respect if this functionality does not align with current priorities.

Screenshot 2023-11-16 at 3 10 29 PM

- Implement unified method for deleting observations and associated data products.
- Resolve bug preventing deletion of thumbnails.
- Introduce new template for observation deletion process.
@davner davner force-pushed the enhancement/delete-observation branch from 28ffe10 to 4428d9b Compare November 16, 2023 23:37
@jchate6 jchate6 self-requested a review November 17, 2023 01:17
@dmcollom
Copy link
Contributor

I'm very far outside of the TOM Toolkit community at this point, so hopefully this isn't too out of the blue, but it seems to me that it might be more desirable to have a bulk data delete as a script, but for an individual delete to simply mark an observation as "deleted" with a deleted_at timestamp, and then hiding those from any views.

To me, the use cases for deletion are to remove visual clutter, and to improve performance (which feels more in line with data product deletion), but there may still be value in keeping a record of the observation, even if the data that came out of it wasn't of value.

Obviously up to the maintainers, just saw this come through and figured I'd offer my two cents.

@davner
Copy link
Contributor Author

davner commented Nov 17, 2023

I agree that a way to bulk delete data is important, and that's a request from our users as well. I will be implementing that soon. I see it being beneficial to offer both capabilities. The function I wrote that loops through an observation record could be used as the bulk deleter.

@jchate6 jchate6 requested a review from phycodurus November 17, 2023 19:04
@griffin-h
Copy link
Contributor

Sorry to jump in here, but we were just discussing a related issue in our group. Is there a way to cancel an observation through the TOM? I'm not aware of one, but it would be helpful. This would not delete the ObservationRequest, but rather send a signal to the observatory API to cancel the request, and set the ObservationRecord's status to 'CANCELED'. Let me know if I should open a separate issue on this.

@rachel3834
Copy link
Contributor

rachel3834 commented Dec 14, 2023 via email

@griffin-h
Copy link
Contributor

It may be a separate issue, since both functions would be very useful for a lot of TOM users.

Great, I made a separate issue, #786.

Copy link
Contributor

@jchate6 jchate6 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure I understand everything you are going for here.
Specifically, the code in tom_common.utils seems strange to me.

It seems like all of this work should be done in the Model?
It's not clear to me why adding the data_product.data.delete() and the data_product.thumbnail.delete() to an extended DataProduct.delete() in the model wouldn't accomplish all of this through cascades.

Is there something I'm missing?

@jchate6 jchate6 added the User Issue Raised by a user label Feb 26, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
User Issue Raised by a user
Projects
Status: Needs Review
Development

Successfully merging this pull request may close these issues.

5 participants