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

Persisting created new campaignApplication to db #657

Merged
merged 10 commits into from
Jul 17, 2024

Conversation

Martbul
Copy link
Contributor

@Martbul Martbul commented Jul 3, 2024

No description provided.

update CampaignApplication model - organizerEmail now is not unique
update create method testing
now created campaig-application data is persisted to db
update CampaignApplication model - organizerEmail now is not unique
update create method testing
now created campaig-application data is persisted to db
Copy link

github-actions bot commented Jul 3, 2024

✅ Tests will run for this PR. Once they succeed it can be merged.

@@ -26,63 +42,61 @@ describe('CampaignApplicationController', () => {
expect(controller).toBeDefined()
})

it('when create called it should delegate to the service create', () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

This is deleted but the logic is still in use. Was this by design?

@@ -2,21 +2,37 @@ import { Test, TestingModule } from '@nestjs/testing'
import { CampaignApplicationController } from './campaign-application.controller'
import { CampaignApplicationService } from './campaign-application.service'
import { SpyOf, autoSpy } from '@podkrepi-bg/testing'
Copy link
Contributor

Choose a reason for hiding this comment

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

Some unuses stuff that github(or an action?) points out - SpyOf and CamapignTypeCategroy

controller.findOne('id')

// assert
// Assert
Copy link
Contributor

Choose a reason for hiding this comment

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

avoid such unnecessary changes - they make the code review that much harder


expect(prismaMock.campaignApplication.create).toHaveBeenCalledWith({
data: {
campaignName: dto.campaignName.trim(),
Copy link
Contributor

Choose a reason for hiding this comment

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

This looks like the logic itself and while it's technically correct I belive we'll get more value having hardcoded value here i.e. {

data :{
campaignName: 'my-campaign',
organizerName: 'test organizer',

...

})
}

const sanitizedData = {
Copy link
Member

Choose a reason for hiding this comment

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

Prisma sanitizes the input by default. Seems like it also trims unnecessary spaces from user input, thus sanitizedData is not needed.

@Martbul Martbul requested a review from sashko9807 July 16, 2024 09:43
@sashko9807 sashko9807 added the run tests Allows running the tests workflows for forked repos label Jul 17, 2024
@github-actions github-actions bot removed the run tests Allows running the tests workflows for forked repos label Jul 17, 2024
@Module({
imports: [PrismaModule],
controllers: [CampaignApplicationController],
providers: [CampaignApplicationService],
providers: [CampaignApplicationService, PersonService, OrganizerService],
Copy link
Member

@sashko9807 sashko9807 Jul 17, 2024

Choose a reason for hiding this comment

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

Prefer importing modules instead of services(e.g. imports:[PrismaModule, PersonModule, OrganizersModule] )

delete PersonService and OrganizerService and add PersonModule and OrganizersModule to campaign-application module
@Martbul Martbul requested a review from sashko9807 July 17, 2024 17:11
@sashko9807 sashko9807 added the run tests Allows running the tests workflows for forked repos label Jul 17, 2024
@github-actions github-actions bot removed the run tests Allows running the tests workflows for forked repos label Jul 17, 2024
@sashko9807 sashko9807 merged commit d540599 into podkrepi-bg:master Jul 17, 2024
10 of 11 checks passed
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