-
Notifications
You must be signed in to change notification settings - Fork 112
Refactor how section ordering is handled #1837
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
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 that your draggable_section check will miss the unpublished state. See my comments and steps to test the scenario.
Otherwise everything else looks good 👍 I like the use of the presenter over a service here
@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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice. I hadn't even heard of presenters in Rails before (learn something new every day) :)
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'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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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) } |
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.
👍 much cleaner
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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? %> |
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.
👍
@@ -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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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: