New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Re-create custom pivot table when definition changes #1735

Merged
merged 4 commits into from Dec 3, 2017

Conversation

Projects
None yet
4 participants
@Dirklectisch
Contributor

Dirklectisch commented Jun 27, 2017

The schema of a pivot table often evolves over time. Changing a pivot table currently means you have manually delete the table from all environments which is quite a hassle. I propose that we just delete and re-init the table when the definition changes.

@mention-bot

This comment has been minimized.

Show comment
Hide comment
@mention-bot

mention-bot Jun 27, 2017

@Dirklectisch, thanks for your PR! By analyzing the history of the files in this pull request, we identified @mworrell, @ddeboer and @arjan to be potential reviewers.

mention-bot commented Jun 27, 2017

@Dirklectisch, thanks for your PR! By analyzing the history of the files in this pull request, we identified @mworrell, @ddeboer and @arjan to be potential reviewers.

@mworrell

This comment has been minimized.

Show comment
Hide comment
@mworrell

mworrell Sep 12, 2017

Member

I am not sure about the equality check for the columns. As one comes from the spec, and one comes from the database. There could be a (small) textual change between them, and still have the same spec. Such a textual difference would then result in the new pivot table being rebuild on every module init.

Member

mworrell commented Sep 12, 2017

I am not sure about the equality check for the columns. As one comes from the spec, and one comes from the database. There could be a (small) textual change between them, and still have the same spec. Such a textual difference would then result in the new pivot table being rebuild on every module init.

@Dirklectisch

This comment has been minimized.

Show comment
Hide comment
@Dirklectisch

Dirklectisch Sep 12, 2017

Contributor

You will lose the table if you start renaming columns in it, that's correct. But in normal scenarios the column names in the db and the spec should match.

Contributor

Dirklectisch commented Sep 12, 2017

You will lose the table if you start renaming columns in it, that's correct. But in normal scenarios the column names in the db and the spec should match.

@mworrell

This comment has been minimized.

Show comment
Hide comment
@mworrell

mworrell Sep 12, 2017

Member

I was thinking of the Type of the columns. Matching on the names is a good idea and stable.

Member

mworrell commented Sep 12, 2017

I was thinking of the Type of the columns. Matching on the names is a good idea and stable.

@Dirklectisch

This comment has been minimized.

Show comment
Hide comment
@Dirklectisch

Dirklectisch Nov 10, 2017

Contributor

@mworrell Changed the comparison so that it only compares column names.

Contributor

Dirklectisch commented Nov 10, 2017

@mworrell Changed the comparison so that it only compares column names.

@ddeboer

This comment has been minimized.

Show comment
Hide comment
@ddeboer

ddeboer Nov 10, 2017

Member

LGTM to now.

@Dirklectisch Can you add a note to the docs explaining that pivot changes will now re-create the table, losing all (albeit transient) data currently in it?

Member

ddeboer commented Nov 10, 2017

LGTM to now.

@Dirklectisch Can you add a note to the docs explaining that pivot changes will now re-create the table, losing all (albeit transient) data currently in it?

@ddeboer ddeboer added this to the 0.35 milestone Nov 10, 2017

@Dirklectisch

This comment has been minimized.

Show comment
Hide comment
@Dirklectisch

Dirklectisch Dec 2, 2017

Contributor

@ddeboer Docs updated

Contributor

Dirklectisch commented Dec 2, 2017

@ddeboer Docs updated

@ddeboer ddeboer merged commit 40dd8ce into zotonic:0.x Dec 3, 2017

2 checks passed

ci/dockercloud Your tests passed in Docker Cloud
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@ddeboer

This comment has been minimized.

Show comment
Hide comment
@ddeboer

ddeboer Dec 3, 2017

Member

Thanks!

Member

ddeboer commented Dec 3, 2017

Thanks!

ddeboer added a commit that referenced this pull request Dec 4, 2017

Re-create custom pivot table when definition changes (#1735)
* Re-create custom pivot table when definition changes

* Only compare columns name when deciding to rebuild pivot table

* Update docs with info on define_custom_pivot changes

(cherry picked from commit 40dd8ce)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment