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: https://github.com/DMPRoadmap/roadmap/blob/bc32927483d4186daca1cc187bb4b7d5a130442a/app/models/concerns/versionable_model.rb#L14

[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]

https://github.com/DMPRoadmap/roadmap/pull/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?

[briri]

I'll be running this script on our production environment next week.

I don't believe its an issue, but please do spot check some of the records with NULLs to make sure they're not active/published templates before you run in production.

As long as all published templates and all of the most recent customized templates (published or unpublished) have versionable_ids you should be ok.

The root of the problem is with the 'Transfer customizations' logic. If an Org admin has customized a funder template in the past, and then the funder updates/publishes changes to the template, the system allows the Org Admin to "transfer" their customizations to the new funder template. The versionable_ids are used to help transfer that information.

@raycarrick-ed did some analysis of the code behind this. You can find it here: https://github.com/DMPRoadmap/roadmap/wiki/Template-Versioning-and-Customisation

[raycarrick-ed]

Agree with @briri. My experience was on our live server it only changed 10% of templates and left far more with nil values. That's why I looked into it. It only matters for the most up to date versions and when I looked into it what we were seeing was lots of archived templates which will never be touched. So I am pretty sure it's doing the right thing.