Skip to content

add setting to configure when to raise the user unload event and remove user always before requesting sign-out - #1391

Closed
pamapa wants to merge 1 commit into
mainfrom
fix-1379-v2
Closed

add setting to configure when to raise the user unload event and remove user always before requesting sign-out#1391
pamapa wants to merge 1 commit into
mainfrom
fix-1379-v2

Conversation

@pamapa

@pamapa pamapa commented Feb 7, 2024

Copy link
Copy Markdown
Member

Closes/fixes #1379

Checklist

  • This PR makes changes to the public API
  • I have included links for closing relevant issue numbers

@pamapa pamapa changed the title add setting to configure when to raise the user unload event and remove user always before requesting signout add setting to configure when to raise the user unload event and remove user always before requesting sign-out Feb 7, 2024
@pamapa pamapa added this to the 3.0.1 milestone Feb 7, 2024
@pamapa pamapa added the bug Something isn't working label Feb 7, 2024
@codecov

codecov Bot commented Feb 7, 2024

Copy link
Copy Markdown

Codecov Report

Attention: 1 lines in your changes are missing coverage. Please review.

Comparison is base (2a69f62) 79.78% compared to head (2f06982) 79.83%.

Files Patch % Lines
src/UserManagerEvents.ts 66.66% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1391      +/-   ##
==========================================
+ Coverage   79.78%   79.83%   +0.05%     
==========================================
  Files          45       45              
  Lines        1731     1741      +10     
  Branches      344      348       +4     
==========================================
+ Hits         1381     1390       +9     
- Misses        312      313       +1     
  Partials       38       38              
Flag Coverage Δ
unittests 79.83% <93.33%> (+0.05%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@pamapa

pamapa commented Feb 7, 2024

Copy link
Copy Markdown
Member Author

@PSanetra Can you review and test?

  • unload event is
    • deferred to callback (default, raiserUserUnloadEventBeforeSignoutRequest=false)
    • done before requesting the sign-out (option: raiserUserUnloadEventBeforeSignoutRequest=true)
  • remove user from storage is always (independent of option) done before requesting the sign-out

Any better name for raiserUserUnloadEventBeforeSignoutRequest?

Comment thread src/UserManager.ts

await this.removeUser();
logger.debug("user removed");
if (!this.settings.raiserUserUnloadEventBeforeSignoutRequest) {

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

maybe we should just call await this._removeUser(true) here, it should not hurt and is simpler (less code)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I think this should be ok as a signed in user would be unexpected in any case, right?

@PSanetra

PSanetra commented Feb 7, 2024

Copy link
Copy Markdown
Contributor

I think raiserUserUnloadEventBeforeSignoutRequest might be renamed to noSignoutCallbacksSupported or signoutCallbackSupported with true as default if the value is undefined.

The user/ developer is expected to set signoutCallbackSupported to false if he expects never to call any signout callback.

@pamapa

pamapa commented Feb 8, 2024

Copy link
Copy Markdown
Member Author

I think this solution is too complicated, i do not like an option for this at all. The previous behavior (<v3.0) was dead simple

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

UserManager signoutRedirect not working

2 participants