Custom actions

[adamscybot]

This pull request is to resolve #1206. Essentially it adds initial support for custom actions on item detail pages. I plan on extending this further to also support list pages. This lets you add extra buttons like this:

Usage:

Post.customAction('Archive', function(req, res, next) {
    var item = req.items[0];
    item.status = 'archived';
    item.save(function(err) {
        if (err) {
            throw new Error("Couldn't archive this post!");
        }

        next('Post was archived successfully!');
    })
})

You can optionally pass an options object that currently accepts mobileText for the button text on mobile. You can also override the unique slug assigned to the endpoint for the action with the slug option.

The item that the action was called on can be accessed via req.items[0]. I intend to go on to add custom actions to Lists, hence the array format. For lists, req.items will be an array of items that were "ticked" on the list page. This is going to require some UI work and probably support for things like batch delete to go with it for it to make sense. You will be able to limit it if you only want it on list view or you only want it on item view.

Errors (via throwing) and success messages (via next()) are pulled through to the UI as flash messages. Instead of utilising Keystone.messages bootstrapped into the template, I parse the asynchronous call response and add them dynamically with react. Please see #2090 for my reasoning behind this. This work has also unlocked proper handling of messages of other asynchronous functions like delete which were marked as TODO. I will do a separate pull request for those. I hope this approach is suitable. It is my view that we should move to all error handling being parsed from responses to the calls and not from Keystone.messages. This will help down the line with true SPA support.

I'm happy with how it works on item views now, so if you're ok with it, it could be pulled in as WIP. I will then do the documentation.

One question I have is over positioning and styling. Is it ok as it is now? Or should we consolidate custom actions into a small dropdown button next to the save button. Just thinking of scenarios where someone has loads of custom actions.

[webteckie]

@adamscybot this looks very good! This will help my app as well! I vote for them being regular buttons as you currently have it. And I think they should inherit the same style as the Save button. To minimize these taking extra real estate in the screen, perhaps the reset changes should just become a Reset button and delete <list> should become a Delete button both with a title tooltip with the extra info.

cc: @jossmac

[JedWatson]

@adamscybot I really love what you've done here. I've been planning to add something like this in a slightly different way for 0.4 (specifically, exposing methods on a model but configuring the access to them client-side, not server-side through an Admin UI plugin API)

I've also been holding off on adding something like this while we explore all the possible ways of implementing custom actions for list items, so that we don't go down one path then have to back-track and break existing customisations.

however I like your approach so much I think it might be worth going with regardless.

Positioning and styling are great for this variant; You might notice we've left room in the sidebar (could be a callout menu on mobile) which could also be used for custom components / actions. We may also want to make actions available at the top of the edit form.

One thing I've been thinking about that hasn't been taken into account here is the availability of custom actions; e.g. if a post is archived, it doesn't make sense to still have an "archive" button.

I'm going to think some more on this before merging, ping @creynders and @ericelliott as well for thoughts.

@adamscybot if you haven't seen it, you might also be interested in watching my nodevember talk from last year in which I outline some of the ways we're planning to open up Keystone for extensions, it's pretty relevant to this: https://www.youtube.com/watch?v=of2VM3Wwkxk

[adamscybot]

Thanks for the feedback guys.

For disabling buttons when you don't wan't them, I guess the only way in this implementation is to use error messaging. However, that's not ideal. Originally I had the custom actions data (name, slug) provided on the API that handles the fetching of the item and not bootstrapped in Keystone.Lists when I saw that's how everything else is. I think I should change it back to the item API and then that would unlock the ability to add custom "visible" and "disabled" callbacks. Is this something that would be desirable?

I'll take a closer look at the positioning for top of form/mobile etc. I should probably also add an option that lets you specify a type for the button.

I guess the merge comes down to if this is the kind of functionality you see living in some kind of plugin or if its a configuration element that's part of the core lib. Personally, I've needed this feature all the time and this time it was a requirement so I went ahead and implemented it to work with an urgent use case I've had. Though I think this would of have helped me in the past many times over.

There's obviously still a lot of value in being able to somehow modify the edit form client side -- especially for complex use cases where you might want some kind of client side form manipulation. However, I feel this covers most use cases.

I'll watch that vid now (probably should of watched it before this comment 🎱 !

[webteckie]

@adamscybot @JedWatson what about adding a dependsOn config here? With all that power @snowkeeper added to dependsOn it shouldn't be a problem?

[adamscybot]

@webteckie That's a good point. I'll look into making that work.

[creynders]

Looks cool. Definitely need to be able to disable specific action buttons though. I'd go for a 'guarded' approach here though, i.e. call a handler per action/button type which returns a boolean to determine the disabled state (and not e.g. an object with booleans for each action type).

Then, I'm not fond of callback as the property name, since it's a bit of a misnomer IMO, wouldn't action be more appropriate? .

One more thing, I'm not entirely sure about the URL. I think I'd prefer /api/<list>/<id>/actions/<action>
Oh, and then I'd change the delete action URL too to /api/<list>/<id>/actions/delete

[adamscybot]

@creynders Regarding disabling, that's exactly what I had in mind originally. Probably one also for visible. I guess implementing the dependsOn expressions passed to the client would be a different competing option. Which would you prefer? The latter would allow cool stuff like changing button state depending on local form data. However, I'd need to do the checks server side too to guard it as you say.

At the moment, the action is passed a fresh copy of the model from the DB. However, for dependsOn to be useful, you'd really want an option to have the action receive a POST of the form data instead. More work, but could be really worthwhile as you'd have total control of the button states. Also for consistencies sake, I think dependsOn makes sense so we're not reinventing the wheel. Note to self: remember csrf.

Agree on the other changes.

[adamscybot]

@JedWatson @creynders @webteckie

I have now updated this pull request:

• There are now options for title and type of button.
• A dependsOn expression can be passed in options which is verified on client side (to enable or disable button) and also server side.
• To suit the above change, the call to the action is a POST with the form data in and not a GET. This is accessible as normal through req.body. req.items will still be the database copy.
• Because one of the most common things you want to do before performing your action will be to save the form data there is a new option called save which defaults to true. When true, the form data is saved to the DB before the action callback is called.
• Change URL to /api/<list>/<id>/actions/<action>. However, I have not moved delete as I feel custom actions should be name spaced separately to those built in ones (unless we at some point make delete a custom action in itself which might be a good idea looking forward).
• callback property is now called action

Possible future work (please suggest):

• As well as a dependsOn configuration, also allow a callback specified server side.

[creynders]

@adamscybot wow, that's a truly great PR. From start to finish. Totally 👍 as far as I'm concerned.
Just one minor thing we need to consider: whether save's a good property name. Since, if I'm not mistaken, the item is not saved after the custom action is triggered (only before), right? The name might be confusing then. How 'bout preSave? Or it could be made more versatile, something like:

{
  save: 'pre'
}

with possible values 'pre', 'post' or ['pre', 'post']?
Or even:

{
  save: { pre: true, post: false}
}

What do you think?

[webteckie]

@adamscybot looking good! Can't wait for this :-) As I was reading your comment the same impression as @creynders about save crossed my mind. Thus, I agree with his suggestions and prefer the very last one. Thanks!

[ericelliott]

Is there some corresponding documentation for the new functionality and API endpoints?

[adamscybot]

@ericelliott I was going to create a pull request on the docs repo if/when this was merged. I'll get the above suggestions done sometime soon and create that at the same time.

[ttsirkia]

Very nice job @adamscybot ! I had almost forgotten that I had created that issue. This implements exactly the feature that I was looking for. Now I have a few use cases for this: For example, I have a model for email validations. If there is a need to resend the validation message, this feature allows to implement that feature into the Admin UI.

[adamscybot]

@ericelliott Docs now added at keystonejs/keystonejs-site#65

Also:

• Changed to a save: { pre: true, post: false} format in options.
• Pre and post now default to false. I feel having true on pre was unexpected.
• Now use req.item instead of req.items[] as it doesn't make sense until I make this change applicable to lists.

@JedWatson How do you feel about this now? If it's not the direction you want to go feel free to close and I will maintain a separate fork for my personal use cases. Hopefully, I will be making more contributions regardless!

I don't know why the test is failing now? Its sudden and unrelated...something wrong with the tests?

[adamscybot]

Anyone know whats going on with the tests? Pass fine locally. Other pull requests seem to be seeing this issue.

[creynders]

💯 times 👍 Let's get this merged!

[webteckie]

👍 👍

[creynders]

@JedWatson could we get your blessing on this one? If so, @adamscybot could you fix the merge conflicts? Sorry for the long wait!

[adamscybot]

@creynders Sure. Would love to see it merged in!

[webteckie]

@creynders @adamscybot I discussed this one with @JedWatson and he thinks it is best to address it after 0.4 since otherwise it will make 0.4 harder to get out, which is his first keystonejs priority. Trust me I want this as bad as you all do:-) I sympathize with Jed and to help get to this faster our efforts at this moment would be better spent helping with getting 0.4 out ASAP. I'm pretty sure this PR will be amongst the first to be considered once 0.4 is published. Now if Jed can change his mind about including this in now then that's something else :-)

[mxstbr]

I'm not entirely up to speed here, why would we wait to get this in until the PR is out of date and hard to rebase instead of now? Is there any more work that'd need to be done for 0.4 to be released, how would this block that?

[webteckie]

@mxstbr there's a few of us that really want this. If you can convince @JedWatson to merge it that would be great. I had a talk about it with him and there were concerns of merging in and how it would affect the remaining of the 0.4 work. If that's no longer a concern then it should definitely be merged!

[adamscybot]

I'd be more than happy to work with @JedWatson on modifying this PR as and when alterations are needed for any other potential 0.4 changes. We use this PR in production so I am motivated to keep it as up to date as possible.

[SharadKumar]

Hi @adamscybot - @JedWatson and team are preparing v0.4 beta. Are you plan to rebase since there has been quite a lot of changes since then and bring this up to date?

Appreciate all the effort mate.

[adamscybot]

I think the internals of keystone has changed significantly since this PR. Theres some very positive changes around how errors are handled now (no more bootstrapping in templates...I think) which changes how this will need to be implemented. It will probably need more than a rebase -- I suspect I would start again.

If these internal API's are stable enough (@JedWatson ?) I will have another go at this.

I most interested in taking this opportunity to address any issues with my user-facing API that I proposed for this. I feel it was in a good place where we left it, but suggestions very welcome.

I think I should also allow custom actions at a list level as well from the beginning as the API ultimately needs to be compatible for both individual items and lists.

[VinayaSathyanarayana]

Any updates on this?

[michaek]

This is a great feature addition. If there's any way I can help prep this to go into KeystoneJS, I would be happy to. It seems like @adamscybot is waiting on @JedWatson's answer on whether the internal API is stable enough to re-roll this. As 4.0 is now in beta, does that make it more likely to be stable?

[VinayaSathyanarayana]

Any updates on this PR getting merged?

[VinayaSathyanarayana]

Any updates on this PR getting merged?

[VinayaSathyanarayana]

Any updates on this PR getting merged?

[ichiriac]

any news ? I also need this PR, exactly the same need

[Twansparant]

This functionality would be great yes! +1

[JedWatson]

This isn't going to land in v4, but is in scope for v5 (see keystonejs/keystone-5#1002)