-
Notifications
You must be signed in to change notification settings - Fork 17
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
feat: populates datatable with subsidies #343
Conversation
08207f3
to
4666299
Compare
* fix: PR updates * fix: updates
6ce74bf
to
dabf3ed
Compare
5995787
to
f140065
Compare
@@ -29,10 +29,10 @@ | |||
"url": "https://github.com/openedx/frontend-app-support-tools/issues" | |||
}, | |||
"dependencies": { | |||
"@edx/brand": "npm:@edx/brand-edx.org@^1.4.2", | |||
"@edx/brand": "npm:@edx/brand-[email protected]", |
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 to handle "~@edx/brand/paragon/overrides"
import for datatable styles
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.
[inform] Additional context: because this is an open-source repo in the openedx
Github organization, the brand-openedx
theme should be the default theme installed. Should a consumer want to use the brand-edx.org
theme for an MFE, that should be configured outside of committed code. For 2U/edX, the @edx/brand
package may be overridden at build+deploy time via configuration (example).
"@edx/frontend-enterprise-utils": "^3.0.0", | ||
"@edx/frontend-platform": "^4.2.0", | ||
"@edx/paragon": "^20.26.0", | ||
"@edx/paragon": "20.45.0", |
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.
f140065
to
3e0f0b8
Compare
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## master #343 +/- ##
==========================================
+ Coverage 87.50% 87.72% +0.21%
==========================================
Files 138 140 +2
Lines 2850 2948 +98
Branches 682 735 +53
==========================================
+ Hits 2494 2586 +92
- Misses 355 361 +6
Partials 1 1
☔ View full report in Codecov by Sentry. |
@@ -1,24 +0,0 @@ | |||
import { screen } from '@testing-library/react'; |
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.
Removed because of duplicate file
26cc377
to
e57189d
Compare
69f5101
to
93e90ab
Compare
93e90ab
to
38b040b
Compare
@@ -29,10 +29,10 @@ | |||
"url": "https://github.com/openedx/frontend-app-support-tools/issues" | |||
}, | |||
"dependencies": { | |||
"@edx/brand": "npm:@edx/brand-edx.org@^1.4.2", | |||
"@edx/brand": "npm:@edx/brand-[email protected]", |
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.
[inform] Additional context: because this is an open-source repo in the openedx
Github organization, the brand-openedx
theme should be the default theme installed. Should a consumer want to use the brand-edx.org
theme for an MFE, that should be configured outside of committed code. For 2U/edX, the @edx/brand
package may be overridden at build+deploy time via configuration (example).
@@ -42,7 +42,6 @@ | |||
"classnames": "2.2.6", | |||
"lodash.debounce": "4.0.8", | |||
"moment": "2.29.1", | |||
"newrelic": "5.13.1", |
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.
[inform] Context: newrelic
is installed, but seemingly unused by this repo. All New Relic related things for MFEs currently comes from @edx/frontend-build
and @edx/frontend-platform
, without consumers needing to install newrelic
themselves.
Having newrelic
in package.json causes issues such as:
❯ npm i
npm WARN EBADENGINE Unsupported engine {
npm WARN EBADENGINE package: '[email protected]',
npm WARN EBADENGINE required: { node: '>=6.0.0 <13.0.0', npm: '>=3.0.0' },
npm WARN EBADENGINE current: { node: 'v18.16.0', npm: '9.5.1' }
npm WARN EBADENGINE }
npm WARN deprecated [email protected]: This version of the New Relic Node Agent has reached the end of life.
src/Configuration/Provisioning/DashboardDatatable/DashboardDatatable.jsx
Outdated
Show resolved
Hide resolved
src/Configuration/Provisioning/DashboardDatatable/DashboardTableActions.jsx
Outdated
Show resolved
Hide resolved
src/Configuration/Provisioning/ProvisioningForm/ProvisioningFormCustomer.jsx
Outdated
Show resolved
Hide resolved
src/Configuration/Provisioning/ProvisioningForm/ProvisioningFormCustomer.jsx
Outdated
Show resolved
Hide resolved
761f446
to
ebc8e12
Compare
ebc8e12
to
db1fb40
Compare
src/Configuration/Provisioning/DashboardDataTable/DashboardDataTable.jsx
Outdated
Show resolved
Hide resolved
src/Configuration/Provisioning/DashboardDataTable/DashboardDataTable.jsx
Outdated
Show resolved
Hide resolved
Header: 'Start date', | ||
accessor: 'activeDatetime', | ||
disableFilters: true, | ||
Cell: ({ row }) => transformDatatableDate(row.values.activeDatetime), |
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.
nit: just a caution that if/when these functions were to return JSX, this line and line 94 below would be going against the no-unstable-nested-components
ESLint rule. Not suggesting you need to change anything here, but just wanted to inform.
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.
Good point, will leave open for reference 👍🏽
Header: 'Start date', | ||
accessor: 'activeDatetime', | ||
disableFilters: true, | ||
Cell: ({ row }) => transformDatatableDate(row.values.activeDatetime), |
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.
nitty nit: transformDataTableDate
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.
Same as above, 👍🏽
src/Configuration/Provisioning/DashboardDataTable/DashboardTableActions.jsx
Outdated
Show resolved
Hide resolved
src/Configuration/Provisioning/DashboardDataTable/tests/DashboardTableActions.test.jsx
Outdated
Show resolved
Hide resolved
src/Configuration/Provisioning/DashboardDataTable/tests/DashboardTableActions.test.jsx
Outdated
Show resolved
Hide resolved
44754e1
to
4558167
Compare
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.
LGTM. 2 additional, non-blocking little nits but
const { hydrateEnterpriseSubsidies } = useDashboardContext(); | ||
const [isLoading, setIsLoading] = useState(true); | ||
|
||
// Implementation due to filterText value displaying accessor value customerName as opposed to Customer Name |
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.
[clarification] Is this comment dangling from the now-moved FilterStatus
above?
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.
lol yeah it is my bad
|
||
const { HOME } = ROUTES.CONFIGURATION.SUB_DIRECTORY.PROVISIONING; | ||
|
||
const useHistoryPush = jest.fn(); |
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.
nit: mockHistoryPush
might be a slightly more descriptive variable name
Populates datatable with data from the subsidy endpoint
Also sorts and filters leveraging manualSort/manualFilter
Revamps validation for message and regex logic to handle '00k' prefix for the opportunity product.
Pending updated tests