Backend for campaign reports #378

batebobo · 2022-12-02T14:38:34Z

Summary

Implements #368

Created CRUD endpoints for campaign reports. They are part of a separate module called campaign-report, but the controller is under the campaign base route.

New entities:

CampaignReport
CampaignReportFile

Both entities support soft delete and have an owner (uploader).

Screenshot of how the new routes look like:

Todo

Endpoint for downloading files from reports
Tests for update service
Tests for update controller
Tests for delete service
Tests for delete controller
Some manual testing of the whole process
Manual test of s3 upload/download
Squash migrations

kachar

@batebobo Whooooah, incredible increment 🚀 🌔

Kudos for the time spent on this!

I've noticed some incompatibilities with our current workflow regarding the storage of currency amounts. Please check them out and let's discuss them.

kachar · 2022-12-02T17:22:47Z

schema.prisma

+  totalFunds          Float?               @map("total_funds")
+  fundsForPeriod      Float?               @map("funds_for_period")
+  spentFundsForPeriod Float?               @map("spent_funds_for_period")


@batebobo Normally we use only Integer types for storing the amounts, so I would suggest that we keep that practice here as well to be consistent with the other use cases.

Stripe also provides the amounts in the smallest currency value.

Since the columns are optional we might add a default value of 0 so we always have a value.

Suggested change

totalFunds Float? @map("total_funds")

fundsForPeriod Float? @map("funds_for_period")

spentFundsForPeriod Float? @map("spent_funds_for_period")

totalFunds Int? @map("total_funds") @default(0)

fundsForPeriod Int? @map("funds_for_period") @default(0)

spentFundsForPeriod Int? @map("spent_funds_for_period") @default(0)

Yeah, makes sense, thanks :) I'll update the schema ✌️

Dockerfile.migrations

imilchev · 2022-12-03T08:37:20Z

migrations/20221128133021_add_campaign_reports/migration.sql

+CREATE TYPE "campaign_report_file_type" AS ENUM ('photo', 'document');
+
+-- CreateTable
+CREATE TABLE "campaign_reports" (


Creating new tables breaks our database backup job currently due to permission changes. Please whoever merges this PR, ping me in a private message on Discord when it's merged. I will need to adjust the permissions before this is shipped to prod

Thanks for letting us know! Is there I can do on this PR to avoid breaking the backup job? Can you also please post a reference to this job, so that I can understand better what it does and why creating tables is breaking it 🙏

imilchev · 2022-12-03T08:38:45Z

apps/api/src/campaign-report/campaign-report.service.ts

+  }
+
+  private async softDeleteFile(fileId: string): Promise<CampaignReportFile> {
+    return this.prisma.campaignReportFile.update({


when do we actually delete the files?

We don't for the moment as per this reply from @igoychev. We could implement the actual deletion in several different ways, but I'd propose to leave this for a future task if it's not absolutely required for the initial version of the feature.

Also tests for updating a report

kachar · 2022-12-04T06:02:43Z

@imilchev The PR check for scanning k8s manifests with mondo showed an error that you might be interested in:

Target:     K8s Manifest dev-manifests
✕ Errors:   rpc error: code = InvalidArgument desc = asset does not match any of the activated policies

https://github.com/podkrepi-bg/api/actions/runs/3608695854/jobs/6081448709

igoychev

Great start and happy to see this code starts to take the shape!
I added some comments to help you with Prisma and naming conventions

igoychev · 2022-12-04T09:49:37Z

apps/api/src/campaign-file/campaign-file.controller.ts

@@ -35,7 +35,7 @@ export class CampaignFileController {
  ) {}

  @Post(':campaign_id')
-  @UseInterceptors(FilesInterceptor('file', 10, { limits: { fileSize: 20485760 } })) //limit uploaded files to 5 at once and 10MB each
+  @UseInterceptors(FilesInterceptor('file', 10, { limits: { fileSize: 1024 * 1024 * 10 } }))


igoychev · 2022-12-04T10:03:02Z

apps/api/src/campaign-report/campaign-report.controller.ts

+    const campaignReports = await this.campaignReportService.getReports(campaignId)
+
+    if (!campaignReports.map((report) => report.id).includes(reportId)) {
+      throw new NotFoundException('The given report is not part of the selected campaign')
+    }


It looks like it will be more optimal to pass the reportId as optional parameter of the function campaignReportService.getReports(campaignId), so that it returns only the required report from the database.

if you did this because of the prisma - it has the nice feature that if in the where clause you pass an optional parameter with missing value it returns all, so no complex logic required there.

getReports and getReport have different return types. In getReports we want the basic information about a report without fetching any of the relations. In getReport we'd like a more complicated fetch. Does that clarify the need for the two service functions?

igoychev · 2022-12-04T10:07:01Z

apps/api/src/campaign-report/campaign-report.controller.ts

+    const fileIsInReport = [
+      ...(report?.photos ?? []).map(photo => photo.id),
+      ...(report?.documents ?? []).map(document => document.id)]
+      .includes(fileId)
+
+    if (!fileIsInReport) {
+      throw new NotFoundException('The given file is not part of the report')
+    }


let's give this check to the database too

igoychev · 2022-12-04T14:06:30Z

apps/api/src/campaign-report/campaign-report.controller.ts

+    const campaignReports = await this.campaignReportService.getReports(campaignId)
+
+    if (!campaignReports.map((report) => report.id).includes(reportId)) {
+      throw new NotFoundException('The given report is not part of the selected campaign')
+    }


it will be more optimal instead of getting the reports to add a where clause in the database query

igoychev · 2022-12-04T14:12:25Z

apps/api/src/campaign-report/campaign-report.controller.ts

+    const campaign = await this.campaignService.getCampaignByIdWithPersonIds(campaignId)
+    const userCanUploadReport = [
+      campaign?.beneficiary.person?.keycloakId,
+      campaign?.organizer?.person.keycloakId,
+      campaign?.coordinator.person.keycloakId,
+    ].includes(user.sub)


more optimal will be to check in db again, see how it is done in getUserCampaigns(). It can be extended with an optional campaignId parameter and the query becomes:

this.prisma.campaign.findMany({
where: {
id: campaignId,
OR: [
{ beneficiary: { person: { keycloakId } } },
{ coordinator: { person: { keycloakId } } },
{ organizer: { person: { keycloakId } } },
],
},

igoychev · 2022-12-04T14:17:50Z

apps/api/src/domain/generated/person/entities/person.entity.ts

+  campaignReports?: CampaignReport[]
+  campaignReportFiles?: CampaignReportFile[]


shouldn't the relation be from person to campaignReport and then from campaignReport to the files?
In other words the Person should not have direct relation to the Files

That's one possibility, yes. I chose to include the relation Person -> Files because there might be different uploaders for the same report and in my mind they should have different permissions for their own uploaded files.

igoychev · 2022-12-04T14:27:24Z

schema.prisma

+  creatorId           String               @map("creator_id") @db.Uuid
+  creator             Person               @relation(fields: [creatorId], references: [id])


let's unify the naming convention:
creator could be createdBy or uploadedBy

This will allow in the next iterations to add columns for 'approvedBy' or 'deletedBy'. Similarly we have the need to add some timestamps like 'createdAt, deletedAt to record when such actions happened

igoychev · 2022-12-04T14:28:53Z

schema.prisma

+  personId  String                 @map("person_id") @db.Uuid
+  person    Person                 @relation(fields: [personId], references: [id])


[naming convention] let's have these uploadedBy to indicate that they indicate who uploaded them. In other context someone may confuse the file is "about the person".

github-actions · 2022-12-07T13:57:36Z

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

igoychev · 2022-12-22T07:50:41Z

migrations/20221128133021_add_campaign_reports/migration.sql

+-- AddForeignKey
+ALTER TABLE "campaigns" ADD CONSTRAINT "campaigns_company_id_fkey" FOREIGN KEY ("company_id") REFERENCES "companies"("id") ON DELETE SET NULL ON UPDATE CASCADE;
+


this was done also on master and now having it two times breaks the migrations

kachar · 2023-02-09T16:39:44Z

@batebobo Can you please update your PR and resolve the open discussions in order to get this PR moving? 🙏

igoychev · 2023-04-03T06:37:45Z

closing in favor of #453

batebobo added 5 commits November 27, 2022 23:55

Initial reports

611ba34

Merge branch 'master' of github.com:batebobo/podkrepi-api

0d5c552

Database schema for campaign reports

619447a

Create report endpoint

98a35cc

Delete, update and fetch reports

7bbb389

kachar requested changes Dec 2, 2022

View reviewed changes

kachar reviewed Dec 2, 2022

View reviewed changes

Dockerfile.migrations Outdated Show resolved Hide resolved

imilchev reviewed Dec 3, 2022

View reviewed changes

batebobo added 4 commits December 3, 2022 13:36

Fix typos and remove redundancies

017cc0e

Merge branch 'master' of github.com:podkrepi-bg/api into reports

2bb5836

Update report funds to be of type interger

66905ce

Endpoint for downloading report files

662bf17

Also tests for updating a report

igoychev reviewed Dec 4, 2022

View reviewed changes

batebobo added 2 commits December 7, 2022 15:49

Move validation logic to db queries

8d11138

Rename createdBy fields in report and report file

5ce0be0

Add update&delete report controller tests

d6ef762

batebobo marked this pull request as ready for review December 7, 2022 14:15

batebobo requested review from igoychev and kachar and removed request for igoychev and kachar December 7, 2022 14:15

igoychev added the run tests Allows running the tests workflows for forked repos label Dec 22, 2022

github-actions bot removed the run tests Allows running the tests workflows for forked repos label Dec 22, 2022

igoychev reviewed Dec 22, 2022

View reviewed changes

igoychev closed this Apr 3, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Backend for campaign reports #378

Backend for campaign reports #378

batebobo commented Dec 2, 2022 •

edited

Loading

kachar left a comment •

edited

Loading

kachar Dec 2, 2022

batebobo Dec 3, 2022

imilchev Dec 3, 2022

batebobo Dec 3, 2022

imilchev Dec 3, 2022

batebobo Dec 3, 2022

kachar commented Dec 4, 2022 •

edited

Loading

igoychev left a comment

igoychev Dec 4, 2022

igoychev Dec 4, 2022

batebobo Dec 6, 2022

igoychev Dec 4, 2022

igoychev Dec 4, 2022

igoychev Dec 4, 2022

igoychev Dec 4, 2022

batebobo Dec 6, 2022

igoychev Dec 4, 2022

igoychev Dec 4, 2022

github-actions bot commented Dec 7, 2022 •

edited

Loading

igoychev Dec 22, 2022

kachar commented Feb 9, 2023

igoychev commented Apr 3, 2023

		campaignReports?: CampaignReport[]
		campaignReportFiles?: CampaignReportFile[]

		creatorId String @map("creator_id") @db.Uuid
		creator Person @relation(fields: [creatorId], references: [id])

		personId String @map("person_id") @db.Uuid
		person Person @relation(fields: [personId], references: [id])

		-- AddForeignKey
		ALTER TABLE "campaigns" ADD CONSTRAINT "campaigns_company_id_fkey" FOREIGN KEY ("company_id") REFERENCES "companies"("id") ON DELETE SET NULL ON UPDATE CASCADE;

Backend for campaign reports #378

Backend for campaign reports #378

Conversation

batebobo commented Dec 2, 2022 • edited Loading

Summary

Todo

kachar left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kachar commented Dec 4, 2022 • edited Loading

igoychev left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Dec 7, 2022 • edited Loading

Choose a reason for hiding this comment

kachar commented Feb 9, 2023

igoychev commented Apr 3, 2023

batebobo commented Dec 2, 2022 •

edited

Loading

kachar left a comment •

edited

Loading

kachar commented Dec 4, 2022 •

edited

Loading

github-actions bot commented Dec 7, 2022 •

edited

Loading