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

Logout Refactor #4860

Merged
merged 51 commits into from
Aug 24, 2023
Merged

Logout Refactor #4860

merged 51 commits into from
Aug 24, 2023

Conversation

Tishasoumya-02
Copy link
Contributor

@Tishasoumya-02 Tishasoumya-02 commented Jun 9, 2023

@netlify
Copy link

netlify bot commented Jun 9, 2023

Deploy Preview for volto canceled.

Name Link
🔨 Latest commit 194f195
🔍 Latest deploy log https://app.netlify.com/sites/volto/deploys/64e488bac98f880008cae9e5

@cypress
Copy link

cypress bot commented Jun 9, 2023

Passing run #6937 ↗︎

0 557 20 0 Flakiness 0

Details:

Merge branch 'master' into logout-refactor
Project: Volto Commit: 194f195e27
Status: Passed Duration: 15:29 💡
Started: Aug 22, 2023 10:10 AM Ended: Aug 22, 2023 10:25 AM

This comment has been generated by cypress-bot as a result of this project's GitHub integration settings.

Copy link
Member

@nileshgulia1 nileshgulia1 left a comment

Choose a reason for hiding this comment

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

Please see my comments. Let me know if anything is unclear.

Copy link
Member

@JeffersonBledsoe JeffersonBledsoe left a comment

Choose a reason for hiding this comment

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

@Tishasoumya-02 Could we get a test/ include as part of one of the tests you have written already a check for the returnUrl feature of this component?

@Tishasoumya-02
Copy link
Contributor Author

Sorry @JeffersonBledsoe , I am not able to understand

@JeffersonBledsoe
Copy link
Member

Sorry @JeffersonBledsoe , I am not able to understand

@Tishasoumya-02 Your current code looks like this:

useEffect(() => {
    dispatch(logout());
    dispatch(purgeMessages());
});

This code means that the effect will run every time the component renders (more info in the React docs for useEffect). You probably want something like this which has the array of dependencies for the effect:

useEffect(() => {
    dispatch(logout());
    dispatch(purgeMessages());
}, [dispatch]);

While it's unlikely that this component will be around long enough for the effect to run multiple times, it is a possibility(and is even something React will do in dev mode).

* @export
* @return {{ token }}
*/
export function useToken() {
Copy link
Member

Choose a reason for hiding this comment

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

@Tishasoumya-02 Where is this useToken used?

Copy link
Member

Choose a reason for hiding this comment

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

I see you are accessing directly from redux state in your component. We can remove this!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes sure, I might have missed it

* @export
* @return {{ token }}
*/
export function useToken() {
Copy link
Member

Choose a reason for hiding this comment

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

I see you are accessing directly from redux state in your component. We can remove this!

@sneridagh sneridagh merged commit 5f7145f into master Aug 24, 2023
37 checks passed
@sneridagh sneridagh deleted the logout-refactor branch August 24, 2023 15:54
sneridagh added a commit that referenced this pull request Sep 11, 2023
* master: (25 commits)
  (feat):Update toc block entries (#5146)
  view cypress test (#5149)
  fix search block search results number (#5171)
  fix : RecursiveWidget is incorrectly translated (#4513)
  Bugfix : Responsive Error in login page  (#4309)
  Fix default template for content type which contains block with schem… (#5159)
  Updated Spanish translation (#5120)
  ContentRules add and edit forms for languages other than English (#5162)
  Fix SelectWidget throwing error when editing a recently created conte… (#5155)
  Don't show ``No value`` option in SelectWidget and ArrayWidget if default value is 0 (#5152)
  Fix storybook config for project generator. Add support for SCSS, upgrade to webpack 5 in there as well. (#5132)
  Fix linkcheckbroken 301 redirect to https://www.dlr.de/de (#5131)
  Release 17.0.0-alpha.25
  fix: Fix querystringResults subrequests id, to work properly in dupli… (#5071)
  Fix accessibility of the content folder buttons (#5101)
  Refactor Comment (#4874)
  Refactor Search Tags (#4873)
  Logout Refactor (#4860)
  Add www.dlr.de to README (#5112)
  Fix default toc renderer for nested entries (#5116)
  ...
sneridagh added a commit that referenced this pull request Sep 12, 2023
* master: (59 commits)
  (feat):Update toc block entries (#5146)
  view cypress test (#5149)
  fix search block search results number (#5171)
  fix : RecursiveWidget is incorrectly translated (#4513)
  Bugfix : Responsive Error in login page  (#4309)
  Fix default template for content type which contains block with schem… (#5159)
  Updated Spanish translation (#5120)
  ContentRules add and edit forms for languages other than English (#5162)
  Fix SelectWidget throwing error when editing a recently created conte… (#5155)
  Don't show ``No value`` option in SelectWidget and ArrayWidget if default value is 0 (#5152)
  Fix storybook config for project generator. Add support for SCSS, upgrade to webpack 5 in there as well. (#5132)
  Fix linkcheckbroken 301 redirect to https://www.dlr.de/de (#5131)
  Release 17.0.0-alpha.25
  fix: Fix querystringResults subrequests id, to work properly in dupli… (#5071)
  Fix accessibility of the content folder buttons (#5101)
  Refactor Comment (#4874)
  Refactor Search Tags (#4873)
  Logout Refactor (#4860)
  Add www.dlr.de to README (#5112)
  Fix default toc renderer for nested entries (#5116)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants