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

Don't require description if the category is hidden #1910

Merged
merged 19 commits into from
Jul 31, 2024

Conversation

andrewleith
Copy link
Member

@andrewleith andrewleith commented Jul 26, 2024

Summary | Résumé

Don't require description if the category is hidden.

Changelog

  • Remove validation for other description when the category is "hidden"
  • Show "Other" on templates list and filters when a template has a hidden template category
  • Add template categories test to ci suite
  • Update tests to match new logic
  • Sort filters alphabetically in fr/en

Test instructions | Instructions pour tester la modification

Retain hidden category

  • Create a new template category
  • Create a template and put it that category
  • Edit the template category to make it hidden
  • Go back to the template, and click edit
    • Ensure "Other" is selected
    • Save the template
    • Ensure the template saves correctly

Sorted filters

  • Go to template list page in english
    • Ensure category filters are listed alphabetically
  • Switch to FR
    • Ensure category filters are listed alphabetically

Copy link

@jzbahrai jzbahrai marked this pull request as ready for review July 29, 2024 15:42
Copy link
Contributor

@whabanks whabanks left a comment

Choose a reason for hiding this comment

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

Tested this locally following the test steps. Couple of things I noticed

✅ When setting the category to hidden both the list page and edit page show Other
🛑 When I try to save an edit to the template (non-category related edit) the validation fires on the category field. The Other option is not selected.
🛑 Selecting Other and saving does not preserve the original Category and instead assigns the template to the generic low category

if template.template_category and not template.template_category.get("hidden")
}
)
# Get the full list of template categories, any hidden ones will be called 'Other'
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if it would be simpler to allow the hidden category as it is labelled, as long as it is pre-selected. For example:

  • User chooses category A on template A
  • Admin makes category A invisible
  • User still see Category A when browsing and editing template A
  • User cannot choose Category A on new templates, or templates not categorized with Category A
  • User can choose Category B on Template A, save template
  • Template A can no longer be of Category A

Copy link
Contributor

Choose a reason for hiding this comment

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

So looping back to your comment line, Something like:
# Get the full list of template categories, any hidden ones matching the current category can be added to the list.

Copy link
Contributor

@amazingphilippe amazingphilippe Jul 30, 2024

Choose a reason for hiding this comment

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

You can ignore the above, I see you have it working now. I am able to

  • ✅ Edit and Save a template, after its category was set to hidden
  • ✅ Open up the category radio list, see the checked "Other", and save the template without a description for "Other"

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm not sure. Part of the problem here is we exposed a piece of functionality in the platform admin UI meant to be in the background for our default categories (low/med/high) and now we have all these edge cases trying to control this.

I don't think this scenario is very likely to happen, and I think we've already spent quite a bit of time on this, so personally I don't want to try to re-work it another way unless there is a clear benefit!

@amazingphilippe amazingphilippe self-requested a review July 30, 2024 19:15
@andrewleith
Copy link
Member Author

Tested this locally following the test steps. Couple of things I noticed

✅ When setting the category to hidden both the list page and edit page show Other 🛑 When I try to save an edit to the template (non-category related edit) the validation fires on the category field. The Other option is not selected. 🛑 Selecting Other and saving does not preserve the original Category and instead assigns the template to the generic low category

I think I have this fixed up now, if you want to test again @whabanks.

Copy link
Contributor

@whabanks whabanks left a comment

Choose a reason for hiding this comment

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

LGTM!

Other label present when assigned to a hidden Category
✅ Updating a template still works when assigned to a hidden category - defaults to other
✅ With Other selected the original (hidden) category is preserved in the DB

@andrewleith andrewleith merged commit b45837b into main Jul 31, 2024
10 checks passed
@andrewleith andrewleith deleted the fix/hidden-category branch July 31, 2024 11:55
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