Versionable id not always set on Phases, Sections and Questions

[briri]

We are seeing records with NULL values for their versionable_id

The Versionable concern defines a default for this value but for some reason we are seeing records with a NULL. This could be due to the use explicitly setting it to nil somewhere in the code or the ActiveRecord ORM is not using the default in all situations:

roadmap/app/models/concerns/versionable_model.rb

Line 14 in bc32927

default: lambda {

[raycarrick-ed]

Had a look at this.
Not 100% sure of this. Haven't had time to test it out (will do tomorrow) but I think:

upgrade_customization_service.rb:94

should be "@target_template". It calls the function target_template above and then modifies it, including fixing up the versionable_ids but then instead of returning that fixed up template it calls the target_template function again and so is returning a template copy which hasn't been fixed up. Hence null versionable_ids.

What do you think @briri ?

[briri]

Yes, good catch and seems like a likely culprit. Having instance variables with the same names is confusing and I've never liked the practice. It's meant to mimic the behavior of a singleton I think. Would be better to use Rails.cache in my opinion if it's a real concern.

It might be better to rename those methods to init_target, init_funder_template and init_customized_template and set them in the initializer method do something like:

def initialize(template)
  # Formerly init_template
  @source_template = template

  # Formerly funder_template (remove the old method)
  @original_funder_template = Template.published(@source_template.customization_of).first

  # Formerly customized_template (remove the old method)
  @copy_of_original_customizations = @source_template.deep_copy(
    attributes: { version: @source_template.version + 1, published: false }
  )

  # Formerly target_template
  @updated_template = init_updated_template
end

Then in the other methods just swap out the old references to the old methods/variables with these new (with hopefully easier to understand names) variables.

It looks like there are also some tests in spec/services/templates/upgrade_customization_service_spec.rb and spec/features/templates/templates_upgrade_customisations_spec.rb which could be helpful in verifying that any changes made work properly. We should also likely add a test or two to verify that this nil problem is gone.

[raycarrick-ed]

#2944

restructures upgrade customization code to remove confusing use of methods and global variables.
Tests still to do.

[nicolasfranck]

So one can fix the versionable_id with task bin/rails templates:fix_templates_with_nil_versionable_ids?
In what way does this affect the application? What happens if you do not execute this task?

[nicolasfranck]

BTW. I still have NULL values in those tables after running this..

[briri]

Hey @nicolasfranck. Yes, that rake task is meant to fix the templates (and associated records) by assigning their versionable_ids. @raycarrick-ed also had records with NULL values afterward. From what I understand this was due to the fact that they were migrated over from the old DMPonline_v4 codebase as 'archived' templates.

[nicolasfranck]

ah ok, probably the reason. Does it matter actually?