Skip to content

Conversation

@aerosol
Copy link
Member

@aerosol aerosol commented Dec 11, 2025

Reapply #5936 with some anti-nil defences

@aerosol aerosol changed the title Fixup goals custom props Reapply+bugfix: goals with custom props (#5936) Dec 11, 2025
@aerosol aerosol requested a review from a team December 11, 2025 09:40
@aerosol aerosol force-pushed the fixup-goals-custom-props branch from 0d14975 to 8e38748 Compare December 11, 2025 09:42
@aerosol aerosol enabled auto-merge December 11, 2025 09:59
@ukutaht
Copy link
Contributor

ukutaht commented Dec 11, 2025

Is there any meaningful difference between custom props being nil vs %{}?

I see that new goals will default to empty map when created via Elixir, but the same default is not set in postgres. I don't know, just having the same state represented in two different ways creates the need to account for both which seems unnecessary detail to have to deal with. Why don't we set the field to custom_props JSONB NOT NULL DEFAULT '{}'::jsonb and only deal with one representation of "no props configured" in the code?

Sorry I missed this in review as well :(

@aerosol
Copy link
Member Author

aerosol commented Dec 11, 2025

I think the difference is how unique constraints are firing. I dislike nullable columns too. Let me try to run this on top of another migration then

@aerosol aerosol marked this pull request as draft December 11, 2025 11:07
auto-merge was automatically disabled December 11, 2025 11:07

Pull request was converted to draft

@aerosol
Copy link
Member Author

aerosol commented Dec 11, 2025

@ukutaht done, but we have to push #5945 first

@aerosol aerosol marked this pull request as ready for review December 11, 2025 12:57
@aerosol
Copy link
Member Author

aerosol commented Dec 11, 2025

Done via #5946

@aerosol aerosol enabled auto-merge December 11, 2025 13:02
@aerosol aerosol added this pull request to the merge queue Dec 11, 2025
@aerosol aerosol removed this pull request from the merge queue due to a manual request Dec 11, 2025
@github-actions
Copy link

Preview environment👷🏼‍♀️🏗️
PR-5944

@aerosol aerosol added this pull request to the merge queue Dec 11, 2025
Merged via the queue into master with commit 3838119 Dec 11, 2025
18 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants