[TM-1766] add initial changes for supporting entity get #64

pachonjcl · 2025-02-24T03:11:46Z

nx-cloud · 2025-02-24T03:12:05Z

View your CI Pipeline Execution ↗ for commit 699eada.

Command	Status	Duration	Result
`nx test entity-service --coverage --runInBand`	❌ Failed	25s	View ↗
`nx test unified-database-service --coverage`	✅ Succeeded	23s	View ↗
`nx run-many -t lint build`	✅ Succeeded	3s	View ↗

☁️ Nx Cloud last updated this comment at 2025-02-26 14:00:06 UTC

pachonjcl · 2025-02-26T13:51:07Z

Will reduce coverage temporarily.

  coverageThreshold: {
    global: {
      branches: 85,
      functions: 95,
      lines: 95,
      statements: 95
    }
  },

roguenet · 2025-02-28T17:22:20Z

apps/entity-service/src/entities/dto/site.dto.ts

 import { FrameworkKey } from "@terramatch-microservices/database/constants/framework";
 import { AdditionalProps, EntityDto } from "./entity.dto";
-
+import { MediaDto } from "./media.dto";
 // TODO: THIS IS A STUB!


Please remove this comment.

roguenet · 2025-02-28T17:23:25Z

apps/entity-service/src/entities/dto/site.dto.ts

@@ -21,7 +21,6 @@ export class SiteLightDto extends EntityDto {
      this.populate(SiteLightDto, {
        ...pickApiProperties(site, SiteLightDto),
        lightResource: true,
-        // these two are untyped and marked optional in the base model.


Not a huge deal that this is removed, but it is kind of a useful comment 🤷🏻

roguenet · 2025-02-28T17:24:12Z

apps/entity-service/src/entities/dto/site.dto.ts

+    description: "The associated project name"
+  })
+  projectName: string | null;
+


There are lots of fields on the spreadsheet that aren't represented in this lite DTO, and projectName I didn't think was needed except in the full DTO. Is it actually needed here?

roguenet · 2025-02-28T17:25:51Z

apps/entity-service/src/entities/dto/site.dto.ts

@@ -85,5 +90,134 @@ export class SiteFullDto extends SiteLightDto {
  lightResource = false;

  @ApiProperty()
-  totalSiteReports: number;
+  siteReportsTotal: number;


Please review the spreadsheet. In the usage notes for this column I requested that it be renamed to totalSiteReports to be consistent with projects. I'm going to skip over the rest of the DTO fields for now; please check what's on the spreadsheet against what you have here and I'll review again when that's done.

roguenet · 2025-02-28T17:26:54Z

apps/entity-service/src/entities/processors/site.processor.ts

+      where: { uuid },
+      include: [
+        { association: "framework" },
+        { association: "project", attributes: ["name"], include: [{ association: "organisation" }] }


unless projectName / organisationName are needed in the LIght DTO, these associations should be removed. If organisationName is needed, please add the attributes: ["name"] bit so the whole org record isn't fetched.

roguenet · 2025-02-28T17:35:11Z

apps/entity-service/src/entities/processors/site.processor.ts

+      .sites([siteId])
+      .findAll({ attributes: ["id", "siteId", "numTreesRegenerating"] });
+
+    const regeneratedTreesCount = sumBy(hasBeenSubmittedSiteReports, "numTreesRegenerating");


Another spot where the audit I did in the spreadsheet will help you. This field is not used in the FE. I would rename the approvedRegeneratedTreesCount to just regeneratedTreesCount, and remove this submitted reports field entirely.

A general guidance from product that I forgot to put on the tickets is that all aggregated fields on these entities should be based only on approved entities. The fact that some of them in PHP are using the submitted entities instead is a mistake.

roguenet · 2025-02-28T17:36:04Z

apps/entity-service/src/entities/processors/site.processor.ts

+      (await DemographicEntry.gender().sum("amount", {
+        where: {
+          demographicId: {
+            [Op.or]: [{ [Op.in]: siteReportWorkdays }]


When there's only one, there's no reason to wrap it in an or. This can be simply

demographicId: { [Op.in]: siteReportWorkdays }

roguenet · 2025-02-28T17:42:07Z

jest.preset.js

+      branches: 50,
+      functions: 50,
+      lines: 50,
+      statements: 50


We'll want this back to normal before the final merge.

roguenet · 2025-02-28T17:45:24Z

libs/common/src/lib/policies/site.policy.ts

+      this.builder.can("read", Site, { frameworkKey: { $in: frameworks } });
+    }
+
+    if (this.permissions.includes("manage-own")) {


For manage-own, we should also be checking projects in the org, per the current PHP policy. I did this recently for project-report.policy.ts, and you can probably just copy that code over.

roguenet · 2025-02-28T17:48:25Z

libs/database/src/lib/entities/site.entity.ts

@@ -96,6 +108,102 @@ export class Site extends Model<Site> {
  @Column(TEXT)
  descriptionSitingStrategy: string | null;

+  @AllowNull
+  @Column(INTEGER.UNSIGNED)


According to the current DB schema, this should be a DECIMAL(15, 1). Please carefully audit these fields against the current DB schema to make sure they all match. Once you've done that, I'll take another look over these new fields.

roguenet · 2025-02-28T21:20:52Z

apps/entity-service/src/entities/dto/site.dto.ts

  @ApiProperty()
  createdAt: Date;

  @ApiProperty()
  updatedAt: Date;
+
+  @ApiProperty({ type: Project })
+  project: Project;


Lots of problems here:

We'd never want to include the full project on another entity's DTO; that's a pattern from PHP we're ditching in v3. Fetching a couple of fields (like UUID and name) is OK, but if an element on the page needs the full project, it can use the project GET connection to pull it.

A model cannot itself be an ApiProperty - it has to be a DTO

I still think the project information isn't needed on the light DTO. Did you find a case where that's incorrect? Also applies to projectUuid below.

Agree, we are removing project as a entity from the dto

Actually the list of sites in admin do require the projectName property, so that one will stay on the light dto.

pachonjcl · 2025-03-01T03:10:06Z

apps/entity-service/src/entities/processors/site.processor.ts

+    if (query.search != null) {
+      // This is they way that sequelize supports for searching in a joined table
+      projectAssociation.where = { name: { [Op.like]: `%${query.search}%` } };
+      // This is to ensure that the project is not required to be joined (simulating an OR)
+      projectAssociation.required = false;
+    }


Please take a look to this comments, if you feel this is an anti pattern o there is a better way to handle this.
Happy to change it.

[TM-1766] add initial changes for supporting entity get

601f7a9

LimberHope and others added 9 commits February 25, 2025 11:37

add update to site entity

dcbb2d9

add scope to site media

cd2d5b6

Merge branch 'staging' into TM-1766_entity_get_and_index_site

95396fa

[TM-1766] update logic to project-manager permissions

a9baa60

[TM-1766] improve logic

d5e8666

[TM-1766] add missing attributes

c17ac31

[TM-1766] fix

50917f7

remove unused file

c531346

add new fields

4d68ab1

pachonjcl added 3 commits February 26, 2025 09:51

add test and reduce coverage

dabf26b

fix lint

699eada

temporal add basic tests

bdd1572

pachonjcl temporarily deployed to dev February 26, 2025 14:49 — with GitHub Actions Inactive

LimberHope and others added 11 commits February 26, 2025 16:13

update site policy

090aca0

Disable usage of NX Cloud

89f3225

Skip NX cloud in deployment.

d8859d1

[TM-1718] First entity association index: Demographics.

7038534

[TM-1718] Get new endpoint documentation in place.

3bebb48

[TM-1718] Abstract the subquery pattern to a builder.

a3c7a55

Merge branch 'staging' into TM-1766_entity_get_and_index_site

1fc0ac0

[TM-1766] updatee site processor test

72c30e0

[TM-1766] remove entity imported

6628ff5

Merge branch 'staging' into TM-1766_entity_get_and_index_site

57fba92

fix unit test

9a76910

LimberHope had a problem deploying to dev February 28, 2025 16:32 — with GitHub Actions Failure

pachonjcl marked this pull request as ready for review February 28, 2025 17:06

pachonjcl requested a review from roguenet February 28, 2025 17:06

Merge branch 'staging' into TM-1766_entity_get_and_index_site

fa25235

roguenet reviewed Feb 28, 2025

View reviewed changes

pachonjcl temporarily deployed to dev February 28, 2025 18:06 — with GitHub Actions Inactive

[TM-1766] updates of feedback

703102c

roguenet reviewed Feb 28, 2025

View reviewed changes

pachonjcl added 6 commits February 28, 2025 19:40

address comment and increase coverage

3f9eead

fix lint

3882513

fix test

da5b038

fix test by using sortUp on test

7c6dfe4

improve branch percentage

4a1fc08

fix unit test on search based on external column

1278b04

pachonjcl requested a review from roguenet March 1, 2025 02:03

pachonjcl temporarily deployed to dev March 1, 2025 02:03 — with GitHub Actions Inactive

Merge branch 'staging' into TM-1766_entity_get_and_index_site

32d2a9d

pachonjcl temporarily deployed to dev March 1, 2025 02:12 — with GitHub Actions Inactive

pachonjcl deployed to dev March 1, 2025 02:12 — with GitHub Actions Active

pachonjcl temporarily deployed to dev March 1, 2025 02:12 — with GitHub Actions Inactive

move name from full to light dto site

89b51ee

pachonjcl commented Mar 1, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[TM-1766] add initial changes for supporting entity get #64

[TM-1766] add initial changes for supporting entity get #64

pachonjcl commented Feb 24, 2025 •

edited

Loading

nx-cloud bot commented Feb 24, 2025 •

edited

Loading

pachonjcl commented Feb 26, 2025

roguenet Feb 28, 2025

roguenet Feb 28, 2025

roguenet Feb 28, 2025

roguenet Feb 28, 2025

roguenet Feb 28, 2025

roguenet Feb 28, 2025

roguenet Feb 28, 2025

roguenet Feb 28, 2025

roguenet Feb 28, 2025

roguenet Feb 28, 2025

roguenet Feb 28, 2025

pachonjcl Feb 28, 2025

pachonjcl Mar 1, 2025

pachonjcl Mar 1, 2025

[TM-1766] add initial changes for supporting entity get #64

Are you sure you want to change the base?

[TM-1766] add initial changes for supporting entity get #64

Conversation

pachonjcl commented Feb 24, 2025 • edited Loading

nx-cloud bot commented Feb 24, 2025 • edited Loading

pachonjcl commented Feb 26, 2025

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pachonjcl commented Feb 24, 2025 •

edited

Loading

nx-cloud bot commented Feb 24, 2025 •

edited

Loading