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

Should be able to delete plugins #928

Closed
taylortom opened this issue Nov 6, 2015 · 18 comments
Closed

Should be able to delete plugins #928

taylortom opened this issue Nov 6, 2015 · 18 comments
Assignees
Labels
S: merged Completed, reviewed, and merged issues T: enhancement Adding additional functionality

Comments

@taylortom
Copy link
Member

Now that we can delete assets, the omission of this feature is much more noticeable.

Related forum posts:
https://community.adaptlearning.org/mod/forum/discuss.php?d=1013
https://community.adaptlearning.org/mod/forum/discuss.php?d=965

@taylortom taylortom added the T: enhancement Adding additional functionality label Nov 6, 2015
@brian-learningpool
Copy link
Member

Implementation of this will probably involve a 'soft' delete, to ensure that courses which use the deleted plugin still build and preview ok. This is how asset deletion works right now.

@taylortom
Copy link
Member Author

I think that's fine as a partial fix

@taylortom taylortom added this to the 0.2 UI/UX refresh and user management milestone Mar 4, 2016
@taylortom taylortom modified the milestones: 0.2 UI/UX refresh, 0.1.8 UI refresh, 0.?.0 UI Rewrite, 0.2.0 UI refresh Jun 22, 2016
@taylortom taylortom removed this from the 0.?.0 UI Rewrite milestone Feb 9, 2017
@taylortom taylortom added this to the Suggested Next milestone Apr 18, 2018
@canstudios-nicolaw
Copy link
Contributor

We have a version of plugin delete working on our adapt which we would like to give back to the community. At the moment, it only lets you delete the plugin if it isn't in use by any courses, if it is used in courses it lists the courses and the course owners.

removeplugin

removeplugins2

I will assign this issue to myself for now and we can give this back soon (I won't add it to the loose ends milestone right now as I don't want to keep growing that milestone and adding features to test).

@canstudios-nicolaw canstudios-nicolaw self-assigned this Jan 31, 2019
@taylortom
Copy link
Member Author

taylortom commented Feb 1, 2019

I'd like to also put together a similar fix for asset deletion, so if we can decide on a strategy (the above sounds fine), then we should tackle that one too 😄

@canstudios-nicolaw
Copy link
Contributor

👍 I will get this into a PR after the loose ends release, we can possibly look at the deletion of assets to go into the same release

@canstudios-nicolaw
Copy link
Contributor

@tomgreenfield @taylortom I'm currently working on getting this into a PR.

Looking at the code I can see it deletes:

  • /plugins/content/[component/extension/theme/menu]/versions/[plugin]/
  • /temp/[tenant id]/adapt_framework/src/[components/extensions/theme/menu]/[plugin]/
  • The relevant entry in the xxxxtypes table

So there will be no traces of the plugin left. (The deletion can only go ahead if the plugin isn't used anywhere).

I have a UI question, happy to take feedback now or wait until I've made the PR...

On our installs we don't have the 'Check for updates' column on the plugin manager, users are free to upload their own plugins, but the managed ones are controlled through releases. Previously the design hasn't had to take into account the presence of this column.

For now I have just added the remove plugin column to the plugin management tables, but I think the UI could be better.

Each item in the updates column will have either a button that says 'Check for updates' or some text 'Uploaded by user'.

Each item in the remove column will have either a button that says 'Remove plugin' or some text that says 'Plugins cannot be removed'.

On every item, one of those two things will be a button, and the other will be text.

Screenshot from 2019-04-10 15:22:13

It seems as though these things should be fairly straightforward to combine into one column, but I can't think of a good way to do it which would also be clear for the user to understand. I'm open to suggestions.

@taylortom
Copy link
Member Author

taylortom commented Apr 11, 2019

I think two columns should be fine. My only suggestion would be that we could switch these label buttons out for icon buttons, and add some kind of rollover tooltip if they're disabled (but this doesn't really follow any of the UI conventions we currently use in the tool).

Out of interest, is there any reason we're blocking the deletion of core plugins? From an end-user perspective, I can think of some use-cases for wanting to do this.

@canstudios-nicolaw
Copy link
Contributor

Hm I will ask around internally and have a think about both points, might be good to discuss in the call today as well.

@canstudios-nicolaw
Copy link
Contributor

I suppose we need to think about what would happen if somebody deleted the text component for example, and then later updated the framework. I think it would get re-installed? I can see that that would get annoying or appear broken.

@canstudios-nicolaw
Copy link
Contributor

I have added in removing plugins that were added through adapt.json. In this case the plugin gets deleted from the following places:

  • /plugins/content/bower/bowercache/[plugin]
  • The relevant entry in the xxxxtypes table

@tomgreenfield
Copy link
Contributor

Just logging this here to expand on @taylortom's icons suggestion. If we follow the list items button style that exists in the scaffold, the icon states could look something like this:

Check for updates

1

Click to update

2

Up to date

3

References:

@canstudios-nicolaw
Copy link
Contributor

@tomgreenfield Thanks for the suggestion, I will get these implemented

@canstudios-nicolaw
Copy link
Contributor

@tomgreenfield Do you have any thoughts for if the plugin was uploaded by the user?

@tomgreenfield
Copy link
Contributor

My first thought is simply a user icon (without button outline) to signify it's user-uploaded.

@canstudios-nicolaw
Copy link
Contributor

Ok, I will try it and see how it feels. I like the look of the buttons, but I suppose I have a slight concern it's less clear what is going on. Eg. that refresh button might make me think that it was going to refresh the plugin immediately rather than just check for updates.

@canstudios-nicolaw
Copy link
Contributor

Screenshot from 2019-05-17 15:43:21

This is how it is looking now

@canstudios-nicolaw
Copy link
Contributor

@tomgreenfield I think I should put the text that used to be in the buttons in title attributes so that they display as tooltips. Do you have any objection to this?

@canstudios-nicolaw
Copy link
Contributor

Tooltips added

@canstudios-nicolaw canstudios-nicolaw added the S: merged Completed, reviewed, and merged issues label Jun 27, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S: merged Completed, reviewed, and merged issues T: enhancement Adding additional functionality
Projects
None yet
Development

No branches or pull requests

4 participants