Skip to content

Fix decidim-templates gem definition to include templates migrations#6899

Merged
tramuntanal merged 4 commits intodevelopfrom
fix/gem-templates-definition
Nov 24, 2020
Merged

Fix decidim-templates gem definition to include templates migrations#6899
tramuntanal merged 4 commits intodevelopfrom
fix/gem-templates-definition

Conversation

@microstudi
Copy link
Copy Markdown
Contributor

🎩 What? Why?

The definition of the gem decidim-templates does not include the db folder meaning it cannot be installed from rubygems.org.
This should fix it but requires to create a new version of the gem (see the backport for 0.23).

📌 Related Issues

Link your PR to an issue

Testing

Describe the best way to test or validate your PR.

📋 Checklist

🚨 Please review the guidelines for contributing to this repository.

  • CONSIDER adding a unit test if your PR resolves an issue.
  • ✔️ DO check open PR's to avoid duplicates.
  • ✔️ DO keep pull requests small so they can be easily reviewed.
  • ✔️ DO build locally before pushing.
  • ✔️ DO make sure tests pass.
  • ✔️ DO make sure any new changes are documented in docs/.
  • ✔️ DO add and modify seeds if necessary.
  • ✔️ DO add CHANGELOG upgrade notes if required.
  • ✔️ DO add to GraphQL API if there are new public fields.
  • ✔️ DO add link to MetaDecidim if it's a new feature.
  • AVOID breaking the continuous integration build.
  • AVOID making significant changes to the overall architecture.

📷 Screenshots

Please add screenshots of the changes you're proposing
Description

♥️ Thank you!

@mrcasals
Copy link
Copy Markdown
Contributor

Should we add some kind of test making sure the command runs fine?

@microstudi
Copy link
Copy Markdown
Contributor Author

That would be great @mrcasals , do you know how we can test this?

@mrcasals
Copy link
Copy Markdown
Contributor

@microstudi maybe something like we do in decidim-generators/spec/generators_spec.rb? Not sure, though!

@microstudi
Copy link
Copy Markdown
Contributor Author

I'll take a look monday, see if I figure something out. However I suspect this should be a part of a broader strategy of testing gems and interactions with included and options decidim modules.

@mrcasals
Copy link
Copy Markdown
Contributor

@microstudi yup, I don't want to block this PR for that!

Comment on lines +78 to +92
# Checks that every table from a migration is included in the generated schema
schema = File.read("#{test_app}/db/schema.rb")
tables = []
dropped = []
Decidim::GemManager.plugins.each do |plugin|
Dir.glob("#{plugin}db/migrate/*.rb").each do |migration|
lines = File.readlines(migration)
tables.concat(lines.filter { |line| line.match? "create_table" }.map { |line| line.match(/(:)([a-z_0-9]+)/)[2] })
dropped.concat(lines.filter { |line| line.match? "drop_table" }.map { |line| line.match(/(:)([a-z_0-9]+)/)[2] })
end
end
tables.each do |table|
next if dropped.include? table

expect(schema).to match(/create_table "#{table}"|create_table :#{table}/)
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@mrcasals I've come out with this trick to test if the generated app has all the tables specified in the different modules. This works for the use case, if the db folder is not included in the gem, migrations are not copy and test fails.

What do you think?

@tramuntanal tramuntanal changed the title fix gem definition to include templates migrations Fix gem definition to include templates migrations Nov 24, 2020
@tramuntanal tramuntanal changed the title Fix gem definition to include templates migrations Fix decidim-templates gem definition to include templates migrations Nov 24, 2020
Copy link
Copy Markdown
Contributor

@tramuntanal tramuntanal left a comment

Choose a reason for hiding this comment

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

Good work @microstudi

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.

Cannot run decidim_templates:install:migrations

3 participants