Skip to content

Extract surveys logic to decidim-forms#3877

Merged
oriolgual merged 32 commits intomasterfrom
rbngzlv/decidim-forms
Oct 31, 2018
Merged

Extract surveys logic to decidim-forms#3877
oriolgual merged 32 commits intomasterfrom
rbngzlv/decidim-forms

Conversation

@rbngzlv
Copy link
Copy Markdown
Contributor

@rbngzlv rbngzlv commented Jul 18, 2018 •
edited
Loading

🎩 What? Why?

As we agreed in the last meeting, we are moving the surveys logic to a separate gem: decidim-forms. This allow us to share this logic with decidim-meetings, that needs a configurable registration form and a configurable survey at the end of the meeting.

📌 Related Issues

📋 Subtasks

  • Move locale entries
  • Move controllers, views
  • Move models, migrations, ...
  • Serializer
  • Migrate data
  • Add CHANGELOG entry

@rbngzlv rbngzlv self-assigned this Jul 18, 2018
@rbngzlv rbngzlv force-pushed the rbngzlv/decidim-forms branch 2 times, most recently from 4c6a254 to 1baf4c3 Compare July 18, 2018 17:34
@rbngzlv rbngzlv force-pushed the rbngzlv/decidim-forms branch from 7526d35 to 821f08d Compare July 25, 2018 23:20
@rbngzlv rbngzlv force-pushed the rbngzlv/decidim-forms branch from 821f08d to 297044e Compare July 27, 2018 15:10
@rbngzlv
Copy link
Copy Markdown
Contributor Author

rbngzlv commented Jul 27, 2018 •
edited
Loading

Hi @decidim/lot-core, I think that this is mostly done, so can you do a review on this, please? I have detected two things:

  • Do you want me to squash the migrations in decidim-forms?
  • We need to migrate the actual data to the new decidim-formstables, really we can rename the tables, except the surveys table. Can you give your opinion on how to proceed with this? A rake task?

@oriolgual
Copy link
Copy Markdown
Contributor

oriolgual commented Jul 30, 2018

Do you want me to squash the migrations in decidim-forms?

I guess yes? I'm not sure if this affects anything or not.

We need to migrate the actual data to the new decidim-forms tables, really we can rename the tables, except the surveys table. Can you give your opinion on how to proceed with this? A rake task?

Can't we migrate the data with a migration @rbngzlv?

@ghost ghost added the status: WIP label Aug 1, 2018
@rbngzlv
Copy link
Copy Markdown
Contributor Author

rbngzlv commented Aug 1, 2018

@oriolgual migrations squashed and added a new migration to migrate the data from surveys to forms.

@decidim/lot-core ready to review!

@oriolgual oriolgual requested review from mrcasals and oriolgual August 6, 2018 13:30
@mrcasals
Copy link
Copy Markdown
Contributor

mrcasals commented Aug 7, 2018

@rbngzlv there seems to be a conflict, can you check it please?

@oriolgual
Copy link
Copy Markdown
Contributor

oriolgual commented Oct 9, 2018

@rbngzlv lets go with your approach then, and maybe add a comment to the upgrade note saying that you should generate a manual migration to delete the old tables when you're sure the data has been properly migrated.

@rbngzlv
Merge branch 'master' into rbngzlv/decidim-forms
3230f75 
# Conflicts:
#	config/i18n-tasks.yml
#	decidim-surveys/config/locales/en.yml
#	decidim-surveys/lib/decidim/surveys/component.rb
@rbngzlv
Move migration to migrate data to a rake task and update changelog
b8ce1bf
@rbngzlv rbngzlv force-pushed the rbngzlv/decidim-forms branch from d600fb0 to b8ce1bf Compare October 17, 2018 11:21
@rbngzlv
Copy link
Copy Markdown
Contributor Author

rbngzlv commented Oct 17, 2018

@decidim/lot-core The commented changes done. The total coverage changes due to the added rake task that migrates the data.

Can you review it?

@tramuntanal
Copy link
Copy Markdown
Contributor

tramuntanal commented Oct 23, 2018

Please @decidim/lot-core, its 6 days since this PR is waiting review.

@ghost ghost assigned josepjaume Oct 23, 2018
@josepjaume
Copy link
Copy Markdown
Contributor

josepjaume commented Oct 23, 2018

@tramuntanal the CI was stuck so I've merged the latest changes from master and triggered a rebuild. If it passes, let's merge this.

@tramuntanal
Copy link
Copy Markdown
Contributor

tramuntanal commented Oct 23, 2018

@decidim/lot-core @rbngzlv total-coverage has changed -0.1% due to decidim-surveys/lib/tasks/decidim_surveys_tasks.rake.

We should close this PR. I think @rbngzlv has no clear view in how to test rake tasks.. how do we do?
Can we mark the rake task as ignored by code climate?

@mrcasals
Copy link
Copy Markdown
Contributor

mrcasals commented Oct 24, 2018

@tramuntanal close it? Or review&merge it? 😕

@rbngzlv
Copy link
Copy Markdown
Contributor Author

rbngzlv commented Oct 24, 2018

@tramuntanal close it? Or review&merge it? 😕

@mrcasals review & merge it :)

@tramuntanal
Copy link
Copy Markdown
Contributor

tramuntanal commented Oct 24, 2018

@rbngzlv @mrcasals review & merge it of course! :)

@tramuntanal
Copy link
Copy Markdown
Contributor

tramuntanal commented Oct 30, 2018

@decidim/lot-core still waiting for feedback

@ghost ghost added the status: WIP label Oct 30, 2018
@oriolgual
Copy link
Copy Markdown
Contributor

oriolgual commented Oct 30, 2018

@rbngzlv @tramuntanal I've added a couple of changes.

@oriolgual oriolgual merged commit ce638ef into master Oct 31, 2018
@ghost ghost removed the status: WIP label Oct 31, 2018
@oriolgual oriolgual deleted the rbngzlv/decidim-forms branch October 31, 2018 07:46
@rbngzlv rbngzlv mentioned this pull request Feb 11, 2019
Merged
3 tasks
@andreslucena andreslucena mentioned this pull request Jul 6, 2020
Closed
@andreslucena andreslucena mentioned this pull request Mar 21, 2025
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@mrcasals mrcasals Awaiting requested review from mrcasals

2 more reviewers

@oriolgual oriolgual oriolgual approved these changes

@josepjaume josepjaume josepjaume left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

@oriolgual oriolgual

@josepjaume josepjaume

@rbngzlv rbngzlv

Labels

in-review module: surveys

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@rbngzlv @oriolgual @mrcasals @andreslucena @tramuntanal @josepjaume