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

Delete slide button in the Table #303

Merged
merged 13 commits into from
Apr 4, 2020
Merged

Conversation

akhil-rana
Copy link
Contributor

ezgif-4-539473bfbf2f

$('#deleteModal').modal('toggle');
$("#confirmDelete").unbind( "click" );
$("#confirmDelete").click(function(){
store.deleteSlide(oid);
Copy link
Contributor

Choose a reason for hiding this comment

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

Akhil I have implemented the same but here it returns a promise so, you can check if data.result.ok is 1 then initialize the table.

@birm birm requested a review from nanli-emory April 3, 2020 02:11
@cjchirag7
Copy link
Contributor

cjchirag7 commented Apr 3, 2020

I think it would be better, if in the Modal we tell the user about the name and id of the slide, which he is going to delete, so that he can make his confirmation to delete, without any doubt. The modal message can be something like :
Are you sure you want to delete the slide ${slide-name} with id ${slide-ID} ?
The 'Close' button can be renamed to 'Cancel'. It makes more sense and the modal header can be 'Delete confirmation'.

Copy link
Contributor

@cjchirag7 cjchirag7 left a comment

Choose a reason for hiding this comment

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

@akhil-rana , It would be nice, if these changes are made.
I have tested it by making these changes locally.

Screencast -
delet-slide

apps/table.html Outdated Show resolved Hide resolved
apps/table.html Outdated Show resolved Hide resolved
apps/table.html Outdated Show resolved Hide resolved
apps/table.html Outdated Show resolved Hide resolved
apps/table.html Show resolved Hide resolved
apps/table.html Show resolved Hide resolved
akhil-rana and others added 7 commits April 4, 2020 00:53
Co-Authored-By: Chirag Jain <cjchirag7@gmail.com>
Co-Authored-By: Chirag Jain <cjchirag7@gmail.com>
Co-Authored-By: Chirag Jain <cjchirag7@gmail.com>
Co-Authored-By: Chirag Jain <cjchirag7@gmail.com>
Co-Authored-By: Chirag Jain <cjchirag7@gmail.com>
Co-Authored-By: Chirag Jain <cjchirag7@gmail.com>
@akhil-rana
Copy link
Contributor Author

Thanks @cjchirag7

@nanli-emory
Copy link
Member

@cjchirag7 @mgautam98, I appreciate that you guys work hard for deleting slides. The problem is that you must determine the current user who has the permission to delete the slide or not. It involves authority management. In the current situation, there are two types of users, one is admin who has the right to delete the slide. Another is normal who can't do delete operation. You guys need to add the authority check for the deleting functionality. What do you think @birm?

@akhil-rana
Copy link
Contributor Author

akhil-rana commented Apr 3, 2020

@nanli-emory
I think the delete route is configured to allow for Admin and Editor only in the ca-back itself.
Since we are using this particular route so.

https://github.com/camicroscope/Caracal/blob/master/caracal.js#L62

@birm What are your thoughts?

@birm
Copy link
Member

birm commented Apr 3, 2020

Yes, in instances where we are checking attributes, slide deletion should only be performable by Admins and Editors (same people who can add a slide), and, as applicable, have a userFilter matching the slide's filter field (same people who can see the slide). I'm not sure it's foolproof yet, but that should be taken care of by the backend.

@cjchirag7
Copy link
Contributor

We can do one thing that display the delete icon only, if the user is Editor or admin, so to avoid unnecessary requests to the backend also.

@nanli-emory
Copy link
Member

@cjchirag7 Thanks. This is what I'm talking about. From the UX perspective, we shouldn't show up a delete button but users actually can't use it.

@akhil-rana
Copy link
Contributor Author

@nanli-emory
Would it work if the user gets an error pop up saying "Delete Permission not granted" on the button click?

@akhil-rana
Copy link
Contributor Author

Or we could send a dummy delete request on page initialisation just to check if the user has permission or not.
and then disable the display button accordingly on page load itself.

@birm
Copy link
Member

birm commented Apr 4, 2020

You may notice a function called parseJwt which can get token data, which should include userType and userFilter, matching what's in the user mongo collection.

@nanli-emory
Copy link
Member

@akhil-rana We should get the user type from the token. show the delete button if user type is admin. Otherwise, don't add the delete button on the page.
@birm, did you change the way to read attr from token?

@akhil-rana
Copy link
Contributor Author

akhil-rana commented Apr 4, 2020

@birm Thanks for the info.
but in develop, we have NULL userType. Please correct me if I am wrong.
Who also has the delete permission(in case of DISABLE_SEC). If I disable the delete button then it'll be gone from develop branch?
Is there any way to check the DISABLE_SEC value?

@nanli-emory
Copy link
Member

I will add the function called getUserType into caMicroscope/common/util.js.
This function will return NULL if the user is normal or no token (when turn off security). It will return Admin if the user has token and is admin.

@akhil-rana
Copy link
Contributor Author

Okay. Great !!
Thank you.

@nanli-emory
Copy link
Member

@akhil-rana I added getUserType() that will return string value.
admin -> Admin or normal/no security -> Null.

@akhil-rana
Copy link
Contributor Author

Thank you for your effort.
I will commit the changes soon.

@akhil-rana
Copy link
Contributor Author

@nanli-emory The function will always return Null when we work with develop.yml since we have no security in that case.
So should the delete button be hidden on no security too?
In that case anyone can delete the slide by going to the correct route anyway.

@birm
Copy link
Member

birm commented Apr 4, 2020

We may need to either add a route which displays some safe-to-show env variables the server has.

@cjchirag7
Copy link
Contributor

cjchirag7 commented Apr 4, 2020

@nanli-emory , the code in util.js is not compiling, after the latest commit.
It gives the error

reutrn "Null";
           ^^^^^^
SyntaxError: Unexpected string

The travis build is also failing.

This is because the word 'return' is misspelled. I have corrected it and fixed some lint issues in PR #310

@akhil-rana
Copy link
Contributor Author

@birm Yes, I was thinking the same thing.
What do you suggest us to do with this right now then?
Should I add a PR on Caracal for the same if you want?

@birm
Copy link
Member

birm commented Apr 4, 2020

Sure! I've added an issue matching what I think may be a scalable strategy.

@akhil-rana
Copy link
Contributor Author

akhil-rana commented Apr 4, 2020

@birm @nanli-emory
I have used the getUserType function to pass the userType as query parameter into the new what can I do route in Caracal.
I have made a new getUserPermissions function in util.js which returns the permissions object response from what can I do route.
Now, the delete button will only be shown to the user who has the permission to delete.

Copy link
Member

@birm birm left a comment

Choose a reason for hiding this comment

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

That must've been quite a journey, thank you!

@birm birm merged commit dec8ccc into camicroscope:develop Apr 4, 2020
@akhil-rana
Copy link
Contributor Author

Thank you all for the help and effort.

@birm birm mentioned this pull request Apr 17, 2020
@akhil-rana akhil-rana deleted the deleteSlide branch June 18, 2020 21:11
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.

5 participants