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

Feature: Update the Query Owner #6801

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

jredblue
Copy link

@jredblue jredblue commented Mar 6, 2024

What type of PR is this?

  • Refactor
  • Feature
  • Bug Fix
  • New Query Runner (Data Source)
  • New Alert Destination
  • Other

Description

How is this tested?

  • Unit tests (pytest, jest)
  • E2E Tests (Cypress)
  • Manually
  • N/A

Related Tickets & Documents

Mobile & Desktop Screenshots/Recordings (if there are UI changes)

Shows the process for updating the owner of a query. The previous owner will get added to the editor permission list after the update is complete.

Redash_Update_Query_Owner.mp4

Shows the permissions restriction that requires the new query owner to have access to the query's data source.

Redash_Query_Owner_Update_2.mp4

@jredblue jredblue changed the title Feature: Update the Owner of a Feature: Update the Query Owner Mar 6, 2024
Copy link

codecov bot commented Mar 6, 2024

Codecov Report

Attention: Patch coverage is 7.69231% with 12 lines in your changes are missing coverage. Please review.

Project coverage is 63.37%. Comparing base (34723e2) to head (444bfd1).

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #6801      +/-   ##
==========================================
- Coverage   63.43%   63.37%   -0.06%     
==========================================
  Files         163      163              
  Lines       13200    13213      +13     
  Branches     1822     1825       +3     
==========================================
+ Hits         8373     8374       +1     
- Misses       4530     4540      +10     
- Partials      297      299       +2     
Files Coverage Δ
redash/handlers/queries.py 81.00% <7.69%> (-5.10%) ⬇️

Copy link
Contributor

@wlach wlach left a comment

Choose a reason for hiding this comment

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

Hey @jredblue, thanks so much for submitting this. On the face of it, it looks pretty well organized and thought out. I'm not feeling confident enough in my knowledge of the redash source to give it a definitive 👍 , but I thought I'd give some preliminary feedback on some things that seem missing.

First, we'd absolutely want some tests for the backend endpoint logic here:

class TestQueryResourcePost(BaseTestCase):

Also ideally (maybe required?) we'd need a cypress test for the frontend, might be able to base it off of something existing:

describe("Create Query", () => {

You may have seen this already, but there's some docs on testing changes here: https://github.com/getredash/redash/wiki/Testing-your-changes

if "data_source_id" in query_def:
data_source = models.DataSource.get_by_id_and_org(query_def["data_source_id"], self.current_org)
require_access(data_source, new_user, not_view_only)
else:
Copy link
Contributor

Choose a reason for hiding this comment

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

As the codecov warnings imply, this new functionality really needs a test.

@justinclift
Copy link
Member

Hmmm, the frontend lint test is failing. @jredblue Are you ok to investigate that and fix whatever is wrong with it?

The wiki docs might be useful, depending on how you've setup your development environment:

😄

@jredblue
Copy link
Author

jredblue commented Apr 1, 2024 via email

[author.id]
);

// useEffect(() => {
Copy link
Member

Choose a reason for hiding this comment

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

Should this be in here? 😄

Copy link
Author

@jredblue jredblue Apr 3, 2024

Choose a reason for hiding this comment

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

Oh good catch, I commented this out when working on this as I wasn't sure what exactly it was there for. I modeled this index file after the PermissionsEditorDialog index file, in which the loadOwner function is akin to loadUsersWithPermission function. It used the same useEffect statement, so I could add this back.

Do you know why this code block was necessary for the PermissionsEditorDialog component by chance?

Copy link
Member

Choose a reason for hiding this comment

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

Do you know why this code block was necessary for the PermissionsEditorDialog component by chance?

Unfortunately no. At least "not yet" as I still need to get around to learning React properly. Am more of a backend guy than frontend dev at this stage. 😉

Someone else might know though. 😄

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.

3 participants