Skip to content
This repository was archived by the owner on Mar 27, 2025. It is now read-only.

Feature/showcase 145#45

Merged
zoemaas merged 24 commits intodevelopfrom
feature/SHOWCASE-145
Mar 25, 2025
Merged

Feature/showcase 145#45
zoemaas merged 24 commits intodevelopfrom
feature/SHOWCASE-145

Conversation

@zoemaas
Copy link
Contributor

@zoemaas zoemaas commented Mar 12, 2025

No description provided.

zoemaas added 3 commits March 12, 2025 15:34
…pi into feature/SHOWCASE-145

# Conflicts:
#	apps/credential-showcase-api-server/src/database/migrations/0000_credential-showcase-api.sql
#	apps/credential-showcase-api-server/src/types/schema/index.ts
#	pnpm-lock.yaml
@zoemaas zoemaas self-assigned this Mar 12, 2025
@zoemaas zoemaas marked this pull request as ready for review March 12, 2025 15:08
}
}

export const newUserFrom = (user: UserRequest): NewUser => {
Copy link
Contributor

Choose a reason for hiding this comment

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

is this needed?

Copy link
Contributor

Choose a reason for hiding this comment

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

no this can be removed if we can map something 1:1

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed

Copy link
Contributor

Choose a reason for hiding this comment

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

We need a new migration instead of modify the existing 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.

Rolled back the changes and created a new migration file

@ronal2do
Copy link
Contributor

I've added few remarks, but looks good

Copy link
Contributor

@Brummos Brummos left a comment

Choose a reason for hiding this comment

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

we should also update the diagram with the User Class

openIdConnectUrl: <https://auth.example.com/.well-known/openid-configuration>
description: OpenID Connect security scheme for OAuth2 flows
schemas:
User:
Copy link
Contributor

Choose a reason for hiding this comment

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

why does user not have an id?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added id

}
}

export const newUserFrom = (user: UserRequest): NewUser => {
Copy link
Contributor

Choose a reason for hiding this comment

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

no this can be removed if we can map something 1:1

export const userDTOFrom = (user: User): UserDTO => {
return {
...user,
identifierType: user.identifierType ? user.identifierType : undefined,
Copy link
Contributor

Choose a reason for hiding this comment

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

user.identifierType ?? undefined

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Made it user.identifierType ?? undefined

return {
...user,
identifierType: user.identifierType ? user.identifierType : undefined,
identifier: user.identifier ? user.identifier : undefined,
Copy link
Contributor

Choose a reason for hiding this comment

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

user.identifier ?? undefined

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Made it user.identifier ?? undefined

credentialDefinitions: string[]
personas: string[]
bannerImage?: string | null
createdAt?: string | null
Copy link
Contributor

Choose a reason for hiding this comment

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

this cannot be correct. createdAt is not optional and you are not allowed to pass this into a "new" object. it is auto generated

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed

example: 2025-03-06T11:30:16.573Z
UserRequest:
type: object
properties:
Copy link
Contributor

Choose a reason for hiding this comment

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

why is identifierType not in here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added the idenfierType

export const users = pgTable('user', {
id: uuid('id').notNull().primaryKey().defaultRandom(),
identifierType: IdentifierTypePg('identifier_type').$type<IdentifierType>(),
identifier: text('identifier'),
Copy link
Contributor

Choose a reason for hiding this comment

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

you can omit the name as it will take the field name by default and that one is already correct

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Name omitted

'500':
$ref: '#/components/responses/InternalServerError'

/users:
Copy link
Contributor

Choose a reason for hiding this comment

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

showcase can now have a user? so i would have expected some changes on the showcase

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added user to showcase

"updated_at" timestamp DEFAULT now() NOT NULL,
CONSTRAINT "scenariosToPersonas_scenario_persona_pk" PRIMARY KEY("scenario","persona")
);

Copy link
Contributor

Choose a reason for hiding this comment

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

we need to generate additional migration files. so do not delete the old ones. pull in develop as we already have new files

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Created new migrations files after pulling in the changes

import { userDTOFrom, newUserFrom } from '../utils/mappers'
import UserService from '../services/UserService'

@JsonController('/users')
Copy link
Contributor

Choose a reason for hiding this comment

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

we need to have a controller test

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Created user controller tests

zoemaas added 4 commits March 18, 2025 10:45
…pi into feature/SHOWCASE-145

# Conflicts:
#	apps/credential-showcase-api-server/src/database/migrations/0003_credential-showcase-api.sql
#	apps/credential-showcase-api-server/src/database/migrations/meta/_journal.json
@github-actions
Copy link

Deployment Resource Location
API Server https://pr-45-api.dev.nborbit.ca/
Traction Adapter https://pr-45-traction.dev.nborbit.ca/

PR Deployment URLs ready for review.


return {
...showcaseResult,
...(showcaseResult as any), // TODO check this typing issue at a later point in time
Copy link
Contributor

Choose a reason for hiding this comment

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

the only negative point is this cast as any, but I'll move this PR

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've spent some time trying to fix it but it didn't work out, so I've just followed the pattern and appended a TODO to the code

@zoemaas zoemaas merged commit 3894b7a into develop Mar 25, 2025
6 checks passed
@zoemaas zoemaas deleted the feature/SHOWCASE-145 branch March 25, 2025 11:41
@zoemaas zoemaas restored the feature/SHOWCASE-145 branch March 26, 2025 14:48
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants