Migrate to RTK Query #553

flacoman91 · 2024-10-24T23:58:23Z

Additions

Removals

Changes

Testing

Screenshots

Notes

Todos

Checklist

Changes are limited to a single goal (no scope creep)
Code can be automatically merged (no conflicts)
Code follows the standards laid out in the front end playbook
Passes all existing automated tests
New functions include new tests
New functions are documented (with a description, list of inputs, and expected output)
Placeholder code is flagged
Visually tested in supported browsers and devices
Project documentation has been updated (including the "Unreleased" section of the CHANGELOG)

cdmh219 · 2024-11-21T17:04:22Z

src/api/hooks/useGetAggregations.js

@@ -0,0 +1,16 @@
+import { useGetAggregationsQuery } from '../complaints/complaints';


First off, I love that the API calls are now being implemented in hooks, which makes for more reusable functionality and smaller components. But I did notice that there is another hooks folder at src/hooks. Should we just have all our hooks in one place, at src/hooks? Then you could have a breakout of subfolders named api, etc.

Yeah, i thought about this, I originally had it under the hooks folder, but I moved them here to keep them closer to the RTKQ API

cdmh219 · 2024-11-21T17:45:35Z

src/utils/chart.js

  const parts = tooltip.title.split(':');
+  newTooltip.values = cloneDeep(tooltip.values);


Do we still need cloneDeep as much, now that we can use mutable functionality through Redux Toolkit? Does the following work as expected for line 238?

Suggested change

newTooltip.values = cloneDeep(tooltip.values);

newTooltip.values = {...tooltip.values};

That was giving an error since tooltip.values is an Array. I went ahead and switched it over to structuredClone and added a global polyfill in setupTests.js

cdmh219 · 2024-11-21T17:54:09Z

src/utils/chart.js

+    // const newObj = { ...obj };
+    if (!Object.hasOwn(obj, 'colorIndex')) {
+      obj.colorIndex =
+        Object.values(colors.DataLens).indexOf(colorMap[obj.name]) || 0;


CouldObject.values(color.DataLens) be put into a variable prior to the for loop at line 239? It just seems like calling Object.values in a multiple times in a loop could affect performance, even in in a minor way.

Suggested change

Object.values(colors.DataLens).indexOf(colorMap[obj.name]) || 0;

const values = Object.values(colors.DataLens);

...

values.indexOf(colorMap[obj.name]) || 0;

cdmh219 · 2024-11-21T17:58:35Z

src/utils/index.js

@@ -456,4 +457,23 @@ export function removeNullProperties(object) {
    }
    return acc;
  }, {});
+
+  for (const key in myObject) {


Not sure if this would work, but it would be easier to read and follow. You could also chain it to line 449.

Suggested change

for (const key in myObject) {

Object.keys(myObject).filter(key => {

return !(Array.isArray(key) && key.length === 0)

});

cdmh219 · 2024-11-21T18:25:05Z

src/reducers/trends/trendsSlice.spec.js

@@ -6,12 +6,9 @@ import trends, {
  depthReset,
  focusChanged,
  focusRemoved,
-  getDefaultState,
-  mainNameLens,
+  // getDefaultState,


Since lines 9 and 11 are commented, they can be removed

cdmh219 · 2024-11-22T14:51:43Z

src/components/Filters/Aggregation/AggregationItem/AggregationItem.spec.js

-      renderComponent(ownProps, aggs, filters);
-
+      renderComponent(ownProps, filters);
+      await screen.findByRole('checkbox');
      expect(screen.getByRole('checkbox')).not.toBeChecked();


Same as above. Can the checkbox be moved to a variable?

cdmh219 · 2024-11-22T16:09:38Z

src/reducers/query/querySlice.spec.js

-      });
-    });
-  });
+  // describe('COMPLAINTS_RECEIVED actions', () => {


Can this commented code be deleted?

That reminds me, I commented out these tests since these need to be moved over to validate the API.

cdmh219 · 2024-11-22T16:16:24Z

src/components/Trends/TrendDepthToggle/TrendDepthToggle.spec.js

+      },
+    );
+
+    await screen.findByRole('button', { name: 'Show less' });


Can button be used in one variable across both lines 118 and 119?

This is intentional, this case, await returns a Promise and we use it here to allow the button to render asynchronously.

Normally in Cypress tests we're not supposed to be saving the selectors in a variable. They don't recommend it.

This is one of the primary reasons why we do not recommend using variables within your tests.

https://learn.cypress.io/cypress-fundamentals/understanding-the-asynchronous-nature-of-cypress

Typically I prefer to have the screen.getBy calls repeated explicitly instead of saving it in a variable because if the DOM somehow changes, each screen.getBy call is a new call to check the document instead of having a saved variable.

src/components/Trends/TrendDepthToggle/TrendDepthToggle.spec.js

cdmh219 · 2024-11-22T16:47:56Z

In addition to the feedback provided in each file, please also make sure to update the Cypress tests accordingly (if needed). When I tried running yarn run cypress run on this branch, I got errors that implied the updated REST calls weren't being recognized. Please verify on your end.

test update, restore detail slice temporary to test something remove detail slice working aggregations query standardize fetch aggs into single hook remove aggsslice remove queryManager middleware cleanup, remove more stuff working map tab logic to disable query hook remove message middleware list view working cleanup, passing query params to api so we can extract to transformResponse Save work save work move color map to api remove query manager things missing dep more loogic to prevent query from firing when not initialized working pagination, moved breakpoints to API, and passing them through pagination component remove breakpoints from querySlice cleanup remove persist clean up comments and errors complaint detail test updates save work, clearing out URL unused clode cleanup querySlice tests updated trends test fixes tile map test squash lines add prettier ignore file updating tests, refactor product save. working tests for Company filter refactor update dist fixing pill test issue test fix restructure files restore update tests save some work trend depth toggle unit test squash moved files focus header test fixes? save stuff searchpanel fix fixing pagination unit tests fixing app.spec test unit test actionbar aggregation item test condense aggregation item test aggregation item test trends panel tests print info tests move file unit test :( working map panel test simple filter fix data export unit test row test tour test line chart test unit test fixes delete detail test for redux store update unit tests test coverage update tests, fix condition causing url to not synch properly when copy/pasting url fix cypress tests fix url req call move api hooks to folder update dist update dist

flacoman91 · 2024-12-02T18:15:31Z

In addition to the feedback provided in each file, please also make sure to update the Cypress tests accordingly (if needed). When I tried running yarn run cypress run on this branch, I got errors that implied the updated REST calls weren't being recognized. Please verify on your end.

The tests were updated since some API calls changed.

I wasn't seeing this. I can look into it further if you can provide a screenshot.

The cypress tests are also running in CI.

…76-rtk-query-upgrade # Conflicts: # dist/ccdb5.css.map # dist/ccdb5.js # dist/ccdb5.js.map

flacoman91 force-pushed the DATAP-1576-rtk-query-upgrade branch 5 times, most recently from d2f14cc to 5b4e295 Compare November 1, 2024 22:29

flacoman91 force-pushed the DATAP-1576-rtk-query-upgrade branch 6 times, most recently from a5f3ee1 to 8e06b0f Compare November 8, 2024 23:40

flacoman91 force-pushed the DATAP-1576-rtk-query-upgrade branch 4 times, most recently from f9ebdd3 to cd3d768 Compare November 21, 2024 00:18

flacoman91 marked this pull request as ready for review November 21, 2024 00:21

flacoman91 requested a review from cdmh219 November 21, 2024 00:46

cdmh219 reviewed Nov 22, 2024

View reviewed changes

flacoman91 force-pushed the DATAP-1576-rtk-query-upgrade branch from cfc89ca to ea8a216 Compare November 27, 2024 00:10

flacoman91 added 4 commits November 26, 2024 20:23

update test

ccfcddf

responding to feedback

b197826

fixing soempty url gets params

b2efcc7

remove cloneDeep in favor of structuredClone

8560036

flacoman91 added 2 commits December 2, 2024 14:54

Merge commit '7fd8619622bbf9e42c2f3ec33998504a242aa74d' into DATAP-15…

ffc6416

…76-rtk-query-upgrade # Conflicts: # dist/ccdb5.css.map # dist/ccdb5.js # dist/ccdb5.js.map

complaints test api

adbf3c4

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Migrate to RTK Query #553

Migrate to RTK Query #553

flacoman91 commented Oct 24, 2024

cdmh219 Nov 21, 2024

flacoman91 Nov 27, 2024

cdmh219 Nov 21, 2024

flacoman91 Nov 27, 2024

cdmh219 Nov 21, 2024

cdmh219 Nov 21, 2024

cdmh219 Nov 21, 2024

cdmh219 Nov 22, 2024

cdmh219 Nov 22, 2024

flacoman91 Nov 27, 2024

cdmh219 Nov 22, 2024

flacoman91 Nov 27, 2024

cdmh219 commented Nov 22, 2024 •

edited

Loading

flacoman91 commented Dec 2, 2024

		@@ -0,0 +1,16 @@
		import { useGetAggregationsQuery } from '../complaints/complaints';

		const parts = tooltip.title.split(':');
		newTooltip.values = cloneDeep(tooltip.values);

	newTooltip.values = cloneDeep(tooltip.values);
	newTooltip.values = {...tooltip.values};

-        Object.values(colors.DataLens).indexOf(colorMap[obj.name]) || 0;
+        const values = Object.values(colors.DataLens);
+        ...
+        values.indexOf(colorMap[obj.name]) || 0;

-  for (const key in myObject) {
+  Object.keys(myObject).filter(key => {
+  return !(Array.isArray(key) && key.length === 0)
+  });

Migrate to RTK Query #553

Are you sure you want to change the base?

Migrate to RTK Query #553

Conversation

flacoman91 commented Oct 24, 2024

Additions

Removals

Changes

Testing

Screenshots

Notes

Todos

Checklist

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

cdmh219 commented Nov 22, 2024 • edited Loading

flacoman91 commented Dec 2, 2024

cdmh219 commented Nov 22, 2024 •

edited

Loading