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

# use endpoint for deleting specific simulation #1762

Merged
merged 3 commits into from
Oct 9, 2024

Conversation

matuszsmig
Copy link
Contributor

@matuszsmig matuszsmig commented Oct 3, 2024

Related PR on backend yaptide/yaptide#1077

@grzanka
Copy link
Contributor

grzanka commented Oct 3, 2024

I haven't tested this myself, but I am curious, which type of simulations can be cancelled this way: submitted to celery ("direct") or submitted to HPC cluster ("batch") ?

@grzanka
Copy link
Contributor

grzanka commented Oct 4, 2024

@matuszsmig how that should work in fact ?

Is it the small X that should delete the simulation, or something else in the UI ?
image

If I click on X it displays popup "Delete local data" which doesn't make much sense now.

@grzanka
Copy link
Contributor

grzanka commented Oct 4, 2024

Clicking Cancel should not delete the simulation ! The popup text should be improved

@matuszsmig
Copy link
Contributor Author

I separated delete callback to two different callback (cancel and delete) and changed tooltip.
Delete button is small "X" in top right corner, cancel is first button in bottom buttuns

@grzanka
Copy link
Contributor

grzanka commented Oct 7, 2024

I cannot remove cancelled simulations, I would like to be able to do so:

image

@matuszsmig matuszsmig marked this pull request as draft October 7, 2024 16:23
@matuszsmig
Copy link
Contributor Author

@grzanka So if simulation is cancelled this "Retry" button should change into delete (x) button?

@grzanka
Copy link
Contributor

grzanka commented Oct 7, 2024

@grzanka So if simulation is cancelled this "Retry" button should change into delete (x) button?

Yes, exactly. This retry button if I remember correctly was intended to fetch again status of simulation

@grzanka
Copy link
Contributor

grzanka commented Oct 7, 2024

I've deleted yaptide_data volume to start with fresh environment locally (means empty database), added a local user, ran some simulation and deleted it. I was suprised that deleted simulation card was removed only 10 seconds later after I clicked on Delete X button. Why wasn't that immediate ?

See movie below. I click delete around 1:10, simulation card is gone around 1:20

Screencast.from.2024-10-07.18-40-12.webm

Was that I had to wait 10 seconds for subsequent call to https://127.0.0.1:8443/user/simulations?page_idx=1&page_size=6&order_type=descend&order_by=start_time ?

# handle delete simulation also on forntend site
# if simulation is cancelled x appears
@matuszsmig
Copy link
Contributor Author

I have made some refactoring so I added @tudde to reviewers.

Also changed that if simulation is state "CANCELLED" it is possible now to delete it.

I used kind of trick, when x is being pressed (delete) I am not only calling delete request, but also delete it from REACT's state, is it good aproach? @grzanka

@grzanka
Copy link
Contributor

grzanka commented Oct 8, 2024

I have made some refactoring so I added @tudde to reviewers.

Also changed that if simulation is state "CANCELLED" it is possible now to delete it.

I havent made any tests yet, so cannot say how does it work now

I used kind of trick, when x is being pressed (delete) I am not only calling delete request, but also delete it from REACT's state, is it good aproach? @grzanka

what if delete request is not successful ?
there may be another request - before deleting ask user if he/she really wants to delete something, does it change your hack ?

@grzanka grzanka requested a review from ostatni5 October 8, 2024 19:14
@grzanka
Copy link
Contributor

grzanka commented Oct 8, 2024

@ostatni5 maybe your advice here may help ?

@matuszsmig
Copy link
Contributor Author

I have made some refactoring so I added @tudde to reviewers.
Also changed that if simulation is state "CANCELLED" it is possible now to delete it.

I havent made any tests yet, so cannot say how does it work now

I used kind of trick, when x is being pressed (delete) I am not only calling delete request, but also delete it from REACT's state, is it good aproach? @grzanka

what if delete request is not successful ? there may be another request - before deleting ask user if he/she really wants to delete something, does it change your hack ?

I don't know what would be if request would fail there should be thrown some error, I may add some prompt asking if you are sure to delete it, it wouldn't change my hack. I think that is common approach

@grzanka
Copy link
Contributor

grzanka commented Oct 9, 2024

I've merged related PR on backend, hence this code needs now to be tested with master branch on backend

@ostatni5
Copy link
Member

ostatni5 commented Oct 9, 2024

I have made some refactoring so I added @tudde to reviewers.
Also changed that if simulation is state "CANCELLED" it is possible now to delete it.

I havent made any tests yet, so cannot say how does it work now

I used kind of trick, when x is being pressed (delete) I am not only calling delete request, but also delete it from REACT's state, is it good aproach? @grzanka

what if delete request is not successful ? there may be another request - before deleting ask user if he/she really wants to delete something, does it change your hack ?

I don't know what would be if request would fail there should be thrown some error, I may add some prompt asking if you are sure to delete it, it wouldn't change my hack. I think that is common approach

When deleting a resource it is a good idea to ask the user for confirmation if this action is irreversible.
Optimistic updates are ok, but if an action fails app should be able to recover valid state by restoring old one or just by fetching the current one.

@grzanka
Copy link
Contributor

grzanka commented Oct 9, 2024

I have made some refactoring so I added @tudde to reviewers.
Also changed that if simulation is state "CANCELLED" it is possible now to delete it.

I havent made any tests yet, so cannot say how does it work now

I used kind of trick, when x is being pressed (delete) I am not only calling delete request, but also delete it from REACT's state, is it good aproach? @grzanka

what if delete request is not successful ? there may be another request - before deleting ask user if he/she really wants to delete something, does it change your hack ?

I don't know what would be if request would fail there should be thrown some error, I may add some prompt asking if you are sure to delete it, it wouldn't change my hack. I think that is common approach

When deleting a resource it is a good idea to ask the user for confirmation if this action is irreversible. Optimistic updates are ok, but if an action fails app should be able to recover valid state by restoring old one or just by fetching the current one.

@ostatni5 and what do you think of refactoring, to my looks OK, but you are much more familiar with this code

Copy link
Contributor

@grzanka grzanka left a comment

Choose a reason for hiding this comment

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

works well, needs documentation but thats an separate issue.

@grzanka
Copy link
Contributor

grzanka commented Oct 9, 2024

Here is an issue related to documentation of this feature: yaptide/docs#72

@grzanka
Copy link
Contributor

grzanka commented Oct 9, 2024

@matuszsmig should we make this a regular PR, not a draft ?

@matuszsmig
Copy link
Contributor Author

matuszsmig commented Oct 9, 2024

@grzanka i need to add this prompt which asks user if he is sure to delete it and I want to test one thing

@grzanka
Copy link
Contributor

grzanka commented Oct 9, 2024

@grzanka i need to add this prompt which asks user if he is sure to delete it and I want to test one thing

Lets make it in the subsequent PR. If @tudde would comment today on the PR then lets wait for the review.
Otherwise lets squash-merge it.

@grzanka grzanka marked this pull request as ready for review October 9, 2024 15:55
@grzanka grzanka added this pull request to the merge queue Oct 9, 2024
@grzanka
Copy link
Contributor

grzanka commented Oct 9, 2024

ok, no review from @tudde , merging

Merged via the queue into master with commit 06df960 Oct 9, 2024
9 checks passed
@grzanka grzanka deleted the 1705-delete-button-to-remove-simulation-from-db branch October 9, 2024 20:11
dudiiiiiiii pushed a commit that referenced this pull request Oct 15, 2024
* # use endpoint for deleting specific simulation

* changes aftert code review

* # small refator
# handle delete simulation also on forntend site
# if simulation is cancelled x appears
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Delete button to remove simulation from DB
3 participants