UIRS-33 Major rework using `react-query` #68

axelhunn · 2021-05-20T09:00:13Z

https://issues.folio.org/browse/UIRS-33

Purpose

Refresh the list after the configuration has been edited
Fix other API-related quirks
Refactor the code structure

Approach

Use react-query instead of Stripes connect
Introduce API abstractions
Move route-related stuff to one place, top-level
Set up integration tests

TODOS and Open Questions

refactor the providers code
improve test coverage
use moment alternative

Pre-Merge Checklist

Before merging this PR, please go through the following list and take appropriate actions.

I've added appropriate record to the CHANGELOG.md
Does this PR meet or exceed the expected quality standards?
- Code coverage on new code is 80% or greater
- Duplications on new code is 3% or less
- There are no major code smells or security issues
Does this introduce breaking changes?
- If any API-related changes - okapi interfaces and permissions are reviewed/changed correspondingly
- There are no breaking changes in this PR.

If there are breaking changes, please STOP and consider the following:

What other modules will these changes impact?
Do JIRAs exist to update the impacted modules?
- If not, please create them
- Do they contain the appropriate level of detail? Which endpoints/schemas changed, etc.
- Do they have all they appropriate links to blocked/related issues?
Are the JIRAs under active development?
- If not, contact the project's PO and make sure they're aware of the urgency.
Do PRs exist for these changes?
- If so, have they been approved?

Ideally all of the PRs involved in breaking changes would be merged in the same day to avoid breaking the folio-testing environment. Communication is paramount if that is to be achieved, especially as the number of intermodule and inter-team dependencies increase.

While it's helpful for reviewers to help identify potential problems, ensuring that it's safe to merge is ultimately the responsibility of the PR assignee.

package.json

NikitaSedyx · 2021-05-20T09:07:32Z

suggestion from EBSO team to do refactoring in separate request, in this one you implement list autorefresh (the purpose of the request), but summary is about rafctoring

axelhunn · 2021-05-20T09:57:39Z

@NikitaSedyx Yes, you're right, "refactoring" is changing the code without altering functionality.
I'll change it to "rework".
Thanks for pointing out!

NikitaSedyx · 2021-05-24T06:09:40Z

plz avoid code smells

This reverts commit e25ca80

This reverts commit ce06d56

id-jenkins · 2021-05-26T12:16:30Z

Test errors found for test. See https://jenkins-aws.indexdata.com/job/folio-org/job/ui-remote-storage/job/PR-68/17/

…UIRS-33-1 # Conflicts: # CHANGELOG.md

jest.config.js

NikitaSedyx · 2021-05-31T08:55:03Z

src/API/Configurations.js

@@ -0,0 +1,90 @@
+import { useQueryClient } from 'react-query';


why do your files start from capital? are they classes or components?

it's a composite module, with several exports

usually it follows camelCase, PascalCase is used by classes or components

I guess there's no strict rule for modules.

As a general rule we use camelCase for something that is insnance and can be multiplied,
and PascalCase for something that can be instantiated OR is used as namespace for static members, like React in

import React from 'react'; class Abc extends React.Component {

They also use the same convention in MDN for module names, see
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Guide/Modules#creating_a_module_object

As for naming the files, in React world we use same case for filename as for it content - same PascalCase for components and classes. So I used to name such modules in PascalCase too.

But again, no strict rules for that, it's an opinionated thing.
So if you point me to the rule on this here in FOLIO, I'll gladly abide it.

Here is elaborate article on the subject https://badgerbadgerbadgerbadger.medium.com/letter-casing-in-names-of-imported-nodejs-modules-707194c6a003

src/API/Configurations.js

NikitaSedyx · 2021-05-31T08:56:15Z

src/API/Configurations.js

+
+import { useOkapiQuery } from './useOkapiQuery';
+import { useOkapiMutation } from './useOkapiMutation';
+


is it new style to have 2 empty lines?

General stripes lint rules added this possibility a while ago.
See https://folio-project.slack.com/archives/C210UCHQ9/p1607449155266300

I guess visual dividing logically separated portions of code is a good thing.
Anything is good if it adds readability.

I think it's enough to have 1 line to have visual dividing logically separated portions of code

What if portions to be separated have empty lines inside?

I mean, these are mostly just functions and not various heterogenous chunks of code, so I don't really think that applies here. But 🤷, I don't really have a stake in this.

NikitaSedyx · 2021-05-31T08:56:49Z

src/API/test/Configurations/useCreateMutation.test.js

+
+let request;
+
+beforeEach(() => {


outside describe? is it legal?

Yes, perfectly so.
Took it from this exapmle https://testing-library.com/docs/react-testing-library/example-intro/

IMO describe provides more readable message in case of fail

describe is just a tool for grouping related test.
Test files are the tool for the same purpose.

If the filename clearly tells what the test is about, the message is informative enough

What is nice about using describe is that you can filter the entire test file with jest -t "some describe text". Without it it's harder to do.

doytch · 2021-06-01T13:02:45Z

src/API/Configurations.js

+
+import { useOkapiQuery } from './useOkapiQuery';
+import { useOkapiMutation } from './useOkapiMutation';
+


I mean, these are mostly just functions and not various heterogenous chunks of code, so I don't really think that applies here. But 🤷, I don't really have a stake in this.

doytch · 2021-06-01T13:11:17Z

src/Configurations/ListPane/List.js

@@ -0,0 +1,55 @@
+import React, { useMemo } from 'react';
+import { FormattedMessage, useIntl } from 'react-intl';
+import moment from 'moment';


I'm trying to not use moment in new code these days given the Moment project's suggestion that you probably shouldn't be using it anymore. At a glance it seems like the vanilla JS libs could satisfy your requirements here - is that the case or did I not dig enough?

That's from the code before. I just left it as-is.
What do we use in folio instead?

Ahhh, hard to tell when it just shows up as a new file in Github! In general, vanilla JS' Date API is good enough for everything I've needed in Folio, beforehand. That link in the comment has some alternatives, including libraries if vanilla isn't good enough.

I guess, I'll leave it a tech debt for now.
Too many changes for one PR )

mkuklis

Nice work @axelhunn!

mkuklis · 2021-06-01T17:18:20Z

src/API/test/Configurations/useCreateMutation.test.js

+
+let request;
+
+beforeEach(() => {


What is nice about using describe is that you can filter the entire test file with jest -t "some describe text". Without it it's harder to do.

sonarcloud · 2021-06-01T18:27:36Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

87.2% Coverage
0.0% Duplication

ViktorSoroka07 · 2021-06-18T23:38:04Z

Good job, like how you used msw and turned off logs for errored requests.

UIRS-33 Major refactor using react-query

e17ba09

axelhunn requested review from NikitaSedyx, uladislausamets and AndreiShumski2 May 20, 2021 09:00

NikitaSedyx reviewed May 20, 2021

View reviewed changes

package.json Outdated Show resolved Hide resolved

axelhunn changed the title ~~UIRS-33 Major refactor using react-query~~ UIRS-33 Major rework using react-query May 20, 2021

axelhunn added 13 commits May 20, 2021 13:01

revert irrelevant version bumps

ef6acee

"refactor" -> "rework"

c1b091e

get rid of key param for useOkapiQuery

07ca20e

f

840c1fd

add ActionMenu component

9612c6a

add Delete scenario component

4ca890a

refactor using ActionMenu & Delete scenario

39b2f5c

dedupe AccessionWorkflow and ReturningWorkflow

a732703

dedupe AccessionWorkflow and ReturningWorkflow deeper

8c6dc21

add Centered component

cfdfd62

add ErrorCentered component

d28705b

refactor LoadingCentered using Centered

6ac8418

add ErrorCentered content to List & DetailPane

e969c0a

axelhunn and others added 8 commits May 24, 2021 12:17

fix code smell

f68503b

fix smell

e25ca80

Merge branch 'master' into UIRS-33-1

552ba2b

Revert "fix smell"

d9621f3

This reverts commit e25ca80

DRY invalidating the list on mutation

ef4bc91

fix smell

ce06d56

Revert "fix smell"

c7fa8a4

This reverts commit ce06d56

add testing dependencies

a0cef89

axelhunn added 5 commits May 26, 2021 20:06

fix propTypes

637e49a

fix keyHandler warning

1de2592

add integration test

2990ad8

update integration test

5d7c2ea

Merge branch 'master' of github.com:folio-org/ui-remote-storage into …

33fec73

…UIRS-33-1 # Conflicts: # CHANGELOG.md

uladislausamets approved these changes May 30, 2021

View reviewed changes

update tests

f3cc2b3

NikitaSedyx reviewed May 31, 2021

View reviewed changes

jest.config.js Outdated Show resolved Hide resolved

fix on review

6bdd972

NikitaSedyx reviewed May 31, 2021

View reviewed changes

src/API/Configurations.js Outdated Show resolved Hide resolved

NikitaSedyx reviewed May 31, 2021

View reviewed changes

axelhunn added 2 commits May 31, 2021 12:28

cleanup

2c11ea8

add API_PATH constant

304fb17

axelhunn requested review from doytch, zburke, ViktorSoroka07, MikeTaylor, mkuklis and skomorokh June 1, 2021 09:45

doytch approved these changes Jun 1, 2021

View reviewed changes

mkuklis approved these changes Jun 1, 2021

View reviewed changes

move keyHandler prop filtering to ActionMenu

5d0360c

axelhunn merged commit b0bc553 into master Jun 1, 2021

axelhunn deleted the UIRS-33-1 branch June 1, 2021 19:22

axelhunn mentioned this pull request Jun 1, 2021

UIRS-33 Refresh the list after the configuration has been edited #63

Closed

11 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

UIRS-33 Major rework using `react-query` #68

UIRS-33 Major rework using `react-query` #68

axelhunn commented May 20, 2021 •

edited

NikitaSedyx commented May 20, 2021

axelhunn commented May 20, 2021

NikitaSedyx commented May 24, 2021

id-jenkins commented May 26, 2021

NikitaSedyx May 31, 2021

axelhunn May 31, 2021

NikitaSedyx May 31, 2021

axelhunn May 31, 2021 •

edited

axelhunn May 31, 2021

NikitaSedyx May 31, 2021

axelhunn May 31, 2021

NikitaSedyx May 31, 2021

axelhunn May 31, 2021

doytch Jun 1, 2021

NikitaSedyx May 31, 2021

axelhunn May 31, 2021

NikitaSedyx May 31, 2021

axelhunn May 31, 2021

mkuklis Jun 1, 2021

doytch Jun 1, 2021

doytch Jun 1, 2021

axelhunn Jun 1, 2021

doytch Jun 1, 2021

axelhunn Jun 1, 2021

mkuklis left a comment

mkuklis Jun 1, 2021

sonarcloud bot commented Jun 1, 2021

ViktorSoroka07 commented Jun 18, 2021

		@@ -0,0 +1,90 @@
		import { useQueryClient } from 'react-query';


		import { useOkapiQuery } from './useOkapiQuery';
		import { useOkapiMutation } from './useOkapiMutation';


		let request;

		beforeEach(() => {


		let request;

		beforeEach(() => {

UIRS-33 Major rework using react-query #68

UIRS-33 Major rework using react-query #68

Conversation

axelhunn commented May 20, 2021 • edited

Purpose

Approach

TODOS and Open Questions

Pre-Merge Checklist

NikitaSedyx commented May 20, 2021

axelhunn commented May 20, 2021

NikitaSedyx commented May 24, 2021

id-jenkins commented May 26, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

axelhunn May 31, 2021 • edited

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mkuklis left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sonarcloud bot commented Jun 1, 2021

ViktorSoroka07 commented Jun 18, 2021

UIRS-33 Major rework using `react-query` #68

UIRS-33 Major rework using `react-query` #68

axelhunn commented May 20, 2021 •

edited

axelhunn May 31, 2021 •

edited