Improve chart visualization #873

yulia-bel · 2023-02-16T12:34:50Z

Update. Chart visualization implemented in 2 ways:

The default one - no parameter sent - for case of 'group by region':

if no origin filters received, level 0 regions will be shown on chart (countries)
if origin filters received and they are of level 0, their descendants of level 1 will be displayed
if origin filters received and they are of level 1, their descendants of level 2 will be displayed
if origin filters received and they are of level 2, the received regions will be displayed
if origin filters received and they are of different level, the chart will display regions and descendants of the lowest level received

Optional parameter 'depth' - can be applied for any group by entity. No matter the parent levels of received filters, user can choose the depth / level to be displayed on the chart

In order to use optional parameter FE changes are needed (add parameter to request)

Tests added.

General description

Please write a description. If the PR is hard to understand, provide a quick explanation of the code.

Designs

Link to the related design prototypes (if applicable)

Testing instructions

Provide minimal instructions on how to test this PR.

Apart from the added feature / bug fix, check overall performance, styling...

Checklist before merging

Branch name / PR includes the related Jira ticket Id.
Tests to check core implementation / bug fix added.
All checks in Continuous Integration workflow pass.
Feature functionally tested by reviewer(s).
Code reviewed by reviewer(s).
Documentation updated (README, CHANGELOG...) (if required)

vercel · 2023-02-16T12:34:54Z

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

2 Ignored Deployments

Name	Status	Preview	Comments	Updated
landgriffon-client	⬜️ Ignored (Inspect)			Mar 2, 2023 at 1:12PM (UTC)
landgriffon-cookie-traceability	⬜️ Ignored (Inspect)			Mar 2, 2023 at 1:12PM (UTC)

alexeh · 2023-02-17T05:29:41Z

api/src/modules/impact/dto/impact-table.dto.ts

@@ -161,6 +163,17 @@ export class GetRankedImpactTableDto extends BaseImpactTableDto {
  @IsUUID(4)
  scenarioId?: string;

+  @ApiPropertyOptional({
+    description:
+      'Optional property for groupBy region and chart view to choose level of admin regions to show',


I think the msg is not correct as the grouping depends on the selected entity, so its not tied to 'admin-regions'

I would say something like:
"Select the depth of the tree for grouping elements, defaults to...."

alexeh · 2023-02-17T05:33:40Z

api/src/modules/impact/dto/impact-table.dto.ts

+  @Type(() => Number)
+  @IsNumber()
+  @Min(1)
+  @Max(3)


I think the minimum should be 0, as discussed. If a user wants to select 6 countries and see a 4 ranked still at country level, that depth should equal 0

Also, I wouldn't set a fixed max limit. Even tho the current limit makes sense, I think its a better idea to not set a max limit and just uncollapse the tree as much as it can be uncollapsed.

i.e, users sends a 5 value but we have a maximum depth of 3, just send back a 3 level deep tree

alexeh · 2023-02-17T05:33:44Z

api/src/modules/impact/dto/impact-table.dto.ts

+  @IsNumber()
+  @Min(1)
+  @Max(3)
+  level?: number;


to be more consistent with other endpoints, I would name this parameter as

depth

alexeh · 2023-02-17T05:34:57Z

api/src/modules/impact/impact.service.ts

+    // If chart is requested for regions and specific level is received, we get only the requested level from admin regions trees of the response
+
+    if (
+      rankedImpactTableDto.groupBy === 'region' &&


Following https://github.com/Vizzuality/landgriffon/pull/873/files#r1109319576, I still dont get why this is somehow tied to regions.

Does not make sense to apply this behaviour to any other entity that is a tree structure?

alexeh · 2023-02-17T05:36:08Z

api/src/modules/impact/impact.service.ts

+    impactTableRows: ImpactTableRows[],
+    level: number,
+  ): ImpactTableRows[] {
+    if (level === 1) {


This should be 0, in order to uncollapse the whole thing

alexeh · 2023-02-17T05:37:23Z

api/src/modules/impact/impact.service.ts

+    if (level === 1) {
+      return impactTableRows;
+    }
+    if (level < 1) {


I think this is not necessary, because:

first level should be 0, which is a valid value

there should be a default behaviour to default to

alexeh · 2023-02-17T05:40:25Z

api/src/modules/impact/impact.service.ts

+        return impactTableRows;
+      },
+      [],
+    );


This is nice :)

However, maybe we should try to use immutability, thoughts?

alexeh · 2023-02-17T05:44:46Z

api/test/e2e/impact/mocks/chart-levels-preconditions/chart-levels.preconditions.ts

+  const sourcingLocations: SourcingLocation[] = [];
+
+  for (const municipality of municipalities) {
+    const i: number = municipalities.indexOf(municipality);


This is a super lightweight improvement, but If you realise, for each element on municipalities, you are running through municipalities again to find the index.

You could either get the index in the for of loop, or use a native array method that can provide you the index as one of the parameters in its callback

alexeh · 2023-02-17T05:46:26Z

api/test/e2e/impact/impact-chart/chart-levels.spec.ts

+        response.body.impactTable[0].others.aggregatedValues[0].value,
+      ).toBe(2000);
+    },
+  );


Nice, very clean, creating preconditions just once etc...

alexeh · 2023-02-17T05:46:51Z

api/test/e2e/impact/impact-chart/chart-levels.spec.ts

+
+    dataSource = testApplication.get<DataSource>(DataSource);
+
+    ({ jwtToken } = await setupTestUser(testApplication));


This is cool, I had no idea you could assign a value like this

…uped by regions

…k approach

yulia-bel requested review from KevSanchez and alexeh February 16, 2023 12:34

alexeh reviewed Feb 17, 2023

View reviewed changes

alexeh force-pushed the dev branch 2 times, most recently from 81763b1 to f876633 Compare February 24, 2023 08:30

andresgnlez force-pushed the dev branch from 60575f4 to 3b7b168 Compare February 27, 2023 09:17

alexeh force-pushed the dev branch 5 times, most recently from e072d47 to 0740fd3 Compare February 27, 2023 13:47

KevSanchez force-pushed the dev branch from 0740fd3 to 339d30a Compare February 27, 2023 14:15

andresgnlez force-pushed the dev branch from b74f7fd to 506e830 Compare February 27, 2023 16:00

alexeh force-pushed the dev branch 4 times, most recently from debfccb to 6d94cb7 Compare February 27, 2023 19:31

yulia-bel force-pushed the api/LANDGRIF-1154-improve-chart-visualization branch from efb5a2f to d526d80 Compare February 28, 2023 14:00

elpamart and others added 2 commits March 1, 2023 11:23

add qa raster to h3 data notebook

34c3f31

Add user delete endpoint

021e72b

alexeh and others added 17 commits March 1, 2023 11:29

Add ingress rule for tiler routing

6facfea

log path

7e637d7

Tweaks and improvements to the /data Dockerfile

4baf6f8

Add prefix to tiler

2a17231

Add routing env vars

17e1808

merging analysis specs; centralizing all intercepts in the specs

66d687f

Fix senario edition permission bug

17bc49e

Include user to infinite scenarios request

a865137

Add fit to changelog

7ca84d7

fix tiler gh workflow

9dddc32

integrate s3 in tiler

205472a

remove silence flag to test k8s deploy

54f0597

add tmp default cog file

441b084

tmp workaround to k8s deployment

e180aa5

rollback k8s deployment workflow

1218577

tiler refactor

3769250

using env production for client test

c495105

alexeh force-pushed the dev branch from 51618cc to c495105 Compare March 1, 2023 08:30

alexeh and others added 7 commits March 1, 2023 11:34

fix impact test

44a76be

Add option to chose levels of Admin Regions when requesting chart gro…

1a6907b

…uped by regions

Add chart level test

4345330

Refactor entities depth management logic for charts

1bc0918

Update chart level visualization

78f9456

Update chart level visualization for suppliers and materials

a4db493

Update admin regions chart level tests in accordance with new fallbac…

a40b9e8

…k approach

yulia-bel force-pushed the api/LANDGRIF-1154-improve-chart-visualization branch from de3dfa6 to a40b9e8 Compare March 1, 2023 08:48

Add tests for supplier levels optimization in charts

a17bd9e

alexeh force-pushed the dev branch 2 times, most recently from 2fff34a to 8a3eb61 Compare March 3, 2023 04:44

alexeh force-pushed the dev branch from 82caef0 to c46e45f Compare April 17, 2023 14:49

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve chart visualization #873

Improve chart visualization #873

yulia-bel commented Feb 16, 2023 •

edited

Loading

vercel bot commented Feb 16, 2023 •

edited

Loading

alexeh Feb 17, 2023

alexeh Feb 17, 2023

alexeh Feb 17, 2023

alexeh Feb 17, 2023

alexeh Feb 17, 2023

alexeh Feb 17, 2023

alexeh Feb 17, 2023

alexeh Feb 17, 2023

alexeh Feb 17, 2023

alexeh Feb 17, 2023


		dataSource = testApplication.get<DataSource>(DataSource);

		({ jwtToken } = await setupTestUser(testApplication));

Improve chart visualization #873

Are you sure you want to change the base?

Improve chart visualization #873

Conversation

yulia-bel commented Feb 16, 2023 • edited Loading

General description

Designs

Testing instructions

Checklist before merging

vercel bot commented Feb 16, 2023 • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yulia-bel commented Feb 16, 2023 •

edited

Loading

vercel bot commented Feb 16, 2023 •

edited

Loading