fix: update app type declaration#133
Conversation
|
Thanks for the pull request, @dcoa! This repository is currently maintained by Once you've gone through the following steps feel free to tag them in a comment and let them know that your changes are ready for engineering review. 🔘 Get product approvalIf you haven't already, check this list to see if your contribution needs to go through the product review process.
🔘 Provide contextTo help your reviewers and other members of the community understand the purpose and larger context of your changes, feel free to add as much of the following information to the PR description as you can:
🔘 Get a green buildIf one or more checks are failing, continue working on your changes until this is no longer the case and your build turns green. DetailsWhere can I find more information?If you'd like to get more details on all aspects of the review process for open source pull requests (OSPRs), check out the following resources: When can I expect my changes to be merged?Our goal is to get community contributions seen and reviewed as efficiently as possible. However, the amount of time that it takes to review and merge a PR can vary significantly based on factors such as:
💡 As a result it may take up to several weeks or months to complete a review and merge your PR. |
openedx-webhooks
added
open-source-contribution
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #133 +/- ##
==========================================
- Coverage 97.64% 97.50% -0.14%
==========================================
Files 68 66 -2
Lines 1572 1525 -47
Branches 396 388 -8
==========================================
- Hits 1535 1487 -48
- Misses 37 38 +1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
dcoa
requested review from
arbrandes,
bra-i-am,
jacobo-dominguez-wgu and
jesusbalderramawgu
|
I'm probably not going to get to this before the conference, but I'll do my best! |
| description: string; | ||
| perms: ExtendedRolePermission[]; | ||
| } | ||
| export type PermissionItem = ResourceMetadata & { |
There was a problem hiding this comment.
Taking into account that src/authz-module/components/RenderPermissionInLine.tsx (sibling of this file) also requires this type, it makes sense to me that this type lives in another file, e.g., UserPermissions or directly in @src/types
| import { buildPermissionMatrixByResource } from './utils'; | ||
|
|
||
| const intl = { formatMessage: jest.fn((msg: any) => msg) }; | ||
| const intl = { formatMessage: jest.fn((msg: any) => msg) } as unknown as IntlShape; |
There was a problem hiding this comment.
We are not using the mocked formatMessage, so I think we can create the intl directly from the base to avoid the as unknown as
| const intl = { formatMessage: jest.fn((msg: any) => msg) } as unknown as IntlShape; | |
| const intl = createIntl({ locale: 'en', messages: {} }); |
| // Lazy so each test file's own `jest.mock('@edx/frontend-platform/auth', ...)` | ||
| // (which is hoisted above this setup file's imports) is in effect by the time | ||
| // callers do `mockHttpClient().mockReturnValue(...)`. | ||
| export const mockHttpClient = (): jest.Mock => { | ||
| // eslint-disable-next-line global-require | ||
| const { getAuthenticatedHttpClient } = require('@edx/frontend-platform/auth'); | ||
| return getAuthenticatedHttpClient as jest.Mock; | ||
| }; | ||
|
|
There was a problem hiding this comment.
I don't share the require() + eslint-disable; I found this and worked in my local environment, what do you think?
export const mockHttpClient = (): jest.Mock => jest.requireMock('@edx/frontend-platform/auth').getAuthenticatedHttpClient;| LANGUAGE_PREFERENCE_COOKIE_NAME: 'openedx-language-preference', | ||
| ...process.env, | ||
| }, | ||
| } as unknown as ConfigDocument, |
There was a problem hiding this comment.
I think the ' mockAppContext ' here can be removed in favor of:
import { mockAppContext } from '@src/setupTest';5 participants
Summary
Tighten TypeScript coverage across the authz module and enable type validation in CI.
CI
npm run types(tsc --noEmit) step between Lint and Testtsconfig
ignoreDeprecations: "6.0"to quiet TS 6.0 deprecation warnings on inherited options, This is temporal and mostly serves as thetsconfig.jsonfile forcompilerOptions.baseUrl+ wildcard*path, kept@src/*alias@arbrandes could you please help me to review the chore changes? I want to make sure the
tsconfigis still correct and following the best practices. Thank you : )Other changes
This was a risk identified during the PRs review for the module refactor.
Please, @bra-i-am , @jesusbalderramawgu and @jacobo-dominguez-wgu review carefully that all is still working as expected, and if you find out another duplication fell free to open an issue or PR so we clean up the code together ;)
Assumptions
RoleCard is not needed as the new UI only displays the Permissions Table
Test plan
npm run typespassesnpm run lintpassesnpm run testpassesTypesstepLLM usage notice
Built with assistance from Claude.