search

marklogic-community / slush-marklogic-node

Slush generator for a MarkLogic/node project
https://github.com/marklogic-community/slush-marklogic-node/wiki
Other
40 stars 28 forks source link

getAuthenticatedStatus needs to always return promise #485

Closed patrickmcelwee closed 7 years ago

patrickmcelwee commented 7 years ago

If the _isAuthenticated memo was already set, it was returned a bald value, without wrapping in a promise

Also, I found and threw the error that wasn't showing up - which would have helped debug this issue.

Fixes #480

joemfb commented 7 years ago

I think i just ran into this in a work session with a prospect.

grtjn commented 7 years ago

Thanks for the PR, super helpful!

And it looks very simple! I had come up with something more complex, like further down in that file, where a promise is created first.

By the way, now you have opened a PR for this anyhow. Something very similar is also with userService.getUser itself.

Also, it is easy to make these function return promises, but not sure if other code that relies on these will be expecting promises. Can you scan through the code to double-check?

patrickmcelwee commented 7 years ago

Yeah, the symptom I ran into is that it would never prompt for login when going to any route except '/'.

I'll try to look at those questions by the end of the day, Geert. I also might look for other places that we don't throw errors from promises - since that would have saved us a good bit of time pinpointing this error.

grtjn commented 7 years ago

That would be great, thanks!

patrickmcelwee commented 7 years ago

My grep through the code indicates that consumers won't break because this is now always a promise. But we probably could simplify this bit of code that actually was implicitly checking whether getAuthenticatedStatus returned a promise: https://github.com/marklogic/slush-marklogic-node/blob/master/app/templates/ui/app/login/login.service.js#L178-L190

I added a commit to do that ... this should be ready to go, I think.

grtjn commented 7 years ago

Did you take a look at userService.getUser yet by any chance? It has a similar issue of not always returning a promise, I thought..

patrickmcelwee commented 7 years ago

I think that is true. I just did the same kind of wrapping there (added a commit).

There is only one consumer of getUser in the code now, and it can handle a promise or not without changes ... but this is probably a good change to make things easier on future consumers.

grtjn commented 7 years ago

Merging now! (sorry for the delay)