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

SJ - Redo Jtm add form func to org #1116

Merged

Conversation

Stuart-Johnson
Copy link
Collaborator

Reverts the original revert, effectively a "redo."

Reverts #1110

@questionnaire = Questionnaire.new(questionnaire_params)
@submission = Submission.new
@submission.questionnaire_responses.build
respond_to do |format|
format.js
if @questionnaire.valid?
Copy link
Contributor

Choose a reason for hiding this comment

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

@BigChiefSmidgeums What would happen here if the questionnaire is not valid? Should it flash an error instead, maybe something like

unless @questionnaire.valid?
  flash[:alert] = "Questionnaire invalid"
end

submissions = {}
if has_incomplete_service
submissions[:Services] = []
ssr.line_items.includes(:service).map(&:service).each do |service|
Copy link
Contributor

Choose a reason for hiding this comment

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

Service.where(id: ssr.line_items.pluck(:service_id)).distinct.each would be a bit more efficient. Instead of hitting the database n services times, it hits it once.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@kyle-glick the use of .includes(:services) means that the services are eager loaded with the line items. Tested in console, this creates only two queries. One to load all the line items, and one to load all the services. There is no query per service.

.col-sm-12.text-center.no-padding
= index+1
= item.item_options.index(item_option)+1
Copy link
Contributor

Choose a reason for hiding this comment

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

This is probably a bit more inefficient than using each_with_index because it will need to loop through item_options for each option. Is there a specific reason to change it?

@Stuart-Johnson Stuart-Johnson merged commit 3a1cdfb into v3.0.0b Oct 4, 2017
@Stuart-Johnson Stuart-Johnson deleted the revert-1110-revert-1093-jtm_add_form_func_to_org branch October 4, 2017 14:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants