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

Mpi auth #36

Open
wants to merge 6 commits into
base: main
Choose a base branch
from
Open

Mpi auth #36

wants to merge 6 commits into from

Conversation

chris-krenz
Copy link

Description

Please include a summary of the change and which issue is fixed. Please also include relevant motivation and context. List any dependencies that are required for this change.

Checklist

  • This PR can be reviewed in under 30 minutes
  • My code follows the style guidelines of this project
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • I have assigned reviewers to this PR.

Copy link

github-actions bot commented Sep 13, 2024

Coverage report

St.
Category Percentage Covered / Total
🔴 Statements
2.73% (-0.44% 🔻)
27/990
🔴 Branches 0% 0/57
🔴 Functions
2.55% (-0.3% 🔻)
7/275
🔴 Lines
2.21% (-0.43% 🔻)
18/813
Show new covered files 🐣
St.
File Statements Branches Functions Lines
🔴
... / mpi.controller.ts
0% 100% 0% 0%
🔴 mpi/mpi.service.ts 0% 0% 0% 0%

Test suite run success

4 tests passing in 2 suites.

Report generated by 🧪jest coverage report action from e728551

Copy link
Member

@cbolles cbolles left a comment

Choose a reason for hiding this comment

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

It looks good, I added some suggestions to keep the code base cohesive


type eLabsStatus = 'PENDING' | 'PROGRESS' | 'COMPLETED';

@Controller('mpi')
Copy link
Member

Choose a reason for hiding this comment

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

Is there a reason this is written using REST instead of GraphQL?

Copy link
Author

Choose a reason for hiding this comment

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

I suppose just to reflect the MPI backend

src/mpi/mpi.service.ts Outdated Show resolved Hide resolved
src/mpi/mpi.service.ts Outdated Show resolved Hide resolved
src/mpi/mpi.service.ts Outdated Show resolved Hide resolved

@Injectable()
export class MPIService {
private tokenStore = new Map<string, { accessToken: string; refreshToken: string; accessTokenExpiration: Date }>();
Copy link
Member

Choose a reason for hiding this comment

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

This should probably be within MongoDB since we already have everything setup for it

const user = this.tokenStore.get(this.currentUserId);
if (user && user.accessToken && user.accessTokenExpiration) {
if (user.accessTokenExpiration > new Date(Date.now())) {
console.log('user ', user, ' has valid token');
Copy link
Member

Choose a reason for hiding this comment

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

Best to avoid having left over console.logs

src/mpi/mpi.service.ts Outdated Show resolved Hide resolved
src/mpi/mpi.service.ts Outdated Show resolved Hide resolved
src/mpi/mpi.service.ts Outdated Show resolved Hide resolved
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.

2 participants