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 mongodb as a default database for application #92

Merged
merged 2 commits into from
Oct 11, 2023

Conversation

mimotej
Copy link
Collaborator

@mimotej mimotej commented Aug 27, 2023

Related issues

Resolves #60

Type of pull request

  • New feature
  • Bug fix
  • Refactoring
  • Feature update
  • Other

Describe your PR in detail

This PR adds mongodb database as a default database for the application. It utilizes mongodb atlas shared tier since it is fairly generous, later with the application would grow too large we might want to reconsider alternatives...
It uses Prisma as ORM
Also adds new states for status -> removed and banned, where removed users can reverify but banned users can't.

@mimotej mimotej requested a review from xvanick1 as a code owner August 27, 2023 16:19
@github-actions
Copy link

Hello 👋, thank you for opening new PR to OqixTS 💗

Please, wait till somebody will review your PR.

@github-actions github-actions bot added the review-needed New PR that needs to be reviewed label Aug 27, 2023
@mimotej mimotej changed the title wip: feat: Add mongodb as a default database for application draft: feat: Add mongodb as a default database for application Aug 27, 2023
@mimotej mimotej marked this pull request as draft August 27, 2023 16:22
@mimotej mimotej force-pushed the add-db branch 3 times, most recently from 8698569 to 3150425 Compare September 3, 2023 10:05
@sonarqubecloud
Copy link

sonarqubecloud bot commented Sep 9, 2023

SonarCloud Quality Gate failed.    Quality Gate failed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
28.8% 28.8% Duplication

idea Catch issues before they fail your Quality Gate with our IDE extension sonarlint SonarLint

@mimotej mimotej marked this pull request as ready for review September 9, 2023 16:15
@mimotej mimotej changed the title draft: feat: Add mongodb as a default database for application feat: Add mongodb as a default database for application Sep 9, 2023
@mimotej mimotej force-pushed the add-db branch 2 times, most recently from acda348 to 774aa8c Compare September 9, 2023 16:37
@mimotej
Copy link
Collaborator Author

mimotej commented Sep 9, 2023

Ok I think it is ready for the review.
/cc @xvanick1

src/model/schema.prisma Outdated Show resolved Hide resolved
Copy link
Collaborator

@xvanick1 xvanick1 left a comment

Choose a reason for hiding this comment

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

Suggestion for the possibility of re-verifying with the requirement of deleting old verification assigned to the thesis. Request to add the date of verification to the user object.

@mimotej
Copy link
Collaborator Author

mimotej commented Sep 16, 2023

Suggestion for the possibility of re-verifying with the requirement of deleting old verification assigned to the thesis. Request to add the date of verification to the user object.

Reverify option is little bit complicated since we need to define when is user able to reverify - For example there is big difference when the user is kicked from the server and when he leaves on its own. Is only situation when he can reverify when he leaves on its own or do we want to have option to "lift the ban". If the second option is true, then I would suggest leaving it for now and making it separate feature.

As for the the date of verification. Do you have something in mind how we would use it? Because first of all I would like to keep amount of data we store to minimum (mainly to protect students) and we also have currently quite a few people who are already verfied therefore simplest thing would be to add same date to each one, and I do not find that too useful 🤔 .

@xvanick1
Copy link
Collaborator

xvanick1 commented Sep 19, 2023

Suggestion for the possibility of re-verifying with the requirement of deleting old verification assigned to the thesis. Request to add the date of verification to the user object.

Reverify option is little bit complicated since we need to define when is user able to reverify - For example there is big difference when the user is kicked from the server and when he leaves on its own. Is only situation when he can reverify when he leaves on its own or do we want to have option to "lift the ban". If the second option is true, then I would suggest leaving it for now and making it separate feature.

As for the the date of verification. Do you have something in mind how we would use it? Because first of all I would like to keep amount of data we store to minimum (mainly to protect students) and we also have currently quite a few people who are already verfied therefore simplest thing would be to add same date to each one, and I do not find that too useful 🤔 .

In my opinion only re-verification when user leaves server should be available, as currently in this case, the record is still in database.
I thought a bit more about the re-verification feature. Discord has features kick & ban, as you mentioned. Afaik, kick is temporary, and ban is permanent. Therefore, every time user is kicked/is banned or leave the server, their verification should be automatically revoked in the database. Because if their kick period has ended, or ban is revoked, they should be able to join the server and verify again. I am unsure if this kind of automatization on "leave" is possible, but as you mentioned, this is a different feature.

Regarding the second request, I see it useful for security reasons when auditing suspicious behaviour... for example cases when there would be some leak from the server, we can filter out users verified in that period and check their behaviour about the leak.
For the users that are already verified, we can use the date of joining the server as this is available by default or just keep the field empty in their case.

@mimotej mimotej marked this pull request as draft September 23, 2023 15:59
@mimotej
Copy link
Collaborator Author

mimotej commented Sep 23, 2023

Converted back to draft since I am finishing the work on kicked and banned reverification and needs little bit more work.

@mimotej mimotej force-pushed the add-db branch 2 times, most recently from 4164fab to cccab70 Compare September 24, 2023 15:58
@sonarqubecloud
Copy link

sonarqubecloud bot commented Oct 1, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@mimotej
Copy link
Collaborator Author

mimotej commented Oct 1, 2023

Now we have also states removed and banned. These states work this way:
removed -> user kicked, left or ban was removed - user can verify again (also new user can verify using proper credentials.
banned -> user is not able to verify again.
verified -> no other user can verify using these credentials

@mimotej mimotej marked this pull request as ready for review October 1, 2023 16:00
@xvanick1
Copy link
Collaborator

/lgtm

@github-actions github-actions bot added lgtm PR was reviewed and can be merged and removed review-needed New PR that needs to be reviewed labels Oct 11, 2023
@mimotej mimotej merged commit 72b4c63 into OqixDevs:master Oct 11, 2023
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
lgtm PR was reviewed and can be merged
Development

Successfully merging this pull request may close these issues.

Feature request: Add database to Oqix
2 participants