[api] Update GET project protected areas list endpoint [MRXN23-451]

[yulia-bel]

Update GET project protected areas list endpoint

Overview

Response structure updated - new property 'name' added. For custom PAs it will contain 'fullName', for global - 'iucnCategory'.

Global PAs are grouped by category.

Sorting and filtering by 'name' property enabled.

Tests updated

Designs

Link to the related design prototypes (if applicable)

Testing instructions

Please explain how to test the PR: ID of a dataset, steps to reach the feature, etc.

Feature relevant tickets

Link to the related task manager tickets

---

Checklist before submitting

• Meaningful commits and code rebased on develop.
• If this PR adds feature that should be tested for regressions when
  deploying to staging/production, please add brief testing instructions
  to the deploy checklist (docs/deployment-checklist.md)
• Update CHANGELOG file

[vercel]

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Comments	Updated (UTC)
marxan	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	Sep 20, 2023 3:38pm

[hotzevzl]

thanks @yulia-bel. Your PR does what we need, but looking at it in details made me feel more uncomfortable as I kept thinking about how we're bending nestjs-base-service... so well, besides a long tirade about my own irrelevant views on clean code 😅 , I ended up proposing to simplify this, to avoid using nestjs-base-service "just" to get filter/sort/partial match (I actually forgot to explicitly mention partial match in the proposed implementation, but that's another Array.filter() away), and overall to simplify how we handle this.

I'd be happy as well if we wanted to keep your current implementation, with some cleanup for the benefit of our future selves as per my inline notes, and ideally stripping unneeded (and IMO misleading) properties from the response, but then... see my whole rationale for proposing to lightly hijack the current implementation of lists of scenario-level protected areas 😬

[hotzevzl]

I think this can be cleaned up like this:

Suggested change

	import _, {groupBy, intersection, isArray} from 'lodash';
	import { groupBy } from 'lodash';

[hotzevzl]

super minor - let your IDE give a bit of 80-characters-long love to comments! 😉

[hotzevzl]

(also, yarn format for minor updates to source formatting)

[hotzevzl]

ok, so this works as expected (kind of - if an API consumer adds sort=name,sort=-name by mistake, the explosion is on them 💥, but we can decide not to be too paranoid about this), but... it's all contingent on pagination being forcibly disabled (if not, filtering after fetching from db would mess up pagination, of course), so I would recommend to lightly polish this with some more comments.

For example, I had missed the switch from findAllPaginated() to findAll(), and someone months from now may just wonder, hey, why are we not using findAllPaginated() here? and have to dig back to understand. I'd signpost that (// Using findAll() here to prevent pagination from ever being applied as we need to post-process results, or something like this).

Also (and this is quite subjective), I feel that the sequence of "take this out, replace it with something else" in this PR is getting a bit out of hand.

I am fine with merging this as is: but do please at least make it explicit somewhere, either in the method's documentation (the `/** */ above the method itself) or within the body of the method, that the whole processing of these lists of protected areas depends on pagination being disabled.

On the other hand, related to my not about things getting out of hand, in general (and again, don't feel you have to change this in this PR), when we do have a sequence of business logic steps whose rationale is not immediately clear, I recommend to either make the code go towards a sort of "literate programming" style (i.e. lots of comments to articulate the why - the how should be inferrable from the implementation and clarifying it should not be the role of comments), or to break down the steps in self-contained methods/functions that can express the why as much as possible through their name, and possibly with the help of some succint method comments.

For example, here we take a deep copy of the fetchSpecification into editedFetchSpecification, and then we mangle the editedFetchSpecification in various ways to avoid breaking db queries through the standard flow of nestjs-base-service, though we still need to rely on the original fetchSpecification later on to check if filters were originally set - it's all clear to me as reviewer now since we have fresh context, but it's not something that anyone else would want to have to deal with months down the line with the fear of inadvertently changing something that was relied upon for some valid business logic concern.

Moreover, breaking "magic steps" down into self-contained functions makes it possible and easy to unit-test these bits of code, rather than having to unit-test a large function that slowly (I am partly to blame for this! I was the one who added the bits to get scenarioUsageCounts, though IIRC I tried to be at least careful with comments 😇) becomes a sort of big ball of mud 😬

[hotzevzl]

😢 consts come almost for free (especially as we're dealing with data structures that are expected to be very small and in sets of very low cardinality), no need to reassign to the same variable 😛

[hotzevzl]

by the time we have this result, in practice some of the item's data will make little sense - we can keep it for the sake of expediency, but it's one of the small grains of grit in this: for example

{
    "data": [
        {
            "type": "protected_areas",
            "id": "d35810fc-6279-4143-b38c-7e94b3bf4a97",
            "attributes": {
                "wdpaId": 6,
                "fullName": null,
                "iucnCategory": "II",
                "shapeLength": null,
                "shapeArea": null,
                "countryId": "ARG",
                "status": "Designated",
                "designation": "Parque Nacional",
                "scenarioUsageCount": 0,
                "isCustom": false,
                "name": "II"
            }
        },
        {
            "type": "protected_areas",
            "id": "2efe4709-2a20-4699-8669-cbf4b01535b6",
            "attributes": {
                "wdpaId": 111,
                "fullName": null,
                "iucnCategory": "IV",
                "shapeLength": null,
                "shapeArea": null,
                "countryId": "CHL",
                "status": "Designated",
                "designation": "Reserva Forestal",
                "scenarioUsageCount": 0,
                "isCustom": false,
                "name": "IV"
            }
        },
        {
            "type": "protected_areas",
            "id": "d267a6ff-9de3-4172-8942-786711a59dfc",
            "attributes": {
                "wdpaId": 23,
                "fullName": null,
                "iucnCategory": "Not Applicable",
                "shapeLength": null,
                "shapeArea": null,
                "countryId": "ARG",
                "status": "Designated",
                "designation": "Reserva de la Biósfera",
                "scenarioUsageCount": 0,
                "isCustom": false,
                "name": "Not Applicable"
            }
        },
        {
            "type": "protected_areas",
            "id": "99d07097-5ab5-4382-a0a6-baf4cd6ce9cc",
            "attributes": {
                "wdpaId": 24,
                "fullName": null,
                "iucnCategory": "Not Reported",
                "shapeLength": null,
                "shapeArea": null,
                "countryId": "BHS",
                "status": "Designated",
                "designation": "National Park",
                "scenarioUsageCount": 0,
                "isCustom": false,
                "name": "Not Reported"
            }
        }
    ]
}

As we compress multiple WDPA areas into placeholders for each distinct iucnCategory in use, several of the attributes for WDPA "areas" in a response such as the one above don't really apply to the set of areas with a given iucnCategory - in a way it doesn't matter, but I think this ends up being both confusing and a bit unsightly.

What we do actually need is much closer to the response for GET /api/v1/scenarios/:scenarioId/protected-areas, but without selected, and with scenarioUsageCount.

[
    {
        "name": "II",
        "id": "II",
        "kind": "global",
        "selected": false
    },
    {
        "name": "IV",
        "id": "IV",
        "kind": "global",
        "selected": false
    },
    {
        "name": "Not Applicable",
        "id": "Not Applicable",
        "kind": "global",
        "selected": false
    },
    {
        "name": "Not Reported",
        "id": "Not Reported",
        "kind": "global",
        "selected": false
    }
]

I know you must be hating this PR (and me for keeping asking for changes to something that works 😬 ) by now, but I'd take a final leap here, and instead of pretending that we are returning a set of ProtectedAreas, return instead something like ProjectProtectedAreaDto[] where type ProjectProtectedAreaDto = Omit<ProtectedAreaDto, 'selected'> & EntityScenarioUsageCount; and where in turn EntityScenarioUsageCount is something like

export class EntityScenarioUsageCount {
  @IsInt()
  scenarioUsageCount!: number;
}

(see implementation of ScenariosController.getProtectedAreasForScenario()), mapping the properties that we get in the result at the end of ProtectedAreasCrudService.listForProject() (what your current implementation is returning to API clients after serialization, basically) to this much simpler DTO.

In pseudocode-ish:

{
  id: result.iucnCategory ?? result.id,
  name: result.name,
  kind: result.isCustom ? ProtectedAreaKind.Project : ProtectedAreaKind.Global,
  scenarioUsageCount: result.isCustom ? <lookup from the count of scenarios in the recent implementation> : 1, // arbitrarily set usage count to 1 for global protected areas, as this doesn't matter in practice
}

In practice, we end up using nestjs-base-service just as a way to process query params such as filter and sort, but we then compose a much simpler response that has only the essential bits that the frontend app requires for the task at hand.

By now it may even be easier to actually forget about nestjs-base-service, and instead use a new method in SelectionGetService.getFor(), which accepts a projectId (instead of a scenario with its set of active protected area ids), then map()ping in the results of the lookup of scenario usage count by protected area id (for custom protected areas) and finally apply filtering and sorting (through your post-processing code above in the current PR) 🤔 .

The benefits of this would be twofold:

• I will (hopefully) stop yelling at clouds about this
• We avoid bending nestjs-base-service to a task it is ultimately ill-suited for 😢

[hotzevzl]

looks good, thanks! thanks for putting up with my changes of heart and well done for cleaning up the code (for some definition of clean aligning with my definition of clean, but still 😬)

let's merge and deploy, please!