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

chore: create permit table and move data #817

Open
wants to merge 16 commits into
base: development
Choose a base branch
from

Conversation

seprintour
Copy link
Contributor

@seprintour seprintour commented Sep 23, 2023

Resolves #811

Quality Assurance:

Screenshot 2023-09-23 at 7 27 19 PM

Screenshot 2023-09-23 at 7 25 21 PM

More QA: Seprintour-Test/test#41 (comment)
Screenshot 2023-09-24 at 3 30 56 PM

I also changed the datatype from char to varchar to fix this error Seprintour-Test/test#41 (comment) @pavlovcik

@netlify
Copy link

netlify bot commented Sep 23, 2023

Deploy Preview for ubiquibot-staging ready!

Name Link
🔨 Latest commit 3d46590
🔍 Latest deploy log https://app.netlify.com/sites/ubiquibot-staging/deploys/6523e49b5607da0008773cf4
😎 Deploy Preview https://deploy-preview-817--ubiquibot-staging.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@seprintour
Copy link
Contributor Author

@pavlovcik PR attached

@0x4007
Copy link
Member

0x4007 commented Sep 24, 2023

The bot is expected to be stable after these changes are implemented which implies that all of the functions that interact with the database should also be refactored to accomodate this new architecture.

From #787

Shouldn't you refactor functions that interact with these tables?

@seprintour
Copy link
Contributor Author

From #787

Shouldn't you refactor functions that interact with these tables?

Yea, I would do that now..

@seprintour
Copy link
Contributor Author

Shouldn't you refactor functions that interact with these tables?

check it out now, i ran into some more issues and fixed.. check description

supabase/migrations/20230923153903_new_permit.sql Outdated Show resolved Hide resolved
src/helpers/permit.ts Show resolved Hide resolved
Copy link
Member

@0x4007 0x4007 left a comment

Choose a reason for hiding this comment

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

Code looks good just have some rename requests

@seprintour
Copy link
Contributor Author

Code looks good just have some rename requests

Fixed issues

@seprintour
Copy link
Contributor Author

Ping @pavlovcik @whilefoo

Copy link
Collaborator

@whilefoo whilefoo left a comment

Choose a reason for hiding this comment

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

I'd rather not use char(x) because we've had some problems in the past related to that because either Supabase or PostgREST library wasn't working correctly, so I would just use TEXT with CHECK (char_length(name) <= x)

Even if we ignore the bug with char(x), people are mostly recommending to use text because char and varchar have some limits

@seprintour
Copy link
Contributor Author

seprintour commented Oct 3, 2023

Even if we ignore the bug with char(x), people are mostly recommending to use text because char and varchar have some limits

@pavlovcik what's your thought on this, and did the schema change again?

I'm seeing some changes on the parent issue

@0x4007
Copy link
Member

0x4007 commented Oct 3, 2023

To be honest I spent a significant amount of time trying to completely redo the database and functions that interact with it. I seem to have underestimated the amount of work involved but it forces me to become intimately familiar with all of these technologies and concepts within our bot.

It might be more productive to focus efforts elsewhere this week as I continue my research

Copy link
Contributor

@0xcodercrane 0xcodercrane left a comment

Choose a reason for hiding this comment

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

The EVM address we're currently using has a fixed size 42. so no need to set varchar(42) type for them

supabase/migrations/20230923153903_new_permit.sql Outdated Show resolved Hide resolved
@seprintour
Copy link
Contributor Author

@0xcodercrane @whilefoo i made it a text with a length constraint like agreed

@whilefoo
Copy link
Collaborator

@seprintour can you make new QA issue to make sure everything is working?

@seprintour
Copy link
Contributor Author

@seprintour can you make new QA issue to make sure everything is working?

Sure

@ubiquibot ubiquibot bot mentioned this pull request Feb 19, 2024
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.

Permits Table Schema
4 participants