School Info Completeness: refactor user school infos update method

[nkiruka]

A new variable, "new school info params" was added to the user_school_infos update method, since full_address and country are not modified in place. The refactored code utilizes memoization to update the school params in place, thus the addition of a new variable is no longer required.

Parent PR

[nkiruka]

@islemaster This is the test that is failing because school_info_params is not saving the re-assigned value.

[nkiruka]

@islemaster I added the puts statement and the output shows that the value is not updated. For instance, full_address is {(nil, "")}

[nkiruka]

Using a temporary variable (I had to pull out CSFUN tricks), I was able to re-assign the new values for full address and country. I am not sure why I cannot do the following:

school_info_params.delete(:full_address) if school_info_params[:full_address]&.blank? (per your recommendation)

and

if school_info_params[:country]&.downcase&.eql? 'united states'
school_info_params[:country] = 'US'
end

I tried to check if this has to do with "permit" in school_info_params, but I didn't come up with anything. Thoughts

[nkiruka]

@islemaster I wonder if it is related to this:
https://stackoverflow.com/questions/18369592/modify-ruby-hash-in-place-rails-strong-params

[islemaster]

I'm pulling this branch to try it out myself. I suspect you're correct, and due to the permit operation the change I suggested doesn't work because we're dealing with something other than a Hash.

I'll write again in a few minutes once I've wrapped my head around this.

Note that the current test failure is actually caused by one of the puts statements:

NoMethodError at /api/v1/user_school_infos | 1411s
-- | --
1726 | ========================================== | 1411s
1727 |   | 1411s
1728 | > undefined method `changes' for nil:NilClass | 1411s
1729 |   | 1411s
1730 | app/controllers/api/v1/user_school_infos_controller.rb, line 25

[islemaster]

It looks like you solved it! The problem with my original suggestion was that school_info_params was a method call, not simply a hash, and therefore wasn't going to make sense to modify in place.

[islemaster]

Now that you've got this working, there's a small bit of cleanup I'd do: It's not obvious where @school_info_params comes from (it's set as a side-effect of calling school_info_params, above). Two solutions come to mind.

1. Use school_info_params instead of @school_info_params throughout def update.
   Now that you've memoized the school_info_params function, you can mutate the hash it returns and every call to that function will include your latest changes.

2. De-memoize the helper and go back to a temporary variable
   Setting new_school_info_params = school_info_params and the mutating the temporary variable would be fine too, and maybe a little more obvious about what's going on.

I'm leaning toward 1, despite it being a bit on the "clever" side, because it will read really nicely.

[nkiruka]

Thank you!

[nkiruka]

@islemaster Is it good practice to use memoization? I am curious about which scenarios it is more beneficial to use.

[islemaster]

We typically memoize to avoid doing something expensive more than once. For example, this function uses memoization to only do a database query the first time it's called:

code-dot-org/pegasus/router.rb

Lines 284 to 287 in 82792e3

	def dashboard_user
	return nil if (id = dashboard_user_id).nil?
	@dashboard_user ||= Dashboard.db[:users][id: id]
	end

Yours is a less typical case. We're not memoizing here because filtering the params is expensive, but more because conceptually we shouldn't need to do it more than once; and because they're effectively global to the controller, and we want the ability to modify them.

So I'm not sure this is a best practice, but its enough of a well-known pattern that I don't worry it will confuse anyone.