Skip to content
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

Add MVP modal for applying inheritable metadata #4664

Merged

Conversation

marcellamaki
Copy link
Member

@marcellamaki marcellamaki commented Aug 20, 2024

This PR adds additional functionality so that when a user uploads, imports, or moves a resource to a folder (moving can be with the "move" button, drag and drop, or copying via the clipboard) that has "bulk edit-able" metadata, they are prompted to select any options they would like that resource to inherit.

Screenshot 2024-08-29 at 2 58 51 PM

Steps for manual QA:

  1. Create a folder and add language, categories, level, and/or requirements (learner needs) to the folder.
  2. Create a second folder and do the same, but with different metadata
  3. Upload some resources into one of the folders, you should see the modal appear
  4. Move a resource into the second folder. you should be prompted to add the second folder's metadata
  5. import a resource to either folder
  6. copy a resource to clipboard, and copy it into a new folder
  7. create a new folder with no metadata. move/import a resource into it. you should not see the modal appear
  8. create a folder that has a language tagged but no additional details. you should not see the "Select details to add" text or any checkboxes
    1. create a folder that has categories (etc_ tagged but no additional langauge. you should see the "Select details to add" text but not "update language" or any checkboxes

@marcellamaki marcellamaki marked this pull request as ready for review August 29, 2024 19:11
Copy link
Member

@akolson akolson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @marcellamaki @rtibbles! The implementation generally looks correct to me. The code makes sense and Manual QA seems to checkout. The code could be improved further in some areas where it is quite nested and tending to get a little confusing, but for an MVP, this should be sufficient and don't see this as a blocker!

@marcellamaki
Copy link
Member Author

thanks so much for the speedy review @akolson. Good point with the functions -- there are definitely some places including where the metadata labels are getting parsed that one can get a bit lost in. I'll open a follow up issue for some cleanup or other things to make the code clearer. I'll link it here and if you have specific suggestions (areas you had to read multiple times to understand, places where comments would help, suggestions for which functions to break up), please go ahead and either edit the issue directly or add a comment!

@marcellamaki marcellamaki merged commit b48d9ea into learningequality:unstable Aug 30, 2024
13 checks passed
@akolson akolson mentioned this pull request Aug 30, 2024
@radinamatic
Copy link
Member

Not sure why, but I got a bit of an inconsistent behavior at point 3. and 4. listed above, but only for the second folder I created: for resources moved from other folders I was not prompted with the modal to apply folder details, but I was when I moved resources from the clipboard into this second folder. Folders test1 and test2 have been created with the usual identical steps, just with different categories, levels, and requirements.

I was also not getting the modal to apply details to resources moved into the test4 folder for the scenario 8. above... 🤔

Could these inconsistencies have something to do with the fact that the current implementation for the bulk editing has the Apply to all resources, folders, and subfolders contained within the selected folders. option, but we do not offer anything of the sort when the user simply uses the ADD button and selects New folder, and sets categories/levels/requirements in the New folder page (at least on the root of the channel as I was doing).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants