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

Update/ux reloading web pages #111

Merged
merged 14 commits into from
Oct 2, 2023
Merged

Update/ux reloading web pages #111

merged 14 commits into from
Oct 2, 2023

Conversation

osala-eng
Copy link
Contributor

Description

Please include a summary of the change, e.g. what the new feature # is and/or what bug # it fixes. Please also include relevant motivation and context. List any dependencies that are required for this change.

Fixes/Implements #(issue/feature)
Fixes page goes to login page on page reload even if you have already logged in.

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Refactor and code improvement (non-breaking change which improves code quality and/or performance)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Documentation
  • Tests
  • Other chores such as maintenance

How Has This Been Tested?

Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration by modifying the list below.

  • Test A
  • Test B
  • Test C

Test Configuration:

Please describe the test setup. List them below as bullet points.

Checklist:

  • My code follows the style guidelines of this project
  • I have performed a self-review of my own code
  • I have commented my code in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes
  • Any dependent changes have been merged and published in downstream modules
  • I have checked my code and corrected any misspellings
  • The commit message follows the convention of this project

@osala-eng osala-eng requested a review from a team as a code owner September 27, 2023 10:39
@osala-eng osala-eng linked an issue Sep 27, 2023 that may be closed by this pull request
@osala-eng osala-eng mentioned this pull request Sep 27, 2023
20 tasks
Copy link
Contributor

@janaka janaka left a comment

Choose a reason for hiding this comment

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

As discussed, remove the auth autologin and auth session cache decorators and implement the needed logic inside the methods.

Handle cookie encrypt secret validation failed in setup, don't throw an error, render a friendly Docq failed to start type message.

@osala-eng
Copy link
Contributor Author

osala-eng commented Sep 27, 2023

Added a friendly UI to show when docq fails to initialize.

image

@janaka
Copy link
Contributor

janaka commented Sep 28, 2023

Added a friendly UI to show when docq fails to initialize.

image

Can you change the copy to: "Something went wrong starting Docq."

@osala-eng
Copy link
Contributor Author

Can you change the copy to: "Something went wrong starting Docq."

Okay

web/utils/layout.py Outdated Show resolved Hide resolved
@janaka
Copy link
Contributor

janaka commented Sep 28, 2023

I left a few comments but feel like the structure is still off. Some of it is just confusion caused by naming conventions. But also cache setting and getting seems a bit spaghetti which could accidently cause a critical security regression in the future. That's my concern.

Maybe it's easier for me to look dive into the code a bit deeper first before you make any changes based on my comments.

So please hold off making any adjustments until I say so. 🙏🏾

web/utils/handlers.py Outdated Show resolved Hide resolved
janaka added 3 commits October 1, 2023 15:37
* refactor: auth token based logic

* fix cookie setting

* refactor: remove AUTO_LOGIN as feature type

adding ability to toggle token based auth per org is making it more brittle and risky

* refactor: rename env vars to prefix "DOCQ_"

* refactor: rename session cookie var to be more specific

* tests: fix auth utils tests after refactor

* fix: remove _auto_login_feature_enabled

* chore: tweak debug logs in auth utils
refactor: cache var names
chore: adjust logging not to leak hmac session id into logs.
@janaka janaka merged commit a6821a6 into main Oct 2, 2023
3 checks passed
@janaka janaka deleted the update/ux-reloading-web-pages branch October 2, 2023 01:26
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.

UX: Reloading pages forces the users to login
2 participants