Skip to content
This repository has been archived by the owner on Nov 15, 2019. It is now read-only.

JDR Delete Button not disabled - Second click causes error #56

Closed
hhovsepy opened this issue Sep 22, 2017 · 13 comments
Closed

JDR Delete Button not disabled - Second click causes error #56

hhovsepy opened this issue Sep 22, 2017 · 13 comments
Assignees
Labels

Comments

@hhovsepy
Copy link

When doing double click or clicking second time immediately on Delete button, while deleting JDR Report, it causes Unexcpected error to occur.

It needs to disable Delete button once first click happened.

screenshot from 2017-09-22 11 35 01

@abonas
Copy link
Member

abonas commented Sep 22, 2017

@israel-hdez I'd like @tumido to tackle this, with your guidance please :)

@tumido
Copy link
Member

tumido commented Sep 22, 2017

@karelhala Thanks for help 👍

@israel-hdez
Copy link
Member

@tumido To solve this:

  • I think you will need to change the controller to show a friendly message if any of the selected reports is already deleted. This is to prevent the case where two users are seeing the same server and one of them deleted the report, and the other one also decides to delete the same report(s), but his/her view is outdated. The code is here: https://github.com/ManageIQ/manageiq-ui-classic/blob/master/app/controllers/middleware_server_controller.rb#L189
  • To disable the button and prevent double-clicking on it, you will need to change the Javascript and, possibly, the view. Javascript code is in here and the view is here.

I'm in IRC as edgarhz. Ping me there if you need help.

@tumido
Copy link
Member

tumido commented Sep 22, 2017

@israel-hdez:

  1. Make sense, I'll take look at it.
  2. Should be already covered in the PR Disable delete button when clicked manageiq-ui-classic#2217. It does disable the button whenever it's clicked so no further action is allowed before the form is actually sent. Does it make sense or do I need to provide more robust solution on the view side?

@israel-hdez
Copy link
Member

Oh! You already have a PR. My eyes didn't see the github references to it. Yes, it makes sense; it should be enough to have the button disabled.

@tumido
Copy link
Member

tumido commented Sep 22, 2017

Cool. That's one part done!

For the multiple users scenario:
When the report is already deleted, there should be some generic message telling the second user, the report is no longer present. And this message should be displayed for any action, not just for the dr_delete but for dr_report_download and possibly others as well. I'll investigate that. ;)

@tumido
Copy link
Member

tumido commented Sep 22, 2017

@israel-hdez that actually should maybe tracked as a separate issue. Because when one user deletes a report the other one can still download it, yet the report is empty. It fails silently. That's not right. What does @abonas think?

@israel-hdez
Copy link
Member

@tumido I'd say it's your choice. Personally, I'd use the same PR. But if you want to solve it in a different PR and/or open a new issue, it's OK!

@abonas
Copy link
Member

abonas commented Sep 24, 2017

I think it sounds like a separate issue to fix and test, but not insisting. both approaches are fine to me. Will leave the decision to @hhovsepy as he is the one who reported the original problem.

@hhovsepy
Copy link
Author

I agree to create a new issue, as the steps to reproduce are different.
Created #58

For verifying ongoing issue I need manageiq docker image to be regenerated.

@hhovsepy
Copy link
Author

Still see the initial issue on master.20170925085939_

@hhovsepy
Copy link
Author

Issues is fixed partly, it still happens when double clicking on Delete button, or clicking on it several times rapidly.
But when clicking on it after 1 second then the Delete button is disabled, right.
But I do not expect to handle such a multi-clicking situations, so putting this issue verified.
Such multi clickings should cause notification message which will be fixed in part of: #58

@hhovsepy
Copy link
Author

@tumido thank you for the fix.

xeviknal pushed a commit to xeviknal/manageiq-ui-classic that referenced this issue Oct 9, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

No branches or pull requests

4 participants