-
Notifications
You must be signed in to change notification settings - Fork 3
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
UINV-557: Improve error message when attempting to save an invoice line when no budget for the fiscal year exists #809
Conversation
…ne when no budget for the fiscal year exists
@usavkov-epam @folio-org/fe-tl-reviewers please review |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The PR has quality gates issue: https://sonarcloud.io/project/issues?sinceLeakPeriod=true&issueStatuses=OPEN%2CCONFIRMED&pullRequest=809&id=org.folio%3Aui-invoice&open=AZIzCX26_UrPZE2OH8d2
This code is becoming increasingly bulky and challenging to maintain. In my view, the logic should be moved out to separate "resolver" functions that will handle errors for a specific error code, and then the switch-case construction will map the code to a specific resolver.
E.g.
case ERROR_CODES.budgetNotFoundByFundIdAndFiscalYearId: {
await handleSomeError(<data>);
break;
}
@@ -126,6 +126,7 @@ describe('InvoiceForm component', () => { | |||
const accountLabel = `${accountNo} (${appSystemNo})`; | |||
|
|||
expect(container.querySelector('#selected-accounting-code-selection-item')).toHaveTextContent(''); | |||
await userEvent.click(screen.getAllByText('stripes-components.selection.controlLabel')[4]); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there another way to bind to an element other than the index in an array of elements of the same type? This is a fairly brittle method that can break tests when new elements are added.
Applicable for all places in this file
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And this not resolved as well. Please use specific anchors for selections like screen.getByRole('button', { name: <name> })
src/invoices/InvoicesList/InvoicesListFilters/FiscalYearFilter/FiscalYearFilter.test.js
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Except the comment
@@ -179,6 +139,7 @@ export const handleInvoiceLineErrors = async ({ | |||
requestData = [], | |||
responses = [], | |||
showCallout, | |||
ky = noop, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
noop.get()
-> TypeError: noop.get is not a function
. ky
it's an object, not the function
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
updated
Quality Gate passedIssues Measures |
Purpose
UINV-557: Improve error message when attempting to save an invoice line when no budget for the fiscal year exists
Approach
Screen.Recording.2024-09-26.at.20.36.08.mp4
Screenshots
Pre-Merge Checklist
Before merging this PR, please go through the following list and take appropriate actions.
If there are breaking changes, please STOP and consider the following:
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.