'signin redirect' only takes a string as second parameter

[hreimer]

Expected behavior

Following this closed issue #1489 I was expecting both versions to work:

keystone.set('signin redirect', '/whatever/url/you/want');
// -or-
keystone.set('signin redirect', function(user, req, res){
  var url = (user.isAdmin) ? '/keystone' : '/whatever/url/you/want';
  res.redirect(url);
});

Upon inspecting keystone/admin/server/routes/signin.js:14 and keystone/admin/server/routes/signout.js:8 I assumed, the signin redirect should also be able to take a function just as the signout redirect does (the latter does work with a string and a function)

Actual/Current behavior

Only the first version (setting the redirect route to a fixed string) worked, when setting it to a function it is simply never executed.

Steps to reproduce the actual/current behavior

Open keystone.js and insert one of the parts of the above code before keystone.start(), start keystone and try to sign into the Admin UI.

The referenced issue is over 2 years old nevertheless so if the current state is the desired behaviour this issue can be closed, although it would be a nice addition.

Environment

Software	Version
Keystone	4.0.0-beta.5
Node	6.9.5

[vamshi9]

@hreimer That would be an ideal solution! I'm thinking the issue is with the params getting passed to the callback function. Is it so?

[frapan]

@hreimer I've got the same problem and I think you explained it very well.
Looking at the singin.js source code you provided I've found out how to solve this problem and make Keystone 4 work the same way Keystone 0.3 worked.
It's a simple addition to that file.
I will provide a pull request as soon as possible, hopefully tomorrow.
Note: issue #4469 is a clone of this issue.

[hreimer]

hey @vamshi9 & @frapan thanks for confirming my issue and providing a PR for that, I didn't have time to do it myself and only fixed it locally for test purposes - now we only have to wait for one of the maintainer to merge that PR