Skip to content
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

fix: Remove extra published_state property in the configurator state #1358

Merged
merged 3 commits into from
Feb 22, 2024

Conversation

ptbrowne
Copy link
Collaborator

Made a silly mistake where I put published_state in the config and also
in the configurator state. Published_state should only be in the config,
not the state.

This resulted in old charts not being able to edited/copied/viewed due
to missing property while decoding configurator state.

Fix #1354

Copy link

vercel bot commented Feb 22, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
visualization-tool ✅ Ready (Inspect) Visit Preview 💬 5 unresolved Feb 23, 2024 3:46pm

Copy link
Collaborator

@bprusinowski bprusinowski left a comment

Choose a reason for hiding this comment

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

@ptbrowne I think you also need to remove the migration then if the property is no longer in the config?

I also wonder why this bug appeared when migration was correctly added? Shouldn't the charts be migrated correctly, maybe we miss a migrateConfiguratorState call somewhere?

@ptbrowne
Copy link
Collaborator Author

Thanks Bartosz, good point, I removed the migration.

I also wonder why this bug appeared when migration was correctly added?

Thanks also for wondering this 😅 So I investigated, and I had forgotten to bump the final version in CONFIGURATOR_STATE_VERSION. So the last migration had not been applied. With the mixing up of the attribute in configurator state, and config, I thought it was working when it was not. I added a test to make sure we do not forget to update the version.

Copy link
Collaborator

@bprusinowski bprusinowski left a comment

Choose a reason for hiding this comment

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

LGTM, thanks 🎉

@ptbrowne ptbrowne merged commit cff6bd1 into main Feb 22, 2024
5 of 8 checks passed
@ptbrowne ptbrowne deleted the fix/extra-published-state branch February 22, 2024 10:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Can't edit old charts, need for migrations
2 participants