Refactor how section ordering is handled #1837
Conversation
| @phase = Phase.includes(:template).find(params[:id]) | ||
| authorize @phase | ||
| @template = @phase.template | ||
| @guidance_presenter = GuidancePresenter.new(Plan.new(template: @phase.template)) |
There was a problem hiding this comment.
Nice. I hadn't even heard of presenters in Rails before (learn something new every day) :)
There was a problem hiding this comment.
It's not a "native" Rails concept. But a good general principle to call on when you'd like to clean up code in an MVC paradigm :)
| end | ||
| end | ||
| end | ||
|
|
||
| # POST /org_admin/templates/:id/transfer_customization | ||
| # the funder template's id is passed through here | ||
| def transfer_customization |
There was a problem hiding this comment.
Should we not move this one to the new TemplateCustomizationsController?
app/controllers/plans_controller.rb
Outdated
| @plan.title = current_user.firstname.blank? ? _('My Plan') + '(' + @plan.template.title + ')' : | ||
| current_user.firstname + "'s" + _(" Plan") | ||
| @plan.title = if current_user.firstname.blank? | ||
| _("My Plan") + "(" + @plan.template.title + ")" |
There was a problem hiding this comment.
are these single -> double quote changes for Rubocop? I remember reading somewhere a few years ago that single quotes are more performant (unless of course you need interpolation ;)). Maybe that's no longer true or the impact is insignificant now
There was a problem hiding this comment.
Most style guides in Ruby prefer double quotes over single quotes, because double quotes provide more functionality (like interpolation, escaping characters, etc.)
There's a slight performance difference between single and double quotes, in favour of single quotes, but that becomes essentially irrelevant when we're using # frozen_string_literal: true
There was a problem hiding this comment.
👍 That works for me.
I just ran rubocop on a controller I am working on and got the "prefer double quotes" message.
app/controllers/plans_controller.rb
Outdated
| @all_ggs_grouped_by_org = @all_ggs_grouped_by_org.sort_by do |org, gg| | ||
| (org.nil? ? "" : org.name) | ||
| end | ||
| @selected_guidance_groups = @selected_guidance_groups.collect { |gg| gg.id } |
There was a problem hiding this comment.
Would suggest removing this line and adding 'pluck' to the line above @selected_guidance_groups = @plan.guidance_groups.pluck(:id) above
There was a problem hiding this comment.
I'll go ya one better...
@selected_guidance_groups = @selected_guidance_groups.ids
app/controllers/plans_controller.rb
Outdated
| format.csv { export_as_csv(file_name) } | ||
| format.text { export_as_text(file_name) } | ||
| format.docx { export_as_docx(file_name) } | ||
| format.pdf { export_as_pdf(file_name) } |
app/helpers/sections_helper.rb
Outdated
| @@ -18,4 +21,9 @@ def header_path_for_section(section, phase, template) | |||
| id: section.id) | |||
| end | |||
| end | |||
|
|
|||
| def draggable_for_section(section, sections) | |||
| section == sections.first && section.template.latest? && section.template.draft? | |||
There was a problem hiding this comment.
I don't think .draft? is enough here.
These states really have more to do with the template family than individual templates. There are 3 separate states:
- Unpublished: There are no published versions for the family
- Published: The family has a published version (either the current or a previous version)
- Draft: The family has a published version, but it is NOT the current/latest version
The draft/unpublished distinction is largely for the UI. We use it to give a visual cue to the user that their changes are not yet available to users creating plans and, for funder templates, have not yet propagated to any customized copies.
I think you would want the section to be draggable when its both 'draft' and 'unpublished'. Could you not just check for modifiable? That might be more versatile and allow you to use this for any section.
sections.template.latest? && section.modifiable?
There was a problem hiding this comment.
Thanks for the suggestion and the background explanation.
| @@ -26,7 +26,7 @@ | |||
| </div> | |||
|
|
|||
| <div class="col-md-12"> | |||
| <% if phase.sections.length > 0 %> | |||
| <% if phase.sections.any? %> | |||
| @@ -53,11 +57,18 @@ | |||
|
|
|||
| <div class="col-sm-6"> | |||
| <div class='text-right text-muted'> | |||
| <i class="fa fa-info-circle small"></i> | |||
| <% if template.draft? %> | |||
There was a problem hiding this comment.
See comment above. You can test it by:
- Publish your template
- Edit it to force the creation of a new version (these arrows should appear)
- Go back to the templates page and 'unpublished' the template
- Go back in to edit the current version (arrows may not appear due to the
.draft?check)
There was a problem hiding this comment.
I agree with @briri that i like rename of GuidanceService to GuidancePresentor. It's a good terminology for when we create objects for the sole purpose of display.
Looks like there's some tests still failing, but other than that, it looks good to me.
The code's definitely getting more easily readable as we start to adhere files to rubocop
|
Adjustments look good. Feel free to merge yourself once the tests are fixed |
|
Tests are passing but the |
|
@Bodacious there's a small conflict after merging @johnpinto1 csv updates. Once you've resolved it feel free to merge the PR. |
|
Looks like tests are failing since you've missed the rename on a guidanceservice in: render_phases_edit within the private methods of the plans controller. |
|
Sorry, fixed the merge conflicts. |
|
Looking good to me |
Fixes #1371 (forces sections to be loaded by number)
Changes proposed in this PR: