fix(university-gateway): Include attachmentKey when sending attachments #16631
Conversation
Add otherDocuments when creating application (extraApplicationField) Fix when setting chosenProgram Remove unecessary mapping for file types
WalkthroughThis pull request includes a migration script that modifies the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
Datadog ReportAll test runs ✅ 5 Total Test Services: 0 Failed, 5 Passed Test Services
🔻 Code Coverage Decreases vs Default Branch (1)
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Outside diff range and nitpick comments (15)
libs/university-gateway/src/lib/model/programExtraApplicationField.ts (1)
Line range hint 3-13: Consider adding JSDoc documentation to the interface.
Adding documentation would improve maintainability by explaining:
- The purpose of the interface
- The meaning and expected format of each field
- The relationship with university APIs
Example improvement:
+/**
+ * Represents an extra field in a university program application.
+ * These fields are defined by universities to collect additional information from applicants.
+ */
export interface IProgramExtraApplicationField {
+ /** Unique key provided by the university's API to identify this field */
externalKey: string
+ /** Field name in Icelandic */
nameIs: string
+ /** Field name in English */
nameEn: string
// ... add documentation for remaining fields
}apps/services/university-gateway/migrations/20241028110548-change-program-extra-application-field-external-key.js (2)
1-2: Remove redundant 'use strict' directive
The 'use strict' directive is unnecessary as JavaScript modules are automatically in strict mode.
-'use strict'
-🧰 Tools
🪛 Biome
[error] 1-1: Redundant use strict directive.
The entire contents of JavaScript modules are automatically in strict mode, with no statement needed to initiate it.
Safe fix: Remove the redundant use strict directive.
(lint/suspicious/noRedundantUseStrict)
24-42: LGTM! Consider adding a comment about constraint implications
The rollback migration is well-structured and provides a clean reversal of the changes. Consider adding a comment explaining the implications of the NOT NULL constraint with default '' for future maintainers.
down(queryInterface, Sequelize) {
return queryInterface.sequelize.query(`
BEGIN;
+ -- Note: This assumes all external_key values are non-null
-- Add back the old columnlibs/application/templates/university/src/shared/types.ts (1)
62-65: Add JSDoc comments to document the type
The type definition looks good and follows TypeScript best practices. Consider adding JSDoc comments to document the purpose of this type and its properties.
Add documentation like this:
+/**
+ * Represents an extra application field for university programs
+ */
type ProgramExtraApplicationField = {
+ /** External key from the university API */
externalKey: string
+ /** Type of the application field */
fieldType: ProgramExtraApplicationFieldFieldTypeEnum
}libs/application/templates/university/src/fields/OtherDocuments/index.tsx (2)
29-29: Consider adding more context to the TODO comment.
The TODO comment lacks specificity about what types of fields need to be displayed and why. This could make it difficult for other developers to understand and implement the enhancement.
Would you like me to help create a GitHub issue to track this enhancement with more detailed requirements?
Line range hint 1-50: Consider improving reusability and type safety.
While the component follows most best practices, there are a few areas for improvement:
- The cast of
application.answers as UniversityAnswerscould be made type-safe using a type guard. - The hardcoded language check (
lang === 'is') could be made more reusable by accepting supported languages as props.
Consider implementing a type guard:
function isUniversityAnswers(answers: unknown): answers is UniversityAnswers {
return answers !== null && typeof answers === 'object' && 'someRequiredProp' in answers
}
// Usage
const answers = isUniversityAnswers(application.answers)
? application.answers
: null
if (!answers) {
return null // or handle error case
}apps/services/university-gateway/src/app/modules/application/dto/createApplicationDto.ts (1)
116-116: Update API documentation for fileUrl property.
While the rename from url to fileUrl improves clarity, the API documentation still references "s3 file". Consider making the documentation more generic since the storage backend could change.
@ApiProperty({
- description: 'A public download link to a s3 file',
+ description: 'A public download URL for the file',
example: '',
})
fileUrl!: stringlibs/application/templates/university/src/fields/ProgramSelection/index.tsx (1)
Line range hint 1-190: Consider performance optimizations and enhanced reusability.
To align with the coding guidelines for reusable components:
- Memoize expensive computations:
- The sorted programs transformation
- The university options mapping
- Extract and export types:
- Program selection props
- University data structure
Consider these improvements:
// Add these type exports
export interface ProgramSelectionProps extends FieldBaseProps {
// ... existing props
}
// Memoize expensive computations
const sortedPrograms = useMemo(() =>
sortedProgramsDeepCopy
.sort((x, y) => {
if (lang === 'is' && x.nameIs > y.nameIs) return 1
else if (lang === 'en' && x.nameEn > y.nameEn) return 1
else return -1
})
.filter((program) => program.universityId === chosenUniversity),
[sortedProgramsDeepCopy, lang, chosenUniversity]
)
// Memoize callbacks
const handleUniversitySelect = useCallback((value: string) => {
ChooseUniversity(value)
}, [ChooseUniversity])libs/clients/university-application/reykjavik-university/src/lib/reykjavikUniversityClient.service.ts (1)
Line range hint 114-120: Update error messages to use consistent terminology.
The error messages still reference externalId. Consider updating them to use externalKey for consistency.
- `EnumError when trying to map program with externalId ${program.externalId} for university (reykjavik-university), update skipped.`,
+ `EnumError when trying to map program with externalKey ${program.externalId} for university (reykjavik-university), update skipped.`,
- `Failed to map program with externalId ${program.externalId} for university (reykjavik-university), reason:`,
+ `Failed to map program with externalKey ${program.externalId} for university (reykjavik-university), reason:`,apps/services/university-gateway/src/app/modules/program/internalProgram.service.ts (2)
Line range hint 1-238: Add unit tests for program update flow.
While the implementation looks solid, consider adding unit tests to verify:
- The program update flow with the new
externalKeyfield - Error handling scenarios during program updates
- Edge cases with specializations and extra application fields
Would you like me to help generate unit test cases for these scenarios?
Line range hint 42-238: Consider optimizing database operations.
The service performs multiple sequential database operations within loops. Consider:
- Using bulk operations for mode of delivery and extra application field updates
- Implementing transaction boundaries for atomic updates
Example optimization for extra application fields:
- // 3b. CREATE program extra application field
- for (let k = 0; k < extraApplicationFieldList.length; k++) {
- await this.programExtraApplicationFieldModel.create({
- programId: programId,
- externalKey: extraApplicationFieldList[k].externalKey,
- // ... other fields
- })
- }
+ // 3b. Bulk create program extra application fields
+ await this.programExtraApplicationFieldModel.bulkCreate(
+ extraApplicationFieldList.map((field) => ({
+ programId,
+ externalKey: field.externalKey,
+ // ... other fields
+ }))
+ )apps/services/university-gateway/src/app/modules/application/universityApplication.service.ts (2)
Line range hint 144-144: Remove hard-coded externalId value
The hard-coded value 'testid124' for externalId should be removed as it appears to be a testing artifact. This could interfere with proper application tracking in production.
- externalId: 'testid124',
+ externalId: undefined, // Will be set after university creates the applicationInconsistent naming convention detected across the codebase
The codebase shows mixed usage of both externalId and externalKey. The PR's objective to rename externalId to externalKey is only partially implemented:
externalIdis still used in program-related interfaces (IProgram,IProgramSpecialization)externalKeyis used in application-related interfaces (IApplicationExtraFields,IProgramExtraApplicationField)- The file under review (
universityApplication.service.ts) still usesexternalId
This inconsistency needs to be addressed systematically across the codebase to maintain a uniform naming convention.
Locations needing attention:
libs/university-gateway/src/lib/model/program.tslibs/university-gateway/src/lib/model/programSpecialization.tsapps/services/university-gateway/src/app/modules/application/universityApplication.service.tsapps/services/university-gateway/src/app/modules/application/model/application.ts
🔗 Analysis chain
Line range hint 144-144: Update externalId references to externalKey
The PR objectives mention renaming externalId to externalKey, but several instances of externalId remain in the code. This needs to be updated for consistency.
Let's verify other occurrences of this field name:
Update the following instances:
- externalId: 'testid124',
+ externalKey: undefined,
- attributes: ['externalId', 'universityId'],
+ attributes: ['externalKey', 'universityId'],
- if (!application.externalId) {
+ if (!application.externalKey) {
throw new Error(
- `Application with id ${applicationId} does not have external id set`,
+ `Application with id ${applicationId} does not have external key set`,
)
}Also applies to: 191-194, 220-223
Let me gather more context about the specific usage patterns to better understand the migration status.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for remaining externalId references that need to be updated
rg -l 'externalId' --type ts
# Search for new externalKey references to ensure consistency
rg -l 'externalKey' --type ts
Length of output: 2316
Script:
#!/bin/bash
# Check the actual usage patterns of externalId and externalKey
rg 'externalId' --type ts -C 2
# Check if there are any TypeScript interfaces or types defining these fields
ast-grep --pattern 'interface $_ {
$$$
externalId: $_
$$$
}'
ast-grep --pattern 'interface $_ {
$$$
externalKey: $_
$$$
}'
Length of output: 31072
libs/application/template-api-modules/src/lib/modules/templates/university/university.service.ts (2)
Line range hint 122-130: Address the TODO comments for 'city' and 'countryCode' fields
There are TODO comments indicating uncertainty about the values for city and countryCode. Please ensure that these fields are populated correctly, possibly by retrieving city from a reliable source and countryCode from the residenceHistory in the data provider.
Would you like assistance in implementing the logic to fetch these values?
260-260: Implement handling for additional field types
The TODO comment indicates that other field types need to be handled. To ensure full functionality, please implement the necessary logic for these additional field types.
Would you like assistance in adding support for these field types?
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (19)
- apps/services/university-gateway/migrations/20241028110548-change-program-extra-application-field-external-key.js (1 hunks)
- apps/services/university-gateway/src/app/modules/application/dto/createApplicationDto.ts (3 hunks)
- apps/services/university-gateway/src/app/modules/application/internalApplication.service.ts (1 hunks)
- apps/services/university-gateway/src/app/modules/application/universityApplication.service.ts (2 hunks)
- apps/services/university-gateway/src/app/modules/program/internalProgram.service.ts (1 hunks)
- apps/services/university-gateway/src/app/modules/program/model/programExtraApplicationField.ts (1 hunks)
- apps/web/screens/queries/UniversityGateway.ts (1 hunks)
- libs/api/domains/university-gateway/src/lib/graphql/models/program.model.ts (1 hunks)
- libs/application/template-api-modules/src/lib/modules/templates/university/university.service.ts (5 hunks)
- libs/application/templates/university/src/fields/OtherDocuments/index.tsx (1 hunks)
- libs/application/templates/university/src/fields/ProgramSelection/index.tsx (1 hunks)
- libs/application/templates/university/src/forms/UniversityForm/FormerEducation/index.ts (1 hunks)
- libs/application/templates/university/src/shared/types.ts (2 hunks)
- libs/clients/university-application/reykjavik-university/src/lib/reykjavikUniversityClient.service.ts (1 hunks)
- libs/clients/university-application/university-of-iceland/src/lib/universityOfIcelandClient.service.ts (2 hunks)
- libs/clients/university-application/university-of-iceland/src/lib/utils/mapUglaApplication.ts (3 hunks)
- libs/clients/university-application/university-of-iceland/src/lib/utils/mapUglaPrograms.ts (2 hunks)
- libs/university-gateway/src/lib/model/application.ts (3 hunks)
- libs/university-gateway/src/lib/model/programExtraApplicationField.ts (1 hunks)
✅ Files skipped from review due to trivial changes (2)
- apps/services/university-gateway/src/app/modules/application/internalApplication.service.ts
- libs/clients/university-application/university-of-iceland/src/lib/universityOfIcelandClient.service.ts
🧰 Additional context used
📓 Path-based instructions (17)
apps/services/university-gateway/migrations/20241028110548-change-program-extra-application-field-external-key.js (2)
Pattern apps/services/**/*: "Confirm that the code adheres to the following:
- NestJS architecture, including modules, services, and controllers.
- Dependency injection patterns and service encapsulation.
- Integration and unit testing coverage and practices."
Pattern apps/**/*: "Confirm that the code adheres to the following:
- NextJS best practices, including file structure, API routes, and static generation methods.
- Efficient state management and server-side rendering techniques.
- Optimal use of TypeScript for component and utility type safety."
apps/services/university-gateway/src/app/modules/application/dto/createApplicationDto.ts (2)
Pattern apps/services/**/*: "Confirm that the code adheres to the following:
- NestJS architecture, including modules, services, and controllers.
- Dependency injection patterns and service encapsulation.
- Integration and unit testing coverage and practices."
Pattern apps/**/*: "Confirm that the code adheres to the following:
- NextJS best practices, including file structure, API routes, and static generation methods.
- Efficient state management and server-side rendering techniques.
- Optimal use of TypeScript for component and utility type safety."
apps/services/university-gateway/src/app/modules/application/universityApplication.service.ts (2)
Pattern apps/services/**/*: "Confirm that the code adheres to the following:
- NestJS architecture, including modules, services, and controllers.
- Dependency injection patterns and service encapsulation.
- Integration and unit testing coverage and practices."
Pattern apps/**/*: "Confirm that the code adheres to the following:
- NextJS best practices, including file structure, API routes, and static generation methods.
- Efficient state management and server-side rendering techniques.
- Optimal use of TypeScript for component and utility type safety."
apps/services/university-gateway/src/app/modules/program/internalProgram.service.ts (2)
Pattern apps/services/**/*: "Confirm that the code adheres to the following:
- NestJS architecture, including modules, services, and controllers.
- Dependency injection patterns and service encapsulation.
- Integration and unit testing coverage and practices."
Pattern apps/**/*: "Confirm that the code adheres to the following:
- NextJS best practices, including file structure, API routes, and static generation methods.
- Efficient state management and server-side rendering techniques.
- Optimal use of TypeScript for component and utility type safety."
apps/services/university-gateway/src/app/modules/program/model/programExtraApplicationField.ts (2)
Pattern apps/services/**/*: "Confirm that the code adheres to the following:
- NestJS architecture, including modules, services, and controllers.
- Dependency injection patterns and service encapsulation.
- Integration and unit testing coverage and practices."
Pattern apps/**/*: "Confirm that the code adheres to the following:
- NextJS best practices, including file structure, API routes, and static generation methods.
- Efficient state management and server-side rendering techniques.
- Optimal use of TypeScript for component and utility type safety."
apps/web/screens/queries/UniversityGateway.ts (1)
Pattern apps/**/*: "Confirm that the code adheres to the following:
- NextJS best practices, including file structure, API routes, and static generation methods.
- Efficient state management and server-side rendering techniques.
- Optimal use of TypeScript for component and utility type safety."
libs/api/domains/university-gateway/src/lib/graphql/models/program.model.ts (1)
Pattern libs/**/*: "Confirm that the code adheres to the following:
- Reusability of components and hooks across different NextJS apps.
- TypeScript usage for defining props and exporting types.
- Effective tree-shaking and bundling practices."
libs/application/template-api-modules/src/lib/modules/templates/university/university.service.ts (1)
Pattern libs/**/*: "Confirm that the code adheres to the following:
- Reusability of components and hooks across different NextJS apps.
- TypeScript usage for defining props and exporting types.
- Effective tree-shaking and bundling practices."
libs/application/templates/university/src/fields/OtherDocuments/index.tsx (1)
Pattern libs/**/*: "Confirm that the code adheres to the following:
- Reusability of components and hooks across different NextJS apps.
- TypeScript usage for defining props and exporting types.
- Effective tree-shaking and bundling practices."
libs/application/templates/university/src/fields/ProgramSelection/index.tsx (1)
Pattern libs/**/*: "Confirm that the code adheres to the following:
- Reusability of components and hooks across different NextJS apps.
- TypeScript usage for defining props and exporting types.
- Effective tree-shaking and bundling practices."
libs/application/templates/university/src/forms/UniversityForm/FormerEducation/index.ts (1)
Pattern libs/**/*: "Confirm that the code adheres to the following:
- Reusability of components and hooks across different NextJS apps.
- TypeScript usage for defining props and exporting types.
- Effective tree-shaking and bundling practices."
libs/application/templates/university/src/shared/types.ts (1)
Pattern libs/**/*: "Confirm that the code adheres to the following:
- Reusability of components and hooks across different NextJS apps.
- TypeScript usage for defining props and exporting types.
- Effective tree-shaking and bundling practices."
libs/clients/university-application/reykjavik-university/src/lib/reykjavikUniversityClient.service.ts (1)
Pattern libs/**/*: "Confirm that the code adheres to the following:
- Reusability of components and hooks across different NextJS apps.
- TypeScript usage for defining props and exporting types.
- Effective tree-shaking and bundling practices."
libs/clients/university-application/university-of-iceland/src/lib/utils/mapUglaApplication.ts (1)
Pattern libs/**/*: "Confirm that the code adheres to the following:
- Reusability of components and hooks across different NextJS apps.
- TypeScript usage for defining props and exporting types.
- Effective tree-shaking and bundling practices."
libs/clients/university-application/university-of-iceland/src/lib/utils/mapUglaPrograms.ts (1)
Pattern libs/**/*: "Confirm that the code adheres to the following:
- Reusability of components and hooks across different NextJS apps.
- TypeScript usage for defining props and exporting types.
- Effective tree-shaking and bundling practices."
libs/university-gateway/src/lib/model/application.ts (1)
Pattern libs/**/*: "Confirm that the code adheres to the following:
- Reusability of components and hooks across different NextJS apps.
- TypeScript usage for defining props and exporting types.
- Effective tree-shaking and bundling practices."
libs/university-gateway/src/lib/model/programExtraApplicationField.ts (1)
Pattern libs/**/*: "Confirm that the code adheres to the following:
- Reusability of components and hooks across different NextJS apps.
- TypeScript usage for defining props and exporting types.
- Effective tree-shaking and bundling practices."
🪛 Biome
apps/services/university-gateway/migrations/20241028110548-change-program-extra-application-field-external-key.js
[error] 1-1: Redundant use strict directive.
The entire contents of JavaScript modules are automatically in strict mode, with no statement needed to initiate it.
Safe fix: Remove the redundant use strict directive.
(lint/suspicious/noRedundantUseStrict)
🔇 Additional comments (28)
libs/university-gateway/src/lib/model/programExtraApplicationField.ts (1)
4-4: LGTM! Property renaming aligns with PR objectives.
The renaming from externalId to externalKey correctly reflects that this is a key from university APIs rather than an identifier.
libs/application/templates/university/src/forms/UniversityForm/FormerEducation/index.ts (2)
Line range hint 1-21: LGTM! Code follows TypeScript and modular design principles.
The implementation:
- Uses TypeScript for type safety
- Follows modular design with separate section components
- Uses reusable builder pattern via
buildSection - Maintains clean separation of concerns
19-19: Verify OtherDocumentsSection implementation for attachment handling.
Since this section is now enabled and will handle attachments, we should verify its implementation aligns with the new attachment handling requirements.
Let's check the OtherDocuments implementation:
✅ Verification successful
Let me gather more information about the attachment handling implementation.
OtherDocumentsSection implementation is properly configured for attachment handling
The section is correctly implemented with:
- Proper attachment field configuration using the standard upload component
- Conditional rendering based on program requirements (
extraApplicationFields) - Integration with the application's data schema that includes attachment support
- Correct file structure and TypeScript typing
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify OtherDocumentsSection implementation and its attachment handling
# Test 1: Check if OtherDocumentsSection uses the new externalKey property
rg -A 10 "OtherDocumentsSection" "libs/application/templates/university/src"
# Test 2: Look for attachment handling in the section
ast-grep --pattern 'export const OtherDocumentsSection = $$$'
Length of output: 7351
Script:
#!/bin/bash
# Check for attachment handling in the OtherDocuments component
rg -A 5 "component: 'OtherDocuments'" --type ts
# Look for the actual OtherDocuments component implementation
fd -e ts -e tsx OtherDocuments
# Check for attachment-related code in the component
rg -A 10 "attachment" --type ts libs/application/templates/university/src
Length of output: 3494
apps/services/university-gateway/migrations/20241028110548-change-program-extra-application-field-external-key.js (1)
4-22: Verify data integrity constraints and consider adding indexes
While the migration is generally well-structured with transaction support, there are a few considerations:
- The
NOT NULLconstraint with default''might not be appropriate ifexternal_idcontained NULL values - Consider adding an index if this column will be used in queries
libs/university-gateway/src/lib/model/application.ts (4)
3-3: LGTM: Import addition is well-placed and necessary
The new FieldType import aligns with the interface changes and follows the established import pattern.
58-60: Breaking changes in IApplicationExtraFields interface require careful verification
The interface has undergone significant changes:
- Replaced
keywithfieldType: FieldTypefor better type safety - Added
externalKey: stringto align with the PR objectives
These changes will require updates to all implementations of this interface.
#!/bin/bash
# Search for implementations and usages of IApplicationExtraFields
ast-grep --pattern 'interface $_ implements IApplicationExtraFields'
rg -A 5 "IApplicationExtraFields.*\{|extraFieldList.*\["54-54: Verify all usages of the renamed url property
The property rename from url to fileUrl improves clarity but requires verification of all usage locations.
#!/bin/bash
# Search for any remaining usage of 'url' property in attachment contexts
rg -A 3 "attachment.*\.url|url.*attachment"17-17: Verify the impact of removing undefined from attachments array type
The type change from Array<IApplicationAttachment | undefined> to Array<IApplicationAttachment> makes the type more strict. While this is generally good practice, we should verify that no code relies on undefined values within the array.
✅ Verification successful
Let me gather more information about the attachments usage.
Based on the search results, I can now generate the final response:
Type change from Array<IApplicationAttachment | undefined> to Array<IApplicationAttachment> is safe
The codebase analysis shows that:
- All usages of attachments array properly handle the optional nature through the parent property using the
?.operator - No code relies on undefined values within the array itself
- The attachments are always processed safely with null checks before access
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any usage of attachments array that might expect undefined values
rg -l "attachments\?.*\[" | xargs rg -A 5 "attachments.*\[.*\]|attachments.*filter|attachments.*map"
Length of output: 2892
Script:
#!/bin/bash
# Search for direct usage of attachments array
rg -A 5 "\.attachments\?"
# Search for any file operations or transformations on attachments
ast-grep --pattern 'attachments.$$$map($$$)'
ast-grep --pattern 'attachments.$$$filter($$$)'
ast-grep --pattern 'attachments.$$$forEach($$$)'
# Search for attachment related interfaces and their usage
rg -A 5 "IApplicationAttachment.*\{|ApplicationAttachment.*\{"
Length of output: 49805
libs/application/templates/university/src/shared/types.ts (1)
7-7: LGTM! Import statement follows best practices
The specific enum import supports effective tree-shaking and bundling.
apps/web/screens/queries/UniversityGateway.ts (1)
103-103: Verify complete migration from externalId to externalKey
The addition of externalKey aligns with the PR objectives. However, I notice that externalId might still be present in other parts of the queries.
Let's verify the complete migration:
libs/api/domains/university-gateway/src/lib/graphql/models/program.model.ts (2)
Line range hint 1-168: TypeScript implementation follows best practices.
The code demonstrates excellent TypeScript usage:
- Proper use of decorators for GraphQL schema definition
- Clear type definitions for all fields
- Exported types that can be reused across the application
149-149: Field renaming looks good but verify GraphQL schema changes.
The renaming from externalId to externalKey is consistent with the PR objectives. This change better reflects that this is a key from university APIs rather than an identifier.
Let's verify the GraphQL schema changes and usage:
✅ Verification successful
Field renaming is consistent across the codebase
The change from externalId to externalKey in UniversityGatewayProgramExtraApplicationField is correct and consistent with the codebase. The search results show that:
- The field is already named
externalKeyin related interfaces likeIProgramExtraApplicationField - Other implementations use
externalKeyfor similar concepts - No GraphQL queries or mutations are using the old field name for this class
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any remaining references to externalId in GraphQL queries/mutations
# and verify the schema changes are consistent
# Search for GraphQL operations using the old field name
rg -t graphql "externalId.*UniversityGatewayProgramExtraApplicationField"
# Search for any remaining references to externalId in the codebase
rg "externalId.*UniversityGatewayProgramExtraApplicationField" --type ts
Length of output: 182
Script:
#!/bin/bash
# Let's try a different approach to find GraphQL and TypeScript files
# and check for both old and new field names
# Find all GraphQL files
echo "=== GraphQL files ==="
fd '\.(graphql|gql)$'
# Search for any references to the old field name
echo -e "\n=== References to externalId ==="
rg "externalId" -t typescript
# Search for any references to the new field name
echo -e "\n=== References to externalKey ==="
rg "externalKey" -t typescript
# Check the model class structure
echo -e "\n=== Model class definition ==="
ast-grep --pattern 'class UniversityGatewayProgramExtraApplicationField {
$$$
}'
Length of output: 20242
Script:
#!/bin/bash
# Let's try a more direct approach to find references in TypeScript files
# Search for old field name
echo "=== References to externalId ==="
rg "externalId" --type-add 'ts:*.ts' -t ts -A 2 -B 2
# Search for new field name
echo -e "\n=== References to externalKey ==="
rg "externalKey" --type-add 'ts:*.ts' -t ts -A 2 -B 2
# Look for the specific class definition
echo -e "\n=== Class definition ==="
rg "class UniversityGatewayProgramExtraApplicationField" --type-add 'ts:*.ts' -t ts -A 10 -B 2
Length of output: 34819
apps/services/university-gateway/src/app/modules/program/model/programExtraApplicationField.ts (2)
53-60: Documentation update needed for breaking changes.
This rename from externalId to externalKey is a breaking change that affects API contracts. Please ensure that:
- API documentation is updated
- Migration guide is provided for API consumers
- Release notes include this breaking change
Let's check for existing documentation:
#!/bin/bash
# Description: Look for API documentation files that might need updates
# Expected: Find documentation files mentioning the old property name
rg -g "*.md" "externalId.*program.*application"53-60: LGTM! Verify database migration execution.
The property renaming from externalId to externalKey is implemented correctly with proper Swagger documentation and Sequelize column definition.
Let's verify the corresponding database migration:
✅ Verification successful
✅ Database migration is properly implemented
The migration file 20241028110548-change-program-extra-application-field-external-key.js correctly handles the column renaming from external_id to external_key:
- Adds new
external_keycolumn - Copies data from
external_idtoexternal_key - Drops the old
external_idcolumn - Includes proper rollback functionality in the
downmigration
The model changes are fully supported by the database migration.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the migration file exists and contains correct column changes
# Expected: Find migration file that renames external_id to external_key
rg -g "*.js" -l "external.*key.*program_extra_application_field" migrations/
Length of output: 143
Script:
#!/bin/bash
# Let's try a broader search for migration files
fd -e js -e ts migration
# Then search for any files containing program_extra_application_field
rg -g "*.{js,ts}" "program_extra_application_field"
# Also search for migration-related changes with external_id or external_key
rg -g "*.{js,ts}" "external_(id|key).*program_extra_application"
Length of output: 4019
Script:
#!/bin/bash
# Let's examine the content of the migration file that seems to handle the external key change
cat "apps/services/university-gateway/migrations/20241028110548-change-program-extra-application-field-external-key.js"
Length of output: 1189
libs/clients/university-application/university-of-iceland/src/lib/utils/mapUglaApplication.ts (2)
19-26: Consider implementing attachment type identification
The current implementation assigns all direct attachments as profskirteini without differentiation. This could lead to incorrect categorization of attachments like CVs or introduction letters.
Consider implementing a mechanism to identify attachment types, such as:
- Adding metadata to the attachment object
- Using file analysis
- Adding a type field in the source data
Let's verify if there are any existing attachment type implementations:
Line range hint 41-83: Address TODO comments and hardcoded values
The implementation contains multiple unimplemented features and hardcoded values that might affect the application processing:
- Unimplemented nomination handling
- Hardcoded consent flags
- Missing validation for required fields
Let's check if these features are implemented elsewhere or if there are related issues:
Would you like me to:
- Create GitHub issues to track these TODO items?
- Help implement the missing features?
libs/clients/university-application/university-of-iceland/src/lib/utils/mapUglaPrograms.ts (2)
9-9: LGTM: Import addition aligns with type safety requirements.
The addition of IProgramExtraApplicationField import supports proper typing of the mapped fields.
135-135: Verify the impact of missing externalKey for exam venues.
The empty externalKey with a TODO comment raises concerns:
- This might affect attachment handling mentioned in the PR objectives
- Could cause issues with data consistency when processing exam venue selections
Let's verify the impact:
✅ Verification successful
Empty externalKey for exam venues is safe to proceed
The empty externalKey for exam venues appears to be a known limitation in the API integration, but the implementation is safe because:
- The exam venue field is properly structured with all required properties (name, type, options)
- The options are correctly populated from
program?.simenntunarstodvar - The field is properly typed as
FieldType.TESTING_SITE - The interface
IProgramExtraApplicationFieldshows that whileexternalKeyis required, it's used for identification purposes and doesn't affect the actual venue selection functionality
The TODO comment should be addressed in future API integration work, but it doesn't impact the current attachment handling or venue selection functionality.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for exam venue handling in the codebase
echo "Searching for exam venue processing..."
rg -l "mustPickExamVenue|TESTING_SITE" --type ts
# Check if the empty externalKey is handled properly in attachment processing
echo "Checking attachment handling..."
rg "attachmentKey.*externalKey" --type ts
Length of output: 506
Script:
#!/bin/bash
# Let's check how externalKey is used in the context of exam venues
echo "Checking exam venue related code..."
rg -A 5 -B 5 "externalKey.*venue|venue.*externalKey" --type ts
# Check the field type definition for exam venues
echo "Checking field type definitions..."
rg -A 10 "TESTING_SITE.*=|mustPickExamVenue.*=" --type ts
# Check attachment handling implementation
echo "Checking attachment implementations..."
ast-grep --pattern 'interface $_ {
$$$
externalKey: $_
$$$
}'
Length of output: 2085
Script:
#!/bin/bash
# Let's check how exam venues are processed in the application
echo "Checking exam venue processing in UniversityForm..."
rg -A 10 "TESTING_SITE.*{" libs/application/templates/university/src/forms/UniversityForm/InformationSection/ModeOfDeliverySubSection.ts
# Check if there are any validations or handlers that depend on externalKey
echo "Checking validations and handlers..."
rg -A 5 "validate.*externalKey|handle.*externalKey" --type ts
# Check the mapping implementation context
echo "Checking the mapping context..."
rg -A 10 -B 10 "externalKey: ''.*TODO" libs/clients/university-application/university-of-iceland/src/lib/utils/mapUglaPrograms.ts
Length of output: 1440
apps/services/university-gateway/src/app/modules/application/dto/createApplicationDto.ts (3)
1-5: LGTM! Import changes are well-structured.
The addition of FieldType to the imports is appropriate for the new field type validation.
Line range hint 1-224: Ensure test coverage for DTO changes.
While the changes follow NestJS patterns well, please ensure that unit tests are updated to cover:
- Validation of the new
fieldTypeproperty - Validation of renamed properties (
fileUrl,externalKey) - Edge cases with the FieldType enum values
#!/bin/bash
# Look for related test files
fd -g '*application*.spec.ts' -g '*application*.test.ts' apps/services/university-gateway/src
# Check for existing test coverage of these DTOs
rg -g '*spec.ts' -g '*test.ts' 'CreateApplicationFileDto|CreateApplicationExtraFieldsDto'210-224: Verify the impact of renaming key to externalKey.
The changes look good! The addition of fieldType and renaming of key to externalKey are well-documented and properly validated. However, since this is a breaking change in the DTO, we should verify its usage across the codebase.
✅ Verification successful
The renaming from key to externalKey appears to be safe
The verification shows that:
- The
CreateApplicationExtraFieldsDtois only used within thecreateApplicationDto.tsfile itself - The service and controller implementations only handle the DTO as a whole object without directly accessing the renamed property
- The test files don't contain any direct usage of the property
Since the property is only used internally within the DTO and there are no direct references to the old key property in the codebase, the renaming is safe to proceed with.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any remaining uses of the old 'key' property name in application-related files
rg -g '*.ts' -g '*.js' -A 3 -B 3 'CreateApplicationExtraFieldsDto.*key[^a-zA-Z]'
# Search for any direct uses of the DTO to ensure all consumers are updated
ast-grep --pattern 'CreateApplicationExtraFieldsDto'
Length of output: 463
Script:
#!/bin/bash
# Search for any usage of the old 'key' property
rg -g '*.ts' 'key:' apps/services/university-gateway/src/app/modules/application
# Search for any usage of the new 'externalKey' property
rg -g '*.ts' 'externalKey:' apps/services/university-gateway/src/app/modules/application
# Search for CreateApplicationExtraFieldsDto usage
rg -g '*.ts' 'CreateApplicationExtraFieldsDto' apps/services/university-gateway/src/app/modules/application
# Look for test files that might use this DTO
fd -e ts -e spec.ts . apps/services/university-gateway/src/app/modules/application
Length of output: 1595
Script:
#!/bin/bash
# Check the content of the test file for any usage of the DTO and its properties
cat apps/services/university-gateway/src/app/modules/application/test/application.controller.spec.ts
# Check the service implementation for any usage
cat apps/services/university-gateway/src/app/modules/application/universityApplication.service.ts
# Check the controller implementation
cat apps/services/university-gateway/src/app/modules/application/universityApplication.controller.ts
Length of output: 13981
libs/application/templates/university/src/fields/ProgramSelection/index.tsx (1)
95-96: LGTM! Enhanced program selection logic.
The additional condition ensures accurate program selection by matching both university and program IDs, preventing potential mismatches.
Let's verify that this change doesn't break existing program selections:
✅ Verification successful
Program selection logic is consistent across the codebase
The verification shows that the program selection logic using both universityId and program.id is correctly implemented and consistent with other program-related operations in the codebase:
- The same pattern is used in program filtering within the ProgramSelection component
- Similar validation exists in the university-gateway service for program lookups
- The implementation aligns with the program model structure in the backend
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other program selection implementations to ensure consistency
ast-grep --pattern 'program.id === $value'
Length of output: 44
Script:
#!/bin/bash
# Let's try a broader search for program selection patterns
rg -l "program\." libs/application/templates/university/src/fields/ProgramSelection
# Also search for similar patterns with universityId
rg "universityId.*program" libs/application/templates/university/src/fields/ProgramSelection -A 2 -B 2
# Check for any other program filtering or selection logic
ast-grep --pattern 'program => program.$_'
Length of output: 833
Script:
#!/bin/bash
# Let's check for any other program selection or filtering logic in the entire codebase
rg "programs\.find\(" -A 3 -B 1
# Also search for any other uses of universityId in program filtering
rg "program.*universityId" -A 2 -B 2
# Look for any program selection components or similar patterns
fd -e tsx -e ts "Program.*Selection"
Length of output: 4838
libs/clients/university-application/reykjavik-university/src/lib/reykjavikUniversityClient.service.ts (2)
82-82: LGTM! Property rename aligns with PR objectives.
The change from externalId to externalKey in the field mapping is consistent with the PR's goal of better reflecting the nature of the data.
82-82: Verify impact on API consumers.
While the property rename is correct, ensure all consumers of this API are updated to use externalKey instead of externalId.
apps/services/university-gateway/src/app/modules/application/universityApplication.service.ts (2)
12-12: LGTM: Import addition aligns with attachment handling changes
The addition of IApplicationAttachment import is appropriate for the enhanced attachment handling described in the PR objectives.
93-96: LGTM: Improved attachment handling implementation
The new implementation is more efficient and type-safe:
- Uses flatMap for better performance
- Adds explicit typing with IApplicationAttachment[]
- Properly filters out null/undefined values
libs/application/template-api-modules/src/lib/modules/templates/university/university.service.ts (2)
24-26: New imports are correctly added and necessary
The added imports CreateApplicationExtraFieldsDto, ProgramExtraApplicationFieldFieldTypeEnum, and CreateApplicationExtraFieldsDtoFieldTypeEnum are appropriate and required for the subsequent code changes.
235-261:
Ensure correct mapping between 'otherDocuments' and 'extraApplicationFields'
The code assumes that the indexes of otherDocuments and chosenProgram.extraApplicationFields align. If these arrays have different lengths or orders, this could lead to incorrect mappings. Consider matching them based on a unique identifier to ensure accurate pairing.
libs/application/templates/university/src/fields/OtherDocuments/index.tsx
Show resolved
Hide resolved
libs/clients/university-application/university-of-iceland/src/lib/utils/mapUglaApplication.ts
Show resolved
Hide resolved
libs/clients/university-application/university-of-iceland/src/lib/utils/mapUglaPrograms.ts
Show resolved
Hide resolved
apps/services/university-gateway/src/app/modules/program/internalProgram.service.ts
Show resolved
Hide resolved
What
Rename externalId -> externalKey for extraApplicationField (since this is not really ID, but more of a key from the university APIs)
Cleanup in attachment code for university application
Include externalKey / attachmentKey with attachments when sending in an application for Ugla schools
Checklist:
Summary by CodeRabbit
New Features
externalKeyproperty in various application and program models to enhance data structure.submitApplicationmethod to streamline handling of education data and extra application fields.Bug Fixes
Documentation
Refactor
Chores