-
Notifications
You must be signed in to change notification settings - Fork 108
feat: add user flags, profile experiences and autocomplete for profile interim #2944
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
base: main
Are you sure you want to change the base?
Conversation
🍹 The Update (preview) for dailydotdev/api/prod (at 7cfac9f) was successful. Resource Changes Name Type Operation
~ vpc-native-generic-referral-reminder-cron kubernetes:batch/v1:CronJob update
~ vpc-native-temporal-deployment kubernetes:apps/v1:Deployment update
~ vpc-native-bg-deployment kubernetes:apps/v1:Deployment update
~ vpc-native-clean-zombie-images-cron kubernetes:batch/v1:CronJob update
~ vpc-native-update-source-public-threshold-cron kubernetes:batch/v1:CronJob update
- vpc-native-api-migration-c7950645 kubernetes:batch/v1:Job delete
~ vpc-native-sync-subscription-with-cio-cron kubernetes:batch/v1:CronJob update
~ vpc-native-clean-gifted-plus-cron kubernetes:batch/v1:CronJob update
~ vpc-native-update-highlighted-views-cron kubernetes:batch/v1:CronJob update
~ vpc-native-update-views-cron kubernetes:batch/v1:CronJob update
~ vpc-native-update-source-tag-view-cron kubernetes:batch/v1:CronJob update
~ vpc-native-update-current-streak-cron kubernetes:batch/v1:CronJob update
~ vpc-native-ws-deployment kubernetes:apps/v1:Deployment update
~ vpc-native-deployment kubernetes:apps/v1:Deployment update
~ vpc-native-clean-zombie-users-cron kubernetes:batch/v1:CronJob update
~ vpc-native-update-tag-recommendations-cron kubernetes:batch/v1:CronJob update
~ vpc-native-check-analytics-report-cron kubernetes:batch/v1:CronJob update
~ vpc-native-clean-zombie-user-companies-cron kubernetes:batch/v1:CronJob update
~ vpc-native-clean-stale-user-transactions-cron kubernetes:batch/v1:CronJob update
~ vpc-native-validate-active-users-cron kubernetes:batch/v1:CronJob update
~ vpc-native-personalized-digest-deployment kubernetes:apps/v1:Deployment update
~ vpc-native-update-tags-str-cron kubernetes:batch/v1:CronJob update
~ vpc-native-hourly-notification-cron kubernetes:batch/v1:CronJob update
~ vpc-native-calculate-top-readers-cron kubernetes:batch/v1:CronJob update
~ vpc-native-daily-digest-cron kubernetes:batch/v1:CronJob update
~ vpc-native-personalized-digest-cron kubernetes:batch/v1:CronJob update
~ vpc-native-generate-search-invites-cron kubernetes:batch/v1:CronJob update
~ vpc-native-private-deployment kubernetes:apps/v1:Deployment update
+ vpc-native-api-migration-be98f313 kubernetes:batch/v1:Job create
~ vpc-native-update-trending-cron kubernetes:batch/v1:CronJob update
|
src/entity/Company.ts
Outdated
@Column({ | ||
type: 'enum', | ||
enum: CompanyType, | ||
default: CompanyType.Business, | ||
}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Type should be string on psql side, we only use enum to validate in code and gql. This allows us to adjust the type more freely without changing column type.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See other examples eg. user_transacton.status
jobPreferences: Promise<UserJobPreferences>; | ||
|
||
@OneToMany(() => UserExperience, (experience) => experience.user) | ||
experiences: Promise<UserExperience[]>; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
onDelete
cascade?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some initial comments, quite a lot of entities added, see my comments and apply on sub entities/migrations as well, I did not want to repeat comments all over because it would just spam 😆
// !! Never send this field to FE, is only stored for better recommendation !! | ||
// Currency must be "ISO-4217" compliant | ||
// Amount is yearly based | ||
@Column({ type: 'jsonb', nullable: true }) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we can make it {}
as other jsonb fields I think
@Column({ default: false }) | ||
openToRelocation: boolean; | ||
|
||
// !! Never send this field to FE, is only stored for better recommendation !! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is controlled on gql type, so make sure its not included there so clients can't request it
src/entity/user/UserSkill.ts
Outdated
@BeforeInsert() | ||
@BeforeUpdate() | ||
generateSlug() { | ||
this.slug = slugify(this.name); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
see my comment about generated column, much better then before hooks of typeorm with only work per app instance
@Column({ | ||
type: 'enum', | ||
enum: UserExperienceType, | ||
nullable: false, | ||
}) | ||
type: UserExperienceType; | ||
|
||
@Column({ | ||
type: 'enum', | ||
enum: ExperienceStatus, | ||
default: ExperienceStatus.Draft, | ||
}) | ||
status: ExperienceStatus; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same comment as above, type should be string
@Column() | ||
userId: string; | ||
|
||
@ManyToOne(() => User, (user) => user.experiences) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
for relentionships use import type { User }
and string for entity:
@ManyToOne(() => User, (user) => user.experiences) | |
@ManyToOne('User', (user) => user.experiences) |
it helps with dependency cycles, please update other places for new entities
@Column({ type: 'jsonb', nullable: true }) | ||
associatedWith: { | ||
type: ExperienceAssociationType; | ||
id: string; | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FK can't be added on jsonb columns I think. Ideally we implement it through referenceId and specific columns workId, and educationId. Is it used anywhere in code, maybe we don't need to define it at all and see later?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a recurring association field for projects/publications/[...].
IMO we should define some kind of relationship, now or during MI-958, because that task will require it.
Maybe we can leave as is and check for it during deletion of experiences using CDC, wdyt?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If its FK you need then you will have to move it to columns as I said. CDC is not ideal better to do it on DB level with cascade.
src/schema/profileAutocomplete.ts
Outdated
throw new ValidationError(`Invalid autocomplete type: ${type}`); | ||
} | ||
|
||
if (!query || query.length < 2 || query.length > 100) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can you do validations over zod?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR implements a comprehensive user profile system with experience tracking and autocomplete functionality. It adds user experience entities for work, education, certifications, awards, publications, courses, and projects, along with job preferences and skills management. A new GraphQL autocomplete endpoint enables searching across various profile fields like job titles, companies, skills, and educational institutions.
- Introduces multiple user experience entity types with a hierarchical structure using TypeORM table inheritance
- Adds user job preferences with compensation tracking and work location preferences
- Implements a flexible autocomplete system supporting 10 different search types with proper validation
Reviewed Changes
Copilot reviewed 16 out of 16 changed files in this pull request and generated 4 comments.
Show a summary per file
File | Description |
---|---|
src/schema/profileAutocomplete.ts | Core autocomplete implementation with GraphQL schema, resolvers, and query mapping |
src/graphql.ts | Registers the new autocomplete schema and resolvers with the GraphQL server |
src/entity/user/experiences/*.ts | Defines experience entity hierarchy with base UserExperience and specialized child entities |
src/entity/user/UserSkill.ts | User skills entity with auto-generated slug and many-to-many relationship to experiences |
src/entity/user/UserJobPreferences.ts | Job preferences including compensation, location type, and role preferences |
src/entity/user/User.ts | Extends User entity with new relationships and location flags |
src/entity/Company.ts | Adds company type enum to distinguish between businesses and schools |
tests/profileAutocomplete.ts | Comprehensive test suite covering validation, skills, job titles, and company autocomplete |
Comments suppressed due to low confidence (1)
src/entity/user/UserJobPreferences.ts:39
- The TODO comment references a different ticket (MI-958) than the one mentioned in the PR description (MI-953). This could cause confusion about which ticket handles the sensitive data protection.
// todo: never send this field to FE while implementing MI-953
src/schema/profileAutocomplete.ts
Outdated
queryRunner.manager | ||
.getRepository(entity) | ||
.createQueryBuilder('entity') | ||
.select(select ?? ['id', propertyName]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The select clause uses propertyName
directly which may not work correctly with TypeORM's select method. TypeORM expects column names prefixed with the alias. This should be entity.${propertyName}
to match the query builder pattern.
.select(select ?? ['id', propertyName]) | |
.select(select ?? [`entity.id`, `entity.${propertyName}`]) |
Copilot uses AI. Check for mistakes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1
src/schema/profileAutocomplete.ts
Outdated
[AutocompleteType.CertificationName]: UserCertificationExperience; | ||
[AutocompleteType.CertificationIssuer]: Company; | ||
[AutocompleteType.AwardName]: UserAwardExperience; | ||
[AutocompleteType.AwardIssuer]: UserAwardExperience; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The mapping for AwardIssuer points to UserAwardExperience but queries the 'issuer' field which is a string property, not a company entity. This is inconsistent with other issuer types like CertificationIssuer which correctly map to Company entity.
[AutocompleteType.AwardIssuer]: UserAwardExperience; | |
[AutocompleteType.AwardIssuer]: Company; |
Copilot uses AI. Check for mistakes.
default: ExperienceStatus.Draft, | ||
}) | ||
status: ExperienceStatus; | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The flags field lacks documentation explaining its purpose, expected structure, or usage patterns. Given its generic type, documentation would help developers understand what kind of flags can be stored.
/** | |
* A JSON object to store additional metadata or flags related to the user experience. | |
* | |
* This field is flexible and can store various key-value pairs. Examples of possible keys: | |
* - "isFeatured": A boolean indicating if the experience is featured. | |
* - "priority": A number representing the priority level of the experience. | |
* - "tags": An array of strings for categorization or tagging. | |
* | |
* Developers should ensure that the keys and values stored in this field are consistent | |
* with the application's requirements and properly validated before use. | |
*/ |
Copilot uses AI. Check for mistakes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should be good to go once comments are addressed 👌
src/schema/profileAutocomplete.ts
Outdated
queryRunner.manager | ||
.getRepository(entity) | ||
.createQueryBuilder('entity') | ||
.select(select ?? ['id', propertyName]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1
src/schema/profileAutocomplete.ts
Outdated
query, | ||
limit, | ||
hits: hits.map((hit) => ({ | ||
__typename: typeNameByEntity[entity.name], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why do we need __typename
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've added one endpoint for Company, UserSkill and UserExperience.
GQL requires to add the typename in order to understand what I'm returning, I don't know how to avoid it (also linked to #2944 (comment) )
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm this is weird, lets take a look tomorrow, I don't think we do this anywhere (at least not manually like this), I see __typename
in some old parts of code but just on types not actual returns.
…lt value for description column
…953-profile-interim # Conflicts: # __tests__/profileAutocomplete.ts # src/migration/1753885628500-UserExperiences.ts
Added user profile user experiences, skills, flags, job preferences, and a new autocomplete endopoint.
Jira ticket
MI-953