-
Notifications
You must be signed in to change notification settings - Fork 81
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
fix: fixed typo in updateContentTaxonomyTags URL [FC-0036] #815
fix: fixed typo in updateContentTaxonomyTags URL [FC-0036] #815
Conversation
Thanks for the pull request, @pomegranited! Please note that it may take us up to several weeks or months to complete a review and merge your PR. Feel free to add as much of the following information to the ticket as you can:
All technical communication about the code itself will be done via the GitHub pull request interface. As a reminder, our process documentation is here. Please let us know once your PR is ready for our review and all tests are green. |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #815 +/- ##
=======================================
Coverage 90.58% 90.58%
=======================================
Files 511 511
Lines 8879 8879
Branches 1912 1912
=======================================
Hits 8043 8043
Misses 811 811
Partials 25 25 ☔ View full report in Codecov by Sentry. |
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 @pomegranited! 👍
- I tested this using my local tutor stack, following the testing instructions
- I read through the code
-
I checked for accessibility issues - Includes documentation
I made a minor suggestion, but if @xitij2000 manages to review this today, I think we are fine without it.
src/content-tags-drawer/data/api.js
Outdated
let url = getContentTaxonomyTagsApiUrl(contentId); | ||
url = `${url}&taxonomy=${taxonomyId}`; | ||
url = `${url}?taxonomy=${taxonomyId}`; | ||
const { data } = await getAuthenticatedHttpClient().put(url, { tags }); |
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.
That way we avoid dealing with direct string manipulation for the URL (and it can handle more complicated things, like arrays in the query string)
const url = getContentTaxonomyTagsApiUrl(contentId);
const params = { taxonomy: taxonomyId };
const { data } = await getAuthenticatedHttpClient().put(url, { tags }, { params });
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.
Ah cool, that's nicer.. fixed 6821ebb
@xitij2000 could you merge this when you're happy with it? |
@xitij2000 I don't think the CI error is related to this change? But please let me know if there's something I need to do to resolve it. |
I'm rerunning the tests and will merge when they pass. |
@pomegranited 🎉 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
#787 introduced a bug in the URL used to update content tags, resulting in 404 errors from the backend.
This PR fixes that bug and the related tests.
Testing instructions
Setup:
Testing Tag Drawer:
Internal-ref: FAL-3577