-
Notifications
You must be signed in to change notification settings - Fork 61
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
feat(seminars): new application #17288
base: main
Are you sure you want to change the base?
Conversation
…is/island.is into feature/new-seminars-application
…/island-is/island.is into feature/new-seminars-application
…is/island.is into feature/new-seminars-application
…/island-is/island.is into feature/new-seminars-application
Important Review skippedMore than 25% of the files skipped due to max files limit. The review is being skipped to prevent a low-quality review. 41 files out of 123 files are above the max files limit of 75. Please upgrade to Pro plan to get higher limits. You can disable this status message by setting the 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
|
…/island-is/island.is into feature/new-seminars-application
…/island-is/island.is into feature/new-seminars-application
…is/island.is into feature/new-seminars-application
…/island-is/island.is into feature/new-seminars-application
…is/island.is into feature/new-seminars-application
Datadog ReportAll test runs ✅ 10 Total Test Services: 0 Failed, 10 Passed Test ServicesThis report shows up to 10 services
🔻 Code Coverage Decreases vs Default Branch (10)
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #17288 +/- ##
==========================================
+ Coverage 35.67% 35.71% +0.04%
==========================================
Files 6936 6948 +12
Lines 148940 149450 +510
Branches 42560 42824 +264
==========================================
+ Hits 53127 53371 +244
- Misses 95813 96079 +266
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 39 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
@@ -57,6 +57,7 @@ export interface AsyncSearchProps { | |||
errorMessage?: string | |||
hasError?: boolean | |||
white?: boolean | |||
isCompanySearch?: boolean |
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 seems really specific. If I'm understanding this correctly, you are applying different styles to the input based on this prop, right? Are the new styles based on the design system?
I'd really like some more clarity on this 🙏
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 for an old component that I'm reusing (I made it myself years ago). But it seems it was never used next to a normal input field. So the style of the async search is completely different than a input field and it seemed really wonky. So this would compliment rest of the styles used in the application system. (CompanySearch is used in a few application in the application system, hence the name)
'createCharge.data.id', | ||
) ?? '' | ||
await this.seminarsClientService | ||
.registerSeminar(auth, { |
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.
Extract-a payload-ið yfir í sína eigin breytu upp á lesanleika ?
@@ -0,0 +1,7 @@ | |||
# application-templates-aosh-seminars |
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.
Gera smá README skjal svipað og nýrri umsóknir ?
libs/application/templates/aosh/seminars/src/fields/Participants/index.tsx
Outdated
Show resolved
Hide resolved
libs/application/templates/aosh/seminars/src/fields/Participants/index.tsx
Outdated
Show resolved
Hide resolved
) | ||
|
||
setBeforeSubmitCallback?.(async () => { | ||
setIsCompanyValid(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.
Er þetta sett true of snemma ? spyr af því það er svo kallað í getIsCompanyValidCallback fyrir neðan.
|
||
export const getFields = () => import('./fields/') | ||
|
||
export { SeminarAnswers } from './shared/types' |
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.
Þessu er export-að 2x með sama nafninu sem skapar warnings í console, bæði í browser og terminal. Þyrfti að breyta öðru nafninu.
params: { | ||
organizationId: InstitutionNationalIds.VINNUEFTIRLITID, | ||
}, | ||
externalDataId: 'payment', |
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.
Tveir providers með sama externalDataId 'payment'. Býr til duplicate key villu í console.
|
||
export const prerequisitesSection = buildSection({ | ||
id: 'externalData', | ||
title: '', |
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.
Bæta við tabTitle attribute útaf það eru engin sections þá verður nafnið á tab í browser tómt.
export const paymentArrangement = { | ||
general: defineMessages({ | ||
sectionTitle: { | ||
id: 'aosh.sr.application:paymentArrangement.general.sectionTitle', |
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.
Misræmi á contentful key í þessum file og í hinum, "aosh.sr" vs. "aosh.sem"
@@ -401,4 +402,8 @@ export const ApplicationConfigurations = { | |||
slug: 'nyskraning-taekis', | |||
translation: ['aosh.rnm.application'], | |||
}, | |||
[ApplicationTypes.SEMINAR_REGISTRATION]: { | |||
slug: 'vinnueftirlitid-namskeid', | |||
translation: ['aosh.sr.application'], |
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.
Translation matchar ekki messages file-a. "aosh.sem" á flestum message file-um
export class SeminarsResolver { | ||
constructor(private readonly seminarsService: SeminarsService) {} | ||
|
||
@Scopes(ApiScope.vinnueftirlitid) |
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.
Set the scope over the resolver instead per endpoint
SeminarsClientModule, | ||
ConfigModule.forRoot({ | ||
isGlobal: true, | ||
load: [SeminarsClientConfig], |
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 unecessary to import the config here if added to apps/application-system/api/src/app/app.module.ts
if (!data) { | ||
throw new TemplateApiError( | ||
{ | ||
summary: `Ekkert námskeið fannst með þessu númeri: ${seminarQueryId}.`, |
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.
get from messages? (applies to all TemplateApiError here)
application, | ||
auth, | ||
}: TemplateApiModuleActionProps): Promise<void> { | ||
const answers = application.answers as unknown as SeminarAnswers |
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.
Dont use "as"?
courseId: seminarQueryId, | ||
paymentContact: { | ||
email: | ||
answers.paymentArrangement.individualOrCompany === 'company' |
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.
compare to enum/const?
this.individualApi.withMiddleware(new AuthMiddleware(user as Auth)) | ||
|
||
async getSeminar(auth: User, courseId: string): Promise<CourseDTO> { | ||
return await this.courseApiWithAuth(auth).apiCourseCourseIdGet({ |
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.
unecessary await
} | ||
|
||
async isCompanyValid(auth: User, nationalId: string): Promise<CompanyDTO> { | ||
return await this.companyApiWithAuth(auth).apiCompanyGet({ |
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.
unecessary await
auth: User, | ||
registration: ApiRegistrationPostRequest, | ||
): Promise<void> { | ||
return await this.registrationApiWithAuth(auth).apiRegistrationPost( |
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.
unecessary await
nationalIds: Array<string>, | ||
courseID: string, | ||
): Promise<Array<IndividualDTO>> { | ||
return await this.individualApiWithAuth(auth).apiIndividualGet({ |
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.
unecessary await
@@ -1130,6 +1139,7 @@ | |||
"@island.is/user-monitoring": ["libs/user-monitoring/src/index.ts"], | |||
"@island.is/web/*": ["apps/web/*"], | |||
"@island.is/web/component*": ["apps/web/component*/real.ts"], | |||
"api/domains/aosh": ["libs/api/domains/aosh/src/index.ts"], |
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.
remove?
Seminars
Attach a link to issue if relevant
What
New application for Vinnueftirlitið for all of their seminars.
Why
Specify why you need to achieve this
Screenshots / Gifs
Checklist: