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. |
jesperhodge
changed the title
|
Dear reviewers, |
|
Thanks, I'm so happy to see this coming together!
Any thoughts on how to handle this? Would it be useful to use infinite scroll instead of pagination, and load sub-tags on demand when their parent tag is expanded?
This doesn't seem to be working. When I add a new top-level tag, it always appears at the top of the list, but when I later refresh, it moves to alphabetical order. I think it should immediately put the tag into the correct position and then "flash" it to highlight where it is in the list. Bugs:
Here's a bunch of UX feedback. I know you're probably aware of many of these already, and they don't have to be fixed within this PR necessarily, but it's easier for me to just list them all.
|
bradenmacdonald
left a comment
bradenmacdonald
left a comment
There was a problem hiding this comment.
Don't have time for a full review now, but here's some initial thoughts.
| // The table has a VIEW, DRAFT, and a PREVIEW mode. It starts in VIEW mode. | ||
| // It switches to DRAFT mode when a user edits or creates a tag. | ||
| // It switches to PREVIEW mode after saving changes, and only switches to VIEW when | ||
| // the user refreshes the page, orders a column, or navigates to a different page. | ||
| // During DRAFT and PREVIEW mode the table makes POST requests and receives | ||
| // success or failure responses. | ||
| // However, the table does not refresh to show the updated data from the backend. | ||
| // This allows us to show the newly created or updated tag in the same place without reordering. |
There was a problem hiding this comment.
I wonder if we can take a simpler approach. What do you think about having just one "mode", and using optimistic updates to inject any newly-created tags into the correct spot? That way, if/when react-query refetches data from the backend, nothing gets disrupted, and we can keep everything in sync.
(plus a toggle to track whether the user is currently creating a new tag or not)
There was a problem hiding this comment.
The modes are mostly necessary to prevent reloading data and alphabetical reloading after the user has successfully saved a new tag, so that new tags and subtags are shown at the top. See my larger comment about this criteria. In terms of optimistic updates, which are there to inject updates even before they have been saved, that would prevent tags to show up in the spot we want, and it doesn't align with our AC, which is to show error messages when the tag does not successfully save and not display the tag optimistically hoping for successful save.
There was a problem hiding this comment.
Can I resolve this conversation?
There was a problem hiding this comment.
I still think the modes are adding a lot of unnecessary complexity. Couldn't we achieve the same thing without these modes, and a much simpler isCurrentlyEditingTag and lastCreatedTag state that correspond to the "draft mode" and "preview mode" respectively?
If you track those two states, then it doesn't matter if/when React query reloads data, and you don't have to have special case behavior or state transitions, or anything else. You just make sure that if there is currently a lastCreatedTag (you'd track both the tag and its parent ID in the state), then it gets "hidden" from its normal place and rendered directly below its parent. I think that would achieve all the AC you have.
You could also do it with optimistic updates too; just because you're using optimistic updates doesn't mean you can't insert the new tag into the desired place (first on the list) or handle errors when they eventually come through. But in that case, it would be more work to avoid the eventual shift into alphabetical order whenever React query does refresh the list, which is trivially avoided by the state approach I suggested above.
I know you're short on time here so I won't block the PR on this but I do think it should be at least marked as a TODO to explore, as a way of simplifying this.
|
@bradenmacdonald thanks for the very helpful feedback! Pagination:
Tags ordering / Preview:
I implemented that as we decided for the ACs. The current way it works is indeed that it always appears at the top of the list, but on refresh moves to alphabetical order. Some of the reasons we landed on these ACs were:
I brought your recommendation about positioning and flashing to the team, and we are going to raise this concern to the product owner, Jenna, to get her input. Bugs:
UX:
Let me know if you have any questions! Your feedback is very valuable to us. |
src/taxonomy/data/apiHooks.ts
Outdated
| queryKey: taxonomyQueryKeys.taxonomyTagListPage(taxonomyId, pageIndex, pageSize), | ||
| queryFn: async () => { | ||
| const { data } = await getAuthenticatedHttpClient().get(apiUrls.tagList(taxonomyId, pageIndex, pageSize)); | ||
| const { data } = await getAuthenticatedHttpClient().get(apiUrls.tagList(taxonomyId, pageIndex, pageSize, 1000)); |
There was a problem hiding this comment.
What is the "1000" here? Do we have a place to make this a constant somewhere?
There was a problem hiding this comment.
It's pretty arbitrary, it's the recommendation in the API docs if you want to request all depths (since there's no "infinite"). I'll just add a comment that this fetches full depth if that's fine.
There was a problem hiding this comment.
@jesperhodge I agree that we may want to make this a constant in each of the places it's used.
src/taxonomy/tag-list/hooks.ts
Outdated
| return 'Name is required'; | ||
| } | ||
| if (!TAG_NAME_PATTERN.test(trimmed)) { | ||
| return 'Invalid character in tag name'; |
There was a problem hiding this comment.
Should this return a guide to fix the issue? e.g. "Invalid character in tag name, allowed characters are uppercase letters, lowercase letters, and numbers" or something to that effect? If we have a guide somewhere else on that page aside from this that's fine.
There was a problem hiding this comment.
Good idea, I'll just make that part of the error message. (mentioning forbidden characters ">", ";", and "\tab")
There was a problem hiding this comment.
It looks like the Acceptance Criteria in the ticket just specify that the message indicates that an invalid character was used. Maybe this can be added to the list of things for consideration for fast follow if the language updates are approved through the BA process. cc @thelmick-unicon
There was a problem hiding this comment.
@jesperhodge Sorry, I hadn't refreshed. If you want to go ahead and make the change, it's probably fine, but please indicate in the ticket what the official language ended up being for this.
There was a problem hiding this comment.
This can be on the list of fast follows.
- Can I resolve this conversation?
There was a problem hiding this comment.
Can you link to the issue where this TODO is being tracked? Then feel free to mark as resolved.
| private validateNoDuplicateValues(items: TagData[]) { | ||
| const seenValues = new Set<string>(); | ||
| for (const item of items) { | ||
| if (seenValues.has(item.value)) { |
There was a problem hiding this comment.
Does we want this to be case insensitive? So "tuba" would be a duplicate of "Tuba" and "TuBa"?
There was a problem hiding this comment.
No, this should be case sensitive, because:
- I don't know if the backend allows duplicates if they have different cases.
- This function is for error handling when building the tag tree, to tell us what happened, not for input validation.
- The input validation lives in
tag-list/hooks.tsand does not check for duplication at all, because we let the backend handle this when we hit "Save".
There was a problem hiding this comment.
Can we resolve this conversation?
There was a problem hiding this comment.
The backend does not allow duplicates that have different cases.
There was a problem hiding this comment.
This function is for error handling when building the tag tree, to tell us what happened, not for input validation.
Do you mean it's like a debug assertion? Or under what circumstances would this be necessary? The database enforces that tags are case-insensitively unique so it's really not likely to happen.
| > | ||
| <Layout.Element> | ||
| <TagListTable taxonomyId={taxonomyId} /> | ||
| <TagListTable taxonomyId={taxonomyId} maxDepth={3} /> |
There was a problem hiding this comment.
Is there a place to move this to a configuration so that if we want to change the depth it doesn't require a code change?
There was a problem hiding this comment.
Yes, I'll move that to constants
There was a problem hiding this comment.
Done, can we resolve this conversation?
If we implement "flashing", we could also implement "change to the correct page".
It's fine with me if you add the option to paginate by 0th-level tags, but I think it may need to be optional, in order to preserve the API compatibility. The current API allows you to quickly load the entire taxonomy into memory by requesting the full depth and as many pages as you need until it's complete, and I think that's another option to consider here unless we think there will be taxonomies too large to performantly display in a react-table. |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #2872 +/- ##
========================================
Coverage 95.51% 95.51%
========================================
Files 1329 1346 +17
Lines 30557 31049 +492
Branches 6925 6831 -94
========================================
+ Hits 29186 29657 +471
- Misses 1303 1335 +32
+ Partials 68 57 -11 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Hi @bradenmacdonald , for now our current designs and acceptance criteria have been aligned on:
|
|
Issue: |
Fixed, but please note that there is a backend bug that makes it so when you create a great-grandchild subtag and refresh, it disappears. That is out of scope and subject of openedx/modular-learning#257. |
mgwozdz-unicon
left a comment
mgwozdz-unicon
left a comment
There was a problem hiding this comment.
Thank you for all your hard work on this @jesperhodge ! It looks good!
I replied there, but I'll mention it here too: it's not a bug; the API just allows you to load 3 levels at any one time, but you can still load children of depth 4 or more as long as you don't try loading them from the root. If you just call "get children" on the level 3 tags, you'll get the level 4 tags, and so on. The initial API response also tells you if any of the level 3 tags have children that weren't included in the response due to this limitation. |
bradenmacdonald
left a comment
bradenmacdonald
left a comment
There was a problem hiding this comment.
Partial review; I haven't quite gone through everything yet. I don't have any major concerns, and I'm excited to get this merged.
Two things stood out to me though: (1) as I mentioned, I am not convinced we need the "modes" you've implemented here, although I won't consider it a blocker, and (2) I think you'd be better off with a TagEditorContext or TagTreeContext that provides some shared data/state to all the components here instead of passing so many variables around as props. But again I won't consider that blocking.
Nothing with a "nit:" is a blocker either.
| tagList: (taxonomyId: number, pageIndex: number, pageSize: number) => makeUrl(`${taxonomyId}/tags/`, { | ||
| page: (pageIndex + 1), page_size: pageSize, | ||
| }), | ||
| tagList: (taxonomyId: number, pageIndex: number | null, pageSize: number | null, fullDepthThreshold?: number) => { |
There was a problem hiding this comment.
Nit: calls like tagList(taxonomyId, null, 15, 1000) are quite vague and add cognitive load to understand. Please consider an API shape like tagList(taxonomyId, {pageIndex: 0, pageSize: 15, fullDepth: true}) which is much easier to read.
| importPlan: (taxonomyId: number, fileId: string) => [...taxonomyQueryKeys.all, 'importPlan', taxonomyId, fileId], | ||
| } satisfies Record<string, (string | number)[] | ((...args: any[]) => (string | number)[])>; | ||
|
|
||
| const getApiErrorMessage = (err: unknown): string => { |
There was a problem hiding this comment.
Nit: if this isn't a hook, it would be better in api.ts instead of apiHooks.ts. And if it's in fact a general method for getting an API error that works with any DRF response, it could go into a more global API/utils file somewhere. Nothing in this function looks specific to tagging. Claude mentioned that it may not properly handle DRF's non_field_errors response, but I'm not sure if that's a real issue or not.
| const error = err as { message?: string; response?: { data?: unknown } }; | ||
| const responseData = error?.response?.data; | ||
|
|
||
| if (Array.isArray(responseData)) { |
There was a problem hiding this comment.
Nit: it would be helpful to have comments that show examples of each of the error shapes that you're expecting here. What API error response gives an array of strings, and what does that look like?
| return camelCaseObject(data); | ||
| } catch (err) { | ||
| throw new Error((err as any).response?.data?.error || (err as any).message); | ||
| throw new Error(getApiErrorMessage(err)); |
| const TABLE_MODES = { | ||
| VIEW: 'view', | ||
| DRAFT: 'draft', | ||
| PREVIEW: 'preview', | ||
| }; |
There was a problem hiding this comment.
Please document this, if you're keeping these modes. But as I mentioned in the other comment, I think we should put a TODO here that there's probably a way to simplify this.
| this.buildTree(); | ||
| } | ||
|
|
||
| getAllFlattenedAsCopy(): TagTreeNode[] { |
There was a problem hiding this comment.
Nit: I'd appreciate a comment that includes an example of the output of this function.
| } | ||
| } | ||
|
|
||
| private validateNoCycles(items: TagData[]) { |
There was a problem hiding this comment.
Why do we need cycle validation? Doesn't the backend already do that?
| @@ -0,0 +1,6 @@ | |||
| /** | |||
| * Warning: This must reflect the `TAXONOMY_MAX_DEPTH` used in the openedx-core backend. | |||
There was a problem hiding this comment.
As we now know, we need to clarify what this setting is. Is this setting: zero indexed? the maximum depth that we allow users to create? the maximum depth that we expect the backend to return? All of the above?
My preference would be that this limits the depth at which users can create in the UI, but the code should try to work with any depth whatsoever that the backend returns, in terms of displaying and reading the tags out, if possible.
| onKeyDown?: (event: React.KeyboardEvent<HTMLInputElement>) => void; | ||
| onChange?: (event: React.ChangeEvent<HTMLInputElement>) => void; | ||
| errorMessage?: string; | ||
| isSaving?: boolean; |
There was a problem hiding this comment.
Nit: please add a /** comment */ explaining isSaving and autoFocus
| validate: (value: string, mode?: 'soft' | 'hard') => boolean; | ||
| } | ||
|
|
||
| const NestedRows = ({ |
There was a problem hiding this comment.
Please add a docstring/comment.
6 participants
Description
This addresses Modular-learning #132: Adding functionality to create tags from a taxonomy list.
Go to /authoring/taxonomy, open a taxonomy, and you should be able to create new tags.
Acceptance Criteria
Found in Modular-learning #132
Architecture
The previously used Paragon DataTable is not designed to allow in-line edit functionality or work well with trees / deeply nested table entries. So I used tanstack/react-table directly to build a new tree-table that is editable inline.
AI Usage
To speed things up, I have heavily worked with Github Copilot. I have reviewed all the code carefully, but I want to point that out for awareness when it comes to review. I created pretty exhaustive tests to ensure that the code works as expected.
What is not in scope