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

Migrate tag api to new media model #312

Merged
merged 6 commits into from
Jun 13, 2023
Merged

Migrate tag api to new media model #312

merged 6 commits into from
Jun 13, 2023

Conversation

vnugent
Copy link
Contributor

@vnugent vnugent commented Jun 10, 2023

Issue #281

  • refactor: migrate tag mutations to work with new media model
  • feat: introduce permission checks to tag mutations

Clean up

  • refactor: add singleton factory function to data source classes
  • chore: remove unused code (I was going to do this in separate PR but the tag api changes touch a lot of other code)

@vnugent vnugent changed the title wip Migrate tag api to new media model Jun 10, 2023
export const createInstance = (): MutableClimbDataSource => new MutableClimbDataSource(getClimbModel())
static instance: MutableClimbDataSource

static getInstance (): MutableClimbDataSource {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

a singleton factory

chore: move datasources to singleton factory pattern
updateUserProfile: and(isOwner, isValidEmail)
updateUserProfile: and(isOwner, isValidEmail),
addEntityTag: or(isMediaOwner, isUserAdmin),
removeEntityTag: or(isMediaOwner, isUserAdmin)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

tag mutations now require permission checks

import { MUUID } from 'uuid-mongodb'
import { Point } from '@turf/helpers'

export type ImageFormatType = 'jpeg' | 'png' | 'webp' | 'avif'

export interface MediaObject {
_id: ObjectId
_id: mongoose.Types.ObjectId
Copy link
Contributor Author

Choose a reason for hiding this comment

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

we've been using ObjectId type incorrectly.

  • mongoose.Types.ObjectId is the typescript type and also the class to generate new mongo object id
  • mongoose.ObjectId should only be used for describing the schema

@vnugent vnugent marked this pull request as ready for review June 10, 2023 20:37
Copy link
Contributor Author

@vnugent vnugent left a comment

Choose a reason for hiding this comment

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

Reviving media tag API

],
"console": "integratedTerminal",
"internalConsoleOptions": "neverOpen",
},
Copy link
Contributor Author

Choose a reason for hiding this comment

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

removing Visual Studio code debugging configs related to media migration

Copy link
Contributor

@zichongkao zichongkao left a comment

Choose a reason for hiding this comment

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

Nice! I like the use of the singleton pattern. Should reduce memory usage!

src/model/MutableMediaDataSource.ts Outdated Show resolved Hide resolved
import { AreaType } from '../../db/AreaTypes.js'
import { EntityTag, MediaObject, MediaObjectGQLInput, AddTagEntityInput } from '../../db/MediaObjectTypes.js'
Copy link
Contributor

Choose a reason for hiding this comment

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

I've been favoring writing integration tests so that we can validate and document the graphQL layer as well rather than just data sources down. They are in src/__tests__/. We could add tests at both layers too, but I'd suggest writing the integration tests if we're only going to write one.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agreed. We can also test the permission layer that way. I've been writing data source tests out of habit. I'll fix it in a future PR.

@vnugent vnugent merged commit 050ee0f into develop Jun 13, 2023
@vnugent vnugent deleted the refactor-281 branch June 13, 2023 00:47
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.

3 participants