-
Notifications
You must be signed in to change notification settings - Fork 3
Reorganize template dropdown menu and refactor TemplateOptionsController
#1199
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
Conversation
f2311b0 to
cbb80dc
Compare
|
The ordering seems to be off in the dropdown when I select |
| # Assign the order of templates in the dropdown menu | ||
| def sort_templates(templates, org) # rubocop:disable Metrics/AbcSize | ||
| # Get all templates belonging to the selected organization | ||
| org_templates = templates.select { |template| template.org_id == org&.id && template.customization_of.nil? } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why are you using template.customization_of.nil??
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seemed to fix this bug that occurs with some orgs (like McMaster for example) where the order would be Alliance Simplified Template -> Org templates -> Alliance Template
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this query result might reveal a bit:

Non-Alliance orgs may have their own customisation of the "Alliance Simplified Template". If an org does have that customisation, then it will be included in org_templates. And further, I'm thinking the .uniq execution at the end would remove the alliance_template = templates.find { |t| t.title == 'Alliance Template' } duplicate.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@CeriMacMillan, what to do in the following scenario?
An org has customised the Alliance Simplified Template (Funding Application Stage) template but the title remains identical. Should this be grouped with the org templates? Or should it be grouped below that in the (Alliance Simplified Template (Funding Application Stage) / Alliance Template) section?
Maybe that same question can be asked of any org customisation of an Alliance template the retains the exact same title.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@aaronskiba That's a really good question, I would lean towards putting it with the org templates as they have taken the time to add local content/context. Maybe they can come after any or templates named as 'org template' but before the Alliance templates.
Can double check this with @closenma though to be sure.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree, if it has customization then let's count it at organizational and sort it above the other Alliance templates.
I'm guessing that's because the Alliance templates get included in |
That's right. Basically, Alliance Template, and then all of the others in alphabetical order. |
aaronskiba
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This snippet mutates @templates by removing default/customization and then prepending it:
@templates.select! { |t| t.id != Template.default.id && t.id != customization.id }
# We want the default template to appear at the beggining of the list
@templates.unshift(customization)
endThat snippet was used for the prior ordering of the templates. Since sort_templates is now fully responsible for the ordering, maybe we should address/remove the old ordering code.
I think that code might still be necessary. I tried removing it, but with that change the Alliance Template now never shows up in the dropdown. I also am not sure how much we should meddle with the existing code, since it seems there's a lot going on with customizations and default templates. |
Yes, this controller is looking pretty fragile. I might take a shot at that change though, just to help a bit with future maintainability. |
TemplateOptionsController
7273b10 to
b7fe417
Compare
- With these changes, if the chosen org is the Alliance, now the first two displayed templates are the Alliance Simplified and Alliance main templates - The previous code incorrectly excluded org templates that were customizations of other templates. Now those templates are included at the top of the dropdown menu along with other org templates.
Simplified template loading logic by removing explicit default template handling. - `Template.latest_customizable.where(org_id: funder.id)` and `Template.published.publicly_visible.where(org_id: funder.id, customization_of: nil)` now fetch the default template as well - Removed manual insertion and filtering of the default template; `sort_templates()` now determines ordering.
The changes in a4fc95e require that the default template belong to the default org.
27ac43c to
40c0299
Compare
aaronskiba
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good. Sorting looks in order and I'm liking the simpler handling of the default template throughout this controller now.
Fixes #1196 .
Changes proposed in this PR:
Template.defaulthandling.Template.latest_customizable.where(org_id: funder.id)andTemplate.published.publicly_visible.where(org_id: funder.id, customization_of: nil)now fetch the default template as wellsort_templates()now determines ordering.