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

[FLAG-1239]: Add GADM Query Parameter When GETing Areas #4919

Open
wants to merge 1 commit into
base: epic/gadm
Choose a base branch
from

Conversation

willian-viana
Copy link
Collaborator

Overview

This card makes the GADM version explicit when a user’s Area is being requested from the Resource Watch (RW) Area microservice.

Making a distinction between different versions of administrative boundary IDs is critical to quickly adopting future boundary updates.

Currently, administrative Areas are referenced with GADM IDs. These are used when saving/loading Areas in Flagship. They’re also used in URL paths within Flagship for dashboards, embeds, and the like. Unfortunately, there is no guarantee from the GADM organization that IDs remain consistent across GADM versions.

For a made-up example, the same subregion in Mexico (or any other country) can have two sets of IDs depending on what GADM version is referenced:
GADM 3.6: MEX 8 14
GADM 4.1: MEX 9 12

This drastically affects Flagship's ability to keep a user’s areas consistent when changing to an updated GADM version.

Scope

This involves only adding two query parameters to a RW Area GET request.

Currently, requests for a user’s Areas look like this in Flagship: GET /v2/area

The updated implementation will add two query parameters: GET /v2/area?source[provider]=gadm&source[version]=3.6

Implementation Details

The RW Area microservice will ignore these parameters until later in the RW Area microservice GADM Epic.

However, the benefit is to get started on implementing the necessary interface updates and verifying integration with a low-risk approach.

It’s totally acceptable to hard-code these values to gadm and 3.6. Future cards will then make these values dynamic (to support 4.1).

Any other query parameters being sent for similar requests will still need to be sent. This is an addition only (in order to preserve backward compatibility).

Resources

Miro sticky with comments

@willian-viana willian-viana self-assigned this Jan 16, 2025
@willian-viana willian-viana marked this pull request as ready for review January 16, 2025 15:46
@willian-viana willian-viana force-pushed the feat/gadm-query-parameters-FLAG-1239 branch from 68d6f87 to 200de30 Compare January 16, 2025 15:47
Copy link

@gtempus gtempus left a comment

Choose a reason for hiding this comment

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

Looking good, @willian-viana.
Let's just focus on the GET requests for this PR.

@@ -2,10 +2,11 @@ import { apiAuthRequest } from 'utils/request';
import { trackEvent } from 'utils/analytics';

const REQUEST_URL = '/v2/area';
const SOURCE = `?application=gfw&source[provider]=gadm&source[version]=3.6`; // these values will come from Redux
Copy link

Choose a reason for hiding this comment

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

You don't have to send application=gfw as part of this PR.
I don't think it'll hurt anything. I just wanted to make sure y'all know it's not required.

It looks like Flagship doesn't send it now, so maybe you shouldn't send it in this PR.

Comment on lines 71 to 73
url: data.id
? `${REQUEST_URL}/${data.id}${SOURCE}`
: `${REQUEST_URL}${SOURCE}`,
Copy link

Choose a reason for hiding this comment

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

These are PATCH and POST URLs. They don't need to have the new query params.
For these, a change in the body will be needed but, it's not part of the scope of this card.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I just realized I replicated the source for all methods instead of GET only haha thank you for notice it!

@@ -90,5 +93,5 @@ export const deleteArea = (id) => {
action: 'User deletes aoi',
label: id,
});
return apiAuthRequest.delete(`${REQUEST_URL}/${id}`);
return apiAuthRequest.delete(`${REQUEST_URL}/${id}${SOURCE}`);
Copy link

Choose a reason for hiding this comment

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

Delete doesn't need the params, either.

@willian-viana willian-viana force-pushed the feat/gadm-query-parameters-FLAG-1239 branch from 200de30 to c8986ac Compare January 16, 2025 20:14
@willian-viana willian-viana force-pushed the feat/gadm-query-parameters-FLAG-1239 branch from c8986ac to 2c9f07d Compare January 16, 2025 20:16
@willian-viana willian-viana requested a review from gtempus January 16, 2025 20:16
@willian-viana willian-viana force-pushed the feat/gadm-query-parameters-FLAG-1239 branch from 2c9f07d to 5fe8e6d Compare January 16, 2025 21:03
@willian-viana willian-viana changed the base branch from develop to epic/gadm January 16, 2025 21:04
Copy link

@gtempus gtempus left a comment

Choose a reason for hiding this comment

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

Your changes are what I was thinking, @willian-viana. 💯

Where are the tests? I do see that y'all have a few services tested.

Just as a warm-up, I added a basic areas.spec.js test for you to consider:

services/__tests__/areas.spec.js

import { getArea } from '../areas';
import { apiAuthRequest } from "../../utils/request";

jest.mock('../../utils/request', () => ({
  apiAuthRequest: {
    get: jest.fn(),
  },
}));

describe('Areas Service', () => {
  describe('Getting a Single Area', () => {
    describe('The Request to the API', () => {
      it('should add source params for gadm 4.1', async () => {
        // arrange
        apiAuthRequest.get.mockResolvedValueOnce({data: {data: {attributes: {}}}});
        
        // act
        await getArea('abcdef123456789');

        // assert
        expect(apiAuthRequest.get).toHaveBeenCalledWith(
          '/v2/area/abcdef123456789?source[provider]=gadm&source[version]=4.1',
          {
            headers: {}
          }
        )
      });
    });
  });
});

Maybe add another for getAll? 😄

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