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

Fixed field is required' validation #4714

Merged
merged 7 commits into from
Sep 13, 2024

Conversation

AllanOXDi
Copy link
Member

@AllanOXDi AllanOXDi commented Sep 6, 2024

Summary

This pull request addresses the issue where the text 'This field is required' is displayed when bulk editing learning activities.
Fixes #4651

Reviewer guidance

How can a reviewer test these changes?

  1. Select multiple resources with different categories.
  2. Verify that the "mixed" tag is not displayed as a category.
  3. Verify that indeterminate state checkboxes are not displayed.
  4. Verify that the helper text is displayed: "You selected resources that have different categories. The categories you choose below will be added to all selected resources. This will not remove existing categories."
  5. Verify that the "save" button is enabled only if one or more boxes are checked.
  6. Verify that no validation is required, and no error message is displayed.

References

#4651

Contributor's Checklist

PR process:

  • If this is an important user-facing change, PR or related issue the CHANGELOG label been added to this PR. Note: items with this label will be added to the CHANGELOG at a later time
  • If this includes an internal dependency change, a link to the diff is provided
  • The docs label has been added if this introduces a change that needs to be updated in the user docs?
  • If any Python requirements have changed, the updated requirements.txt files also included in this PR
  • Opportunities for using Google Analytics here are noted
  • Migrations are safe for a large db

Studio-specifc:

  • All user-facing strings are translated properly
  • The notranslate class been added to elements that shouldn't be translated by Google Chrome's automatic translation feature (e.g. icons, user-generated text)
  • All UI components are LTR and RTL compliant
  • Views are organized into pages, components, and layouts directories as described in the docs
  • Users' storage used is recalculated properly on any changes to main tree files
  • If there new ways this uses user data that needs to be factored into our Privacy Policy, it has been noted.

Testing:

  • Code is clean and well-commented
  • Contributor has fully tested the PR manually
  • If there are any front-end changes, before/after screenshots are included
  • Critical user journeys are covered by Gherkin stories
  • Any new interactions have been added to the QA Sheet
  • Critical and brittle code paths are covered by unit tests

Reviewer's Checklist

This section is for reviewers to fill out.

  • Automated test coverage is satisfactory
  • PR is fully functional
  • PR has been tested for accessibility regressions
  • External dependency files were updated if necessary (yarn and pip)
  • Documentation is updated
  • Contributor is in AUTHORS.md

@AllanOXDi AllanOXDi changed the title remove mixed tag as a category Fixed field is required' validation Sep 6, 2024
@AllanOXDi AllanOXDi marked this pull request as draft September 6, 2024 19:15
@@ -171,6 +181,8 @@
cancelAction: 'Cancel',
updateDescendantCheckbox:
'Apply to all resources, folders, and subfolders contained within the selected folders.',
addAdditionalCategoriesDescription:
Copy link
Member

Choose a reason for hiding this comment

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

This string exists inshared/translators.js already as addAdditionalCatgoriesDescription, so let's use that one!

@AllanOXDi AllanOXDi marked this pull request as ready for review September 10, 2024 19:57
Copy link
Member

@AlexVelezLl AlexVelezLl left a comment

Choose a reason for hiding this comment

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

Hi @AllanOXDi! Apologies it seems there have been a misunderstanding here 😅. This needs to be a change for all "boolean maps" fields, not just the categories. Boolean maps fields are fields that are represented as a boolean map, these are: Categories, Levels, ResourcesNeeded and Learning Activities. So to achieve the requirements:

  1. Don't display a "mixed" tag as a category. You have already removed the option from the categoryOptions, but further changes are needed:
  1. Don't display any indeterminate state checkboxes. This should be done for all boolean maps fields. For this
  1. If resources have different, existing categories (what would be currently shown as "mixed"), add a condition to display the new helper text

You selected resources that have different categories. The categories you choose below will be added to all selected resources. This will not remove existing categories.

  • For this, we will need to fix the hasMixedCategories as the nodes can have multiple categories selected, but all these categories must be selected in all selected nodes, right now I see the hasMixedCategoriesMessage even if we just select one resource, and this has two selected categories. So hasMixedCategories should return true if any of the selectedValues has an array with a length less than this.nodes.length which would mean tha here is a selectedValue that is not common to all selected nodes.
  • We will need to update the handleSave method so that if there is mixed Categories, we get the current values of the nodes, and add any new value in selectedValues, without removing old values.
  1. The "save" button only be active if one or more boxes is checked, but no validation is required (and no error message). We wont need this for all cases, but just for these who has provided a validator like the learning activities modal https://github.com/AllanOXDi/studio/blob/bulk_editting-fix/contentcuration/contentcuration/frontend/channelEdit/components/QuickEditModal/EditLearningActivitiesModal.vue#L8. As we can save empty boolean maps for categories for example if we want to remove all categories of a single selected resource.

cc: @marcellamaki you can correct me if I said something wrong 😅

@marcellamaki
Copy link
Member

Thanks @AlexVelezLl for this clarification, yes it looks like I did not explain the requirements so well. The additional details here all seem correct to me. @AllanOXDi, please let us know if you have follow up questions about any of this. I know there's a lot of details here, especially for someone who is coming in and relatively new to the feature. We're happy to help.

(On a somewhat tangential note, there is a little bit of a mix-up with the strings which is totally my responsibility, and I've been chatting about it already with @radinamatic. For now, you can continue to use the same string you are currently using, and I'll manage any additional changes in follow up.)

Copy link
Member

@marcellamaki marcellamaki left a comment

Choose a reason for hiding this comment

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

great work @AllanOXDi! I think this looks good as I've gone through the code and the manual QA and all seems to be working as expected and aligned with the requested changes. I have one small request for cleaning up some commented out code, but otherwise this should be good to go! Nice job implementing the feedback from Alex.

After this is merged, I'll get some feedback on the strings situation and I'll take on any follow up on that myself. 🎉

{{ error }}
</span>
</span> -->
Copy link
Member

Choose a reason for hiding this comment

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

I think this is no longer needed at all with the change that you've made, so let's delete rather than just comment it out

} = optionsValues;
expect(dailyLifeValue).toBe(CheckboxValue.CHECKED);
expect(foundationsValue).toBe(CheckboxValue.INDETERMINATE);
});
Copy link
Member

Choose a reason for hiding this comment

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

Thanks for cleaning up the tests, too! I hadn't remembered about that, but it's great to stay on top of that 🎉

Copy link
Member

@marcellamaki marcellamaki left a comment

Choose a reason for hiding this comment

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

thanks, Allan!

@marcellamaki marcellamaki merged commit cfff054 into learningequality:unstable Sep 13, 2024
13 checks passed
@akolson akolson mentioned this pull request Sep 13, 2024
Copy link
Member

@AlexVelezLl AlexVelezLl left a comment

Choose a reason for hiding this comment

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

Hey! I just noted a couple of things, I can take care of this over the weekend, just left these notes here to recall this later 😅

@AllanOXDi
Copy link
Member Author

Let me open a new pr for that fix . Thanks

@akolson
Copy link
Member

akolson commented Sep 13, 2024

@AllanOXDi we can also just revert the merge so to can merge the same back when ready 🙂 .The "Revert" button should be just above Alex's latest comment!

AllanOXDi added a commit that referenced this pull request Sep 17, 2024
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.

Unstable - Bulk edit learning activities - Seeing 'This field is required' validation message
4 participants