search

julianlam / nodebb-plugin-session-sharing

Allows login sessions from your app to persist in NodeBB
MIT License
88 stars 65 forks source link

Unify payload extraction #35

Closed graupe closed 7 years ago

graupe commented 7 years ago

This was mainly motivated by the username not being stripped from special characters on profile updates. Or is there a special reason, this was not put in place?

It also adds some hopefully meaning full log messages as well as cleaner code, YMMV.

julianlam commented 7 years ago

At first glace it looks good! Looking forward to checking out your PR in depth 😄

graupe commented 7 years ago

I think I have to reevaluate the logic... for example, the location property should not be mandatory. Right now the profiles's location will just be set to empty string, if it is not given in the payload, which is a breaking change I guess. It actually is working alright. But more by accident.

julianlam commented 7 years ago

@graupe You may want to consider rewriting if it helps, but one thing that would be nice would be to actually allow the update of any field in the user hash, instead of just username, firstname, lastname, etc...

So we can do things like birthday update or location update.

graupe commented 7 years ago

I had really a lot WTF moments, since this is my first time developing for any node application. After I updated the code to support all user properties, I realized that upon creation some of these fields are actually not being initialized. Wich made me rethink the whole findUser-Method. I might have employed serveral anti-patterns, since I am new to this arcane beast of callbacks.

Please test this, if you think my changes are worth to being integrated. If not, I will find a simpler solution to add 'location' to the profile, since that was what I actually wanted to do :)

graupe commented 7 years ago

Ahh, sorry! Just noticed that I missed the admin-panel for the configuration. Will do that ASAP. Shouldn't bar you from having a look.

graupe commented 7 years ago

Sorry for the noise, I expected github to keep the history of temporarily added commits. (I am usually working with bitbucket)

graupe commented 7 years ago

Are you considering this PR? In the end, the only thing I really need to workout is the "location"-property being transfered. I will have to find some means to do it, whether it is by this PR, a smaller change set or my own fork. I had rather not publish yet another plugin with almost the same functionality.

julianlam commented 7 years ago

Hi @graupe -- my main concern is that it maintains backwards compatibility with the older version, so no config changes are requires as we have clients that also use this plugin. Will take a look today.

julianlam commented 7 years ago

It looks like payloadParent is one change that would make this PR a breaking change (which is not a problem as long as it is documented)...

One could make this not breaking by checking for both payload:parent and payloadParent in the plugin settings. If the former is set while the latter is not, then set the latter and erase the former.

julianlam commented 7 years ago

Added some migration logic for payloadParent

Well done, and thank you for the contribution 😄 Apologies for the delay.

graupe commented 7 years ago

Thanks a lot @julianlam! You are absolutely right about the configuration not being backwards compatible. Didn't think about that dilemma! On a similar matter, just looking at the code: It may be, that a fresh installation of this plugin might actually not load, since there is no secret in the database. I circumvented it by adding a default secret value in the code on first install, but I won't take the time to confirm it now.

julianlam commented 7 years ago

Just tested by deleting settings:session-sharing. The warning about a missing secret comes up and you can set one in the plugin settings, so all's well.