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

feat: add the new auth service to prepare for the new sdk #1632

Merged

Conversation

martin-trajanovski
Copy link
Collaborator

@martin-trajanovski martin-trajanovski commented Oct 29, 2024

Description

The PR is made towards the "new-sdk-release" branch. The idea is to make the whole feature and migration easier for review and once after all the changes are merged to the new branch the PR towards the "master" branch will be opened where all the needed checks will be run to verify that everything works as expected.

This is the first part of migration towards the new generated sdk. It adds the needed services for cookies management in the frontend as the new sdk doesn't include it by default like the old one.

Motivation

It is part of the plan for new sdk migration that is generated against the new scicat (nestjs) backend and cleanup of the old one that was using the loopback api.

Fixes:

Changes:

  • Added new authorization and cookies management services

Tests included

  • Included for each change/fix?
  • Passing? (Merge will not be approved unless this is checked)

Documentation

  • swagger documentation updated [required]
  • official documentation updated [nice-to-have]

official documentation info

If you have updated the official documentation, please provide PR # and URL of the pages where the updates are included

Backend version

  • Does it require a specific version of the backend
  • which version of the backend is required:

Summary by Sourcery

Add a new authentication service to manage user sessions and tokens, preparing the application for integration with a new SDK. This includes the introduction of storage handling classes for cookies and local storage.

New Features:

  • Introduce a new AuthService to manage authentication tokens and user data, including methods for setting, getting, and clearing tokens.

Enhancements:

  • Add CookieBrowser and LocalStorageBrowser classes to handle cookie and local storage operations, respectively, for authentication purposes.

Summary by Sourcery

Add a new authentication service to manage user sessions and tokens, preparing the application for integration with a new SDK. This includes the introduction of storage handling classes for cookies and local storage.

New Features:

  • Introduce a new AuthService to manage authentication tokens and user data, including methods for setting, getting, and clearing tokens.

Enhancements:

  • Add CookieBrowser and LocalStorageBrowser classes to handle cookie and local storage operations, respectively, for authentication purposes.

@martin-trajanovski martin-trajanovski marked this pull request as ready for review October 31, 2024 07:58
Copy link

@sourcery-ai sourcery-ai bot left a comment

Choose a reason for hiding this comment

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

Hey @martin-trajanovski - I've reviewed your changes - here's some feedback:

Overall Comments:

  • Consider using Angular's built-in CookieService instead of direct DOM manipulation for better maintainability and consistency with the framework
  • Error handling could be more robust - consider implementing a proper error handling strategy instead of just console.logging errors
Here's what I looked at during the review
  • 🟡 General issues: 2 issues found
  • 🟢 Security: all looks good
  • 🟢 Testing: all looks good
  • 🟡 Complexity: 1 issue found
  • 🟢 Documentation: all looks good

Sourcery is free for open source - if you like our reviews please consider sharing them ✨
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.

src/app/shared/services/auth/auth.service.ts Show resolved Hide resolved
src/app/shared/services/auth/storage.browser.ts Outdated Show resolved Hide resolved
src/app/shared/services/auth/auth.service.ts Show resolved Hide resolved
Copy link
Member

@Junjiequan Junjiequan left a comment

Choose a reason for hiding this comment

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

Looks good to me in general, would be nice to have unit test for each methods in the future

@martin-trajanovski martin-trajanovski merged commit 0489f0d into new-sdk-release Nov 1, 2024
2 checks passed
@martin-trajanovski martin-trajanovski deleted the SWAP-4278-new-sdk-frotnend-code-refactor branch November 1, 2024 11:57
@nitrosx nitrosx added DCS DAPHNE Contribution to SciCat Release New SDK Refactoring FE to use autogenerated SDK labels Nov 1, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
DCS DAPHNE Contribution to SciCat Release New SDK Refactoring FE to use autogenerated SDK
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants