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

Allow getCells to populate Connections #1791

Closed
wants to merge 12 commits into from
Closed

Conversation

davidfig
Copy link
Collaborator

@davidfig davidfig commented Aug 24, 2024

  • replace {{x, y, sheetName?}} in connection query with the value at that location
  • show an error if the sheet name does not exist
  • update cells_accessed with all get cell calls
  • allow inserting cells with a button in the connection editor
  • support relative references
  • show cursor when code editor loses focus (not perfect yet)
  • cell accessed highlighting

Copy link

qa-wolf bot commented Aug 24, 2024

QA Wolf here! As you write new code it's important that your test coverage is keeping up.
Click here to request test coverage for this PR!

@cla-bot cla-bot bot added the cla-signed label Aug 24, 2024
Copy link

vercel bot commented Aug 24, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated (UTC)
quadratic ✅ Ready (Inspect) Visit Preview Sep 17, 2024 3:53pm

@davidfig davidfig added the prototype exploring an idea that can be played with label Aug 24, 2024
@davidkircos davidkircos temporarily deployed to quadratic-api-dev-pr-1791 August 24, 2024 15:06 Inactive
@davidkircos davidkircos temporarily deployed to quadratic-api-dev-pr-1791 August 24, 2024 16:01 Inactive
Copy link

codecov bot commented Aug 24, 2024

Codecov Report

Attention: Patch coverage is 98.40637% with 4 lines in your changes missing coverage. Please review.

Project coverage is 90.99%. Comparing base (46ac83c) to head (c901dac).
Report is 539 commits behind head on qa.

Files with missing lines Patch % Lines
...rc/controller/execution/run_code/run_connection.rs 98.72% 3 Missing ⚠️
quadratic-core/src/error_run.rs 0.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##               qa    #1791      +/-   ##
==========================================
+ Coverage   90.95%   90.99%   +0.04%     
==========================================
  Files         220      220              
  Lines       46068    46308     +240     
==========================================
+ Hits        41902    42139     +237     
- Misses       4166     4169       +3     

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

@davidkircos davidkircos temporarily deployed to quadratic-api-dev-pr-1791 August 25, 2024 12:34 Inactive
@davidfig davidfig linked an issue Aug 25, 2024 that may be closed by this pull request
@davidfig davidfig added status: ready for code review status: ready for user test and removed prototype exploring an idea that can be played with labels Aug 26, 2024
@luke-quadratic
Copy link
Contributor

This is rly awesome - no bugs found running through basic tests of feature

@davidkircos davidkircos temporarily deployed to quadratic-api-dev-pr-1791 August 28, 2024 11:09 Inactive
@davidkircos davidkircos temporarily deployed to quadratic-api-dev-pr-1791 September 27, 2024 03:10 Inactive
@davidkircos
Copy link
Collaborator

@davidfig is this safe from SQL injection?

@davidfig
Copy link
Collaborator Author

No, not yet. It's a much worse experience b/c the library requires that we know the types.

For our current implementation and permissions, any user who has access to the SQL doesn't need injection since they already have full ability to change the SQL. In the future, when we allow users to run pre-existing SQL but not change the SQL (but they can change values that feed into the SQL), then we will need to be much better about injection protections.

Copy link
Collaborator

@davidkircos davidkircos left a comment

Choose a reason for hiding this comment

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

@davidfig looks great to me! Fix merge conflicts and it's good to go

@davidkircos
Copy link
Collaborator

Blocked by waiting for switch to A1

@davidfig davidfig closed this Oct 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add getCells to connection cells
4 participants