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
Use connection ids instead of names #3341
Conversation
@mathemancer or @Anish9901 I'd like your help to determine whether additional backend changes are needed here. I had to modify some of the Django routing, but I was unclear about some of the surrounding code. See this code. Before this PR, we had the connection name at that scope. Now we have the connection id at that scope instead. Some special logic seems to be necessary for the live demo, but I didn't take the time to fully understand it. I'd like a backend engineer to look over this to be see what other changes are necessary to use connection IDs in our routes instead of connection names. |
No additional backend changes should be necessary @seancolsen, you're gtg! |
Thanks @Anish9901! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@seancolsen Looks good to me!
I added a commit a3c030f to fix formatting errors that were identified after the recent prettier fix in develop.
else: | ||
# TODO: refactor connection names to ids |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I do not think this will break the demo setup. I'd like @mathemancer to take a look and confirm once, post merge.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@seancolsen I hope you'll followup regarding this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Based on Anish's review, I'm assuming we're good to go and don't need @mathemancer to review. I'll follow up with a PR that removes that code comment.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I removed the code comment in #3354.
Related to #3328
This PR:
Fixes a bug where the user is unable to create new schemas.
This bug stems from filter and create schemas using connection_id #3349 where we made some changes to the schemas API when listing schemas and creating a new schema.
In
develop
, creating a new schema is currently broken due to breaking API changes that we failed to handle. Whoops!This PR fixes that bug in 9feaca3 by changing the frontend code to follow the modified API.
Anish made the other API changes in a backwards-compatible manner, so they do not affect functionality in
develop
.Changes the route for the Database Page and all child pages
/db/<connection_nickname>/
/db/<connection_id>/
Modifies the data structures used for the front end stores to reference connections by their ids instead of by their names. The purpose of this change is to lay groundwork for Allow editing a connection nickname #3333.
Checklist
Update index.md
).develop
branch of the repositoryDeveloper Certificate of Origin
Developer Certificate of Origin