Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

added afterLogout trigger #6217

Merged
merged 5 commits into from
Nov 16, 2019

Conversation

mtrezza
Copy link
Member

@mtrezza mtrezza commented Nov 15, 2019

Analogous to the existing beforeLogin trigger, this afterLogout trigger can be used to perform certain clean-up actions upon user logout.

The afterLogout trigger contains the _Session object that was deleted. Usually, a session object contains a pointer to a _User object with which the session is associated. With that, actions can be taken for the specific user that logged out.

Caveat:
The afterLogout is only triggered when a session object was deleted upon logout. If a user logs out but there is not session object to be deleted, the afterLogout trigger is not called. This is by design, because without a session object to determine who logged out, the afterLogout trigger does not seem useful.

@mtrezza
Copy link
Member Author

mtrezza commented Nov 15, 2019

Notes:

  • tests included
  • afterLogout is a trigger on the _Session class, so the trigger validation had to be changed. Before, any trigger on the _Session class was forbidden, now only the afterLogout trigger is allowed for the _Session class.

@codecov
Copy link

codecov bot commented Nov 15, 2019

Codecov Report

Merging #6217 into master will decrease coverage by 0.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #6217      +/-   ##
==========================================
- Coverage   93.82%   93.81%   -0.02%     
==========================================
  Files         167      167              
  Lines       11293    11302       +9     
==========================================
+ Hits        10596    10603       +7     
- Misses        697      699       +2
Impacted Files Coverage Δ
src/Routers/UsersRouter.js 94.26% <100%> (+0.07%) ⬆️
src/cloud-code/Parse.Cloud.js 97.95% <100%> (+0.28%) ⬆️
src/triggers.js 94.09% <100%> (+0.02%) ⬆️
src/Adapters/Storage/Mongo/MongoStorageAdapter.js 92.27% <0%> (-0.71%) ⬇️
...dapters/Storage/Postgres/PostgresStorageAdapter.js 97.01% <0%> (-0.09%) ⬇️
src/RestWrite.js 93.75% <0%> (+0.16%) ⬆️
src/Routers/PushRouter.js 96.55% <0%> (+3.44%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5cfaaf0...0c19b04. Read the comment docs.

Copy link
Member

@dplewis dplewis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have a few comments.

src/RestWrite.js Outdated Show resolved Hide resolved
spec/CloudCode.spec.js Outdated Show resolved Hide resolved
src/triggers.js Outdated Show resolved Hide resolved
@dplewis
Copy link
Member

dplewis commented Nov 15, 2019

The afterLogout trigger contains the _Session object that was deleted.

Is this true?

@mtrezza
Copy link
Member Author

mtrezza commented Nov 15, 2019

@dplewis Yes, the session object is deleted from the DB and returned here. If there was any session object to delete.

See this._runAfterLogoutTrigger(req, records.results[0]);.

@dplewis
Copy link
Member

dplewis commented Nov 15, 2019

Oh i see from the tests. Would I get the user?

@mtrezza
Copy link
Member Author

mtrezza commented Nov 15, 2019

@dplewis Yes, you can get the user, if there was a user associated with the session.

Copy link
Member

@dplewis dplewis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@mtrezza
Copy link
Member Author

mtrezza commented Nov 15, 2019

@dplewis I just noticed that in the tests I do not get the user from the session. The session object only looks like this:
ParseSession { _objCount: 5, className: '_Session', id: 'IigxK0tazS' }.

There seem to be fields missing, do you know why?
I assume the tests only mock the session object?

I do however get the whole session object incl. the user in a development environment setup.

@dplewis
Copy link
Member

dplewis commented Nov 15, 2019

It is an object that extends ParseObject. I assume you can do req.object.get(“user”) and fetch the user. Can you add it to the tests.

@mtrezza
Copy link
Member Author

mtrezza commented Nov 15, 2019

Oh you are right, that works. I just didn't see it in the console log. I will add this to the tests.

Copy link
Contributor

@acinader acinader left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

super nice @mtrezza

@dplewis dplewis merged commit 5ed0885 into parse-community:master Nov 16, 2019
@mtrezza
Copy link
Member Author

mtrezza commented Nov 16, 2019

Thanks all for the fast review 🚀

@mtrezza mtrezza deleted the add-afterLogout-trigger branch November 16, 2019 04:39
UnderratedDev pushed a commit to UnderratedDev/parse-server that referenced this pull request Mar 21, 2020
* added afterLogout trigger

* added verification of session object in tests

* removed obsolete code

* removed unsued code

* improved tests to verify user ID
@vince1995
Copy link
Contributor

Sorry for pulling up this old thread but is there a reason fornow only the afterLogout trigger is allowed for the _Session class? Why not all the other triggers?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants