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

Use schemas RPC APIs in the front end on the database page #3648

Merged
merged 4 commits into from
Jul 9, 2024

Conversation

seancolsen
Copy link
Contributor

@seancolsen seancolsen commented Jul 1, 2024

Notes

This PR implements the front end side of our RPC refactor for the following REST endpoints:

Endpoint HTTP Method Function
/api/db/v0/schemas/{schemaId}/ DELETE schemas.delete
/api/db/v0/schemas/{schemaId}/ PATCH schemas.patch
/api/db/v0/schemas/ GET schemas.list
/api/db/v0/schemas/ POST schemas.add

Working functionality

  • Everything on the database page including listing schemas, adding schemas, editing schemas, and deleting schemas

Notable user-facing changes

  • Exploration counts are removed from each schema card

    Before After
    image image

    We discussed this previously and made this choice in order to simplify implementation.

Broken functionality

Basically every other page of Mathesar is broken except for the root page which shows connections.

For example, the schema page looks like this:

Checklist

  • My pull request has a descriptive title (not a vague title like Update index.md).
  • My pull request targets the develop branch of the repository
  • My commit messages follow best practices.
  • My code follows the established code style of the repository.
  • I added tests for the changes I made (if applicable).
  • I added or updated documentation (if applicable).
  • I tried running the project locally and verified that there are no visible errors.

Developer Certificate of Origin

Developer Certificate of Origin
Developer Certificate of Origin
Version 1.1

Copyright (C) 2004, 2006 The Linux Foundation and its contributors.
1 Letterman Drive
Suite D4700
San Francisco, CA, 94129

Everyone is permitted to copy and distribute verbatim copies of this
license document, but changing it is not allowed.


Developer's Certificate of Origin 1.1

By making a contribution to this project, I certify that:

(a) The contribution was created in whole or in part by me and I
    have the right to submit it under the open source license
    indicated in the file; or

(b) The contribution is based upon previous work that, to the best
    of my knowledge, is covered under an appropriate open source
    license and I have the right under that license to submit that
    work with modifications, whether created in whole or in part
    by me, under the same open source license (unless I am
    permitted to submit under a different license), as indicated
    in the file; or

(c) The contribution was provided directly to me by some other
    person who certified (a), (b) or (c) and I have not modified
    it.

(d) I understand and agree that this project and the contribution
    are public and that a record of the contribution (including all
    personal information I submit with it, including my sign-off) is
    maintained indefinitely and may be redistributed consistent with
    this project or the open source license(s) involved.

@seancolsen seancolsen added this to the Beta milestone Jul 1, 2024
@seancolsen seancolsen added the pr-status: review A PR awaiting review label Jul 1, 2024
@seancolsen
Copy link
Contributor Author

Heads up that I just realized that common_data is still broken here. The schemas load when the page is loaded client-side, but not when it's loaded fully from the server. I think I'd like to handle that in a separate PR.

@seancolsen
Copy link
Contributor Author

Thanks @Anish9901, that works.

@seancolsen seancolsen changed the title Use schemas RPC APIs in the front end Use schemas RPC APIs in the front end on the database page Jul 3, 2024
@seancolsen seancolsen removed the pr-status: review A PR awaiting review label Jul 3, 2024
@seancolsen seancolsen marked this pull request as draft July 3, 2024 13:33
@seancolsen

This comment was marked as resolved.

@seancolsen

This comment was marked as resolved.

@seancolsen seancolsen marked this pull request as ready for review July 3, 2024 13:40
@seancolsen seancolsen added the pr-status: review A PR awaiting review label Jul 3, 2024
Copy link
Member

@pavish pavish left a comment

Choose a reason for hiding this comment

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

@seancolsen The frontend changes look good. However, I'm facing the following issues while testing:

  • When I edit a schema name, the request succeeds, but the schema name doesn't get modified in the underlying db.
  • Request to delete a schema fails.

There are a number of other commits in the PR unrelated to schemas (eg., from 7c31e76 to 6d17171, and more). I assume it's because the base branch isn't upto date with develop and this branch is. Can we update the base branch so that the PR is easier for me to look commit by commit?

@pavish pavish assigned seancolsen and unassigned pavish Jul 8, 2024
@pavish pavish added pr-status: revision A PR awaiting follow-up work from its author after review and removed pr-status: review A PR awaiting review labels Jul 8, 2024
@seancolsen
Copy link
Contributor Author

seancolsen commented Jul 9, 2024

Ok @pavish I think you actually ran into three problems here...

  1. I accidentally merged develop into this PR branch instead of merging the base branch in. That's why you were seeing unrelated commits. I've fixed that now. Sorry.

  2. Your QA actually caught a real bug which I can't believe evaded my own QA! Thank you! I've opened Cast OIDs to bigint before putting in json #3666 to fix it. It would be good for you to read over that PR to understand it because I think your PR Implement RPC endpoint for listing roles in server #3663 actually needs the same treatment. I think you'll understand what's going on in this PR when you read over that one. And I think we'll need to merge that PR into develop, then merge develop into rpc_frontend, and then merge rpc_frontend into schemas_rpc_fe before you can actually test the fix out in this PR. Sheesh!

  3. That bug evaded my QA process because, although I tested that my SQL function actually modified schemas, I failed to subject the UI to the same level of rigor. Instead I think I must have just seen that the API request succeeded and that the schema name was modified in the front end, and then incorrectly assumed that it had actually been modified in the DB. So that leads me to another problem — why did the API request succeed when the DB function didn't do what it was supposed to do?? To address that problem, I've added an agenda item to our Technical Beta agenda so we can discuss improvements to our patterns in order to avoid this sort of "false success" behavior.

@seancolsen
Copy link
Contributor Author

I'll keep this labeled as pr-status: revision and assigned to me until #3666 is merged at which point I'll pass it along.

@seancolsen seancolsen assigned pavish and unassigned seancolsen Jul 9, 2024
@seancolsen seancolsen added pr-status: review A PR awaiting review and removed pr-status: revision A PR awaiting follow-up work from its author after review labels Jul 9, 2024
@seancolsen
Copy link
Contributor Author

Ok @pavish this is ready for re-review now.

Copy link
Member

@pavish pavish left a 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! Thanks for the fix in #3667 as well!

@pavish pavish merged commit ec6619a into rpc_frontend Jul 9, 2024
35 checks passed
@pavish pavish deleted the schemas_rpc_fe branch July 9, 2024 14:08
@kgodey kgodey modified the milestones: Beta, Pre-beta test build #1 Sep 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pr-status: review A PR awaiting review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants