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

UIRS-33 Major rework using react-query #68

Merged
merged 35 commits into from Jun 1, 2021
Merged
Show file tree
Hide file tree
Changes from 32 commits
Commits
Show all changes
35 commits
Select commit Hold shift + click to select a range
e17ba09
UIRS-33 Major refactor using `react-query`
axelhunn May 20, 2021
ef6acee
revert irrelevant version bumps
axelhunn May 20, 2021
c1b091e
"refactor" -> "rework"
axelhunn May 20, 2021
07ca20e
get rid of `key` param for `useOkapiQuery`
axelhunn May 20, 2021
840c1fd
f
axelhunn May 20, 2021
9612c6a
add ActionMenu component
axelhunn May 21, 2021
4ca890a
add Delete scenario component
axelhunn May 21, 2021
39b2f5c
refactor using ActionMenu & Delete scenario
axelhunn May 21, 2021
a732703
dedupe AccessionWorkflow and ReturningWorkflow
axelhunn May 21, 2021
8c6dc21
dedupe AccessionWorkflow and ReturningWorkflow deeper
axelhunn May 21, 2021
cfdfd62
add Centered component
axelhunn May 21, 2021
d28705b
add ErrorCentered component
axelhunn May 21, 2021
6ac8418
refactor LoadingCentered using Centered
axelhunn May 21, 2021
e969c0a
add ErrorCentered content to List & DetailPane
axelhunn May 21, 2021
f68503b
fix code smell
axelhunn May 24, 2021
e25ca80
fix smell
axelhunn May 24, 2021
552ba2b
Merge branch 'master' into UIRS-33-1
axelhunn May 24, 2021
d9621f3
Revert "fix smell"
axelhunn May 24, 2021
ef4bc91
DRY invalidating the list on mutation
axelhunn May 24, 2021
ce06d56
fix smell
axelhunn May 24, 2021
c7fa8a4
Revert "fix smell"
axelhunn May 24, 2021
a0cef89
add testing dependencies
axelhunn May 26, 2021
e291ad7
remove custom `coverageDirectory` from Jest config
axelhunn May 26, 2021
761014e
add API tests
axelhunn May 26, 2021
dd1a1a2
Merge branch 'master' into UIRS-33-1
axelhunn May 26, 2021
637e49a
fix propTypes
axelhunn May 26, 2021
1de2592
fix keyHandler warning
axelhunn May 28, 2021
2990ad8
add integration test
axelhunn May 28, 2021
5d7c2ea
update integration test
axelhunn May 29, 2021
33fec73
Merge branch 'master' of github.com:folio-org/ui-remote-storage into …
axelhunn May 29, 2021
f3cc2b3
update tests
axelhunn May 31, 2021
6bdd972
fix on review
axelhunn May 31, 2021
2c11ea8
cleanup
axelhunn May 31, 2021
304fb17
add API_PATH constant
axelhunn May 31, 2021
5d0360c
move keyHandler prop filtering to ActionMenu
axelhunn Jun 1, 2021
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
14 changes: 13 additions & 1 deletion .eslintrc
@@ -1,4 +1,16 @@
{
"parser": "babel-eslint",
"extends": ["@folio/eslint-config-stripes/acquisitions"]
"extends": ["@folio/eslint-config-stripes/acquisitions"],
"rules": {
"no-multiple-empty-lines": "off"
},
"overrides": [
{
"files": ["*test.*", "test/**"],
"rules": {
"react/prop-types": "off",
"padding-line-between-statements": "off"
}
}
]
}
3 changes: 2 additions & 1 deletion CHANGELOG.md
Expand Up @@ -4,9 +4,10 @@
* [UIRS-15](https://issues.folio.org/browse/UIRS-15) Remote Storage Settings: Credential properties (apiKey)
* [UIRS-24](https://issues.folio.org/browse/UIRS-24) Add Returning workflow preference
* [UIRS-27](https://issues.folio.org/browse/UIRS-27) Add to "Remote storage: Create, read, update, delete" all the missing permissions on configurations and providers
* [UIRS-30](https://issues.folio.org/browse/UIRS-30) Compile Translation Files into AST Format.
* [UIRS-30](https://issues.folio.org/browse/UIRS-30) Compile Translation Files into AST Format
* [UIRS-41](https://issues.folio.org/browse/UIRS-41) Add Accession holdings workflow preference
* [UIRS-42](https://issues.folio.org/browse/UIRS-42) Update "remote-storage-mappings" interface
* [UIRS-33](https://issues.folio.org/browse/UIRS-33) Major rework using `react-query`

## [1.0.0](https://github.com/folio-org/ui-remote-storage/tree/v1.0.0) (2021-03-19)

Expand Down
7 changes: 1 addition & 6 deletions jest.config.js
@@ -1,6 +1 @@
const commonCofig = require('@folio/stripes-acq-components/jest.config');

module.exports = {
...commonCofig,
coverageDirectory: './artifacts/coverage-jest/',
};
module.exports = require('@folio/stripes-acq-components/jest.config');
13 changes: 10 additions & 3 deletions package.json
Expand Up @@ -76,19 +76,24 @@
"@folio/stripes": "^6.0.0",
"@folio/stripes-cli": "^2.0.0",
"@formatjs/cli": "^4.2.10",
"@testing-library/dom": "^7.29.6",
"@testing-library/jest-dom": "^5.11.1",
"@testing-library/react": "^11.0.2",
"@testing-library/react-hooks": "^6.0.0",
"@testing-library/user-event": "^13.1.9",
"babel-eslint": "^10.0.0",
"babel-jest": "^26.3.0",
"core-js": "^3.6.1",
"eslint": "^6.2.1",
"cross-fetch": "^3.1.4",
"eslint": "^7.26.0",
"eslint-plugin-filenames": "^1.3.2",
"eslint-plugin-jest": "^24.0.0",
"faker": "^4.1.0",
"identity-obj-proxy": "^3.0.0",
"jest": "^26.4.2",
"jest-css-modules": "^2.1.0",
"jest-junit": "^11.1.0",
"msw": "^0.28.2",
"react": "^16.5.1",
"react-dom": "^16.5.1",
"react-intl": "^5.7.0",
Expand All @@ -98,8 +103,6 @@
},
"dependencies": {
"@folio/stripes-acq-components": "^2.1.0",
"@testing-library/dom": "^7.29.6",
"@testing-library/user-event": "^12.8.0",
"lodash": "^4.17.5",
"moment": "^2.29.1",
"prop-types": "^15.5.10",
Expand All @@ -111,10 +114,14 @@
"peerDependencies": {
"@folio/stripes": "^6.0.0",
"final-form": "^4.18.2",
"ky": "*",
"react": "*",
"react-intl": "^5.7.0 ",
"react-query": "^3.6.0",
"react-router": "^5.2.0",
"react-router-dom": "^5.2.0"
},
"resolutions": {
"babel-eslint/@babel/parser": "7.7.5"
}
}
90 changes: 90 additions & 0 deletions src/API/Configurations.js
@@ -0,0 +1,90 @@
import { useQueryClient } from 'react-query';
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it's a composite module, with several exports

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@axelhunn axelhunn May 31, 2021

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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


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

Copy link
Contributor

Choose a reason for hiding this comment

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

is it new style to have 2 empty lines?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What if portions to be separated have empty lines inside?

Copy link

Choose a reason for hiding this comment

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

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.


export const useListQuery = options => {
const query = useOkapiQuery({
path: 'remote-storage/configurations',
queryKey: 'remote-storage/configurations',
...options,
});

return {
configurations: query.data?.configurations ?? [],
...query,
};
};


export const useSingleQuery = ({ id, onError, ...rest }) => {
const queryClient = useQueryClient();

const query = useOkapiQuery({
path: `remote-storage/configurations/${id}`,
queryKey: ['remote-storage/configurations', id],
axelhunn marked this conversation as resolved.
Show resolved Hide resolved
onError: () => {
// One of the most possible sources of an error here
// is navigating to the item that was already deleted (from another workplace).
// Refreshing the list couldn't hurt in this case.
queryClient.invalidateQueries(['remote-storage/configurations'], { exact: true });

return onError?.();
},
...rest,
});

return {
configuration: query.data ?? {},
...query,
};
};


const useMutation = ({ onSettled, ...rest }) => {
const queryClient = useQueryClient();

return useOkapiMutation({
// We refresh the list on any result of item mutation, success or error:
// One of the most possible sources of an error here
// is trying to mutate the item that was already deleted (from another workplace).
// Refreshing the list couldn't hurt in this case.
onSettled: () => {
queryClient.invalidateQueries(['remote-storage/configurations'], { exact: true });

return onSettled?.();
},
...rest,
});
};


export const useCreateMutation = options => useMutation({
method: 'post',
path: 'remote-storage/configurations',
...options,
});


export const useUpdateMutation = ({ id, onSuccess, ...rest }) => {
const queryClient = useQueryClient();

return useMutation({
method: 'put',
path: `remote-storage/configurations/${id}`,
onSuccess: () => {
queryClient.invalidateQueries(['remote-storage/configurations', id], { exact: true });

return onSuccess?.();
},
...rest,
});
};


export const useDeleteMutation = ({ id, ...options }) => useMutation({
method: 'delete',
path: `remote-storage/configurations/${id}`,
...options,
});
13 changes: 13 additions & 0 deletions src/API/Mappings.js
@@ -0,0 +1,13 @@
import { useOkapiQuery } from './useOkapiQuery';

export const useListQuery = options => {
const query = useOkapiQuery({
path: 'remote-storage/mappings',
...options,
});

return {
mappings: query?.data?.mappings ?? [],
...query,
};
};
2 changes: 2 additions & 0 deletions src/API/index.js
@@ -0,0 +1,2 @@
export * as Mappings from './Mappings';
export * as Configurations from './Configurations';
73 changes: 73 additions & 0 deletions src/API/test/Configurations/useCreateMutation.test.js
@@ -0,0 +1,73 @@
import { server, rest, API_BASE } from '../../../test/net';
import { renderAPIHook, ERROR_RESPONSE } from '../setup'; // must be imported before the tested hooks

import { useCreateMutation, useListQuery } from '../../Configurations';


const data = {
name: 'RS1',
providerName: 'DEMATIC_EMS',
};

const url = {
create: `${API_BASE}/configurations`,
list: `${API_BASE}/configurations`,
};

let request;

beforeEach(() => {
Copy link
Contributor

Choose a reason for hiding this comment

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

outside describe? is it legal?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

IMO describe provides more readable message in case of fail

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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
image

Copy link

Choose a reason for hiding this comment

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

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.

request = undefined;
server.use(rest.post(
url.create,
(req, res, ctx) => {
request = req;

return res(ctx.status(201)); // "Created"
},
));
});


it('POSTs data to server', async () => {
const { result, waitFor } = renderAPIHook(useCreateMutation);

result.current.mutate(data);

await waitFor(() => result.current.isSuccess);
expect(request?.body).toEqual(data);
});

describe('Invalidation of List query', () => {
const checkListInvalidatedOn = async (status) => {
const { result, waitFor } = renderAPIHook(useCreateMutation);

const listQueryHook = renderAPIHook(useListQuery);

await waitFor(() => listQueryHook.result.current.isFetching);
await waitFor(() => !listQueryHook.result.current.isFetching && listQueryHook.result.current.isSuccess);

result.current.mutate(data);

// await waitFor(() => result.current.isSuccess);
await waitFor(() => result.current.status === status);

return listQueryHook.result.current.isFetching;
};

beforeEach(() => {
server.use(rest.get(url.list, (req, res, ctx) => res(ctx.json({
configurations: [1, 2, 3],
}))));
});

it('is made on success', async () => {
expect(await checkListInvalidatedOn('success')).toBeTruthy();
});

it('is made on error', async () => {
server.use(rest.post(url.create, ERROR_RESPONSE));

expect(await checkListInvalidatedOn('error')).toBeTruthy();
});
});
62 changes: 62 additions & 0 deletions src/API/test/Configurations/useDeleteMutation.test.js
@@ -0,0 +1,62 @@
import { server, rest, API_BASE } from '../../../test/net';
import { renderAPIHook, ERROR_RESPONSE } from '../setup'; // must be imported before the tested hooks

import { useDeleteMutation, useListQuery } from '../../Configurations';


const id = 42;
const url = {
delete: `${API_BASE}/configurations/${id}`,
list: `${API_BASE}/configurations`,
};


beforeEach(() => {
server.use(rest.delete(
url.delete,
(req, res, ctx) => res(ctx.status(204)), // "No Content"
));
});


it('DELETESs data from server', async () => {
const { result, waitFor } = renderAPIHook(() => useDeleteMutation({ id }));

result.current.mutate();

await expect(waitFor(() => result.current.isSuccess)).resolves.not.toThrow();
});


describe('Invalidation of List query', () => {
const checkListInvalidatedOn = async (status) => {
const { result, waitFor } = renderAPIHook(() => useDeleteMutation({ id }));

const listQueryHook = renderAPIHook(useListQuery);

await waitFor(() => listQueryHook.result.current.isFetching);
await waitFor(() => !listQueryHook.result.current.isFetching && listQueryHook.result.current.isSuccess);

result.current.mutate();

await waitFor(() => result.current.status === status);

return listQueryHook.result.current.isFetching;
};

beforeEach(() => {
server.use(rest.get(url.list, (req, res, ctx) => res(ctx.json({
configurations: [1, 2, 3],
}))));
});

it('is made on success', async () => {
expect(await checkListInvalidatedOn('success')).toBeTruthy();
});

it('is made on error', async () => {
server.use(rest.delete(url.delete, ERROR_RESPONSE));

expect(await checkListInvalidatedOn('error')).toBeTruthy();
});
});
40 changes: 40 additions & 0 deletions src/API/test/Configurations/useListQuery.test.js
@@ -0,0 +1,40 @@
// This should be imported before the tested hooks
import { server, rest, API_BASE } from '../../../test/net';
import { renderAPIHook, ERROR_RESPONSE } from '../setup'; // must be imported before the tested hooks

import { useListQuery } from '../../Configurations';


const url = `${API_BASE}/configurations`;

beforeEach(() => {
server.use(rest.get(url, (req, res, ctx) => res(ctx.json({
configurations: [1, 2, 3],
}))));
});


it('returns list of configurations when loaded', async () => {
const { result, waitFor } = renderAPIHook(useListQuery);

expect(result.current.isLoading).toBeTruthy();

await waitFor(() => result.current.isSuccess);
expect(result.current.configurations).toEqual([1, 2, 3]);
});

it('returns empty list while loading', () => {
const { result } = renderAPIHook(useListQuery);

expect(result.current.configurations).toEqual([]);
expect(result.current.isLoading).toBeTruthy();
});

it('returns empty list on error', async () => {
server.use(rest.get(url, ERROR_RESPONSE));

const { result, waitFor } = renderAPIHook(useListQuery);

await waitFor(() => result.current.isError);
expect(result.current.configurations).toEqual([]);
});