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

Star button for plugin list and detail page #95

Merged
merged 14 commits into from Sep 28, 2020

Conversation

jramcast
Copy link
Contributor

Fixes #73.

@jbernal0019 This is almost ready. Star icons are now clickable in the plugins list and in the plugin detail page. The calls to the backend are also implemented. I need some help with some issues I found, mainly related to backend calls and authentication:

  1. Using client.createPluginStar({plugin_name:<name>}) to fav an item creates and association between the starred item and the user. However, when later using the client.getPluginStars() function to retrieve the stars, I get the stars from all users, which looks like a security issue to me (did we forget to filter by user in the backend?). My understanding is that we should only be receiving the authenticated user's stars here. I am authenticating the calls with a token and as far as I know, that should be enough to identify the user in the backend?
  2. Related to the previous problem. Using PluginStar.getPluginStar(starId).then(star => star.delete) always results in the backend responding with a 403 Forbidden error (You do not have permission to perform this action).
  3. In case we have 2 plugins with the same name, getPluginStars and createPluginStar are problematic if filtering by plugin_name. It would make more sense to create/get a plugin stars using the plugin id.
  4. My CSS knowledge is very limited, I did my best but someone with better command of CSS may want to take a look.

@jbernal0019
Copy link
Member

jbernal0019 commented Sep 22, 2020

Hey @jramcast This is great!

  1. I've fixed client.getPluginStars() to only retrieve the authenticated user's stars

  2. There was a bug in client.getPluginStar(<id>) that was internally using getPlugins method instead of getPluginStars method. Sorry for that. Also make sure the id number you are passing here identifies a plugin star that belongs to the authenticated user.

  3. Actually a plugin star is applied to a plugin's meta which is a meta data object that is the same across all versions of the same plugin. The plugin_name parameter actually refers to the plugin's meta's name which is unique in the DB. Two plugin metas can not have the same name. So when you star a plugin you are actually making a favorite all the plugin versions of that plugin. Hope this makes sense.

I've updated the above methods' documentation: https://fnndsc.github.io/fnndsc/chrisstoredoc/class/src/client.js~Client.html#instance-method-getPluginStar
https://fnndsc.github.io/fnndsc/chrisstoredoc/class/src/client.js~Client.html#instance-method-getPluginStars

I've pushed the latest version of the ChRIS Store JS client "2.0.2" to npm. Could you please update the code in your PR to use the new version with the fixed bugs?

Please let me know any issues.

@mindreeper2420 Could you please help out with the CSS here?

Thank you!

@anujsingla
Copy link
Contributor

Looks good to me

Copy link
Contributor

@anujsingla anujsingla left a comment

Choose a reason for hiding this comment

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

Reviewed.

@jramcast jramcast marked this pull request as ready for review September 28, 2020 08:11
@jramcast
Copy link
Contributor Author

@jbernal0019 after updating the backend and the client version to 2.0.2, everything looks good :).

The PR is now open for review. Please feel free to do it.

@jramcast jramcast removed their assignment Sep 28, 2020
@jbernal0019 jbernal0019 merged commit 6f454d0 into FNNDSC:master Sep 28, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants