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 numerical person properties turning to strings on save #3202

Merged
merged 3 commits into from Feb 5, 2021

Conversation

yakkomajuri
Copy link
Collaborator

Changes

Please describe.
If this affects the frontend, include screenshots.

Close #3189

Tagging you @paolodamico because I wonder when we'll be releasing persons-2353. I'm not the biggest fan of all the UI updates but just the new logic is almost a necessity.

Spent more time than I'd like working on the old persons "logic" and decided it wasn't worth it from a prioritization standpoint.

Hence this fixes the logic on PersonV2 as I imagine we'll roll out the logic updates even if the experiment turns out bad from a UX perspective.

Checklist

  • All querysets/queries filter by Organization, by Team, and by User
  • Django backend tests
  • Jest frontend tests
  • Cypress end-to-end tests

@timgl timgl temporarily deployed to posthog-fix-property-up-8m2p1w February 4, 2021 15:38 Inactive
@timgl timgl temporarily deployed to posthog-fix-property-up-8m2p1w February 5, 2021 06:28 Inactive
@timgl timgl temporarily deployed to posthog-fix-property-up-8m2p1w February 5, 2021 06:32 Inactive
Copy link
Contributor

@paolodamico paolodamico left a comment

Choose a reason for hiding this comment

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

Great stuff, lgtm! Did a minor change so that we attempt to parse as number based on the new input not the old value. Please review before merging.

@paolodamico
Copy link
Contributor

In a more general sense, I'll definitely check for the release of persons-2353 this release cycle. Regardless I think it's definitely worth that you comment on the UI stuff that you don't like / would improve so we can work on those too if we do release it (#2353 or feel free to open a new issue).

@yakkomajuri
Copy link
Collaborator Author

makes sense, cheers!

@yakkomajuri yakkomajuri merged commit bc625ea into master Feb 5, 2021
@yakkomajuri yakkomajuri deleted the fix-property-update branch February 5, 2021 09:01
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.

Updating numerical properties via the UI changes type to string
3 participants