-
Notifications
You must be signed in to change notification settings - Fork 80
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
[FC-0059] refactor: convert files to ts and improve typings/tests #1181
[FC-0059] refactor: convert files to ts and improve typings/tests #1181
Conversation
Thanks for the pull request, @rpenido! What's next?Please work through the following steps to get your changes 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. 🔘 Let us know that your PR is ready for review:Who will review my changes?This repository is currently maintained by Where 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. |
f1a33ba
to
bdd6c3b
Compare
@@ -142,7 +142,7 @@ const CardHeader = ({ | |||
{(isVertical || isSequential) && ( | |||
<CardStatus status={status} showDiscussionsEnabledBadge={showDiscussionsEnabledBadge} /> | |||
)} | |||
{ getConfig().ENABLE_TAGGING_TAXONOMY_PAGES === 'true' && contentTagCount > 0 && ( | |||
{ getConfig().ENABLE_TAGGING_TAXONOMY_PAGES === 'true' && !!contentTagCount && ( |
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.
contentTag
can be undefined if the api call throws an error
bdd6c3b
to
9c93799
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #1181 +/- ##
==========================================
+ Coverage 92.82% 92.89% +0.06%
==========================================
Files 747 750 +3
Lines 13372 13474 +102
Branches 2856 2929 +73
==========================================
+ Hits 12413 12517 +104
+ Misses 924 922 -2
Partials 35 35 ☔ View full report in Codecov by Sentry. |
import { useContentTagsCount } from './apiHooks'; | ||
|
||
jest.mock('@tanstack/react-query', () => ({ | ||
useQuery: 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.
We were mocking the hook here, not testing much.
|
||
it('should return success response', async () => { | ||
const courseId = 'course-v1:edX+TestX+Test_Course'; | ||
axiosMock.onGet(getTagsCountApiUrl(courseId)).reply(200, { [courseId]: 10 }); |
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.
Testing mocking axios now (testing the hook and api function)
0d566f5
to
306aff2
Compare
306aff2
to
b604b6f
Compare
*/ | ||
export async function getCourseRerun(courseId) { | ||
export async function getCourseRerun(courseId: string): Promise<unknown> { |
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.
For now, we are using unknown
. If/when we migrate the code that uses this, we could create a type for the API return.
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.
👍 @rpenido LGTM, great work! Small nit/question.
- I tested this: I followed testing instructions and read through the tests
- I read through the code
-
I checked for accessibility issues -
Includes documentation
Hi @bradenmacdonald! Could you review this? |
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.
Nice, thanks!
@rpenido 🎉 Your pull request was merged! Please take a moment to answer a two question survey so we can improve your experience in the future. |
Description
This PR is a minor refactor to convert some files to typescript and improve the tests related to the
useContentTagsCount
hook.Testing instructions
Private ref: FAL-3769