Rename taxonomy tags#2939
Conversation
openedx-webhooks
added
the
open-source-contribution
|
Thanks for the pull request, @jesperhodge! 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. |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #2939 +/- ##
========================================
Coverage 95.56% 95.57%
========================================
Files 1349 1347 -2
Lines 31159 31247 +88
Branches 6833 7096 +263
========================================
+ Hits 29777 29863 +86
+ Misses 1332 1322 -10
- Partials 50 62 +12 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
I'll add the missing tests for coverage. |
| .map((cell, index) => { | ||
| const content = flexRender(cell.column.columnDef.cell, cell.getContext()); | ||
| const isFirstColumn = index === 0; | ||
| {editingRowId === `${row.original.id}:${String(row.original.value)}` ? ( |
There was a problem hiding this comment.
Review comment copied over from older review:
(Reviewer: @mgwozdz-unicon) I think there should be a unit test to check both these cases
|
I'll update the tests & make sure coverage is sufficient. |
kdmccormick
added
the
create-sandbox
|
I'm on this PR's sandbox and I don't see any way to create or edit tags. Am I missing a change, or maybe a permission? This is using the admin user (UN/PW=openedx/openedx). 
|
|
@jesperhodge mind rebasing onto master now that the parent PR is merged? |
After it's merged, yes! (still waiting. #2872) |
|
Ping me once this is ready for review :) |
jesperhodge
force-pushed
the
jhodge/edit-delete-tags
branch
from
d439387 to
eaa833e
Compare
|
The annoying text selection bug is making renames not work properly. As soon as I type a single letter, it selects the whole value, and the second letter I type then erases everything. Could you please add a fix to this PR or open a separate PR with the fix? Screen.Recording.2026-03-30.at.2.56.29.PM.movIt may not be clear, but I put my cursor after "Cognitive" then press "/" and right after I do that it selects the whole value for some reason, causing my next keystroke to erase it all. |
|
If I rename a tag to give it a name that already exists elsewhere in the table, the whole UI goes crazy. It seems like our API is incorrectly returning an HTML result in this case :/ So this is likely something we should fix on the backend too. 
|
| {intl.formatMessage(messages.addSubtag)} | ||
| </Dropdown.Item> | ||
| <Dropdown.Item | ||
| as={Button} |
There was a problem hiding this comment.
| as={Button} |
Is there a reason for the as=Button here, and on the one above? It's causing the menu items to be centered, which looks bad and is inconsistent with all the other dropdown menus in the UI.
| This | Other |
|---|---|
 |
 |
| onClick={editTag} | ||
| disabled={disableEditTag} | ||
| > | ||
| {intl.formatMessage(messages.renameTag)} |
There was a problem hiding this comment.
| {intl.formatMessage(messages.renameTag)} | |
| <FormattedMessage {...messages.renameTag} /> |
Nit: it's not at all a big deal, but the react-intl docs recommend using declarative over imperative whenever possible. (Basically, only use intl.format...() when you need to set an HTML attribute like aria-label or a string-only prop.
Same thing on the "Add Subtag" entry above.
| onStartDraft(); | ||
| setDraftError(''); | ||
| setEditingRowId(`${rowData.id}:${rowData.value}`); | ||
| setCreatingParentId(null); | ||
| setIsCreatingTopTag(false); | ||
| setActiveActionMenuRowId(null); |
There was a problem hiding this comment.
Nit: this seems a little fragile that you have to remember to change all these separate states when starting an edit action. Since you implemented VIEW and DRAFT modes with transition states, I would expect that "editing a tag" involves transitioning from (any state) to DRAFT, and that the state machine would take care of all these detailed things for us as part of that transition.
However, instead of that, I would strongly recommend that you create a TagEditorContext context that is shared among all these components, and give it a type such that it cannot have both editingRowId and creatingParentId set at the same time.
export interface TagEditorContext {
activeEditType:
| { rename: { id: number, value: string } }
| { createTag: { parentTagId: number | null } }
}That way, Typescript enforces for you that only one edit type is possible at any given time. And you don't have to pass things like setCreatingParentId around so much (it shows up 26 times in the code) - you can just pull it from the context wherever needed.
| act(() => { | ||
| if (expandButton) { | ||
| fireEvent.click(expandButton); | ||
| } | ||
| }); |
There was a problem hiding this comment.
| act(() => { | |
| if (expandButton) { | |
| fireEvent.click(expandButton); | |
| } | |
| }); | |
| if (expandButton) { | |
| fireEvent.click(expandButton); | |
| } |
You do not need to call act when using RTL APIs like fireEvent, because RTL APIs call act for you internally.
Also, nit: using user-event is preferred over using fireEvent directly. Like this:
No need to change existing tests, but please prefer userEvent in new tests that you write. You should also be able to ask Claude/Gemini/ChatGPT to refactor it and it should do it very quickly and easily.
| const openActionsMenuForTag = (tagName, actionButtonName = /actions/i) => { | ||
| if (!screen.queryAllByText(tagName)?.length) { | ||
| // expand all | ||
| const expandButton = screen.queryAllByText('Expand All')?.[0]; |
There was a problem hiding this comment.
| const expandButton = screen.queryAllByText('Expand All')?.[0]; | |
| const expandButton = screen.queryByRole('button', { name: 'Expand All' }); |
If you're specifically selecting a button in your test code, please use the *ByRole queries. It's more efficient, more accurate, and less likely to break if something else changes elsewhere on the page (e.g. a non-button thing is added that says "Click on 'Expand All' to see more" - that would break your test if it came before the button in the DOM).
Also using queryAll...(...)?.[0] won't throw an error if more than one element matches your query, whereas queryByRole will, while it also handles zero matches correctly (no error). So using queryAll...(...)?.[0] is bad, because e.g. if someone adds another button earlier in the DOM, it would make this test start failing and be harder to debug without a clear error explaining that there are multiple matches.
| fireEvent.click(screen.getByText('Rename')); | ||
| const input = screen.getByRole('textbox'); |
There was a problem hiding this comment.
| fireEvent.click(screen.getByText('Rename')); | |
| const input = screen.getByRole('textbox'); | |
| fireEvent.click(screen.getByRole('button', { name: 'Rename' })); | |
| const input = screen.getByRole('textbox', { name: /type tag name/i }); |
Selectors like this ensure that we're using the correct accessible roles and labels on our UI elements. In particular, we want to be very sure that the textbox is labelled ("type tag name") for non-visual users, so writing your tests this way ensures that.
I think you may need to prompt your copilot to follow the RTL guiding principles and query priority recommendations because it doesn't seem to be aware of them.
| </td> | ||
| ))} | ||
| </tr> | ||
| {editingRowId === `${row.original.id}:${String(row.original.value)}` ? ( |
There was a problem hiding this comment.
Nit: I'd prefer we use a proper data structure to hold the ID and value of the row we're editing rather than this colon-separated string format.
| @@ -90,6 +90,7 @@ export const apiUrls = { | |||
| /** URL to plan (preview what would happen) a taxonomy import */ | |||
There was a problem hiding this comment.
There's plenty of horizontal space here. Could we make the text input wider, so that it's more often able to show the whole tag during editing?
Fine to ticket up for a followup PR if you'd rather do that.
View:
Edit:
There was a problem hiding this comment.
Yes! Good catch.
Added it to openedx/modular-learning#256
@bradenmacdonald yup already got a subtask for it openedx/modular-learning#261 |
|
Hi @bradenmacdonald ! Thanks for the review. Here's what I did: Because of our strong time constraints, I left anything that's a nit. I would have liked to look into some of these but I just have to prioritize some other features. Are any of those nits particularly important to you? Which ones? I'm hoping we can make a low-prio ticket so hopefully I can take care of it when I finally have a moment of slack. What do you think? |
Yes, I would really appreciate if you can follow up on that soon. I wish we'd started with that pattern, which is what we're using for all the other new features in this codebase. |
Noted for the future! |
|
UX nit for future follow-up: if you have a tag rename active, then the "Add subtags" and "Rename tag" actions on other tags are disabled. If the rename is off-screen, this may be confusing. I would expect that those options are still available, and selecting either one just cancels the current rename attempt. 
|
Hi @bradenmacdonald , I hear you. It would have indeed been good. For the future: what's the best way to get feedback like that from you in time to do larger refactorings like introducing a context? Should I keep asking for early-stage feedback on draft PRs like I did for create-tags, or is there a better way? |
bradenmacdonald
left a comment
bradenmacdonald
left a comment
There was a problem hiding this comment.
OK, just a couple super minor things and then I'll merge this.
Yes, I guess that's really the only option. But also you'll get more familiar with the patterns we use over time, like React Context, React Query, and React Testing Library. None of those are specific to Open edX, so there's also a lot of general purpose documentation and knowledge out there to learn from. e.g. the React docs are great, if you haven't read them: https://react.dev/learn/passing-data-deeply-with-context |
|
I posted the question in slack, but for transparency just copying it here: Before I hand the "Rename tags" PR back into review, I just wanted to check: |
|
Thanks @bradenmacdonald ! I fixed the two outstanding things you mentioned. |
bradenmacdonald
left a comment
bradenmacdonald
left a comment
There was a problem hiding this comment.
One last thing, I think you left this in accidentally.
|
Links to issue: openedx/modular-learning#131 |
5 participants
Description
Implementing openedx/modular-learning#259 which is part of openedx/modular-learning#131.
Acceptance Criteria are outlined in the ticket.
This is pointing to branch
temp--tag-table-baseso the PR can be reviewed and the diff seen. Logically it depends on #2872 and includes that branch's changes, so we should wait with merge until 2872 is merged and then change the target branch to master.Use of AI Tools
I used ChatGPT via Github Copilot to support this PR. Generally all submitted code has been reviewed by me as human as well. ChatGPT has mainly helped writing tests, where:
Testing instructions Step 1
Testing instructions Step 2
Test against the ACs in the ticket.