diff --git a/plugins/notification_templates_plugin/__init__.py b/plugins/notification_templates_plugin/__init__.py new file mode 100644 index 000000000..e69de29bb diff --git a/plugins/query_review_handler_plugin/__init__.py b/plugins/query_review_handler_plugin/__init__.py new file mode 100644 index 000000000..940c88d28 --- /dev/null +++ b/plugins/query_review_handler_plugin/__init__.py @@ -0,0 +1 @@ +PLUGIN_QUERY_REVIEW_HANDLER = None diff --git a/querybook/config/querybook_public_config.yaml b/querybook/config/querybook_public_config.yaml index 87281cbcb..925cb1da0 100644 --- a/querybook/config/querybook_public_config.yaml +++ b/querybook/config/querybook_public_config.yaml @@ -44,3 +44,32 @@ table_sampling: github_integration: enabled: false + +peer_review: + enabled: true + request_texts: # When users request reviews + description: | + This peer review system helps ensure query quality and data safety. + + Note: Queries that fail after approval will require a new review request. + + Important Guidelines: + • Choose reviewers familiar with the affected data + • Include relevant references in query title or justification + • Document query purpose and impact + guide_link: 'https://www.querybook.org/' + tip: | + Before Submitting: + • Run query to verify syntax + • Validate table names and fields + • Check query performance + reviewer_texts: # When reviewers take actions + approve_message: | + As a reviewer, you are responsible for validating: + + • Query purpose and necessity + • Potential data impact + • Compliance with data policies + + Request clarification if more context is needed. + Approval cannot be reversed. diff --git a/querybook/migrations/versions/d3582302cf48_add_query_review_table.py b/querybook/migrations/versions/d3582302cf48_add_query_review_table.py new file mode 100644 index 000000000..87c606aef --- /dev/null +++ b/querybook/migrations/versions/d3582302cf48_add_query_review_table.py @@ -0,0 +1,132 @@ +"""Add Query Review table + +Revision ID: d3582302cf48 +Revises: aa328ae9dced +Create Date: 2024-12-17 22:41:28.700769 + +""" + +from alembic import op +import sqlalchemy as sa + +# revision identifiers, used by Alembic. +revision = "d3582302cf48" +down_revision = "aa328ae9dced" +branch_labels = None +depends_on = None + +# Define the old and new QueryExecutionStatus enum types for non-PostgreSQL databases +old_status_enum = sa.Enum( + "INITIALIZED", + "DELIVERED", + "RUNNING", + "DONE", + "ERROR", + "CANCEL", + name="queryexecutionstatus", +) + +new_status_enum = sa.Enum( + "INITIALIZED", + "DELIVERED", + "RUNNING", + "DONE", + "ERROR", + "CANCEL", + "PENDING_REVIEW", + "REJECTED", + name="queryexecutionstatus", +) + + +def upgrade(): + # ### commands auto generated by Alembic - please adjust! ### + + # ### Enum Modifications ### + bind = op.get_bind() + dialect = bind.dialect.name + + if dialect == "postgresql": + # PostgreSQL: Add new enum values to existing 'queryexecutionstatus' enum + op.execute("ALTER TYPE queryexecutionstatus ADD VALUE 'PENDING_REVIEW'") + op.execute("ALTER TYPE queryexecutionstatus ADD VALUE 'REJECTED'") + else: + # Other Databases (e.g., MySQL, SQLite): Alter 'query_execution.status' column to use the new enum + op.alter_column( + "query_execution", + "status", + existing_type=old_status_enum, + type_=new_status_enum, + existing_nullable=True, + postgresql_using=None, + ) + + # ### Table Creation ### + op.create_table( + "query_review", + sa.Column("id", sa.Integer(), autoincrement=True, nullable=False), + sa.Column("query_execution_id", sa.Integer(), nullable=False), + sa.Column("reviewed_by", sa.Integer(), nullable=True), + sa.Column("request_reason", sa.String(length=5000), nullable=False), + sa.Column("rejection_reason", sa.String(length=5000), nullable=True), + sa.Column("created_at", sa.DateTime(), nullable=False), + sa.Column("updated_at", sa.DateTime(), nullable=False), + sa.ForeignKeyConstraint( + ["query_execution_id"], ["query_execution.id"], ondelete="CASCADE" + ), + sa.ForeignKeyConstraint(["reviewed_by"], ["user.id"], ondelete="SET NULL"), + sa.PrimaryKeyConstraint("id"), + sa.UniqueConstraint("query_execution_id"), + mysql_charset="utf8mb4", + mysql_engine="InnoDB", + ) + op.create_table( + "query_execution_reviewer", + sa.Column("id", sa.Integer(), autoincrement=True, nullable=False), + sa.Column("query_review_id", sa.Integer(), nullable=False), + sa.Column("uid", sa.Integer(), nullable=False), + sa.Column("created_at", sa.DateTime(), nullable=False), + sa.ForeignKeyConstraint( + ["query_review_id"], ["query_review.id"], ondelete="CASCADE" + ), + sa.ForeignKeyConstraint(["uid"], ["user.id"], ondelete="CASCADE"), + sa.PrimaryKeyConstraint("id"), + sa.UniqueConstraint( + "query_review_id", "uid", name="unique_query_execution_reviewer" + ), + mysql_charset="utf8mb4", + mysql_engine="InnoDB", + ) + # ### end Alembic commands ### + + +def downgrade(): + # ### commands auto generated by Alembic - please adjust! ### + + # ### Table Dropping ### + op.drop_table("query_execution_reviewer") + op.drop_table("query_review") + + # ### Enum Modifications ### + bind = op.get_bind() + dialect = bind.dialect.name + + if dialect == "postgresql": + # PostgreSQL: does not support removing enum values directly + op.execute("ALTER TYPE queryexecutionstatus RENAME TO queryexecutionstatus_old") + old_status_enum.create(bind, checkfirst=True) + op.execute( + "ALTER TABLE query_execution ALTER COLUMN status TYPE queryexecutionstatus USING status::text::queryexecutionstatus" + ) + op.execute("DROP TYPE queryexecutionstatus_old") + else: + # Other Databases (e.g., MySQL, SQLite): Revert 'query_execution.status' column to the old enum + op.alter_column( + "query_execution", + "status", + existing_type=new_status_enum, + type_=old_status_enum, + existing_nullable=True, + postgresql_using=None, + ) + # ### end Alembic commands ### diff --git a/querybook/notification_templates/query_review_approved.md b/querybook/notification_templates/query_review_approved.md new file mode 100644 index 000000000..2f435c615 --- /dev/null +++ b/querybook/notification_templates/query_review_approved.md @@ -0,0 +1,3 @@ +Your query review has been approved by {{ reviewer_name }} on {{ approved_at }}. + +**Here is the url to view the running query: <{{ query_execution_url }}>** diff --git a/querybook/notification_templates/query_review_rejected.md b/querybook/notification_templates/query_review_rejected.md new file mode 100644 index 000000000..bccc870cb --- /dev/null +++ b/querybook/notification_templates/query_review_rejected.md @@ -0,0 +1,8 @@ +Your query review (Query ID: {{ query_execution_id }}) has been rejected by {{ reviewer_name }} on {{ rejected_at }}. + +**Rejection reason provided:** +{{ rejection_reason }} + +**Here is the url to view the query: <{{ query_execution_url }}>** + +Please address the feedback and submit a new review request once changes are made. diff --git a/querybook/notification_templates/query_review_request.md b/querybook/notification_templates/query_review_request.md new file mode 100644 index 000000000..8b3898e6b --- /dev/null +++ b/querybook/notification_templates/query_review_request.md @@ -0,0 +1,9 @@ +New Query Review Required: Request from {{ requested_by }} + +Query review requested on {{ requested_at }} + +**Request Reason:** {{ review_request_reason }} + +**Here is the url for review: <{{ query_execution_url }}>** + +Please review the query at your earliest convenience to ensure it meets standards and requirements. diff --git a/querybook/server/const/query_execution.py b/querybook/server/const/query_execution.py index d734dfcf6..d00269b2b 100644 --- a/querybook/server/const/query_execution.py +++ b/querybook/server/const/query_execution.py @@ -1,4 +1,5 @@ from enum import Enum +from typing import List, TypedDict # Keep these the same as const/queryExecution.ts @@ -15,6 +16,8 @@ class QueryExecutionStatus(Enum): DONE = 3 ERROR = 4 CANCEL = 5 + PENDING_REVIEW = 6 + REJECTED = 7 class StatementExecutionStatus(Enum): @@ -45,4 +48,9 @@ class QueryEngineStatus(Enum): ERROR = 3 # Executor is down, queries cannot be issued +class PeerReviewParamsDict(TypedDict): + reviewer_ids: List[int] + request_reason: str + + QUERY_EXECUTION_NAMESPACE = "/query_execution" diff --git a/querybook/server/datasources/__init__.py b/querybook/server/datasources/__init__.py index bad09b4ef..e41b35317 100644 --- a/querybook/server/datasources/__init__.py +++ b/querybook/server/datasources/__init__.py @@ -19,7 +19,7 @@ from . import survey from . import query_transform from . import github - +from . import query_review # Keep this at the end of imports to make sure the plugin APIs override the default ones try: @@ -50,3 +50,4 @@ query_transform api_plugin github +query_review diff --git a/querybook/server/datasources/query_execution.py b/querybook/server/datasources/query_execution.py index 97ab2cd8d..40e62fb79 100644 --- a/querybook/server/datasources/query_execution.py +++ b/querybook/server/datasources/query_execution.py @@ -1,4 +1,5 @@ from datetime import datetime +from typing import Dict, Optional from flask import abort, Response, redirect, request from flask_login import current_user @@ -27,6 +28,7 @@ INVALID_SEMANTIC_STATUS_CODE, ) from const.query_execution import ( + PeerReviewParamsDict, QueryExecutionExportStatus, QueryExecutionStatus, QUERY_EXECUTION_NAMESPACE, @@ -56,45 +58,110 @@ from app.db import with_session from env import QuerybookSettings from lib.notify.utils import notify_user +from lib.query_review import query_review_handler QUERY_RESULT_LIMIT_CONFIG = get_config_value("query_result_limit") +def process_query_execution_metadata( + query_execution_id, metadata, data_cell_id, session +): + """Process execution metadata and associate query execution with DataDoc.""" + metadata = metadata or {} + + used_api_token = request.headers.get("api-access-token") is not None + if used_api_token: + metadata["used_api_token"] = used_api_token + + if metadata: + logic.create_query_execution_metadata( + query_execution_id, metadata, session=session + ) + + data_doc = None + if data_cell_id: + datadoc_logic.append_query_executions_to_data_cell( + data_cell_id, [query_execution_id], session=session + ) + data_cell = datadoc_logic.get_data_cell_by_id(data_cell_id, session=session) + data_doc = data_cell.doc if data_cell else None + return data_doc + + +def initiate_query_execution(query_execution, uid, peer_review_params, session): + """Initiate the query execution based on peer_review_params.""" + # Initiate peer review workflow + if peer_review_params: + query_review_handler.initiate_query_peer_review_workflow( + query_execution_id=query_execution.id, + uid=uid, + peer_review_params=peer_review_params, + session=session, + ) + # Start immediate execution + else: + run_query_task.apply_async( + args=[ + query_execution.id, + ] + ) + + @register("/query_execution/", methods=["POST"]) def create_query_execution( - query, engine_id, metadata=None, data_cell_id=None, originator=None + query: str, + engine_id: int, + metadata: Optional[Dict] = None, + data_cell_id: Optional[int] = None, + originator: Optional[str] = None, + peer_review_params: Optional[PeerReviewParamsDict] = None, ): + """ + Creates a new QueryExecution. + + Args: + query (str): The SQL query to execute. + engine_id (int): The ID of the query engine to use. + metadata (dict, optional): Additional metadata for the query execution. + data_cell_id (int, optional): ID of the DataDoc cell to associate with. + originator (str, optional): Identifier for the originator of the request. + peer_review_params (dict, optional): Parameters for peer review workflow. + + Returns: + dict: A dictionary representation of the created QueryExecution. + """ with DBSession() as session: verify_query_engine_permission(engine_id, session=session) uid = current_user.id + status = ( + QueryExecutionStatus.PENDING_REVIEW + if peer_review_params + else QueryExecutionStatus.INITIALIZED + ) query_execution = logic.create_query_execution( - query=query, engine_id=engine_id, uid=uid, session=session + query=query, + engine_id=engine_id, + uid=uid, + status=status, + session=session, ) - metadata = metadata or {} - used_api_token = request.headers.get("api-access-token") is not None - if used_api_token: - metadata["used_api_token"] = used_api_token - if metadata: - logic.create_query_execution_metadata( - query_execution.id, metadata, session=session - ) - - data_doc = None - if data_cell_id: - datadoc_logic.append_query_executions_to_data_cell( - data_cell_id, [query_execution.id], session=session - ) - data_cell = datadoc_logic.get_data_cell_by_id(data_cell_id, session=session) - data_doc = data_cell.doc + data_doc = process_query_execution_metadata( + query_execution_id=query_execution.id, + metadata=metadata, + data_cell_id=data_cell_id, + session=session, + ) try: - run_query_task.apply_async( - args=[ - query_execution.id, - ] + initiate_query_execution( + query_execution=query_execution, + uid=uid, + peer_review_params=peer_review_params, + session=session, ) + query_execution_dict = query_execution.to_dict() if data_doc: @@ -130,7 +197,11 @@ def create_query_execution( def get_query_execution(query_execution_id): verify_query_execution_permission(query_execution_id) execution = logic.get_query_execution_by_id(query_execution_id) - execution_dict = execution.to_dict(True) if execution is not None else None + execution_dict = ( + execution.to_dict(with_statement=True, with_query_review=True) + if execution is not None + else None + ) return execution_dict @@ -683,3 +754,25 @@ def transpile_query( ): transpiler = get_transpiler_by_name(transpiler_name) return transpiler.transpile(query, from_language, to_language) + + +@register("/query_execution//approve_review/", methods=["PUT"]) +def approve_query_review(query_execution_id): + verify_query_execution_permission(query_execution_id) + reviewer_id = current_user.id + + query_execution = query_review_handler.approve_review( + query_execution_id, reviewer_id + ) + return query_execution.to_dict(with_statement=False, with_query_review=True) + + +@register("/query_execution//reject_review/", methods=["PUT"]) +def reject_query_review(query_execution_id, rejection_reason): + verify_query_execution_permission(query_execution_id) + reviewer_id = current_user.id + + query_execution = query_review_handler.reject_review( + query_execution_id, reviewer_id, rejection_reason + ) + return query_execution.to_dict(with_statement=False, with_query_review=True) diff --git a/querybook/server/datasources/query_review.py b/querybook/server/datasources/query_review.py new file mode 100644 index 000000000..7d7dee8c5 --- /dev/null +++ b/querybook/server/datasources/query_review.py @@ -0,0 +1,24 @@ +from flask_login import current_user +from app.datasource import register +from lib.logger import get_logger +from logic import query_review as logic + +LOG = get_logger(__file__) + + +@register("/query_review/created_by_me/", methods=["GET"]) +def get_reviews_created_by_me(): + """Get all query reviews where the current user is the creator""" + reviews = logic.get_reviews_created_by_user( + user_id=current_user.id, + ) + return [review.to_dict(with_execution=True) for review in reviews] + + +@register("/query_review/assigned_to_me/", methods=["GET"]) +def get_reviews_assigned_to_me(): + """Get all query reviews where the current user is an assigned reviewer""" + reviews = logic.get_reviews_assigned_to_user( + user_id=current_user.id, + ) + return [review.to_dict(with_execution=True) for review in reviews] diff --git a/querybook/server/lib/notify/utils.py b/querybook/server/lib/notify/utils.py index d2d420010..3e4c5f35a 100644 --- a/querybook/server/lib/notify/utils.py +++ b/querybook/server/lib/notify/utils.py @@ -34,8 +34,14 @@ def notify_user(user, template_name, template_params, notifier_name=None, sessio def render_message(template_name, context): - jinja_env = jinja2.Environment( - loader=jinja2.FileSystemLoader("./querybook/notification_templates/") - ) + """ + Render notification template from core or plugin paths. + Plugin templates in plugins/notification_templates_plugin/ override core templates. + """ + template_paths = [ + "./plugins/notification_templates_plugin/", + "./querybook/notification_templates/", + ] + jinja_env = jinja2.Environment(loader=jinja2.FileSystemLoader(template_paths)) template = jinja_env.get_template(f"{template_name}.md") return template.render(context) diff --git a/querybook/server/lib/query_review/__init__.py b/querybook/server/lib/query_review/__init__.py new file mode 100644 index 000000000..0317eff75 --- /dev/null +++ b/querybook/server/lib/query_review/__init__.py @@ -0,0 +1,3 @@ +from .all_query_review_handlers import get_query_review_handler + +query_review_handler = get_query_review_handler() diff --git a/querybook/server/lib/query_review/all_query_review_handlers.py b/querybook/server/lib/query_review/all_query_review_handlers.py new file mode 100644 index 000000000..ff26d9c92 --- /dev/null +++ b/querybook/server/lib/query_review/all_query_review_handlers.py @@ -0,0 +1,14 @@ +from typing import Optional +from lib.utils.import_helper import import_module_with_default +from lib.query_review.base_query_review_handler import BaseQueryReviewHandler + +PLUGIN_QUERY_REVIEW_HANDLER = import_module_with_default( + "query_review_handler_plugin", + "PLUGIN_QUERY_REVIEW_HANDLER", + default=None, +) + + +def get_query_review_handler() -> Optional[BaseQueryReviewHandler]: + """Returns the configured query review handler or None if no plugin is configured""" + return PLUGIN_QUERY_REVIEW_HANDLER diff --git a/querybook/server/lib/query_review/base_query_review_handler.py b/querybook/server/lib/query_review/base_query_review_handler.py new file mode 100644 index 000000000..f2a6e8f44 --- /dev/null +++ b/querybook/server/lib/query_review/base_query_review_handler.py @@ -0,0 +1,256 @@ +from abc import ABCMeta, abstractmethod +from typing import Any, Dict, Tuple +from app.datasource import api_assert +from app.db import with_session +from const.datasources import ( + INVALID_SEMANTIC_STATUS_CODE, + RESOURCE_NOT_FOUND_STATUS_CODE, +) +from lib.logger import get_logger +from lib.query_review.utils import ( + notify_query_author_of_approval, + notify_query_author_of_rejection, + notify_reviewers_of_new_request, +) +from logic import query_review as logic, query_execution as qe_logic +from const.query_execution import PeerReviewParamsDict, QueryExecutionStatus +from models.query_execution import QueryExecution +from models.query_review import QueryReview +from tasks.run_query import run_query_task + +LOG = get_logger(__file__) + + +class BaseQueryReviewHandler(metaclass=ABCMeta): + """ + Abstract base class for query review handlers. + + Required Overrides: + - _additional_approval_actions: Custom post-approval logic + + Optional Overrides: + - notification_template: Override to customize notification template + - notification_template_params: Override to add custom parameters + """ + + @property + def notification_template(self) -> str: + """Template used for notifications. + Defaults to 'query_review_request'. + Override to customize.""" + return "query_review_request" + + @property + def notification_template_params(self) -> Dict[str, Any]: + """Additional parameters for notification template. + Override to provide custom parameters.""" + return {} + + @abstractmethod + def _additional_approval_actions( + self, query_review_id: int, reviewer_id: int, session=None + ) -> None: + """Custom post-approval actions""" + raise NotImplementedError() + + def initiate_query_peer_review_workflow( + self, + query_execution_id: int, + uid: int, + peer_review_params: PeerReviewParamsDict, + session=None, + ) -> Dict[str, Any]: + """ + Initiates the query peer review workflow. + + Creates a review record and notifies reviewers. + + Args: + query_execution_id (int): ID of the query execution. + uid (int): ID of the user initiating the review. + peer_review_params (PeerReviewParamsDict): Parameters for peer review. + session: Database session. + + Returns: + Dict[str, Any]: Dictionary representation of the created query review. + """ + reviewer_ids = peer_review_params["reviewer_ids"] + request_reason = peer_review_params["request_reason"] + + if uid in reviewer_ids: + api_assert( + False, + "You cannot assign yourself as a reviewer.", + INVALID_SEMANTIC_STATUS_CODE, + ) + + query_review = logic.create_query_review( + query_execution_id=query_execution_id, + request_reason=request_reason, + reviewer_ids=reviewer_ids, + session=session, + ) + + notify_reviewers_of_new_request( + query_review_id=query_review.id, + uid=uid, + template_name=self.notification_template, + additional_params=self.notification_template_params, + session=session, + ) + + return query_review.to_dict() + + def _validate_review( + self, query_execution_id: int, reviewer_id: int, session=None + ) -> Tuple[QueryExecution, QueryReview]: + """ + Validate review request and permissions. + + Performs all necessary validation checks: + 1. Query execution exists + 2. Review exists and is pending + 3. Reviewer has permission + + Args: + query_execution_id (int): ID of the query execution. + reviewer_id (int): ID of the reviewing user. + session: Database session. + + Returns: + Tuple[QueryExecution, QueryReview]: The query execution and review objects. + + Raises: + ValueError: If any validation fails. + """ + query_execution = qe_logic.get_query_execution_by_id( + query_execution_id, session=session + ) + api_assert( + query_execution is not None, + f"QueryExecution with id {query_execution_id} does not exist.", + RESOURCE_NOT_FOUND_STATUS_CODE, + ) + + query_review = query_execution.review + api_assert( + query_review is not None, + "Review not found", + RESOURCE_NOT_FOUND_STATUS_CODE, + ) + + api_assert( + query_execution.status == QueryExecutionStatus.PENDING_REVIEW, + "This review has already been processed.", + INVALID_SEMANTIC_STATUS_CODE, + ) + + api_assert( + reviewer_id in [r.id for r in query_review.assigned_reviewers], + "You are not assigned as a reviewer for this query.", + INVALID_SEMANTIC_STATUS_CODE, + ) + + return query_execution, query_review + + @with_session + def approve_review( + self, query_execution_id: int, reviewer_id: int, session=None + ) -> QueryExecution: + """Approves a review request and initiates query execution.""" + try: + # 1. Validate review + query_execution, query_review = self._validate_review( + query_execution_id, reviewer_id, session + ) + + # 2. Update review status + logic.update_query_review( + query_review_id=query_review.id, + reviewed_by=reviewer_id, + commit=False, + session=session, + ) + + # 3. Update execution status + qe_logic.update_query_execution( + query_execution_id=query_execution.id, + status=QueryExecutionStatus.INITIALIZED, + commit=False, + session=session, + ) + + # 4. Run approval actions + self._additional_approval_actions( + query_review.id, reviewer_id, session=session + ) + + session.commit() + + run_query_task.apply_async(args=[query_execution.id]) + notify_query_author_of_approval( + query_review=query_review, + query_execution=query_execution, + reviewer_id=reviewer_id, + session=session, + ) + + return query_execution + + except Exception as e: + LOG.error(f"approve_review failed: {str(e)}") + session.rollback() + raise + + @with_session + def reject_review( + self, + query_execution_id: int, + reviewer_id: int, + rejection_reason: str, + session=None, + ) -> QueryExecution: + """ + Reject a review request and notifies review author. + + Args: + query_execution_id (int): ID of the query execution. + reviewer_id (int): ID of the reviewing user. + rejection_reason (str): Reason for rejection. + session: Database session. + + Returns: + QueryExecution: The updated query execution object. + """ + query_execution, query_review = self._validate_review( + query_execution_id, reviewer_id, session + ) + + # Update review status + logic.update_query_review( + query_review_id=query_review.id, + reviewed_by=reviewer_id, + rejection_reason=rejection_reason, + commit=False, + session=session, + ) + + # Update execution status + qe_logic.update_query_execution( + query_execution_id=query_execution.id, + status=QueryExecutionStatus.REJECTED, + commit=False, + session=session, + ) + + session.commit() + + notify_query_author_of_rejection( + query_review=query_review, + query_execution=query_execution, + reviewer_id=reviewer_id, + rejection_reason=rejection_reason, + session=session, + ) + + return query_execution diff --git a/querybook/server/lib/query_review/utils.py b/querybook/server/lib/query_review/utils.py new file mode 100644 index 000000000..26f6c9e34 --- /dev/null +++ b/querybook/server/lib/query_review/utils.py @@ -0,0 +1,140 @@ +from typing import Any, Dict, Optional +from datetime import datetime +from app.db import with_session +from env import QuerybookSettings +from lib.notify.utils import notify_user +from logic import query_review as logic, user as user_logic +from logic.query_execution import get_environments_by_execution_id +from models.query_execution import QueryExecution +from models.query_review import QueryReview + + +def _format_timestamp(dt: datetime) -> str: + """Format datetime for notifications. + + Args: + dt: Datetime to format + Returns: + str: Formatted timestamp string + """ + return dt.strftime("%Y-%m-%d %H:%M:%S") + + +def get_query_execution_url(query_execution_id: int, session=None) -> Optional[str]: + """Construct query execution URL. + + Args: + query_execution_id: ID of query execution + session: Database session + Returns: + Optional[str]: Full URL or None if environment not found + """ + execution_envs = get_environments_by_execution_id( + query_execution_id, session=session + ) + if not execution_envs: + return None + + env = execution_envs[0] + return f"{QuerybookSettings.PUBLIC_URL}/{env.name}/query_execution/{query_execution_id}/" + + +@with_session +def notify_reviewers_of_new_request( + query_review_id: int, + uid: int, + template_name: str, + additional_params: Dict[str, Any], + session=None, +) -> None: + """Notify reviewers about new review request.""" + query_review = logic.get_query_review(query_review_id, session=session) + if not query_review: + return + + user = user_logic.get_user_by_id(uid, session=session) + requested_by = user.get_name() if user else "User" + + execution_url = get_query_execution_url( + query_execution_id=query_review.query_execution_id, + session=session, + ) + + template_params = { + "requested_by": requested_by, + "requested_at": _format_timestamp(query_review.created_at), + "query_execution_url": execution_url, + "review_request_reason": query_review.request_reason, + **additional_params, + } + + for reviewer in query_review.assigned_reviewers: + notify_user( + user=reviewer, + template_name=template_name, + template_params=template_params, + session=session, + ) + + +@with_session +def notify_query_author_of_rejection( + query_review: QueryReview, + query_execution: QueryExecution, + rejection_reason: str, + reviewer_id: int, + session=None, +) -> None: + """Notify query author about rejection.""" + reviewer = user_logic.get_user_by_id(reviewer_id, session=session) + reviewer_name = reviewer.get_name() if reviewer else "Reviewer" + + execution_url = get_query_execution_url( + query_execution_id=query_execution.id, + session=session, + ) + + template_params = { + "reviewer_name": reviewer_name, + "query_execution_url": execution_url, + "rejection_reason": rejection_reason, + "query_execution_id": query_execution.id, + "rejected_at": _format_timestamp(query_review.updated_at), + } + + notify_user( + user=query_execution.owner, + template_name="query_review_rejected", + template_params=template_params, + session=session, + ) + + +@with_session +def notify_query_author_of_approval( + query_review: QueryReview, + query_execution: QueryExecution, + reviewer_id: int, + session=None, +) -> None: + """Notify query author about approval.""" + reviewer = user_logic.get_user_by_id(reviewer_id, session=session) + reviewer_name = reviewer.get_name() if reviewer else "Reviewer" + + execution_url = get_query_execution_url( + query_execution_id=query_execution.id, + session=session, + ) + + template_params = { + "reviewer_name": reviewer_name, + "query_execution_url": execution_url, + "approved_at": _format_timestamp(query_review.updated_at), + } + + notify_user( + user=query_execution.owner, + template_name="query_review_approved", + template_params=template_params, + session=session, + ) diff --git a/querybook/server/logic/query_review.py b/querybook/server/logic/query_review.py new file mode 100644 index 000000000..37429f6f4 --- /dev/null +++ b/querybook/server/logic/query_review.py @@ -0,0 +1,121 @@ +from app.db import with_session +from logic.query_execution import get_query_execution_by_id +from models.query_execution import QueryExecution +from models.query_review import QueryExecutionReviewer, QueryReview +from logic.user import get_user_by_id + + +@with_session +def create_query_review( + query_execution_id: int, + request_reason: str = "", + reviewer_ids: list[int] = None, + commit=True, + session=None, +): + query_execution = get_query_execution_by_id(query_execution_id, session=session) + assert ( + query_execution is not None + ), f"QueryExecution with id {query_execution_id} does not exist." + + query_review = QueryReview.create( + fields={ + "query_execution_id": query_execution_id, + "request_reason": request_reason, + }, + commit=False, + session=session, + ) + + # Add reviewers to the query_review assigned reviewers relationship + for reviewer_id in reviewer_ids: + reviewer = get_user_by_id(reviewer_id, session=session) + if reviewer: + query_review.assigned_reviewers.append(reviewer) + + if commit: + session.commit() + else: + session.flush() + + session.refresh(query_review) + return query_review + + +@with_session +def update_query_review(query_review_id: int, commit=True, session=None, **fields): + query_review = get_query_review(query_review_id, session=session) + + if not query_review: + return + + updated = QueryReview.update( + id=query_review_id, + fields=fields, + skip_if_value_none=True, + commit=False, + session=session, + ) + + if updated: + if commit: + session.commit() + else: + session.flush() + session.refresh(query_review) + + return query_review + + +@with_session +def get_query_review(query_review_id: int, session=None): + return QueryReview.get(id=query_review_id, session=session) + + +@with_session +def get_query_review_from_query_execution_id(query_execution_id: int, session=None): + return QueryReview.get(query_execution_id=query_execution_id, session=session) + + +@with_session +def get_reviews_created_by_user(user_id: int, session=None): + """ + Get all query reviews created by a specific user. + + Args: + user_id: The ID of the user who created the reviews + session: SQLAlchemy session + + Returns: + List[QueryReview]: List of query reviews created by the user, + ordered by creation date descending + """ + return ( + session.query(QueryReview) + .join(QueryExecution) + .filter(QueryExecution.uid == user_id) + .order_by(QueryReview.created_at.desc()) + .all() + ) + + +@with_session +def get_reviews_assigned_to_user(user_id: int, session=None): + """ + Get all query reviews where a specific user is assigned as a reviewer. + + Args: + user_id: The ID of the assigned reviewer + session: SQLAlchemy session + + Returns: + List[QueryReview]: List of query reviews assigned to the user, + ordered by creation date descending + """ + return ( + session.query(QueryReview) + .join(QueryExecutionReviewer) + .filter(QueryExecutionReviewer.uid == user_id) + .order_by(QueryReview.created_at.desc()) + .all() + ) diff --git a/querybook/server/models/__init__.py b/querybook/server/models/__init__.py index 6550df625..a885e7a51 100644 --- a/querybook/server/models/__init__.py +++ b/querybook/server/models/__init__.py @@ -16,3 +16,4 @@ from .comment import * from .survey import * from .github import * +from .query_review import * diff --git a/querybook/server/models/query_execution.py b/querybook/server/models/query_execution.py index 82ceeabc2..9c87c0122 100644 --- a/querybook/server/models/query_execution.py +++ b/querybook/server/models/query_execution.py @@ -63,7 +63,7 @@ class QueryExecution(Base): ) @with_formatted_date - def to_dict(self, with_statement=True): + def to_dict(self, with_statement=True, with_query_review=False): item = { "id": self.id, "task_id": self.task_id, @@ -80,6 +80,9 @@ def to_dict(self, with_statement=True): s.to_dict() for s in self.statement_executions ] + if with_query_review: + item["review"] = self.review.to_dict() if self.review else None + return item diff --git a/querybook/server/models/query_review.py b/querybook/server/models/query_review.py new file mode 100644 index 000000000..ee60d9024 --- /dev/null +++ b/querybook/server/models/query_review.py @@ -0,0 +1,128 @@ +import sqlalchemy as sql +from sqlalchemy.orm import relationship, backref + +from app import db +from const.db import now, description_length +from const.query_execution import QueryExecutionStatus +from lib.sqlalchemy import CRUDMixin +from lib.utils.serialize import with_formatted_date + +Base = db.Base + + +class QueryReview(Base, CRUDMixin): + __tablename__ = "query_review" + __table_args__ = {"mysql_engine": "InnoDB", "mysql_charset": "utf8mb4"} + + id = sql.Column(sql.Integer, primary_key=True, autoincrement=True) + query_execution_id = sql.Column( + sql.Integer, + sql.ForeignKey("query_execution.id", ondelete="CASCADE"), + nullable=False, + unique=True, + ) + reviewed_by = sql.Column( + sql.Integer, + sql.ForeignKey("user.id", ondelete="SET NULL"), + nullable=True, + ) + request_reason = sql.Column(sql.String(length=description_length), nullable=False) + rejection_reason = sql.Column(sql.String(length=description_length), nullable=True) + created_at = sql.Column(sql.DateTime, default=now, nullable=False) + updated_at = sql.Column(sql.DateTime, default=now, onupdate=now, nullable=False) + + # Relationships + query_execution = relationship( + "QueryExecution", + backref=backref( + "review", uselist=False, cascade="all, delete-orphan", passive_deletes=True + ), + ) + reviewer = relationship( + "User", + foreign_keys=[reviewed_by], + backref=backref( + "reviewed_query_reviews", cascade="all, delete-orphan", passive_deletes=True + ), + ) + assigned_reviewers = relationship( + "User", + secondary="query_execution_reviewer", + backref=backref("assigned_query_reviews", cascade="all", passive_deletes=True), + ) + + @with_formatted_date + def to_dict(self, with_execution=False): + data = { + "id": self.id, + "query_execution_id": self.query_execution_id, + "requested_by": self.query_execution.uid if self.query_execution else None, + "reviewed_by": self.reviewed_by, + "status": self.get_status(), + "request_reason": self.request_reason, + "rejection_reason": self.rejection_reason, + "created_at": self.created_at, + "updated_at": self.updated_at, + "reviewer_ids": [reviewer.id for reviewer in self.assigned_reviewers], + } + + if with_execution: + data["execution"] = ( + self.query_execution.to_dict( + with_statement=False, + with_query_review=False, # Prevent circular reference + ) + if self.query_execution + else None + ) + + return data + + def get_status(self): + """ + Determines the current status of the QueryReview based on the associated QueryExecution. + + Returns: + str: One of 'pending', 'rejected', or 'approved'. + """ + if not self.query_execution: + return None + + status = self.query_execution.status + if status == QueryExecutionStatus.PENDING_REVIEW: + return "pending" + elif status == QueryExecutionStatus.REJECTED: + return "rejected" + else: + return "approved" + + +class QueryExecutionReviewer(Base, CRUDMixin): + __tablename__ = "query_execution_reviewer" + __table_args__ = ( + sql.UniqueConstraint( + "query_review_id", "uid", name="unique_query_execution_reviewer" + ), + {"mysql_engine": "InnoDB", "mysql_charset": "utf8mb4"}, + ) + + id = sql.Column(sql.Integer, primary_key=True, autoincrement=True) + query_review_id = sql.Column( + sql.Integer, + sql.ForeignKey("query_review.id", ondelete="CASCADE"), + nullable=False, + ) + uid = sql.Column( + sql.Integer, + sql.ForeignKey("user.id", ondelete="CASCADE"), + nullable=False, + ) + created_at = sql.Column(sql.DateTime, default=now, nullable=False) + + def to_dict(self): + return { + "id": self.id, + "query_review_id": self.query_review_id, + "uid": self.uid, + "created_at": self.created_at, + } diff --git a/querybook/webapp/components/AppAdmin/AdminQueryEngine.tsx b/querybook/webapp/components/AppAdmin/AdminQueryEngine.tsx index 1ddee72ea..446693327 100644 --- a/querybook/webapp/components/AppAdmin/AdminQueryEngine.tsx +++ b/querybook/webapp/components/AppAdmin/AdminQueryEngine.tsx @@ -34,6 +34,7 @@ import { import { AdminDeletedList } from './AdminDeletedList'; import './AdminQueryEngine.scss'; +import { usePeerReview } from 'lib/peer-review/config'; interface IProps { queryEngines: IAdminQueryEngine[]; @@ -262,6 +263,8 @@ export const AdminQueryEngine: React.FunctionComponent = ({ item: IAdminQueryEngine, onChange: (fieldName: string, fieldValue: any) => void ) => { + const { isEnabled: isPeerReviewEnabled } = usePeerReview(); + const updateExecutor = (executor: string) => { onChange('executor', executor); onChange( @@ -437,6 +440,15 @@ export const AdminQueryEngine: React.FunctionComponent = ({ type="toggle" label="(Experimental) Enable Row Limit" /> + + {isPeerReviewEnabled && ( + + )} diff --git a/querybook/webapp/components/DataDocQueryCell/DataDocQueryCell.tsx b/querybook/webapp/components/DataDocQueryCell/DataDocQueryCell.tsx index 707b612bf..b4ea522eb 100644 --- a/querybook/webapp/components/DataDocQueryCell/DataDocQueryCell.tsx +++ b/querybook/webapp/components/DataDocQueryCell/DataDocQueryCell.tsx @@ -15,6 +15,7 @@ import { QueryCellTitle } from 'components/QueryCellTitle/QueryCellTitle'; import { runQuery, transformQuery } from 'components/QueryComposer/RunQuery'; import { BoundQueryEditor } from 'components/QueryEditor/BoundQueryEditor'; import { IQueryEditorHandles } from 'components/QueryEditor/QueryEditor'; +import { QueryPeerReviewModal } from 'components/QueryPeerReviewModal/QueryPeerReviewModal'; import { IQueryRunButtonHandles, QueryEngineSelector, @@ -27,6 +28,7 @@ import { UDFForm } from 'components/UDFForm/UDFForm'; import { ComponentType, ElementType } from 'const/analytics'; import { IDataQueryCellMeta, + IPeerReviewParams, ISamplingTables, TDataDocMetaVariables, } from 'const/datadoc'; @@ -115,6 +117,7 @@ interface IState { tableNamesInQuery: string[]; samplingTables: ISamplingTables; showTableSamplingInfoModal: boolean; + showPeerReviewModal: boolean; transpilerConfig?: { toEngine: IQueryEngine; @@ -144,6 +147,7 @@ class DataDocQueryCellComponent extends React.PureComponent { tableNamesInQuery: [], samplingTables: {}, showTableSamplingInfoModal: false, + showPeerReviewModal: false, }; } @@ -443,29 +447,59 @@ class DataDocQueryCellComponent extends React.PureComponent { } @bind - public async onRunButtonClick() { + public async executeQuery(options: { + element: ElementType; + peerReviewParams?: IPeerReviewParams; + onSuccess?: (queryId: number) => void; + }): Promise { + const { element, peerReviewParams, onSuccess } = options; + trackClick({ component: ComponentType.DATADOC_QUERY_CELL, - element: ElementType.RUN_QUERY_BUTTON, + element, aux: { lintError: this.state.hasLintError, sampleRate: this.sampleRate, }, }); - return runQuery( - await this.getTransformedQuery(), - this.engineId, - async (query, engineId) => { - const queryId = ( - await this.props.createQueryExecution( - query, - engineId, - this.props.cellId, - this.getQueryExecutionMetadata() - ) - ).id; + const transformedQuery = await this.getTransformedQuery(); + const executionMetadata = this.getQueryExecutionMetadata(); + + try { + const queryId = await runQuery( + transformedQuery, + this.engineId, + async (query: string, engineId: number) => { + const queryExecution = + await this.props.createQueryExecution( + query, + engineId, + this.props.cellId, + executionMetadata, + peerReviewParams + ); + return queryExecution.id; + } + ); + + if (onSuccess) { + onSuccess(queryId); + } + return queryId; + } catch (error) { + toast.error('Failed to execute query.'); + console.error('Query Execution Error:', error); + throw error; + } + } + + @bind + public async onRunButtonClick() { + return this.executeQuery({ + element: ElementType.RUN_QUERY_BUTTON, + onSuccess: (queryId: number) => { // Only trigger survey if the query is modified within 5 minutes if (Date.now() - this.state.modifiedAt < 5 * 60 * 1000) { triggerSurvey(SurveySurfaceType.QUERY_AUTHORING, { @@ -473,10 +507,16 @@ class DataDocQueryCellComponent extends React.PureComponent { cell_id: this.props.cellId, }); } + }, + }); + } - return queryId; - } - ); + @bind + public async onPeerReviewSubmit(peerReviewParams: IPeerReviewParams) { + await this.executeQuery({ + element: ElementType.PEER_REVIEW_QUERY_BUTTON, + peerReviewParams, + }); } @bind @@ -562,6 +602,7 @@ class DataDocQueryCellComponent extends React.PureComponent { public getAdditionalDropDownButtonDOM() { const { isEditable, queryEngines, queryTranspilers } = this.props; const queryEngine = this.queryEngine; + const hasPeerReviewFeature = queryEngine?.feature_params?.peer_review; const queryCollapsed = this.queryCollapsed; @@ -636,6 +677,16 @@ class DataDocQueryCellComponent extends React.PureComponent { }); } + if (hasPeerReviewFeature && isEditable) { + additionalButtons.push({ + name: 'Request Query Review', + onClick: this.toggleShowPeerReviewModal, + icon: 'Send', + tooltip: 'Request a peer review for your query', + tooltipPos: 'right', + }); + } + return additionalButtons.length > 0 ? ( { })); } + @bind + public toggleShowPeerReviewModal() { + const { query } = this.props; + if (this.state.showPeerReviewModal === false && query.trim() === '') { + toast.error('Cannot request a review for an empty query.'); + return; + } + this.setState(({ showPeerReviewModal }) => ({ + showPeerReviewModal: !showPeerReviewModal, + })); + } + @bind public toggleShowTableSamplingInfoModal() { this.setState(({ showTableSamplingInfoModal }) => ({ @@ -943,6 +1006,13 @@ class DataDocQueryCellComponent extends React.PureComponent { /> ); + const peerReviewModalDOM = this.state.showPeerReviewModal ? ( + + ) : null; + return ( <> {editorDOM} @@ -951,6 +1021,7 @@ class DataDocQueryCellComponent extends React.PureComponent { {UDFModal} {transpilerModal} {renderTableSamplingInfoDOM} + {peerReviewModalDOM} ); } @@ -1084,8 +1155,18 @@ function mapDispatchToProps(dispatch: Dispatch) { query: string, engineId: number, cellId: number, - metadata: Record - ) => dispatch(createQueryExecution(query, engineId, cellId, metadata)), + metadata: Record, + peerReviewParams?: IPeerReviewParams + ) => + dispatch( + createQueryExecution( + query, + engineId, + cellId, + metadata, + peerReviewParams + ) + ), setTableSidebarId: (id: number) => dispatch(setSidebarTableId(id)), diff --git a/querybook/webapp/components/EnvironmentAppSidebar/EntitySidebar.tsx b/querybook/webapp/components/EnvironmentAppSidebar/EntitySidebar.tsx index 7667a4647..0d6ce3c9f 100644 --- a/querybook/webapp/components/EnvironmentAppSidebar/EntitySidebar.tsx +++ b/querybook/webapp/components/EnvironmentAppSidebar/EntitySidebar.tsx @@ -9,6 +9,7 @@ import { SearchContainer } from 'components/Search/SearchContainer'; import { UserMenu } from 'components/UserMenu/UserMenu'; import { ComponentType, ElementType } from 'const/analytics'; import { trackClick } from 'lib/analytics'; +import { usePeerReview } from 'lib/peer-review/config'; import { queryMetastoresSelector } from 'redux/dataSources/selector'; import { currentEnvironmentSelector } from 'redux/environment/selector'; import { IconButton } from 'ui/Button/IconButton'; @@ -17,6 +18,7 @@ import { Link } from 'ui/Link/Link'; import { Entity } from './types'; import './EntitySidebar.scss'; +import { QueryReviewButton } from 'components/QueryReviewsNavigator/QueryReviewButton'; interface IEntitySidebarProps { selectedEntity: Entity; @@ -27,6 +29,7 @@ export const EntitySidebar: React.FunctionComponent = React.memo(({ selectedEntity, onSelectEntity }) => { const environment = useSelector(currentEnvironmentSelector); const queryMetastores = useSelector(queryMetastoresSelector); + const { isEnabled: isPeerReviewEnabled } = usePeerReview(); return (
@@ -153,6 +156,18 @@ export const EntitySidebar: React.FunctionComponent = }} active={selectedEntity === 'execution'} /> + {isPeerReviewEnabled && ( + { + trackClick({ + component: ComponentType.LEFT_SIDEBAR, + element: ElementType.REVIEWS_BUTTON, + }); + onSelectEntity('review'); + }} + /> + )}
diff --git a/querybook/webapp/components/EnvironmentAppSidebar/EnvironmentAppSidebar.tsx b/querybook/webapp/components/EnvironmentAppSidebar/EnvironmentAppSidebar.tsx index c66e1f594..aeb12612f 100644 --- a/querybook/webapp/components/EnvironmentAppSidebar/EnvironmentAppSidebar.tsx +++ b/querybook/webapp/components/EnvironmentAppSidebar/EnvironmentAppSidebar.tsx @@ -4,6 +4,7 @@ import { useDispatch, useSelector } from 'react-redux'; import { DataDocNavigator } from 'components/DataDocNavigator/DataDocNavigator'; import { DataDocSchemaNavigator } from 'components/DataDocSchemaNavigator/DataDocSchemaNavigator'; +import { QueryReviewsNavigator } from 'components/QueryReviewsNavigator/QueryReviewsNavigator'; import { QuerySnippetNavigator } from 'components/QuerySnippetNavigator/QuerySnippetNavigator'; import { QueryViewNavigator } from 'components/QueryViewNavigator/QueryViewNavigator'; import { useEvent } from 'hooks/useEvent'; @@ -95,6 +96,8 @@ export const EnvironmentAppSidebar: React.FunctionComponent = () => { /> ) : entity === 'execution' ? ( + ) : entity === 'review' ? ( + ) : (
); diff --git a/querybook/webapp/components/EnvironmentAppSidebar/types.ts b/querybook/webapp/components/EnvironmentAppSidebar/types.ts index 82bc8bf5d..92bd55d48 100644 --- a/querybook/webapp/components/EnvironmentAppSidebar/types.ts +++ b/querybook/webapp/components/EnvironmentAppSidebar/types.ts @@ -1 +1 @@ -export type Entity = 'datadoc' | 'table' | 'snippet' | 'execution'; +export type Entity = 'datadoc' | 'table' | 'snippet' | 'execution' | 'review'; diff --git a/querybook/webapp/components/QueryComposer/QueryComposer.tsx b/querybook/webapp/components/QueryComposer/QueryComposer.tsx index 38d930437..3755360cb 100644 --- a/querybook/webapp/components/QueryComposer/QueryComposer.tsx +++ b/querybook/webapp/components/QueryComposer/QueryComposer.tsx @@ -16,6 +16,7 @@ import { DataDocTemplateVarForm } from 'components/DataDocTemplateButton/DataDoc import { detectVariableType } from 'components/DataDocTemplateButton/helpers'; import { BoundQueryEditor } from 'components/QueryEditor/BoundQueryEditor'; import { IQueryEditorHandles } from 'components/QueryEditor/QueryEditor'; +import { QueryPeerReviewModal } from 'components/QueryPeerReviewModal/QueryPeerReviewModal'; import { IQueryRunButtonHandles, QueryRunButton, @@ -29,7 +30,7 @@ import { TemplatedQueryView } from 'components/TemplateQueryView/TemplatedQueryV import { TranspileQueryModal } from 'components/TranspileQueryModal/TranspileQueryModal'; import { UDFForm } from 'components/UDFForm/UDFForm'; import { ComponentType, ElementType } from 'const/analytics'; -import { IDataDocMetaVariable } from 'const/datadoc'; +import { IDataDocMetaVariable, IPeerReviewParams } from 'const/datadoc'; import KeyMap from 'const/keyMap'; import { IDataTable } from 'const/metastore'; import { IQueryEngine } from 'const/queryEngine'; @@ -431,6 +432,9 @@ const QueryComposer: React.FC = () => { const [showTableSamplingInfoModal, setShowTableSamplingInfoModal] = useState(false); + const [showPeerReviewModal, setShowPeerReviewModal] = useState(false); + const hasPeerReviewFeature = engine?.feature_params?.peer_review; + const runButtonRef = useRef(null); const clickOnRunButton = useCallback(() => { if (runButtonRef.current) { @@ -477,9 +481,10 @@ const QueryComposer: React.FC = () => { navigateWithinEnv(`/datadoc/${dataDoc.id}/`); }, [executionId, query, engine.id, templatedVariables]); - const getCurrentSelectedQuery = useCallback(() => { - return queryEditorRef.current?.getSelection?.() ?? query; - }, [queryEditorRef, query]); + const getCurrentSelectedQuery = useCallback( + () => queryEditorRef.current?.getSelection?.() ?? query, + [queryEditorRef, query] + ); const triggerSurvey = useSurveyTrigger(); @@ -494,67 +499,96 @@ const QueryComposer: React.FC = () => { return Object.keys(metadata).length === 0 ? null : metadata; }, [getSampleRate]); - const handleRunQuery = React.useCallback(async () => { - const sampleRate = getSampleRate(); - const samplingTables = getSamplingTables(); - const queryExecutionMetadata = getQueryExecutionMetadata(); + const handleRunQuery = useCallback( + async (options?: { + element?: ElementType; + peerReviewParams?: IPeerReviewParams; + onSuccess?: (queryId: number) => void; + }) => { + const { + element = ElementType.RUN_QUERY_BUTTON, + peerReviewParams, + onSuccess, + } = options ?? {}; + + const sampleRate = getSampleRate(); + const samplingTables = getSamplingTables(); + const queryExecutionMetadata = getQueryExecutionMetadata(); + + trackClick({ + component: ComponentType.ADHOC_QUERY, + element, + aux: { + lintError: hasLintErrors, + sampleRate, + }, + }); - trackClick({ - component: ComponentType.ADHOC_QUERY, - element: ElementType.RUN_QUERY_BUTTON, - aux: { - lintError: hasLintErrors, - sampleRate, - }, - }); - // Throttle to prevent double run - await sleep(250); + // Throttle to prevent double run + await sleep(250); + + const transformedQuery = await transformQuery( + getCurrentSelectedQuery(), + engine.language, + templatedVariables, + engine, + rowLimit, + samplingTables, + sampleRate + ); - const transformedQuery = await transformQuery( - getCurrentSelectedQuery(), - engine.language, - templatedVariables, - engine, - rowLimit, - samplingTables, - sampleRate - ); + const queryId = await runQuery( + transformedQuery, + engine.id, + async (query, engineId) => { + const data = await dispatch( + queryExecutionsAction.createQueryExecution( + query, + engineId, + null, + queryExecutionMetadata, + peerReviewParams + ) + ); + return data.id; + } + ); + + triggerSurvey(SurveySurfaceType.QUERY_AUTHORING, { + query_execution_id: queryId, + }); - const queryId = await runQuery( - transformedQuery, - engine.id, - async (query, engineId) => { - const data = await dispatch( - queryExecutionsAction.createQueryExecution( - query, - engineId, - null, - queryExecutionMetadata - ) - ); - return data.id; + if (queryId != null) { + setExecutionId(queryId); + setResultsCollapsed(false); + onSuccess?.(queryId); } - ); - triggerSurvey(SurveySurfaceType.QUERY_AUTHORING, { - query_execution_id: queryId, - }); - if (queryId != null) { - setExecutionId(queryId); - setResultsCollapsed(false); - } - }, [ - getSampleRate, - getSamplingTables, - getQueryExecutionMetadata, - hasLintErrors, - getCurrentSelectedQuery, - engine, - templatedVariables, - rowLimit, - triggerSurvey, - dispatch, - setExecutionId, - ]); + }, + [ + getSampleRate, + getSamplingTables, + getQueryExecutionMetadata, + hasLintErrors, + getCurrentSelectedQuery, + engine, + templatedVariables, + rowLimit, + triggerSurvey, + dispatch, + setExecutionId, + ] + ); + + const onPeerReviewSubmit = useCallback( + async (peerReviewParams: IPeerReviewParams) => { + setShowPeerReviewModal(false); + await handleRunQuery({ + element: ElementType.PEER_REVIEW_QUERY_BUTTON, + peerReviewParams, + }); + }, + [handleRunQuery, setShowPeerReviewModal] + ); const keyMap = useKeyMap(clickOnRunButton, queryEngines, setEngineId); @@ -767,6 +801,13 @@ const QueryComposer: React.FC = () => { ); + const peerReviewModalDOM = showPeerReviewModal && ( + setShowPeerReviewModal(false)} + /> + ); + const getAdditionalDropDownButtonDOM = () => { const additionalButtons: IListMenuItem[] = [ { @@ -827,6 +868,16 @@ const QueryComposer: React.FC = () => { }); } + if (hasPeerReviewFeature) { + additionalButtons.push({ + name: 'Request Query Review', + onClick: () => setShowPeerReviewModal(true), + icon: 'Send', + tooltip: 'Request a peer review for your query', + tooltipPos: 'right', + }); + } + return ( <> { {templatedModalDOM} {templatedQueryViewModalDOM} + {peerReviewModalDOM} ); }; diff --git a/querybook/webapp/components/QueryPeerReviewModal/QueryPeerReviewModal.scss b/querybook/webapp/components/QueryPeerReviewModal/QueryPeerReviewModal.scss new file mode 100644 index 000000000..59c99b751 --- /dev/null +++ b/querybook/webapp/components/QueryPeerReviewModal/QueryPeerReviewModal.scss @@ -0,0 +1,125 @@ +.QueryPeerReviewModal { + .flex-row { + width: 100%; + + .Message { + width: 100%; + } + } + + .description-section { + margin-bottom: 24px; + + .Message { + padding: 0; + overflow: hidden; + border-radius: var(--border-radius); + } + + .description-content { + display: grid; + grid-template-columns: 1.5fr 1fr; + min-height: 100%; + + h4 { + font-size: var(--med-text-size); + font-weight: var(--bold-font); + color: var(--text-dark); + margin-bottom: 12px; + padding-bottom: 8px; + border-bottom: 1px solid var(--border); + } + + .main-description { + padding: 20px; + background: var(--bg); + + .description-text { + color: var(--text); + font-size: var(--text-size); + line-height: 1.6; + white-space: pre-wrap; + + ul { + list-style-type: none; + padding-left: 0; + margin: 12px 0; + + li { + position: relative; + padding-left: 20px; + margin-bottom: 8px; + + &:before { + content: "•"; + position: absolute; + left: 0; + color: var(--color-accent); + } + } + } + } + } + + .checklist-box { + padding: 20px; + background: var(--color-accent-lightest-0); + border-left: 1px solid var(--border); + + .checklist-content { + color: var(--text); + font-size: var(--text-size); + line-height: 1.6; + + ul { + list-style-type: none; + padding-left: 0; + margin: 12px 0; + + li { + position: relative; + padding-left: 20px; + margin-bottom: 8px; + + &:before { + content: "✓"; + position: absolute; + left: 0; + color: var(--color-true); + } + } + } + } + + .guide-link { + margin-top: 16px; + padding-top: 16px; + border-top: 1px solid var(--border); + + a { + display: inline-flex; + align-items: center; + gap: 6px; + color: var(--color-accent); + font-size: var(--small-text-size); + padding: 6px 12px; + border-radius: var(--border-radius-sm); + transition: background-color 0.2s ease; + + &:hover { + background: var(--color-accent-lightest); + text-decoration: none; + } + } + } + } + } + } + + .modal-footer-buttons { + display: flex; + justify-content: flex-end; + padding: 16px 0 0; + border-top: 1px solid var(--border); + } +} diff --git a/querybook/webapp/components/QueryPeerReviewModal/QueryPeerReviewModal.tsx b/querybook/webapp/components/QueryPeerReviewModal/QueryPeerReviewModal.tsx new file mode 100644 index 000000000..5e67326f3 --- /dev/null +++ b/querybook/webapp/components/QueryPeerReviewModal/QueryPeerReviewModal.tsx @@ -0,0 +1,179 @@ +import { Form, Formik } from 'formik'; +import React, { useCallback } from 'react'; +import { toast } from 'react-hot-toast'; +import * as Yup from 'yup'; + +import { MultiCreatableUserSelect } from 'components/UserSelect/MultiCreatableUserSelect'; +import { IPeerReviewParams } from 'const/datadoc'; +import { AsyncButton } from 'ui/AsyncButton/AsyncButton'; +import { FormField } from 'ui/Form/FormField'; +import { FormWrapper } from 'ui/Form/FormWrapper'; +import { SimpleField } from 'ui/FormikField/SimpleField'; +import { Link } from 'ui/Link/Link'; +import { Message } from 'ui/Message/Message'; +import { Modal } from 'ui/Modal/Modal'; +import { IStandardModalProps } from 'ui/Modal/types'; +import { Icon } from 'ui/Icon/Icon'; + +import './QueryPeerReviewModal.scss'; +import { usePeerReview } from 'lib/peer-review/config'; + +interface IQueryPeerReviewFormProps { + onSubmit: (peerReviewParams: IPeerReviewParams) => Promise; + onHide: () => void; +} + +interface IDescriptionSectionProps { + description: string; + tip: string; + guideLink: string; +} + +const DescriptionSection: React.FC = ({ + description, + tip, + guideLink, +}) => ( +
+ +
+
+

About Peer Review

+
{description}
+
+ +
+

Review Checklist

+
{tip}
+
+ + + View Complete Guidelines + +
+
+
+ +
+); +export const QueryPeerReviewForm: React.FC = ({ + onSubmit, + onHide, +}) => { + const initialValues = { + reviewers: [], + requestReason: '', + }; + + const peerReviewFormSchema = Yup.object().shape({ + reviewers: Yup.array() + .min(1, 'Please select at least one reviewer') + .required('Please select at least one reviewer'), + requestReason: Yup.string() + .trim() + .required('Justification is required'), + }); + + const { + requestTexts: { description, guideLink, reviewTip }, + } = usePeerReview(); + + const handleSubmit = useCallback( + async (values) => { + try { + const reviewerIds = values.reviewers + .filter((v) => 'isUser' in v && v.isUser) + .map((v) => v.value); + + const peerReviewParams = { + reviewer_ids: reviewerIds, + request_reason: values.requestReason, + }; + await onSubmit(peerReviewParams); + + onHide(); + toast.success( + 'Review request sent! Reviewers were notified and your query will run upon approval.', + { duration: 3000 } + ); + } catch (error) { + toast.error('Failed to request review.'); + } + }, + [onHide, onSubmit] + ); + + return ( + + {({ submitForm, isSubmitting, isValid, setFieldValue, values }) => ( + +
+ + + + { + setFieldValue('reviewers', selected); + }} + selectProps={{ + isClearable: true, + placeholder: + 'Select reviewers for the query', + isValidNewOption: () => false, + }} + /> + + + + +
+ +
+ +
+ )} + + ); +}; + +export const QueryPeerReviewModal: React.FC< + IQueryPeerReviewFormProps & IStandardModalProps +> = ({ onSubmit, onHide, ...modalProps }) => ( + + + +); diff --git a/querybook/webapp/components/QueryReviewsNavigator/QueryReviewButton.tsx b/querybook/webapp/components/QueryReviewsNavigator/QueryReviewButton.tsx new file mode 100644 index 000000000..e59c2df4a --- /dev/null +++ b/querybook/webapp/components/QueryReviewsNavigator/QueryReviewButton.tsx @@ -0,0 +1,63 @@ +import React from 'react'; +import { useDispatch } from 'react-redux'; +import { useShallowSelector } from 'hooks/redux/useShallowSelector'; +import { TooltipDirection } from 'const/tooltip'; +import { selectAssignedReviews } from 'redux/queryReview/selector'; +import { fetchAssignedReviews } from 'redux/queryReview/action'; +import { IStoreState } from 'redux/store/types'; +import { IconButton } from 'ui/Button/IconButton'; +import { IQueryReview } from 'const/queryExecution'; + +interface IQueryReviewButtonProps { + tooltipPos?: TooltipDirection; + onClick: () => void; + active?: boolean; +} + +function useAssignedReviews() { + const { reviews } = useShallowSelector((state: IStoreState) => ({ + reviews: selectAssignedReviews(state), + })); + + const pendingReviews = reviews.filter( + (review: IQueryReview) => review.status === 'pending' + ); + + const dispatch = useDispatch(); + React.useEffect(() => { + dispatch(fetchAssignedReviews()); + }, [dispatch]); + + return { + pendingReviews, + }; +} + +export const QueryReviewButton = React.memo( + ({ tooltipPos = 'right', onClick, active }) => { + const { pendingReviews } = useAssignedReviews(); + + const buttonTitle = pendingReviews.length + ? `You have ${pendingReviews.length} pending ${ + pendingReviews.length === 1 ? 'review' : 'reviews' + }` + : 'No pending reviews'; + return ( + + 0 + ? pendingReviews.length.toString() + : null + } + title="Reviews" + /> + + ); + } +); diff --git a/querybook/webapp/components/QueryReviewsNavigator/QueryReviewItem.tsx b/querybook/webapp/components/QueryReviewsNavigator/QueryReviewItem.tsx new file mode 100644 index 000000000..c5a818c7e --- /dev/null +++ b/querybook/webapp/components/QueryReviewsNavigator/QueryReviewItem.tsx @@ -0,0 +1,169 @@ +import React, { useCallback, useMemo, useRef, useState } from 'react'; + +import { UserBadge } from 'components/UserBadge/UserBadge'; +import { IQueryReview } from 'const/queryExecution'; +import { Status } from 'const/queryStatus'; +import history from 'lib/router-history'; +import { generateFormattedDate } from 'lib/utils/datetime'; +import { navigateWithinEnv } from 'lib/utils/query-string'; +import { UrlContextMenu } from 'ui/ContextMenu/UrlContextMenu'; +import { ShowMoreText } from 'ui/ShowMoreText/ShowMoreText'; +import { StatusIcon } from 'ui/StatusIcon/StatusIcon'; +import { AccentText } from 'ui/StyledText/StyledText'; +import { Tag } from 'ui/Tag/Tag'; + +import './QueryReviewsNavigator.scss'; + +type ReviewType = 'myReviews' | 'assigned'; + +interface IQueryReviewItemProps { + review: IQueryReview; + type: ReviewType; +} + +const STATUS_COLOR_MAP: Record = { + pending: Status.warning, + approved: Status.success, + rejected: Status.error, +} as const; + +const ReviewHeader: React.FC<{ review: IQueryReview; statusColor: Status }> = ({ + review, + statusColor, +}) => ( +
+
+ + + Execution {review.query_execution_id} + +
+ + {review.status.charAt(0).toUpperCase() + review.status.slice(1)} + {' '} +
+); + +const AssignedReviewContent: React.FC<{ review: IQueryReview }> = ({ + review, +}) => ( + <> +
+ Requested By: + +
+
+ Request Reason: + + + +
+ +); + +const MyReviewContent: React.FC<{ review: IQueryReview }> = ({ review }) => { + const [showAllReviewers, setShowAllReviewers] = useState(false); + const hasMoreReviewers = review.reviewer_ids.length > 3; + const displayedReviewers = showAllReviewers + ? review.reviewer_ids + : review.reviewer_ids.slice(0, 3); + + return ( +
+ Reviewers: +
+ {displayedReviewers.map((reviewerId) => ( + + ))} + {hasMoreReviewers && ( + { + e.stopPropagation(); + setShowAllReviewers(!showAllReviewers); + }} + > + {showAllReviewers + ? ' show less' + : ` +${review.reviewer_ids.length - 3} more`} + + )} +
+
+ ); +}; + +const ReviewTimestamp: React.FC<{ + review: IQueryReview; + type: ReviewType; +}> = ({ review, type }) => ( + + + {type === 'assigned' ? 'Requested' : 'Created'} at:{' '} + {generateFormattedDate(review.created_at)} + + +); + +export const QueryReviewItem: React.FC = ({ + review, + type, +}) => { + const selfRef = useRef(); + + const queryExecutionUrl = useMemo( + () => `/query_execution/${review.query_execution_id}/`, + [review.query_execution_id] + ); + + const handleClick = useCallback(() => { + navigateWithinEnv(queryExecutionUrl); + }, [queryExecutionUrl]); + + const statusColor: Status = + STATUS_COLOR_MAP[review.status] ?? Status.warning; + + return ( + <> +
+ + +
+ {type === 'assigned' ? ( + + ) : ( + + )} +
+ + +
+ + + ); +}; diff --git a/querybook/webapp/components/QueryReviewsNavigator/QueryReviewsNavigator.scss b/querybook/webapp/components/QueryReviewsNavigator/QueryReviewsNavigator.scss new file mode 100644 index 000000000..d7900a6a1 --- /dev/null +++ b/querybook/webapp/components/QueryReviewsNavigator/QueryReviewsNavigator.scss @@ -0,0 +1,116 @@ +@import '../../scss_variables.scss'; + +.QueryReviewsNavigator { + position: relative; + display: flex; + flex-direction: column; + height: 100%; + + .reviews-header { + border-bottom: var(--border); + background-color: var(--bg-lightest); + padding: 8px 16px; + display: flex; + align-items: center; + + .list-header { + background-color: transparent !important; + margin: 0; + } + + .Tabs { + flex: 1; + ul { + flex: 1; + li { + flex: 1; + } + } + } + } + + .list-content { + overflow-y: auto; + padding: 8px; + + .empty-section-message { + text-align: center; + font-size: var(--med-text-size); + user-select: none; + padding: 16px; + font-weight: bold; + color: var(--text-light); + } + } +} + +.QueryReviewItem { + cursor: pointer; + background: var(--bg-light); + border-radius: var(--border-radius-sm); + padding: 12px; + margin-bottom: var(--margin-xs); + transition: background-color 0.2s ease; + + &:hover { + background-color: var(--bg-hover); + } + + .review-header { + .StatusIcon { + margin-right: 8px; + } + } + + .review-content { + .review-row { + display: flex; + margin-bottom: 8px; + + .label { + color: var(--text-light); + min-width: 100px; + margin-right: 12px; + padding-top: 4px; + } + + .value { + flex: 1; + word-break: break-word; + + &.text-ellipsis { + display: -webkit-box; + -webkit-line-clamp: 2; + line-clamp: 2; + -webkit-box-orient: vertical; + overflow: hidden; + text-overflow: ellipsis; + } + } + + .reviewers-list { + display: flex; + flex-wrap: wrap; + gap: 8px; + align-items: center; + + .UserBadge { + margin-right: 0; + } + + .show-more-reviewers { + color: var(--text-light); + font-size: var(--xsmall-text-size); + cursor: pointer; + padding: 2px 4px; + border-radius: var(--border-radius-sm); + + &:hover { + background: var(--bg-hover); + color: var(--text); + } + } + } + } + } +} diff --git a/querybook/webapp/components/QueryReviewsNavigator/QueryReviewsNavigator.tsx b/querybook/webapp/components/QueryReviewsNavigator/QueryReviewsNavigator.tsx new file mode 100644 index 000000000..c67cca834 --- /dev/null +++ b/querybook/webapp/components/QueryReviewsNavigator/QueryReviewsNavigator.tsx @@ -0,0 +1,129 @@ +import React, { useCallback, useEffect } from 'react'; +import { useDispatch } from 'react-redux'; + +import { useShallowSelector } from 'hooks/redux/useShallowSelector'; +import { + fetchAssignedReviews, + fetchMyReviews, + setActiveTab, +} from 'redux/queryReview/action'; +import { + selectAssignedReviews, + selectLoadingAssignedReviews, + selectLoadingMyReviews, + selectMyReviews, +} from 'redux/queryReview/selector'; +import { IStoreState } from 'redux/store/types'; +import { Icon } from 'ui/Icon/Icon'; +import { AccentText } from 'ui/StyledText/StyledText'; +import { Tabs } from 'ui/Tabs/Tabs'; + +import { QueryReviewItem } from './QueryReviewItem'; + +import './QueryReviewsNavigator.scss'; +import { TooltipDirection } from 'const/tooltip'; + +type TabType = 'myReviews' | 'assigned'; + +const NAVIGATOR_TABS = [ + { + key: 'myReviews' as TabType, + name: 'Pending Reviews', + icon: 'Clock' as const, + tooltip: 'Reviews you have requested', + tooltipPos: 'down' as TooltipDirection, + }, + { + key: 'assigned' as TabType, + name: 'Assigned Reviews', + icon: 'ListOrdered' as const, + tooltip: 'Reviews assigned to you', + tooltipPos: 'down' as TooltipDirection, + }, +]; + +export const QueryReviewsNavigator: React.FC = () => { + const { reviews, isLoading, activeTab } = useShallowSelector( + (state: IStoreState) => { + const tab = state.queryReview.activeTab; + return { + reviews: + tab === 'myReviews' + ? selectMyReviews(state) + : selectAssignedReviews(state), + isLoading: + tab === 'myReviews' + ? selectLoadingMyReviews(state) + : selectLoadingAssignedReviews(state), + activeTab: tab, + }; + } + ); + + const dispatch = useDispatch(); + + useEffect(() => { + if (activeTab === 'myReviews') { + dispatch(fetchMyReviews()); + } else { + dispatch(fetchAssignedReviews()); + } + }, [activeTab, dispatch]); + + const handleTabChange = useCallback( + (key: string) => { + if (key === 'myReviews' || key === 'assigned') { + dispatch(setActiveTab(key as TabType)); + } + }, + [dispatch] + ); + + const loadingDOM = isLoading ? ( +
+ + + Loading Reviews... + +
+ ) : null; + + const noResultDOM = + !isLoading && reviews.length === 0 ? ( +
+ No {activeTab === 'myReviews' ? 'Requested' : 'Assigned'}{' '} + Reviews +
+ ) : null; + + const reviewListDOM = reviews.map((review) => ( + + )); + + const tabsDOM = ( +
+ +
+ ); + + return ( +
+ {tabsDOM} +
+ {isLoading && loadingDOM} + {reviews.length > 0 && reviewListDOM} + {noResultDOM} +
+
+ ); +}; diff --git a/querybook/webapp/components/QueryView/QueryView.tsx b/querybook/webapp/components/QueryView/QueryView.tsx index 1ee6143cb..f69a613d2 100644 --- a/querybook/webapp/components/QueryView/QueryView.tsx +++ b/querybook/webapp/components/QueryView/QueryView.tsx @@ -2,6 +2,8 @@ import React from 'react'; import { useDispatch, useSelector } from 'react-redux'; import { AccessRequestButton } from 'components/AccessRequestButton/AccessRequestButton'; +import { QueryViewReview } from 'components/QueryViewReview/QueryViewReview'; +import { useQueryReview } from 'hooks/useQueryReview'; import { formatError } from 'lib/utils/error'; import * as queryExecutionsActions from 'redux/queryExecutions/action'; import * as queryExecutionsSelector from 'redux/queryExecutions/selector'; @@ -23,6 +25,7 @@ export const QueryView: React.FunctionComponent = ({ queryId }) => { const queryExecution = useSelector((state: IStoreState) => queryExecutionsSelector.queryExecutionSelector(state, queryId) ); + const queryReviewState = useQueryReview(queryExecution?.id); const handleQueryExecutionAccessRequest = React.useCallback(() => { dispatch( @@ -43,7 +46,7 @@ export const QueryView: React.FunctionComponent = ({ queryId }) => { return ( = ({ queryId }) => { item={queryExecution} itemKey={queryId} itemLoader={fetchQueryExecution} - errorRenderer={(error) => errorPage(error)} + errorRenderer={errorPage} > + {queryReviewState.review && ( + + )}
); diff --git a/querybook/webapp/components/QueryViewReview/QueryViewReview.scss b/querybook/webapp/components/QueryViewReview/QueryViewReview.scss new file mode 100644 index 000000000..1b22f58cd --- /dev/null +++ b/querybook/webapp/components/QueryViewReview/QueryViewReview.scss @@ -0,0 +1,196 @@ +.QueryViewReview { + --review-spacing: 16px; + margin: 24px 0; + border: 1px solid var(--border); + border-radius: 8px; + background: var(--bg); + transition: box-shadow 0.2s ease; + + &:hover { + box-shadow: var(--shadow-mid); + } + + .review-header { + padding: var(--review-spacing); + display: flex; + justify-content: space-between; + align-items: center; + background: var(--bg-lightest); + border-bottom: 1px solid var(--border-light); + } + + .header-left { + display: flex; + align-items: center; + gap: 8px; + } + + .review-content { + padding: var(--review-spacing); + + .details-grid { + display: grid; + grid-template-columns: repeat(auto-fit, minmax(250px, 1fr)); + gap: 16px; + } + + .detail-item { + display: flex; + flex-direction: column; + align-items: flex-start; + gap: 8px; + + // Center UserBadge + &:first-child { + align-items: center; + text-align: center; + } + + .request-reason { + width: 100%; + background: var(--bg-light); + padding: 12px; + border-radius: 6px; + line-height: 1.6; + font-size: 15px; + border: 1px solid var(--border-light); + color: var(--text); + } + } + } + + .review-actions { + padding: var(--review-spacing); + border-top: 1px solid var(--border-light); + background: var(--bg-lightest); + + .action-buttons { + display: flex; + justify-content: flex-end; + gap: 12px; + } + } + + .reject-form-container { + padding: var(--review-spacing); + + .reject-form { + display: flex; + flex-direction: column; + gap: 16px; + + textarea { + min-height: 120px; + width: 100%; + resize: vertical; + } + + .form-buttons { + display: flex; + justify-content: flex-end; + gap: 8px; + margin-top: 8px; + } + } + } + + .review-notifier, + .outcome-msg { + margin: 0; + border-radius: 0; + border-top: 1px solid var(--border-light); + + &.error { + background-color: var(--bg-error-light); + border-left: 4px solid var(--color-error); + } + } + + @media (max-width: 768px) { + .details-grid { + grid-template-columns: 1fr; + } + } + + .Modal { + .form-buttons { + margin-top: 16px; + display: flex; + justify-content: flex-end; + gap: 8px; + } + } + + .rejection-details { + .rejection-header { + display: flex; + align-items: center; + gap: 8px; + padding: 8px 12px; + background: var(--bg-lightest); + border-radius: 6px; + margin-bottom: 16px; + margin: 8px 0 16px; + + .reviewer-info { + display: flex; + align-items: center; + gap: 8px; + padding: 12px; + background: var(--bg-lightest); + border-radius: 8px; + } + } + + .rejection-reason { + background: var(--bg); + padding: 12px; + border-radius: 6px; + line-height: 1.6; + font-size: 15px; + white-space: pre-wrap; + word-wrap: break-word; + color: var(--text); + margin: 12px 0; + border: 1px solid var(--border-light); + } + + .rejection-content { + padding: 16px; + background: var(--bg-light); + border-radius: 8px; + border: 1px solid var(--border-light); + transition: background-color 0.2s ease; + + .rejection-reason { + margin: 12px 0; + padding: 12px; + background: var(--bg); + border-radius: 6px; + line-height: 1.6; + font-size: 15px; + white-space: pre-wrap; + word-wrap: break-word; + color: var(--text); + } + + .rejection-note { + color: var(--text-light); + font-style: italic; + } + } + } + + .rejection-title { + display: flex; + align-items: center; + gap: 8px; + font-size: 16px; + color: var(--text); + } +} + +.Button:focus-visible { + outline: 2px solid var(--color-accent); + outline-offset: 2px; +} diff --git a/querybook/webapp/components/QueryViewReview/QueryViewReview.tsx b/querybook/webapp/components/QueryViewReview/QueryViewReview.tsx new file mode 100644 index 000000000..207abd48a --- /dev/null +++ b/querybook/webapp/components/QueryViewReview/QueryViewReview.tsx @@ -0,0 +1,369 @@ +import React, { useState, useCallback, memo } from 'react'; +import { useDispatch } from 'react-redux'; +import toast from 'react-hot-toast'; +import { Form, Formik } from 'formik'; +import * as Yup from 'yup'; + +import { AccentText } from 'ui/StyledText/StyledText'; +import { Card } from 'ui/Card/Card'; +import { Button } from 'ui/Button/Button'; +import { SimpleField } from 'ui/FormikField/SimpleField'; +import { StatusIcon } from 'ui/StatusIcon/StatusIcon'; +import { Tag } from 'ui/Tag/Tag'; +import { Message } from 'ui/Message/Message'; +import { UserBadge } from 'components/UserBadge/UserBadge'; +import { Modal } from 'ui/Modal/Modal'; + +import { IQueryExecution, IQueryReview } from 'const/queryExecution'; +import { Status } from 'const/queryStatus'; +import * as queryExecutionsActions from 'redux/queryExecutions/action'; +import { getStatusProps } from './statusUtils'; +import { generateFormattedDate } from 'lib/utils/datetime'; +import { ConfirmationMessage } from 'components/ConfirmationManager/ConfirmationMessage'; +import { Icon } from 'ui/Icon/Icon'; + +import './QueryViewReview.scss'; +import { QueryReviewState } from 'hooks/useQueryReview'; +import { usePeerReview } from 'lib/peer-review/config'; + +const rejectSchema = Yup.object().shape({ + rejectionReason: Yup.string() + .required('Please provide a reason for rejecting this query') + .min(10, 'Rejection reason must be at least 10 characters'), +}); + +const ReviewHeader: React.FC<{ + status: Status; + tooltip: string; + label: string; + tagClass: string; + tagText: string; +}> = memo(({ status, tooltip, label, tagClass, tagText }) => ( +
+
+ + + {label} + +
+ + {tagText} + +
+)); + +const ReviewContent: React.FC<{ + review: IQueryReview; +}> = memo(({ review }) => ( +
+
+
+ + Requested By + + +
+
+ + Reason Provided by Author + + + {review.request_reason || 'No reason provided'} + +
+
+ + Review Requested At + + + {generateFormattedDate(review.created_at)} + +
+
+
+)); + +const ReviewActions: React.FC<{ + showRejectForm: boolean; + onShowRejectForm: (show: boolean) => void; + onApprove: () => void; + onReject: (values: { rejectionReason: string }) => void; +}> = memo(({ showRejectForm, onShowRejectForm, onApprove, onReject }) => { + const [showApproveModal, setShowApproveModal] = useState(false); + const { reviewerTexts } = usePeerReview(); + + return ( +
+
+
+ + {showApproveModal && ( + setShowApproveModal(false)} + onHide={() => setShowApproveModal(false)} + confirmText="Approve" + cancelText="Cancel" + confirmColor="confirm" + cancelColor="cancel" + confirmIcon="Check" + cancelIcon="X" + /> + )} + + {showRejectForm && ( + onShowRejectForm(false)} + > +
+ + {({ submitForm }) => ( +
+ +
+
+ + )} +
+
+ + )} +
+ ); +}); + +const ReviewStatus: React.FC<{ + isReviewer: boolean; + isPending: boolean; + isApproved: boolean; + isRejected: boolean; + queryReview: IQueryReview; + showRejectForm: boolean; + onShowRejectForm: (show: boolean) => void; + onApprove: () => void; + onReject: (values: { rejectionReason: string }) => void; +}> = memo( + ({ + isReviewer, + isPending, + isApproved, + isRejected, + queryReview, + showRejectForm, + onShowRejectForm, + onApprove, + onReject, + }) => { + if (!isReviewer && isPending) { + return ( + + This execution is currently under review. No further actions + are required. + + ); + } + + if (isReviewer && isPending) { + return ( + + ); + } + + if (isApproved) { + return ( + + The query has been approved and is now executing. + + ); + } + + if (isRejected) { + return ( + + + Query Rejected +
+ } + > +
+
+
+ + + has reviewed and rejected this query + +
+
+
+ + Rejection Reason: + + + {queryReview.rejection_reason} + + + Note: A new query review will need to be + submitted after addressing these changes. + +
+
+ + ); + } + + return null; + } +); + +export const QueryViewReview: React.FC<{ + queryExecution: IQueryExecution; + queryReviewState: QueryReviewState; +}> = memo(({ queryExecution, queryReviewState }) => { + const dispatch = useDispatch(); + const { + permissions: { isReviewer }, + status: { isPending, isRejected, isApproved }, + review: queryReview, + } = queryReviewState; + + const [showRejectForm, setShowRejectForm] = useState(false); + + const handleApprove = useCallback(async () => { + try { + await dispatch( + queryExecutionsActions.approveQueryReview(queryExecution.id) + ); + toast.success('Query approved successfully.'); + } catch { + toast.error('Failed to approve the query.'); + } + }, [dispatch, queryExecution.id]); + + const handleReject = useCallback( + async (values: { rejectionReason: string }) => { + try { + await dispatch( + queryExecutionsActions.rejectQueryReview( + queryExecution.id, + values.rejectionReason.trim() + ) + ); + toast.success('Query rejected successfully.'); + } catch { + toast.error('Failed to reject query.'); + } + }, + [dispatch, queryExecution.id] + ); + + if (!queryReview) { + return null; + } + + const { status, tooltip, tagClass, tagText } = getStatusProps( + isApproved, + isRejected + ); + + return ( + + + + + + + + ); +}); diff --git a/querybook/webapp/components/QueryViewReview/statusUtils.ts b/querybook/webapp/components/QueryViewReview/statusUtils.ts new file mode 100644 index 000000000..0f61605c7 --- /dev/null +++ b/querybook/webapp/components/QueryViewReview/statusUtils.ts @@ -0,0 +1,38 @@ +import { Status } from 'const/queryStatus'; + +interface StatusProperties { + status: Status; + tooltip: string; + tagClass: string; + tagText: string; +} + +export const getStatusProps = ( + isApproved: boolean, + isRejected: boolean +): StatusProperties => { + if (isApproved) { + return { + status: Status.success, + tooltip: 'Query has been approved.', + tagClass: 'Tag--success', + tagText: 'APPROVED', + }; + } + + if (isRejected) { + return { + status: Status.error, + tooltip: 'Changes have been requested.', + tagClass: 'Tag--error', + tagText: 'CHANGES REQUESTED', + }; + } + + return { + status: Status.warning, + tooltip: 'The query is pending review.', + tagClass: 'Tag--warning', + tagText: 'PENDING REVIEW', + }; +}; diff --git a/querybook/webapp/config.d.ts b/querybook/webapp/config.d.ts index 9961d9686..c5e19e67f 100644 --- a/querybook/webapp/config.d.ts +++ b/querybook/webapp/config.d.ts @@ -133,6 +133,19 @@ declare module 'config/querybook_public_config.yaml' { github_integration: { enabled: boolean; }; + peer_review: { + enabled: boolean; + request_texts: { + // When users request reviews + description: string; + guide_link: string; + tip: string; + }; + reviewer_texts: { + // When reviewers take actions + approve_message: string; + }; + }; }; export default data; } diff --git a/querybook/webapp/const/analytics.ts b/querybook/webapp/const/analytics.ts index 3ea663fbf..4cd40e71e 100644 --- a/querybook/webapp/const/analytics.ts +++ b/querybook/webapp/const/analytics.ts @@ -44,6 +44,7 @@ export enum ElementType { STATUS_BUTTON = 'STATUS_BUTTON', SETTINGS_BUTTON = 'SETTINGS_BUTTON', HELP_BUTTON = 'HELP_BUTTON', + REVIEWS_BUTTON = 'REVIEWS_BUTTON', // Table navigator search TABLE_ORDER_BY_BUTTON = 'TABLE_ORDER_BY_BUTTON', @@ -90,6 +91,7 @@ export enum ElementType { CREATE_DATADOC_BUTTON = 'CREATE_DATADOC_BUTTON', RESULT_EXPORT_BUTTON = 'RESULT_EXPORT_BUTTON', INSERT_SNIPPET_BUTTON = 'INSERT_SNIPPET_BUTTON', + PEER_REVIEW_QUERY_BUTTON = 'PEER_REVIEW_QUERY_BUTTON', // Chart Cell CHART_CONFIG_BUTTON = 'CHART_CONFIG_BUTTON', diff --git a/querybook/webapp/const/datadoc.ts b/querybook/webapp/const/datadoc.ts index 48bc6535c..9531b7d47 100644 --- a/querybook/webapp/const/datadoc.ts +++ b/querybook/webapp/const/datadoc.ts @@ -142,3 +142,8 @@ export type ISamplingTables = Record< string, { sampled_table?: string; sample_rate?: number } >; + +export interface IPeerReviewParams { + reviewer_ids: number[]; + request_reason: string; +} diff --git a/querybook/webapp/const/queryEngine.ts b/querybook/webapp/const/queryEngine.ts index df0341c66..f34b5a3bf 100644 --- a/querybook/webapp/const/queryEngine.ts +++ b/querybook/webapp/const/queryEngine.ts @@ -15,6 +15,7 @@ export interface IQueryEngine { status_checker?: string; upload_exporter?: string; validator?: string; + peer_review?: boolean; }; } diff --git a/querybook/webapp/const/queryExecution.ts b/querybook/webapp/const/queryExecution.ts index c261bb55c..791641e2d 100644 --- a/querybook/webapp/const/queryExecution.ts +++ b/querybook/webapp/const/queryExecution.ts @@ -16,6 +16,8 @@ export enum QueryExecutionStatus { DONE, ERROR, CANCEL, + PENDING_REVIEW, + REJECTED, } export enum StatementExecutionStatus { @@ -63,6 +65,7 @@ export interface IQueryExecution { uid: number; statement_executions?: number[]; + query_review_id?: number; // If the query is still running // it may have a field called total which @@ -84,6 +87,7 @@ export interface IQueryExecutionExportStatusInfo { export type IRawQueryExecution = IQueryExecution & { statement_executions: IStatementExecution[]; + review?: IQueryReview; }; export interface IStatementExecution { @@ -109,6 +113,20 @@ export interface IStatementExecution { downloadUrlFailed?: boolean; } +export interface IQueryReview { + id: number; + query_execution_id: number; + requested_by: number; + reviewed_by?: number; + status: 'pending' | 'approved' | 'rejected'; + request_reason: string; + rejection_reason?: string; + created_at: number; + updated_at: number; + reviewer_ids: number[]; + execution?: IQueryExecution; +} + export interface IStatementResult { data?: string[][]; error?: any; diff --git a/querybook/webapp/const/queryStatus.ts b/querybook/webapp/const/queryStatus.ts index e6f4ce2e1..f3ec2c8aa 100644 --- a/querybook/webapp/const/queryStatus.ts +++ b/querybook/webapp/const/queryStatus.ts @@ -15,6 +15,8 @@ export const STATUS_TO_TEXT_MAPPING = { [QueryExecutionStatus.DONE]: 'Success', [QueryExecutionStatus.ERROR]: 'Error', [QueryExecutionStatus.CANCEL]: 'Cancelled', + [QueryExecutionStatus.PENDING_REVIEW]: 'Pending Review', + [QueryExecutionStatus.REJECTED]: 'Rejected', }; export const queryStatusToStatusIcon = { @@ -24,4 +26,6 @@ export const queryStatusToStatusIcon = { [QueryExecutionStatus.DONE]: Status.success, [QueryExecutionStatus.ERROR]: Status.error, [QueryExecutionStatus.CANCEL]: Status.none, + [QueryExecutionStatus.PENDING_REVIEW]: Status.warning, + [QueryExecutionStatus.REJECTED]: Status.error, }; diff --git a/querybook/webapp/hooks/useQueryReview.ts b/querybook/webapp/hooks/useQueryReview.ts new file mode 100644 index 000000000..17a8749be --- /dev/null +++ b/querybook/webapp/hooks/useQueryReview.ts @@ -0,0 +1,67 @@ +import { IQueryReview, QueryExecutionStatus } from 'const/queryExecution'; +import { useMemo } from 'react'; +import { useSelector } from 'react-redux'; +import { IStoreState } from 'redux/store/types'; +import { + queryExecutionSelector, + queryReviewByExecutionIdSelector, +} from 'redux/queryExecutions/selector'; +import { myUserInfoSelector } from 'redux/user/selector'; + +export interface QueryReviewState { + status: { + isPending: boolean; + isRejected: boolean; + isApproved: boolean; + }; + permissions: { + isReviewer: boolean; + }; + review: IQueryReview | null; +} + +const DEFAULT_REVIEW_STATE: QueryReviewState = { + status: { + isPending: false, + isRejected: false, + isApproved: false, + }, + permissions: { + isReviewer: false, + }, + review: null, +}; + +export function useQueryReview(executionId?: number): QueryReviewState { + const { queryExecution, currentUser, queryReview } = useSelector( + (state: IStoreState) => ({ + queryExecution: executionId + ? queryExecutionSelector(state, executionId) + : null, + currentUser: myUserInfoSelector(state), + queryReview: executionId + ? queryReviewByExecutionIdSelector(state, executionId) + : null, + }) + ); + + return useMemo(() => { + if (!executionId || !queryExecution || !queryReview) { + return DEFAULT_REVIEW_STATE; + } + + const isPending = + queryExecution.status === QueryExecutionStatus.PENDING_REVIEW; + const isRejected = + queryExecution.status === QueryExecutionStatus.REJECTED; + const isApproved = !isPending && !isRejected; + + return { + status: { isPending, isRejected, isApproved }, + permissions: { + isReviewer: queryReview.reviewer_ids?.includes(currentUser.id), + }, + review: queryReview, + }; + }, [executionId, queryExecution, queryReview, currentUser]); +} diff --git a/querybook/webapp/lib/peer-review/config.ts b/querybook/webapp/lib/peer-review/config.ts new file mode 100644 index 000000000..077a13f23 --- /dev/null +++ b/querybook/webapp/lib/peer-review/config.ts @@ -0,0 +1,32 @@ +import { useMemo } from 'react'; +import { PEER_REVIEW_CONFIG } from 'lib/public-config'; + +export interface IPeerReviewConfig { + isEnabled: boolean; + requestTexts: { + description: string; + guideLink: string; + reviewTip: string; + }; + reviewerTexts: { + approveMessage: string; + }; +} + +export function usePeerReview(): IPeerReviewConfig { + return useMemo( + () => ({ + isEnabled: PEER_REVIEW_CONFIG.enabled, + requestTexts: { + description: PEER_REVIEW_CONFIG.request_texts.description, + guideLink: PEER_REVIEW_CONFIG.request_texts.guide_link, + reviewTip: PEER_REVIEW_CONFIG.request_texts.tip, + }, + reviewerTexts: { + approveMessage: + PEER_REVIEW_CONFIG.reviewer_texts.approve_message, + }, + }), + [] + ); +} diff --git a/querybook/webapp/lib/public-config.ts b/querybook/webapp/lib/public-config.ts index 59b26f61e..b8c5f64f4 100644 --- a/querybook/webapp/lib/public-config.ts +++ b/querybook/webapp/lib/public-config.ts @@ -37,3 +37,15 @@ export const getTableSamplingRateOptions = () => { label: rate === 0 ? 'none' : rate + '%', })); }; + +export const PEER_REVIEW_CONFIG = PublicConfig.peer_review ?? { + enabled: false, + request_texts: { + description: '', + guide_link: '', + tip: '', + }, + reviewer_texts: { + approve_message: '', + }, +}; diff --git a/querybook/webapp/redux/queryExecutions/action.ts b/querybook/webapp/redux/queryExecutions/action.ts index b548f4a41..1dad4871f 100644 --- a/querybook/webapp/redux/queryExecutions/action.ts +++ b/querybook/webapp/redux/queryExecutions/action.ts @@ -2,6 +2,7 @@ import { normalize, schema } from 'normalizr'; import type { Socket } from 'socket.io-client'; import { IAccessRequest } from 'const/accessRequest'; +import { IPeerReviewParams } from 'const/datadoc'; import { IQueryExecution, IQueryExecutionViewer, @@ -34,13 +35,41 @@ import { } from './types'; const statementExecutionSchema = new schema.Entity('statementExecution'); +const reviewSchema = new schema.Entity('review'); const dataCellSchema = new schema.Entity('dataCell'); const queryExecutionSchema = new schema.Entity('queryExecution', { statement_executions: [statementExecutionSchema], data_cell: dataCellSchema, + review: reviewSchema, }); export const queryExecutionSchemaList = [queryExecutionSchema]; +export function approveQueryReview( + executionId: number +): ThunkResult> { + return async (dispatch) => { + const { data: execution } = await QueryExecutionResource.approveReview( + executionId + ); + dispatch(receiveQueryExecution(execution)); + return execution; + }; +} + +export function rejectQueryReview( + executionId: number, + rejectionReason: string +): ThunkResult> { + return async (dispatch) => { + const { data: execution } = await QueryExecutionResource.rejectReview( + executionId, + rejectionReason + ); + dispatch(receiveQueryExecution(execution)); + return execution; + }; +} + export function addQueryExecutionAccessRequest( executionId: number ): ThunkResult> { @@ -166,12 +195,15 @@ export function receiveQueryExecution( const { queryExecution: queryExecutionById = {}, statementExecution: statementExecutionById = {}, + review: queryReviewById = {}, } = normalizedData.entities; + return { type: '@@queryExecutions/RECEIVE_QUERY_EXECUTION', payload: { queryExecutionById, statementExecutionById, + queryReviewById, dataCellId, }, }; @@ -379,7 +411,8 @@ export function createQueryExecution( query: string, engineId?: number, cellId?: number, - metadata?: Record + metadata?: Record, + peerReviewParams?: IPeerReviewParams ): ThunkResult> { return async (dispatch, getState) => { const state = getState(); @@ -389,7 +422,8 @@ export function createQueryExecution( query, selectedEngineId, cellId, - metadata + metadata, + peerReviewParams ); dispatch(receiveQueryExecution(queryExecution, cellId)); diff --git a/querybook/webapp/redux/queryExecutions/reducer.ts b/querybook/webapp/redux/queryExecutions/reducer.ts index fe0f01b65..cc9b757f4 100644 --- a/querybook/webapp/redux/queryExecutions/reducer.ts +++ b/querybook/webapp/redux/queryExecutions/reducer.ts @@ -2,7 +2,7 @@ import { produce } from 'immer'; import moment from 'moment'; import { combineReducers } from 'redux'; -import { StatementExecutionStatus } from 'const/queryExecution'; +import { IQueryReview, StatementExecutionStatus } from 'const/queryExecution'; import { arrayGroupByField, linkifyLog } from 'lib/utils'; import { IQueryExecutionState, QueryExecutionAction } from './types'; @@ -10,6 +10,7 @@ import { IQueryExecutionState, QueryExecutionAction } from './types'; const initialState: IQueryExecutionState = { queryExecutionById: {}, statementExecutionById: {}, + queryReviewByExecutionId: {}, dataCellIdQueryExecution: {}, queryExecutionIdToCellInfo: {}, @@ -186,6 +187,38 @@ function queryExecutionByIdReducer( }); } +function queryReviewByExecutionIdReducer( + state = initialState.queryReviewByExecutionId, + action: QueryExecutionAction +) { + switch (action.type) { + case '@@queryExecutions/RECEIVE_QUERY_EXECUTION': { + const { queryReviewById } = action.payload; + if (queryReviewById) { + const queryReviewByExecutionId = {}; + + Object.values(queryReviewById).forEach( + (queryReview: IQueryReview) => { + if (queryReview) { + queryReviewByExecutionId[ + queryReview.query_execution_id + ] = queryReview; + } + } + ); + + return { + ...state, + ...queryReviewByExecutionId, + }; + } + return state; + } + default: + return state; + } +} + function getPartialLogHeader() { return [ `${moment().format('MMM D, h:mma')} Logging starting...`, @@ -449,4 +482,5 @@ export default combineReducers({ statementExporters: statementExportersReducer, accessRequestsByExecutionIdUserId: accessRequestsByExecutionIdUserIdReducer, viewersByExecutionIdUserId: viewersByExecutionIdUserIdReducer, + queryReviewByExecutionId: queryReviewByExecutionIdReducer, }); diff --git a/querybook/webapp/redux/queryExecutions/selector.ts b/querybook/webapp/redux/queryExecutions/selector.ts index 3ba2a0f09..7e5076946 100644 --- a/querybook/webapp/redux/queryExecutions/selector.ts +++ b/querybook/webapp/redux/queryExecutions/selector.ts @@ -1,6 +1,6 @@ import { createSelector } from 'reselect'; -import { IQueryExecution } from 'const/queryExecution'; +import { IQueryExecution, IQueryReview } from 'const/queryExecution'; import { IStoreState } from 'redux/store/types'; const queryExecutionIdSetSelector = (state: IStoreState, cellId: number) => @@ -46,6 +46,16 @@ export const queryExecutionSelector = ( executionId: number ) => state.queryExecutions.queryExecutionById[executionId] as IQueryExecution; +export const queryReviewByExecutionIdSelector = ( + state: IStoreState, + queryExecutionId: number | null +): IQueryReview => { + if (queryExecutionId == null) { + return null; + } + return state.queryExecutions.queryReviewByExecutionId[queryExecutionId]; +}; + // returns array of query statement ids export const makeQueryExecutionStatementExecutionSelector = () => createSelector( diff --git a/querybook/webapp/redux/queryExecutions/types.ts b/querybook/webapp/redux/queryExecutions/types.ts index d700ca3dd..162e0a5f0 100644 --- a/querybook/webapp/redux/queryExecutions/types.ts +++ b/querybook/webapp/redux/queryExecutions/types.ts @@ -7,6 +7,7 @@ import { IQueryExecution, IQueryExecutionViewer, IQueryResultExporter, + IQueryReview, IStatementExecution, IStatementLog, IStatementResult, @@ -30,6 +31,7 @@ export interface IReceiveQueryExecutionAction extends Action { payload: { queryExecutionById: Record; statementExecutionById: Record; + queryReviewById: Record; dataCellId?: number; }; } @@ -186,6 +188,7 @@ export type QueryExecutionAction = export interface IQueryExecutionState { queryExecutionById: Record; statementExecutionById: Record; + queryReviewByExecutionId: Record; dataCellIdQueryExecution: Record>; queryExecutionIdToCellInfo: Record< diff --git a/querybook/webapp/redux/queryReview/action.ts b/querybook/webapp/redux/queryReview/action.ts new file mode 100644 index 000000000..ad855d1cd --- /dev/null +++ b/querybook/webapp/redux/queryReview/action.ts @@ -0,0 +1,56 @@ +import { Dispatch } from 'redux'; + +import { QueryReviewResource } from 'resource/queryReview'; + +import { + FETCH_ASSIGNED_REVIEWS_FAILURE, + FETCH_ASSIGNED_REVIEWS_REQUEST, + FETCH_ASSIGNED_REVIEWS_SUCCESS, + FETCH_MY_REVIEWS_FAILURE, + FETCH_MY_REVIEWS_REQUEST, + FETCH_MY_REVIEWS_SUCCESS, + QueryReviewAction, + SET_ACTIVE_TAB, + ThunkResult, +} from './types'; + +export const fetchMyReviews = + (): ThunkResult => async (dispatch: Dispatch) => { + try { + dispatch({ type: FETCH_MY_REVIEWS_REQUEST }); + const reviews = await QueryReviewResource.getReviewsCreatedByMe(); + dispatch({ + type: FETCH_MY_REVIEWS_SUCCESS, + payload: reviews.data, + }); + } catch (error) { + dispatch({ + type: FETCH_MY_REVIEWS_FAILURE, + payload: error.message, + }); + } + }; + +export const fetchAssignedReviews = + (): ThunkResult => async (dispatch: Dispatch) => { + try { + dispatch({ type: FETCH_ASSIGNED_REVIEWS_REQUEST }); + const reviews = await QueryReviewResource.getReviewsAssignedToMe(); + dispatch({ + type: FETCH_ASSIGNED_REVIEWS_SUCCESS, + payload: reviews.data, + }); + } catch (error) { + dispatch({ + type: FETCH_ASSIGNED_REVIEWS_FAILURE, + payload: error.message, + }); + } + }; + +export const setActiveTab = ( + tab: 'myReviews' | 'assigned' +): QueryReviewAction => ({ + type: SET_ACTIVE_TAB, + payload: tab, +}); diff --git a/querybook/webapp/redux/queryReview/reducer.ts b/querybook/webapp/redux/queryReview/reducer.ts new file mode 100644 index 000000000..9e7a01a3f --- /dev/null +++ b/querybook/webapp/redux/queryReview/reducer.ts @@ -0,0 +1,67 @@ +import { produce } from 'immer'; + +import { + FETCH_ASSIGNED_REVIEWS_FAILURE, + FETCH_ASSIGNED_REVIEWS_REQUEST, + FETCH_ASSIGNED_REVIEWS_SUCCESS, + FETCH_MY_REVIEWS_FAILURE, + FETCH_MY_REVIEWS_REQUEST, + FETCH_MY_REVIEWS_SUCCESS, + IQueryReviewState, + QueryReviewAction, + SET_ACTIVE_TAB, +} from './types'; + +const initialState: IQueryReviewState = { + myReviews: [], + assignedReviews: [], + loadingMyReviews: false, + loadingAssignedReviews: false, + errorMyReviews: null, + errorAssignedReviews: null, + activeTab: 'myReviews', +}; + +export const queryReviewReducer = ( + state = initialState, + action: QueryReviewAction +): IQueryReviewState => + produce(state, (draft) => { + switch (action.type) { + case FETCH_MY_REVIEWS_REQUEST: + draft.loadingMyReviews = true; + draft.errorMyReviews = null; + break; + + case FETCH_MY_REVIEWS_SUCCESS: + draft.loadingMyReviews = false; + draft.myReviews = action.payload; + break; + + case FETCH_MY_REVIEWS_FAILURE: + draft.loadingMyReviews = false; + draft.errorMyReviews = action.payload; + break; + + case FETCH_ASSIGNED_REVIEWS_REQUEST: + draft.loadingAssignedReviews = true; + draft.errorAssignedReviews = null; + break; + + case FETCH_ASSIGNED_REVIEWS_SUCCESS: + draft.loadingAssignedReviews = false; + draft.assignedReviews = action.payload; + break; + + case FETCH_ASSIGNED_REVIEWS_FAILURE: + draft.loadingAssignedReviews = false; + draft.errorAssignedReviews = action.payload; + break; + + case SET_ACTIVE_TAB: + draft.activeTab = action.payload; + break; + } + }); + +export default queryReviewReducer; diff --git a/querybook/webapp/redux/queryReview/selector.ts b/querybook/webapp/redux/queryReview/selector.ts new file mode 100644 index 000000000..b44f23ae5 --- /dev/null +++ b/querybook/webapp/redux/queryReview/selector.ts @@ -0,0 +1,35 @@ +import { createSelector } from 'reselect'; + +import { IStoreState } from '../store/types'; + +const selectQueryReviewState = (state: IStoreState) => state.queryReview; + +export const selectMyReviews = createSelector( + selectQueryReviewState, + (queryReview) => queryReview.myReviews +); + +export const selectAssignedReviews = createSelector( + selectQueryReviewState, + (queryReview) => queryReview.assignedReviews +); + +export const selectLoadingMyReviews = createSelector( + selectQueryReviewState, + (queryReview) => queryReview.loadingMyReviews +); + +export const selectLoadingAssignedReviews = createSelector( + selectQueryReviewState, + (queryReview) => queryReview.loadingAssignedReviews +); + +export const selectErrorMyReviews = createSelector( + selectQueryReviewState, + (queryReview) => queryReview.errorMyReviews +); + +export const selectErrorAssignedReviews = createSelector( + selectQueryReviewState, + (queryReview) => queryReview.errorAssignedReviews +); diff --git a/querybook/webapp/redux/queryReview/types.ts b/querybook/webapp/redux/queryReview/types.ts new file mode 100644 index 000000000..aed7c23f7 --- /dev/null +++ b/querybook/webapp/redux/queryReview/types.ts @@ -0,0 +1,80 @@ +import { Action } from 'redux'; +import { ThunkAction } from 'redux-thunk'; + +import { IQueryReview } from 'const/queryExecution'; + +import { IStoreState } from '../store/types'; + +export const FETCH_MY_REVIEWS_REQUEST = + '@@queryReview/FETCH_MY_REVIEWS_REQUEST'; +export const FETCH_MY_REVIEWS_SUCCESS = + '@@queryReview/FETCH_MY_REVIEWS_SUCCESS'; +export const FETCH_MY_REVIEWS_FAILURE = + '@@queryReview/FETCH_MY_REVIEWS_FAILURE'; + +export const FETCH_ASSIGNED_REVIEWS_REQUEST = + '@@queryReview/FETCH_ASSIGNED_REVIEWS_REQUEST'; +export const FETCH_ASSIGNED_REVIEWS_SUCCESS = + '@@queryReview/FETCH_ASSIGNED_REVIEWS_SUCCESS'; +export const FETCH_ASSIGNED_REVIEWS_FAILURE = + '@@queryReview/FETCH_ASSIGNED_REVIEWS_FAILURE'; +export const SET_ACTIVE_TAB = '@@queryReview/SET_ACTIVE_TAB'; + +export interface IQueryReviewState { + myReviews: IQueryReview[]; + assignedReviews: IQueryReview[]; + loadingMyReviews: boolean; + loadingAssignedReviews: boolean; + errorMyReviews: string | null; + errorAssignedReviews: string | null; + activeTab: 'myReviews' | 'assigned'; +} + +interface IFetchMyReviewsRequestAction extends Action { + type: typeof FETCH_MY_REVIEWS_REQUEST; +} + +interface IFetchMyReviewsSuccessAction extends Action { + type: typeof FETCH_MY_REVIEWS_SUCCESS; + payload: IQueryReview[]; +} + +interface IFetchMyReviewsFailureAction extends Action { + type: typeof FETCH_MY_REVIEWS_FAILURE; + payload: string; +} + +interface IFetchAssignedReviewsRequestAction extends Action { + type: typeof FETCH_ASSIGNED_REVIEWS_REQUEST; +} + +interface IFetchAssignedReviewsSuccessAction extends Action { + type: typeof FETCH_ASSIGNED_REVIEWS_SUCCESS; + payload: IQueryReview[]; +} + +interface IFetchAssignedReviewsFailureAction extends Action { + type: typeof FETCH_ASSIGNED_REVIEWS_FAILURE; + payload: string; +} + +interface ISetActiveTabAction extends Action { + type: typeof SET_ACTIVE_TAB; + payload: 'myReviews' | 'assigned'; +} + +export type QueryReviewAction = + | IFetchMyReviewsRequestAction + | IFetchMyReviewsSuccessAction + | IFetchMyReviewsFailureAction + | IFetchAssignedReviewsRequestAction + | IFetchAssignedReviewsSuccessAction + | IFetchAssignedReviewsFailureAction + | ISetActiveTabAction; + +export type ThunkResult = ThunkAction< + R, + IStoreState, + undefined, + QueryReviewAction +>; diff --git a/querybook/webapp/redux/store/rootReducer.ts b/querybook/webapp/redux/store/rootReducer.ts index 1d67785e9..3956c5f90 100644 --- a/querybook/webapp/redux/store/rootReducer.ts +++ b/querybook/webapp/redux/store/rootReducer.ts @@ -12,6 +12,7 @@ import notificationService from '../notificationService/reducer'; import querybookUI from '../querybookUI/reducer'; import queryEngine from '../queryEngine/reducer'; import queryExecutions from '../queryExecutions/reducer'; +import queryReview from '../queryReview/reducer'; import querySnippets from '../querySnippets/reducer'; import queryView from '../queryView/reducer'; import scheduledDocs from '../scheduledDataDoc/reducer'; @@ -38,4 +39,5 @@ export default combineReducers({ tag, scheduledDocs, comment, + queryReview, }); diff --git a/querybook/webapp/redux/store/types.ts b/querybook/webapp/redux/store/types.ts index 82cdc2fd3..41a79753c 100644 --- a/querybook/webapp/redux/store/types.ts +++ b/querybook/webapp/redux/store/types.ts @@ -21,6 +21,7 @@ import { IQueryExecutionState, QueryExecutionAction, } from 'redux/queryExecutions/types'; +import { IQueryReviewState, QueryReviewAction } from 'redux/queryReview/types'; import { IQuerySnippetsState, QuerySnippetsAction, @@ -53,6 +54,7 @@ export interface IStoreState { readonly tag: ITagState; readonly scheduledDocs: IScheduledDataDocState; readonly comment: ICommentState; + readonly queryReview: IQueryReviewState; } export type AllAction = @@ -73,6 +75,7 @@ export type AllAction = | GlobalStateAction | TagAction | ScheduledDataDocAction - | CommentAction; + | CommentAction + | QueryReviewAction; export type Dispatch = ThunkDispatch; diff --git a/querybook/webapp/resource/queryExecution.ts b/querybook/webapp/resource/queryExecution.ts index 400e123f9..59d804d09 100644 --- a/querybook/webapp/resource/queryExecution.ts +++ b/querybook/webapp/resource/queryExecution.ts @@ -1,5 +1,5 @@ import type { IAccessRequest } from 'const/accessRequest'; -import { TDataDocMetaVariables } from 'const/datadoc'; +import { IPeerReviewParams, TDataDocMetaVariables } from 'const/datadoc'; import { IQueryTranspiler, ITranspiledQuery } from 'const/queryEngine'; import { IQueryError, @@ -71,7 +71,8 @@ export const QueryExecutionResource = { query: string, engineId: number, cellId?: number, - metadata?: Record + metadata?: Record, + peerReviewParams?: IPeerReviewParams ) => { const params = { query, @@ -87,6 +88,10 @@ export const QueryExecutionResource = { params['originator'] = dataDocSocket.socketId; } + if (peerReviewParams != null) { + params['peer_review_params'] = peerReviewParams; + } + return ds.save('/query_execution/', params); }, @@ -94,6 +99,19 @@ export const QueryExecutionResource = { getError: (executionId: number) => ds.fetch(`/query_execution/${executionId}/error/`), + + approveReview: (executionId: number) => + ds.update( + `/query_execution/${executionId}/approve_review/` + ), + + rejectReview: (executionId: number, rejectionReason: string) => + ds.update( + `/query_execution/${executionId}/reject_review/`, + { + rejection_reason: rejectionReason, + } + ), }; export const QueryExecutionMetadataResource = { diff --git a/querybook/webapp/resource/queryReview.ts b/querybook/webapp/resource/queryReview.ts new file mode 100644 index 000000000..d25901329 --- /dev/null +++ b/querybook/webapp/resource/queryReview.ts @@ -0,0 +1,10 @@ +import { IQueryReview } from 'const/queryExecution'; +import ds from 'lib/datasource'; + +export const QueryReviewResource = { + getReviewsCreatedByMe: () => + ds.fetch('/query_review/created_by_me/'), + + getReviewsAssignedToMe: () => + ds.fetch('/query_review/assigned_to_me/'), +}; diff --git a/querybook/webapp/ui/Tabs/Tabs.scss b/querybook/webapp/ui/Tabs/Tabs.scss index 44ace8931..545e06474 100644 --- a/querybook/webapp/ui/Tabs/Tabs.scss +++ b/querybook/webapp/ui/Tabs/Tabs.scss @@ -15,14 +15,12 @@ justify-content: center; a { + display: flex; + align-items: center; + gap: 8px; color: var(--text); .Icon { - & ~ span { - // To create gap between icon and title - margin-left: 4px; - } - svg { height: 20px; width: 20px;