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

Invitation Code Verification Module #32

Closed

Conversation

MatthewTurk247
Copy link
Collaborator

@MatthewTurk247 MatthewTurk247 commented May 23, 2024

Invitation Code Verification Module

♻️ Current situation & Problem

This pull request introduces source code for a npm package that encapsulates the invitation code verification logic. The current implementation of invitation codes is tightly coupled with the PAWS app, making it difficult to reuse the functionality in other projects.

⚙️ Release Notes

  • Added a new InvitationCodeVerifier module that encapsulates the logic for verifying invitation codes and validating user invitation codes.
  • The module provides two methods:
    • verifyInvitationCode(invitationCode, userId): Verifies the provided invitation code for the given user ID.
    • validateUserInvitationCode(userId): Validates the invitation code for current user during account creation.

Example usage:

const admin = require("firebase-admin");
const InvitationCodeVerifier = require("./invitation-code-verifier");

admin.initializeApp();
const firestore = admin.firestore();
const invitationCodeVerifier = new InvitationCodeVerifier(firestore);

// Verify invitation code.
await invitationCodeVerifier.verifyInvitationCode(invitationCode, userId);

// Validate user.
await invitationCodeVerifier.validateUserInvitationCode(userId);

📚 Documentation

See #54.

✅ Testing

See spezi-invitation-codes/tests.

📝 Code of Conduct & Contributing Guidelines

By submitting creating this pull request, you agree to follow our Code of Conduct and Contributing Guidelines:

Copy link
Member

@PSchmiedmayer PSchmiedmayer left a comment

Choose a reason for hiding this comment

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

Thank you for starting with this effort @MatthewTurk247; nice first steps!

I had a few comments across the PR on how we might be generalize this + add some testing setup around this. I would suggest the same firebase emulator setup as we have for the other repos. We might need to extract some elements of these GitHub Actions into this repo.

The cloud functions could reference this file or package locally.

You can test the publishing with the GitHub Package Registry (https://docs.github.com/en/packages/working-with-a-github-packages-registry/working-with-the-npm-registry#installing-packages[…]-other-organizations) before we try to publish to npmjs to test out everything is working as expected.

spezi-invitation-code/index.js Outdated Show resolved Hide resolved
spezi-invitation-code/index.js Outdated Show resolved Hide resolved
spezi-invitation-code/index.js Outdated Show resolved Hide resolved
spezi-invitation-code/index.js Outdated Show resolved Hide resolved
spezi-invitation-code/index.js Outdated Show resolved Hide resolved
spezi-invitation-code/index.js Outdated Show resolved Hide resolved
spezi-invitation-code/index.js Outdated Show resolved Hide resolved
spezi-invitation-code/package.json Outdated Show resolved Hide resolved
Copy link

codecov bot commented May 26, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 61.39%. Comparing base (00ff0db) to head (8e6b624).

Current head 8e6b624 differs from pull request most recent head 49fccef

Please upload reports for the commit 49fccef to get more accurate results.

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff           @@
##             main      #32   +/-   ##
=======================================
  Coverage   61.39%   61.39%           
=======================================
  Files          19       19           
  Lines        1155     1155           
=======================================
  Hits          709      709           
  Misses        446      446           

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 00ff0db...49fccef. Read the comment docs.

Copy link
Member

@PSchmiedmayer PSchmiedmayer left a comment

Choose a reason for hiding this comment

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

Thank you for all the improvements @MatthewTurk247!

I added some comments before our meeting today to provide some additional feedback and context; this is going in the right direction, very nice!

spezi-invitation-code/firebase.json Outdated Show resolved Hide resolved
spezi-invitation-code/firebase.json.license Outdated Show resolved Hide resolved
spezi-invitation-code/firebasestorage.rules Outdated Show resolved Hide resolved
spezi-invitation-code/firebase.json Outdated Show resolved Hide resolved
spezi-invitation-code/index.js Show resolved Hide resolved
Comment on lines 14 to 27
jest.mock("firebase-admin", () => {
const firestore = {
doc: jest.fn(),
collection: jest.fn(),
runTransaction: jest.fn(),
};
return {
initializeApp: jest.fn(),
firestore: jest.fn(() => firestore),
app: jest.fn(() => ({
delete: jest.fn(),
})),
};
});
Copy link
Member

Choose a reason for hiding this comment

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

I am wondering: Do we even need to local firebase setup if we can mock all interactions here? I think it could still be helpful to actually test the "real" system integration.

It would be great to see this text executed in a GitHub Action.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

How do you suggest that I set up the GitHub Action to run in the spezi-invitation-code directory? This reusable workflow does not support a custom path, I believe: https://github.com/StanfordBDHG/.github/blob/main/.github/workflows/npm-test-and-coverage.yml

Copy link
Member

Choose a reason for hiding this comment

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

Sorry that I missed the message!

Fixed this in this release, you should be able to pass a working directory to the action: https://github.com/StanfordBDHG/.github/releases/tag/v2.0.59

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Cool, so I have passed in the working directory, but it looks like something is still going wrong with Node setup: https://github.com/StanfordSpezi/SpeziFirebase/actions/runs/9505720812/job/26201248460?pr=32

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Seems like Firebase is not initializing properly in the CI: https://github.com/StanfordSpezi/SpeziFirebase/actions/runs/9508175619/job/26208994158?pr=32

Any ideas for how to load credentials in (if that is the problem)? @PSchmiedmayer

Copy link
Member

@PSchmiedmayer PSchmiedmayer left a comment

Choose a reason for hiding this comment

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

@MatthewTurk247 I had a discussion about some invitation code logic with the ENAGE-HF team and we have a few smaller elements that I would like to add to this PR.

spezi-invitation-code/firebasestorage.rules Outdated Show resolved Hide resolved
spezi-invitation-code/firebasestorage.rules.license Outdated Show resolved Hide resolved
spezi-invitation-code/firebase.json Outdated Show resolved Hide resolved
spezi-invitation-code/index.js Outdated Show resolved Hide resolved
@PSchmiedmayer
Copy link
Member

@MatthewTurk247 Seems like the Spezi Firebase documentation on the Swift Package index is no longer available: https://swiftpackageindex.com/StanfordSpezi/SpeziFirebase. A bit of a strange thing, might just be that they are working on something or have updated some of their infrastructure. We can disregard the links to that documentation for now.

@PSchmiedmayer
Copy link
Member

The documentation is back online; the MD link check works again: https://github.com/StanfordSpezi/SpeziFirebase/actions/runs/9508175618/job/26244702663?pr=32

@PSchmiedmayer PSchmiedmayer added the enhancement New feature or request label Jun 26, 2024
@PSchmiedmayer
Copy link
Member

We are closing this PR for now as we focus on getting a more sophisticated setup in place for the ENGAGE-HF app. As we have finished that we can go ahead and extract this into smaller npm packages that could be shared across different Firebase-related packages. We should most likely create a Firebase Template Repo and a few reusable npm packages.

CC: @pauljohanneskraft

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

2 participants