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.

Sign up for GitHub

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

Jump to bottom

feat: throw error if duplicate map #1082

Draft
wants to merge 4 commits into
base: main
Choose a base branch
from
Draft

feat: throw error if duplicate map #1082

wants to merge 4 commits into from
+42 −0

Conversation

jef
Copy link

@jef jef commented Dec 11, 2024 •
edited
Loading

Closes #245

  • Tries to find if the hash of the .bsp is in the database. If it is, then it throws a ConflictException.

Screenshots (if applicable)

Checks

  • !! DONT IGNORE ME !! I have ran nx run db:create-migration <name> and committed the migration if I've made DB schema changes
  • I have included/updated tests where applicable (see Testing)
  • I have followed semantic commit messages e.g. feat: Add foo, chore: Update bar, etc...
  • My branch has a clear history of changes that can be easy to follow when being reviewed commit-by-commit
  • My branch is functionally complete; the only changes to be done will be those potentially requested in code review
  • All changes requested in review have been fixuped into my original commits

@jef jef marked this pull request as ready for review December 12, 2024 16:14
tsa96
tsa96 requested changes Dec 12, 2024
View reviewed changes
Copy link
Member

@tsa96 tsa96 left a comment

Choose a reason for hiding this comment

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

Thanks for first contribution! This check makes sense, though prefer .exists (see comments). Though we actually have two different endpoints that handle BSP uploads, the initial submission, and updated versions after then. So you need a check in MapsService.submitMapVersion as well, and E2E tests there as well.

The Volta thing I'm okay with, I'd been meaning to get round to pinning an Node version at some point anyway. Personally I'm using NVM but fine with switching.

apps/backend/src/app/modules/maps/maps.service.ts Outdated Show resolved Hide resolved
apps/backend/src/app/modules/maps/maps.service.spec.ts Outdated Show resolved Hide resolved
apps/backend/src/app/modules/maps/maps.service.ts Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
apps/backend/src/app/modules/maps/maps.service.spec.ts Outdated Show resolved Hide resolved
apps/backend/src/app/modules/maps/maps.service.spec.ts Outdated Show resolved Hide resolved
apps/backend/src/app/modules/maps/maps.service.ts Outdated Show resolved Hide resolved
@jef jef marked this pull request as draft December 12, 2024 21:32
@tsa96
Copy link
Member

tsa96 commented Dec 12, 2024

Something weird's happened with Git here - if you need a hand just ping me on Discord in #website!

@tsa96
Copy link
Member

tsa96 commented Dec 15, 2024

Just to clarify. might be that you just haven't been available over past few days, but in case you missed it, you seem to have botched the commits here, most stuff is now missing and we wouldn't accept a merge commit. No rush with the PR though!

@jef
Copy link
Author

jef commented Dec 16, 2024 •
edited
Loading

Just to clarify. might be that you just haven't been available over past few days, but in case you missed it, you seem to have botched the commits here, most stuff is now missing and we wouldn't accept a merge commit. No rush with the PR though!

Sorry, yes, I've been AFK and before I left only put the implementation and reverted everything and made a force push. I will update the E2E when I get the oomph again :D

@tsa96
Copy link
Member

tsa96 commented Dec 20, 2024

All good, no rush

@tsa96
Copy link
Member

tsa96 commented Dec 25, 2024

Can't tell what one cus on phone, but one of these is missing e2e test!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tsa96 tsa96 tsa96 requested changes

Requested changes must be addressed to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Hash the map on the backend and compare against other map hashes to prevent duplicate file uploading
2 participants
@jef @tsa96