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.

Sign up for GitHub

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

Jump to bottom

Update URL to url in field_type enum #1032

Merged
merged 1 commit into from
Jul 17, 2024
Merged

Update URL to url in field_type enum #1032

merged 1 commit into from
Jul 17, 2024

Conversation

hminsky2002
Copy link
Contributor

This PR remedies a typo from the field_type enum, which is used to validate the type of a basefield. The value of 'url' was originally capitalized as 'URL', but this goes against our convention. It adds a new migration, and updates the seed file accordingly. Notably, it also casts any existing base-fields with type 'URL' to 'url'

Closes #989

@hminsky2002 hminsky2002 requested a review from slifty May 24, 2024 17:14
@codecov Codecov
Copy link

codecov bot commented May 24, 2024 •
edited
Loading

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 88.57%. Comparing base (625be13) to head (3e56720).

Additional details and impacted files 
@@           Coverage Diff           @@
##             main    #1032   +/-   ##
=======================================
  Coverage   88.57%   88.57%           
=======================================
  Files         129      129           
  Lines        1725     1725           
  Branches      220      213    -7     
=======================================
  Hits         1528     1528           
  Misses        197      197

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

reefdog
reefdog reviewed May 24, 2024
View reviewed changes
Copy link
Contributor

@reefdog reefdog left a comment •
edited
Loading

Choose a reason for hiding this comment

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

I believe BaseField.ts will need to be updated too, correct?

diff --git a/src/types/BaseField.ts b/src/types/BaseField.ts
index c0d9c61..6af8a4d 100644
--- a/src/types/BaseField.ts
+++ b/src/types/BaseField.ts
@@ -7,7 +7,7 @@ export enum BaseFieldDataType {
 	NUMBER = 'number',
 	PHONE_NUMBER = 'phone_number',
 	EMAIL = 'email',
-	URL = 'URL',
+	URL = 'url',
 	BOOLEAN = 'boolean',
 }

@hminsky2002
Copy link
Contributor Author

I believe BaseField.ts will need to be updated too, correct?

diff --git a/src/types/BaseField.ts b/src/types/BaseField.ts
index c0d9c61..6af8a4d 100644
--- a/src/types/BaseField.ts
+++ b/src/types/BaseField.ts
@@ -7,7 +7,7 @@ export enum BaseFieldDataType {
 	NUMBER = 'number',
 	PHONE_NUMBER = 'phone_number',
 	EMAIL = 'email',
-	URL = 'URL',
+	URL = 'url',
 	BOOLEAN = 'boolean',
 }

I do believe you are correct @reefdog ! Updating and force pushing now

bickelj
bickelj reviewed Jul 10, 2024
View reviewed changes
src/database/migrations/0028-alter-field_type-enum.sql
ELSE data_type::text::updated_field_type
END;
DROP TYPE field_type CASCADE;
ALTER TYPE updated_field_type RENAME to field_type;
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice. I thought for a minute there may be another more brief way of doing it but this is probably the straightest-forward approach.

Copy link
Member

@slifty slifty Jul 10, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

This is great, and I have no qualms. I was curious though so asked GPT how it would do it:

-- Step 1: Add the new value to the enum type
ALTER TYPE user_type ADD VALUE 'boss';

-- Step 2: Update the table to use the new value
UPDATE users SET user_type = 'boss' WHERE user_type = 'BOSS';

-- Step 3: Remove the old value (PostgreSQL does not support removing enum values directly)
-- So instead, create a new type without 'BOSS' and replace the old type

-- Create a new enum type without 'BOSS'
CREATE TYPE user_type_new AS ENUM('boss', /* other values */);

-- Alter the table to use the new enum type
ALTER TABLE users
  ALTER COLUMN user_type TYPE user_type_new
  USING user_type::text::user_type_new;

-- Drop the old enum type
DROP TYPE user_type;

-- Rename the new enum type to the original name
ALTER TYPE user_type_new RENAME TO user_type;

It's very similar to your approach, but by adding the url value to the original type / updating the fields to use url, and then swapping the enum to one without the (now unused URL) you could avoid the complexity of the CASE statement.

I defer to you @hminsky2002 on which you prefer!

Copy link
Contributor Author

@hminsky2002 hminsky2002 Jul 11, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

@slifty I think that avoiding a case statement is preferable!
I shall edit this commit with gusto

Copy link
Member

Choose a reason for hiding this comment

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

Following up here since Harry and I went offline to do some rapid back and forth -- Harry did implement this change but psql doesn't like using enums that aren't committed in a transaction (idk why, but whatever.)

Having two migrations felt nastier than a case statement, so he landed back where he started with the case statement!

That was fun :D

src/database/seeds/0001-insert-base_fields.sql Show resolved Hide resolved
src/types/BaseField.ts
@@ -7,7 +7,7 @@ export enum BaseFieldDataType {
NUMBER = 'number',
PHONE_NUMBER = 'phone_number',
EMAIL = 'email',
URL = 'URL',
URL = 'url',
Copy link
Contributor

Choose a reason for hiding this comment

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

🎉

@slifty
Copy link
Member

slifty commented Jul 16, 2024

@hminsky2002 just to confirm I don't see the change yet, unless I'm missing it!

@hminsky2002
Copy link
Contributor Author

@hminsky2002 just to confirm I don't see the change yet, unless I'm missing it!

@slifty apologies, I was running into an issue where altering the enum was failing the migration, with the error
Reason: unsafe use of new value "url" of enum type field_type
Which seems to suggest that directly altering the enum is not liked by our system? I kept the feature of making a new enum, but have been able to avoid the case statement which does feel better!

slifty
slifty approved these changes Jul 17, 2024
View reviewed changes
Copy link
Member

@slifty slifty left a comment

Choose a reason for hiding this comment

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

Looks good!

@hminsky2002
Update URL to url in field_type enum
3e56720 
This commit remedies a typo from the field_type enum, which is used to
validate the type of a basefield. The value of 'url' was originally
capitalized as 'URL', but this goes against our convention. It adds a
new migration, and updates the seed file accordingly

Issue #989 Update URL enum value to url
@hminsky2002 hminsky2002 merged commit a9cc3af into main Jul 17, 2024
5 checks passed
@hminsky2002 hminsky2002 deleted the 989-update-url-enum branch July 17, 2024 17:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@reefdog reefdog reefdog left review comments

@slifty slifty slifty approved these changes

@bickelj bickelj bickelj approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Update data type URL enum value to be url
4 participants
@hminsky2002 @slifty @reefdog @bickelj