8000 Refactor how section ordering is handled by Bodacious · Pull Request #1837 · DMPRoadmap/roadmap · GitHub
[go: up one dir, main page]

Skip to content

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

Merged
merged 24 commits into from
Aug 28, 2018

Conversation

Bodacious
Copy link
Contributor

Fixes #1371 (forces sections to be loaded by number)

Changes proposed in this PR:

  • Refactored controllers for better readability
  • Ensure sections are being loaded by number in views
  • Add a button to sort sections manually—this now creates a new template if published
  • Refactor code for Rubocop adherence
  • Renamed GuidanceService to GuidancePresenter—this isn't a service object.

@Bodacious Bodacious requested review from briri and xsrust August 22, 2018 17:10
Copy link
Contributor
@briri briri left a 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))
Copy link
Contributor

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) :)

Copy link
Contributor Author

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
Copy link
Contributor

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?

@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 + ")"
Copy link
Contributor

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

Copy link
Contributor Author

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

Copy link
Contributor

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.

@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 }
Copy link
Contributor

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

Copy link
Contributor Author

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

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) }
Copy link
Contributor

Choose a reason for hiding this comment

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

👍 much cleaner

@@ -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?
Copy link
Contributor

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?

Copy link
Contributor Author

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? %>
Copy link
Contributor

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? %>
Copy link
Contributor

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)

Copy link
Contributor
@xsrust xsrust left a 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

@briri
Copy link
Contributor
briri commented Aug 24, 2018

Adjustments look good. Feel free to merge yourself once the tests are fixed

@Bodacious
Copy link
Contributor Author

Tests are passing but the bundle-audit check --update is failing because of rubyzip/rubyzip#369

@briri
Copy link
Contributor
briri commented Aug 27, 2018

@Bodacious there's a small conflict after merging @johnpinto1 csv updates. Once you've resolved it feel free to merge the PR.

@xsrust
Copy link
Contributor
xsrust commented Aug 28, 2018

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.

@Bodacious
Copy link
Contributor Author

Sorry, fixed the merge conflicts.

@xsrust
Copy link
Contributor
xsrust commented Aug 28, 2018

Looking good to me

@xsrust xsrust merged commit 3039ef5 into development Aug 28, 2018
@Bodacious Bodacious deleted the refactor/section-reordering branch October 19, 2018 16:47
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.

3 participants
0