UIIN-1406 Allow remote storage locations to be changed to non-remote storage location when moving the items between holdings #1318

axelhunn · 2021-02-24T13:43:49Z

https://issues.folio.org/browse/UIIN-1406
Allow remote storage locations to be changed to non-remote storage location when moving the items between holdings

Purpose

Approach

TODOS and Open Questions

Learning

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.

id-jenkins · 2021-02-24T14:09:20Z

Test errors found. See https://jenkins-aws.indexdata.com/job/folio-org/job/ui-inventory/job/PR-1318/1/

- useConfirmationModal - useMoveItemsMutation

id-jenkins · 2021-02-25T08:50:08Z

Test errors found. See https://jenkins-aws.indexdata.com/job/folio-org/job/ui-inventory/job/PR-1318/4/

aliaksei-chumakou

interesting style of grouping modal parts... At least kind of unique

mkuklis

Nice and clean @axelhunn. It's great to see react query in use too.

mkuklis · 2021-02-25T15:28:43Z

src/Instance/InstanceMovement/InstanceMovementContainer.js


    history.push({
      pathname: `/inventory/view/${instanceId}`,
      search: location.search,
    });
-  });
+  }, [history, location, instanceFrom, instanceTo]);


@axelhunn the history and location are objects and useCallback doesn't use deep comparison

facebook/react#14476

so this won't work as expected (it will basically recreate onClose more often). I would suggest either removing them from the dependency array or using something similar to:

https://github.com/kotarella1110/use-custom-compare

Thanks for pointing this out. In fact I made this change for linter not to yell at me.
It happens the history is a mutable object (see https://reactrouter.com/web/api/history).
It is the same object every time, so we can rely on === comparison.

As for the location - it's not needed here, we can use history.location instead,

I've refactored the thing.

doytch

This looks good! I love the separation of smart/dumb work you've accomplished here with hooks. It looks easier to maintain and reason through than the previous mixing of business logic into components. Hopefully that bears out in the future!

And definitely a novel way of project structure compared to other spots in Folio but I see the good it brings.

I just put a couple of notes on minor things that I noticed.

src/Instance/Move/useMoveItems.js

doytch · 2021-02-25T18:23:55Z

src/RemoteStorageService/index.js

@@ -0,0 +1,3 @@
+export * as Check from './Check';


I like the way you built up the chain for these components. They're clearly related and encoding that relationship in their access (eg, RemoteStorage.Warning.ForHoldings) hammers it into the readers head.

doytch · 2021-02-25T18:27:56Z

src/RemoteStorageService/Confirmation.js

+import { Translate } from '../common';
+
+
+export const Heading = () => <Translate id="Removing from remote storage" />;


I do like how this looks when browsing the code... 😆

Yeah, but we batted it around at #stripes-architecture 2021-02-05 ago and specifically decided against it.

I agree with you, @doytch, this does read nicely, but instead of ignoring established precedent and the decision we made as a group, @axelhunn please raise this again at #stripes-architecture and we can discuss the existing tools that are built into react-intl that provide the same functionality with description or defaultMessage? Maybe we can have the best of both worlds here.

@zburke sorry, didn't get it we decided against it.
Anyway, I made this part as more of a POC.
Reverted to unified way now, please review.

doytch · 2021-02-25T18:34:13Z

src/RemoteStorageService/Check.js

+export const useByLocation = () => {
+  const remoteMap = useRemoteStorageMappings();
+
+  return ({ fromLocationId, toLocationId }) => (fromLocationId in remoteMap) && !(toLocationId in remoteMap);


Could this be called with an undefined for either of the location ids? There're too many usages throughout the PR for me to track down completely, but it caught my eye. undefined in remoteMap will evaluate to false. The first thing that came to mind was situations where you could have some stale data or something.

It's legitimate to say that "no, that should never happen and it should spectacularly and loudly fail if it does." 😄

The point here is to return true only if we move something from remote location to non-remote location.

when toLocationId is undefined - it could be a black hole, but it's not marked as remote, so
(depends-on-from) && !(false) === depends-on-from

when fromLocationId is undefined - it certainly is not known remote location,
(false) && !(whatever) === false

when both of them are undefined - the fromLocationId is undefined enough
(false) && !(false) === false

mkuklis · 2021-02-25T19:50:21Z

src/RemoteStorageService/Check.js

+  const { holdingsById } = useHoldings();
+  const check = useByLocation();
+
+  return ({ fromHoldingsId, toHoldingsId }) => {


@axelhunn I wonder if this should be wrapped in useCallback. Otherwise I think it will be recreated on every render. What do you think?

Since I've read this article by Kent C. Dodds, I try not to overuse the useCallback.

But you're right, this function is used in dependencies of another useCallbacks and useMemos.
Fixed it.

Thank you for sharing that article @axelhunn

Co-authored-by: Mark Deutsch <[email protected]>

id-jenkins · 2021-02-25T21:51:27Z

Test errors found. See https://jenkins-aws.indexdata.com/job/folio-org/job/ui-inventory/job/PR-1318/5/

id-jenkins · 2021-02-25T22:40:54Z

Test errors found. See https://jenkins-aws.indexdata.com/job/folio-org/job/ui-inventory/job/PR-1318/6/

id-jenkins · 2021-02-25T23:30:55Z

Test errors found. See https://jenkins-aws.indexdata.com/job/folio-org/job/ui-inventory/job/PR-1318/7/

…1406-2

id-jenkins · 2021-02-26T00:11:36Z

Test errors found. See https://jenkins-aws.indexdata.com/job/folio-org/job/ui-inventory/job/PR-1318/8/

id-jenkins · 2021-02-26T00:27:42Z

Test errors found. See https://jenkins-aws.indexdata.com/job/folio-org/job/ui-inventory/job/PR-1318/9/

id-jenkins · 2021-02-26T02:18:22Z

Test errors found. See https://jenkins-aws.indexdata.com/job/folio-org/job/ui-inventory/job/PR-1318/10/

id-jenkins · 2021-02-26T02:43:04Z

Test errors found. See https://jenkins-aws.indexdata.com/job/folio-org/job/ui-inventory/job/PR-1318/11/

id-jenkins · 2021-02-26T06:49:32Z

Test errors found. See https://jenkins-aws.indexdata.com/job/folio-org/job/ui-inventory/job/PR-1318/12/

id-jenkins · 2021-02-26T08:23:43Z

Test errors found. See https://jenkins-aws.indexdata.com/job/folio-org/job/ui-inventory/job/PR-1318/13/

id-jenkins · 2021-02-26T08:51:36Z

Test errors found. See https://jenkins-aws.indexdata.com/job/folio-org/job/ui-inventory/job/PR-1318/14/

id-jenkins · 2021-02-26T09:24:10Z

Test errors found. See https://jenkins-aws.indexdata.com/job/folio-org/job/ui-inventory/job/PR-1318/15/

id-jenkins · 2021-02-26T10:06:11Z

Test errors found. See https://jenkins-aws.indexdata.com/job/folio-org/job/ui-inventory/job/PR-1318/16/

id-jenkins · 2021-02-26T10:47:52Z

Test errors found. See https://jenkins-aws.indexdata.com/job/folio-org/job/ui-inventory/job/PR-1318/17/

browserDisconnectTolerance: 10, browserNoActivityTimeout: 100000, flags: [ '--disable-gpu', '--no-sandbox' ],

id-jenkins · 2021-02-26T11:24:22Z

Test errors found. See https://jenkins-aws.indexdata.com/job/folio-org/job/ui-inventory/job/PR-1318/19/

id-jenkins · 2021-02-26T11:30:21Z

Test errors found. See https://jenkins-aws.indexdata.com/job/folio-org/job/ui-inventory/job/PR-1318/18/

sonarqubecloud · 2021-02-26T11:57:44Z

SonarCloud Quality Gate failed.

2 Bugs
0 Vulnerabilities
0 Security Hotspots
1 Code Smell

61.7% Coverage
0.0% Duplication

axelhunn added 7 commits February 25, 2021 02:25

add react-query

e73b0f2

refactor MoveToDropdown

8ce8676

add common hooks

a59f8c8

- useConfirmationModal - useMoveItemsMutation

use MoveToDropdown without locationsById

c447dd2

remove remote-storage-configurations optional okapi interface

0c0b9f8

add Translate component

935e260

add the feature

2eeb6a2

axelhunn force-pushed the UIIN-1406-2 branch from f0f0cb6 to 2eeb6a2 Compare February 25, 2021 01:20

axelhunn marked this pull request as ready for review February 25, 2021 01:23

axelhunn requested review from NikitaSedyx, MikeTaylor, uladislausamets, mkuklis, aliaksei-chumakou and doytch February 25, 2021 01:23

fix useConfirmationModal typings

575d67a

aliaksei-chumakou approved these changes Feb 25, 2021

View reviewed changes

uladislausamets approved these changes Feb 25, 2021

View reviewed changes

mkuklis approved these changes Feb 25, 2021

View reviewed changes

mkuklis reviewed Feb 25, 2021

View reviewed changes

doytch approved these changes Feb 25, 2021

View reviewed changes

mkuklis reviewed Feb 25, 2021

View reviewed changes

fix null guards

69a4fcd

Co-authored-by: Mark Deutsch <[email protected]>

axelhunn added 2 commits February 26, 2021 00:56

revert translation keys to unified form

056a558

revert to using FormattedMessage

180afc2

axelhunn requested a review from zburke February 25, 2021 22:15

remove location dependency in InstanceMovementContainer

323af6b

axelhunn added 2 commits February 26, 2021 02:42

wrap checks in useCallback

1a3041b

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

2d2d6ab

…1406-2

revert yarn stripes to stripes

fb280a2

increase Karma browserDisconnectTimeout to 20s

a4ce11e

axelhunn added 2 commits February 26, 2021 13:54

browserDisconnectTimeout: 600s

90bf528

browserDisconnectTimeout: 100000,

b26a256

browserDisconnectTolerance: 10, browserNoActivityTimeout: 100000, flags: [ '--disable-gpu', '--no-sandbox' ],

mocha: { timeout: 100000 },

714b218

axelhunn merged commit 3fb5cbb into master Feb 26, 2021

axelhunn deleted the UIIN-1406-2 branch February 26, 2021 12:04

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

UIIN-1406 Allow remote storage locations to be changed to non-remote storage location when moving the items between holdings #1318

UIIN-1406 Allow remote storage locations to be changed to non-remote storage location when moving the items between holdings #1318

axelhunn commented Feb 24, 2021

id-jenkins commented Feb 24, 2021

id-jenkins commented Feb 25, 2021

aliaksei-chumakou left a comment

mkuklis left a comment

mkuklis Feb 25, 2021 •

edited

Loading

axelhunn Feb 25, 2021

doytch left a comment

doytch Feb 25, 2021

doytch Feb 25, 2021

zburke Feb 25, 2021

axelhunn Feb 25, 2021

doytch Feb 25, 2021

axelhunn Feb 25, 2021

mkuklis Feb 25, 2021

axelhunn Feb 25, 2021

mkuklis Feb 26, 2021

id-jenkins commented Feb 25, 2021

id-jenkins commented Feb 25, 2021

id-jenkins commented Feb 25, 2021

id-jenkins commented Feb 26, 2021

id-jenkins commented Feb 26, 2021

id-jenkins commented Feb 26, 2021

id-jenkins commented Feb 26, 2021

id-jenkins commented Feb 26, 2021

id-jenkins commented Feb 26, 2021

id-jenkins commented Feb 26, 2021

id-jenkins commented Feb 26, 2021

id-jenkins commented Feb 26, 2021

id-jenkins commented Feb 26, 2021

id-jenkins commented Feb 26, 2021

id-jenkins commented Feb 26, 2021

sonarqubecloud bot commented Feb 26, 2021

		import { Translate } from '../common';


		export const Heading = () => <Translate id="Removing from remote storage" />;

UIIN-1406 Allow remote storage locations to be changed to non-remote storage location when moving the items between holdings #1318

UIIN-1406 Allow remote storage locations to be changed to non-remote storage location when moving the items between holdings #1318

Conversation

axelhunn commented Feb 24, 2021

Purpose

Approach

TODOS and Open Questions

Learning

Pre-Merge Checklist

id-jenkins commented Feb 24, 2021

id-jenkins commented Feb 25, 2021

aliaksei-chumakou left a comment

Choose a reason for hiding this comment

mkuklis left a comment

Choose a reason for hiding this comment

mkuklis Feb 25, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

doytch left a 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

id-jenkins commented Feb 25, 2021

id-jenkins commented Feb 25, 2021

id-jenkins commented Feb 25, 2021

id-jenkins commented Feb 26, 2021

id-jenkins commented Feb 26, 2021

id-jenkins commented Feb 26, 2021

id-jenkins commented Feb 26, 2021

id-jenkins commented Feb 26, 2021

id-jenkins commented Feb 26, 2021

id-jenkins commented Feb 26, 2021

id-jenkins commented Feb 26, 2021

id-jenkins commented Feb 26, 2021

id-jenkins commented Feb 26, 2021

id-jenkins commented Feb 26, 2021

id-jenkins commented Feb 26, 2021

sonarqubecloud bot commented Feb 26, 2021

mkuklis Feb 25, 2021 •

edited

Loading