UINV-522: display the exchanged calculated total amount when invoice in foreign currency #757

alisher-epam · 2024-02-05T14:54:41Z

Purpose

https://folio-org.atlassian.net/browse/UINV-522 -Display the exchanged calculated total amount when invoice in foreign currency

Approach

Screen.Recording.2024-02-05.at.19.47.04.mp4

Screenshots

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.

github-actions · 2024-02-05T14:58:11Z

Jest Unit Test Statistics

    1 files ±0 115 suites +2 4m 49s ⏱️ +5s
353 tests +3 353 ✔️ +3 0 💤 ±0 0 ❌ ±0
357 runs +3 357 ✔️ +3 0 💤 ±0 0 ❌ ±0

Results for commit 9385663. ± Comparison against base commit f075b2c.

♻️ This comment has been updated with latest results.

github-actions · 2024-02-05T14:58:44Z

BigTest Unit Test Statistics

0 tests ±0 0 ✔️ ±0 0s ⏱️ ±0s
0 suites ±0 0 💤 ±0
0 files ±0 0 ❌ ±0

Results for commit 9385663. ± Comparison against base commit f075b2c.

♻️ This comment has been updated with latest results.

alisher-epam · 2024-02-06T15:15:36Z

@folio-org/fe-tl-reviewers Please review it when you are available

usavkov-epam

I've faced issues testing this locally:

NaN in the request results in bad data and incorrect view

chrome_FFUHnOjcOV.mp4

multiple sending of requests

chrome_KLxnC68Dv9.mp4

Question: what is the purpose of displaying the "Exchanged" when creating a new invoice if it does not contain the total amount?

src/common/constants/api.js

src/common/hooks/useExchangeCalculation/useExchangeCalculation.js

usavkov-epam · 2024-02-07T17:10:03Z

src/common/components/DisplayExchangedAmount/DisplayExchangedAmount.js

+
+import { useExchangeCalculation } from '../../hooks';
+
+export const DisplayExchangedAmount = ({ currency, exchangeRate, total }) => {


"DisplayExchangedAmount" name sounds like a verb. Could you please change the name of the component

alisher-epam · 2024-02-09T10:20:34Z

@usavkov-epam add debounce with 500ms to prevent multiple requests

alisher-epam · 2024-02-09T10:21:00Z

@folio-org/fe-tl-reviewers please review

usavkov-epam

Question: what is the purpose of displaying the "Exchanged" when creating a new invoice if it does not contain the total amount?

This question still seems open to me.

usavkov-epam · 2024-02-12T12:04:34Z

src/common/components/CalculatedExchangeAmount/CalculatedExchangeAmount.js

+    rate: exchangeRate,
+  });
+
+  const debouncedSave = useCallback(debounce(() => {


please pay attention to linter warnings here

alisher-epam · 2024-02-12T15:59:54Z

Question: what is the purpose of displaying the "Exchanged" when creating a new invoice if it does not contain the total amount?

This question still seems open to me.

I updated the condition here. And the amount is required to display it. https://github.com/folio-org/ui-invoice/pull/757/files#diff-8b144285a54c8b51f8ec063b9bb75a054cc4f99aab3edc30793c0bee11127b8cR21

NikitaSedyx · 2024-02-13T07:46:27Z

src/common/components/CalculatedExchangeAmount/CalculatedExchangeAmount.js

+  const systemCurrency = stripes.currency;
+  const enabled = Boolean(systemCurrency !== currency && total);
+
+  const [exchangeProps, setExchangeProps] = useState({


this state, debouncedSave and useEffect look strange, what do you solve with it?

in query key you don't use object, but it's values - so you don't need to preserve previous state

The main purpose is to prevent multiple requests via debounce function

you don't need state and effect at all, and it won't make extra requests

const { exchangedAmount } = useExchangeCalculation({ to: systemCurrency, amount: +total, etc }, { enabled });

it's the same with what you did, but without extra strange code

there is a chance you'll need to update useQuery config a bit to achieve this, but no need to do what you've done

now I see why it's required - form changes values, at the same time all this logic should be moved to useExchangeCalculation, component shouldn't know anything about it

NikitaSedyx · 2024-02-13T14:01:13Z

package.json

@@ -12,7 +12,7 @@
    "start": "yarn stripes serve",
    "build": "yarn stripes build --output ./output",
    "test": "yarn run test:unit",
-    "test:unit": "jest --ci --coverage",
+    "test:unit": "jest --ci --coverage --updateSnapshot",


remove updateSnapshot, it shouldn't be done on CI level

Sure. This was a workaround to fix the failing snapshots. Because the github action couldn't resolve the snapshot issue even though the tests passed locally.

update tests locally and commit them

It doesn't help. I have done it twice already

But "test:unit": "jest --ci --coverage --updateSnapshot", did the track

stripes-components could be updated after your commits

I am on it. I have just merged

NikitaSedyx

it works only when exchange rate is typed manually, try to select Euro and do not enter exchange rate

package.json

NikitaSedyx · 2024-02-14T12:32:32Z

src/invoices/InvoiceForm/InvoiceForm.js

@@ -131,6 +138,13 @@ const InvoiceForm = ({
    vendorInvoiceNo,
    fiscalYearId,
  } = initialValues;
+
+  const { exchangeRate: exchangeRateValue } = useExchangeRateValue(


move this to useExchangeCalculation

alisher-epam · 2024-02-14T14:17:09Z

it works only when exchange rate is typed manually, try to select Euro and do not enter exchange rate

Screen.Recording.2024-02-14.at.17.10.37.mp4

NikitaSedyx · 2024-02-14T14:28:18Z

src/common/hooks/useExchangeCalculation/useExchangeCalculation.js

+  const ky = useOkapiKy();
+  const [namespace] = useNamespace({ key: 'exchange-calculation' });
+
+  const { exchangeRate } = useExchangeRateValue(


const { exchangeRate } = useExchangeRateValue(from, to, rate)

NikitaSedyx · 2024-02-14T14:29:55Z

fix tests please

Yury is on vac and his comments are addressed

sonarqubecloud · 2024-02-14T15:01:59Z

Quality Gate passed

Issues
0 New issues

Measures
0 Security Hotspots
87.5% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarCloud

…in foreign currency (#757) * UINV-522: display exchanged amount * tests: fix failing snapshot tests * add changelog * debounce api request and rename component name * tests: update failing snapshots * disable usecallback linter warning * tests: remove failing snapshot tests * tests: add missing snapshots * rename function name * update `yarn test` command to update snapshot tests * remove `--updateSnapshot` flag after fixing snapshot test issue * regenerate snapshot tests * refactor exchange calculation hook * refactor `useExchangeCalculation` hook * tests: update failing snapshot tests * inline `useExchangeRateValue` hook arguments in one line * fix sonar issue `Remove this unused import of 'useStripes'.`

UINV-522: display exchanged amount

9336bbf

tests: fix failing snapshot tests

1d3e59d

alisher-epam requested review from a team February 5, 2024 15:11

add changelog

277c2b3

Merge branch 'develop' into UINV-522

ecc538d

alisher-epam requested a review from usavkov-epam February 6, 2024 15:32

alisher-epam self-assigned this Feb 7, 2024

usavkov-epam previously requested changes Feb 7, 2024

View reviewed changes

alisher-epam added 3 commits February 9, 2024 15:00

debounce api request and rename component name

2e7b58c

tests: update failing snapshots

8910a5e

Merge branch 'develop' into UINV-522

57303e1

alisher-epam requested a review from usavkov-epam February 9, 2024 10:12

usavkov-epam reviewed Feb 12, 2024

View reviewed changes

alisher-epam added 3 commits February 12, 2024 23:59

disable usecallback linter warning

e6ac3d3

tests: remove failing snapshot tests

b497c12

tests: add missing snapshots

be5b072

NikitaSedyx reviewed Feb 13, 2024

View reviewed changes

alisher-epam added 2 commits February 13, 2024 18:16

rename function name

6572194

update yarn test command to update snapshot tests

c385d42

alisher-epam requested review from usavkov-epam and a team February 13, 2024 13:36

OleksandrHladchenko1 approved these changes Feb 13, 2024

View reviewed changes

NikitaSedyx reviewed Feb 13, 2024

View reviewed changes

alisher-epam added 2 commits February 13, 2024 19:22

remove --updateSnapshot flag after fixing snapshot test issue

27508e6

regenerate snapshot tests

521a523

NikitaSedyx requested changes Feb 14, 2024

View reviewed changes

refactor exchange calculation hook

e7e6c0a

NikitaSedyx reviewed Feb 14, 2024

View reviewed changes

package.json Show resolved Hide resolved

NikitaSedyx reviewed Feb 14, 2024

View reviewed changes

alisher-epam mentioned this pull request Feb 14, 2024

UISACQCOMP-179: Export useExchangeRateValue hook for reusability purposes folio-org/stripes-acq-components#757

Merged

11 tasks

refactor useExchangeCalculation hook

32bad4c

NikitaSedyx reviewed Feb 14, 2024

View reviewed changes

alisher-epam added 2 commits February 14, 2024 19:37

tests: update failing snapshot tests

d62b9b0

inline useExchangeRateValue hook arguments in one line

606bbb4

NikitaSedyx approved these changes Feb 14, 2024

View reviewed changes

fix sonar issue Remove this unused import of 'useStripes'.

9385663

alisher-epam merged commit 6c7ed75 into develop Feb 14, 2024
6 checks passed

alisher-epam deleted the UINV-522 branch February 14, 2024 15:04

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

UINV-522: display the exchanged calculated total amount when invoice in foreign currency #757

UINV-522: display the exchanged calculated total amount when invoice in foreign currency #757

alisher-epam commented Feb 5, 2024 •

edited

Loading

github-actions bot commented Feb 5, 2024 •

edited

Loading

github-actions bot commented Feb 5, 2024 •

edited

Loading

alisher-epam commented Feb 6, 2024

usavkov-epam left a comment •

edited

Loading

usavkov-epam Feb 7, 2024

alisher-epam commented Feb 9, 2024

alisher-epam commented Feb 9, 2024

usavkov-epam left a comment

usavkov-epam Feb 12, 2024

alisher-epam commented Feb 12, 2024

NikitaSedyx Feb 13, 2024

alisher-epam Feb 13, 2024

NikitaSedyx Feb 13, 2024

NikitaSedyx Feb 13, 2024

NikitaSedyx Feb 13, 2024

NikitaSedyx Feb 14, 2024

NikitaSedyx Feb 13, 2024

alisher-epam Feb 13, 2024

NikitaSedyx Feb 14, 2024

alisher-epam Feb 14, 2024

alisher-epam Feb 14, 2024

NikitaSedyx Feb 14, 2024

alisher-epam Feb 14, 2024

NikitaSedyx left a comment

NikitaSedyx Feb 14, 2024

alisher-epam commented Feb 14, 2024

NikitaSedyx Feb 14, 2024

NikitaSedyx commented Feb 14, 2024

sonarqubecloud bot commented Feb 14, 2024


		import { useExchangeCalculation } from '../../hooks';

		export const DisplayExchangedAmount = ({ currency, exchangeRate, total }) => {

UINV-522: display the exchanged calculated total amount when invoice in foreign currency #757

UINV-522: display the exchanged calculated total amount when invoice in foreign currency #757

Conversation

alisher-epam commented Feb 5, 2024 • edited Loading

Purpose

Approach

Screenshots

Pre-Merge Checklist

github-actions bot commented Feb 5, 2024 • edited Loading

Jest Unit Test Statistics

github-actions bot commented Feb 5, 2024 • edited Loading

BigTest Unit Test Statistics

alisher-epam commented Feb 6, 2024

usavkov-epam left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alisher-epam commented Feb 9, 2024

alisher-epam commented Feb 9, 2024

usavkov-epam left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alisher-epam commented Feb 12, 2024

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

NikitaSedyx left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alisher-epam commented Feb 14, 2024

Choose a reason for hiding this comment

NikitaSedyx commented Feb 14, 2024

sonarqubecloud bot commented Feb 14, 2024

Quality Gate passed

alisher-epam commented Feb 5, 2024 •

edited

Loading

github-actions bot commented Feb 5, 2024 •

edited

Loading

github-actions bot commented Feb 5, 2024 •

edited

Loading

usavkov-epam left a comment •

edited

Loading