Readonly trigger for _Session hook#5155
Readonly trigger for _Session hook#5155georgesjamous wants to merge 22 commits intoparse-community:masterfrom
Conversation
| RestWrite.prototype.buildUpdatedObject = function(extraData) { | ||
| const updatedObject = triggers.inflate(extraData, this.originalData); | ||
| let updatedObject = triggers.inflate(extraData, this.originalData); | ||
| Object.keys(this.data).reduce(function(data, key) { |
There was a problem hiding this comment.
I did not really understand what is the purpose of this reduce.
Didn't touch it, but RestWrite.prototype.buildUpdatedObject = function(extraData) { could be fixed to have less duplicated code (like inflating twice in the case of session)
Codecov Report
@@ Coverage Diff @@
## master #5155 +/- ##
==========================================
- Coverage 93.93% 93.81% -0.13%
==========================================
Files 123 123
Lines 8975 8954 -21
==========================================
- Hits 8431 8400 -31
- Misses 544 554 +10
Continue to review full report at Codecov.
|
flovilmart
left a comment
flovilmart
left a comment
There was a problem hiding this comment.
Why do we treat session creation differently when singing up than in logging in?
src/RestWrite.js
Outdated
There was a problem hiding this comment.
I don’t recommend passing default paramètres as truths, as different implementations with bottom values will produce different results (undefined, null,false)
There was a problem hiding this comment.
i will set it to false by default, but didn't know if its being used somewhere else. Was afraid from breaking something. Tests will show anyhow.
There was a problem hiding this comment.
False / undefined /null should be the default behavior. If true is passed, then we should use the new code path
| RestWrite.prototype.buildUpdatedObject = function(extraData) { | ||
| const updatedObject = triggers.inflate(extraData, this.originalData); | ||
| let updatedObject = triggers.inflate(extraData, this.originalData); | ||
| Object.keys(this.data).reduce(function(data, key) { |
src/RestWrite.js
Outdated
There was a problem hiding this comment.
Please do an early return instead
src/triggers.js
Outdated
There was a problem hiding this comment.
What’s the point of keeping that? Perphas use ‘readOnlyClassNames’ instead
There was a problem hiding this comment.
no point, will remove it
src/triggers.js
Outdated
There was a problem hiding this comment.
If it’s a readOnlyTrigger, why would we perform additional checks?
If the trigger is a simple event based one errors should simply get ignored. So it should resolved always.
There was a problem hiding this comment.
I want to give the possibility to reject a login, rejecting all readOnlyTrigger events would get in the way of that.
I am thinking now of refactoring the name to sessionTrigger rather than readonlyTrigger, it seems more appropriate.
Rejecting a session during signup will not prevent the RestWrite from creating the user, it will require a bit more refactoring to achieve this, since a user is created before the session. Rejecting a signup can be done on user triggers, while a session could be used to track logins and reject them for existing users, and maybe later on use them for 2fa capability. Once a user is signed up (and an account is created _User) there seems to be no point in rejecting a session. |
|
But still creating a user is one thing, creating a session is another thing. Either it is fully ignore or it isn’t. |
Preventing a logIn? Ensuring users have gone through a verification step before actually have credentials etc... lots of reasons actually to prevent session creation. Whicj is exactly the point no? |
|
So you think both should be independent ?
|
|
What do you think? |
|
And what are you trying to achieve with the PR, because if it’s simply having hooks in session, I believe there is an ulterior motive |
|
I think its good, i will look into it. Sent with GitHawk |
|
Yea, just that i need to cancel some logins easily. There are other ways though, but a session hook is the most direct way. Sent with GitHawk |
|
How about something like this ? The only error that gets ignored is in beforeDelete. |
|
Sorry for the late response! Sounds good then. If errors are thrown in session’s before Delete, then we should probably indicate by a log that it will continue and proceed |
flovilmart
left a comment
flovilmart
left a comment
There was a problem hiding this comment.
Overall we’re very close. Just a few nits and we’ll be great!
spec/CloudCodeSessionTrigger.spec.js
Outdated
There was a problem hiding this comment.
This try / catch is not necessary
spec/CloudCodeSessionTrigger.spec.js
Outdated
spec/CloudCodeSessionTrigger.spec.js
Outdated
There was a problem hiding this comment.
You can also use a simple spy or counter to ensure the function has been called. This makes the test suite faster :)
| } | ||
|
|
||
| // delay | ||
| function delay(millis) { |
There was a problem hiding this comment.
I believe this function is declared at many places already, do you plan to reuse it everywhere?
There was a problem hiding this comment.
will check if i can consolidate them all here.
| error: function (error) { | ||
| // ignore errors thrown in before-delete (during logout for example) | ||
| if (isSessionTrigger && request.triggerName === Types.beforeDelete) { | ||
| return resolve(undefined, error); |
There was a problem hiding this comment.
Not sure resolve supports two arguments as it is a native promise IIRC.
There was a problem hiding this comment.
Resolve here is a custom function, defined here. Its not the the promise resolve.
Line 560 in 56dc2e4
src/triggers.js
Outdated
There was a problem hiding this comment.
If this line has changed, let’s replace var by const
src/triggers.js
Outdated
There was a problem hiding this comment.
Use const if we change this line as well
|
Will look into the requested changes as soon as i can. |
|
@flovilmart i think I got them all. |
|
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. |
2 participants
Allowing a ReadOnly trigger on the session targetting this workflow:
beforeSave: triggered whenever a session is created.
Thrown errors during signup are ignored; to cancel signup a hook on _User can be used.
Thrown errors during login prevent the user from login in, a session will no be created.
Any edits to the session object will be discarded.
beforeDelete: thrown errors are discarded.
no restrictions on afterSave or afterDelete.
beforeFind and afterFind are not allowed on _Session. (not changed from before)
@flovilmart does this seem okay?