korbit-ai[bot] commented on code in PR #34220: URL: https://github.com/apache/superset/pull/34220#discussion_r2215455222
########## superset/report_templates/models.py: ########## @@ -0,0 +1,25 @@ +from __future__ import annotations + +from flask_appbuilder import Model +from sqlalchemy import Column, Integer, String, Text, ForeignKey +from sqlalchemy.orm import relationship + +from superset.models.helpers import AuditMixinNullable +from superset.connectors.sqla.models import SqlaTable + + +class ReportTemplate(Model, AuditMixinNullable): Review Comment: ### Missing domain logic encapsulation <sub></sub> <details> <summary>Tell me more</summary> ###### What is the issue? The class lacks validation methods and business logic encapsulation for template operations. ###### Why this matters Business logic may end up scattered across the application, making it harder to maintain and potentially leading to inconsistencies. ###### Suggested change ∙ *Feature Preview* Add domain-specific methods to encapsulate business logic: ```python class ReportTemplate(Model, AuditMixinNullable): def validate_template(self) -> bool: # Template validation logic pass def render(self, context: dict) -> str: # Template rendering logic pass ``` ###### Provide feedback to improve future suggestions [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/16cbe587-d280-4b9e-ad64-365dea6fe476/upvote) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/16cbe587-d280-4b9e-ad64-365dea6fe476?what_not_true=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/16cbe587-d280-4b9e-ad64-365dea6fe476?what_out_of_scope=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/16cbe587-d280-4b9e-ad64-365dea6fe476?what_not_in_standard=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/16cbe587-d280-4b9e-ad64-365dea6fe476) </details> <sub> 💬 Looking for more details? Reply to this comment to chat with Korbit. </sub> <!--- korbi internal id:eafb9bbb-ad2d-4558-b0fe-f4557c315fad --> [](eafb9bbb-ad2d-4558-b0fe-f4557c315fad) ########## superset/report_templates/models.py: ########## @@ -0,0 +1,25 @@ +from __future__ import annotations + +from flask_appbuilder import Model +from sqlalchemy import Column, Integer, String, Text, ForeignKey +from sqlalchemy.orm import relationship + +from superset.models.helpers import AuditMixinNullable +from superset.connectors.sqla.models import SqlaTable + + +class ReportTemplate(Model, AuditMixinNullable): + """Template for generating reports based on datasets.""" + + __tablename__ = "report_templates" + + id = Column(Integer, primary_key=True) + name = Column(String(250), nullable=False) + description = Column(Text) + dataset_id = Column(Integer, ForeignKey("tables.id"), nullable=False) + template_path = Column(String(1024), nullable=False) Review Comment: ### Missing Template Path Validation <sub></sub> <details> <summary>Tell me more</summary> ###### What is the issue? The template_path column doesn't validate if the path actually exists or is accessible when storing a value. ###### Why this matters Without path validation, the system might store invalid paths, leading to runtime errors when attempting to generate reports. ###### Suggested change ∙ *Feature Preview* Add a validator or property to check path existence and accessibility: ```python @validates('template_path') def validate_template_path(self, key, path): if not os.path.exists(path): raise ValueError(f"Template path {path} does not exist") return path ``` ###### Provide feedback to improve future suggestions [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/b6b00ad5-9a44-4049-a76d-091327b08d4e/upvote) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/b6b00ad5-9a44-4049-a76d-091327b08d4e?what_not_true=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/b6b00ad5-9a44-4049-a76d-091327b08d4e?what_out_of_scope=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/b6b00ad5-9a44-4049-a76d-091327b08d4e?what_not_in_standard=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/b6b00ad5-9a44-4049-a76d-091327b08d4e) </details> <sub> 💬 Looking for more details? Reply to this comment to chat with Korbit. </sub> <!--- korbi internal id:7721d677-d152-420c-a6e8-1b96da11c006 --> [](7721d677-d152-420c-a6e8-1b96da11c006) ########## superset/report_templates/models.py: ########## @@ -0,0 +1,25 @@ +from __future__ import annotations + +from flask_appbuilder import Model +from sqlalchemy import Column, Integer, String, Text, ForeignKey +from sqlalchemy.orm import relationship + +from superset.models.helpers import AuditMixinNullable +from superset.connectors.sqla.models import SqlaTable + + +class ReportTemplate(Model, AuditMixinNullable): + """Template for generating reports based on datasets.""" + + __tablename__ = "report_templates" + + id = Column(Integer, primary_key=True) + name = Column(String(250), nullable=False) Review Comment: ### Missing Name Uniqueness Constraint <sub></sub> <details> <summary>Tell me more</summary> ###### What is the issue? The name field lacks uniqueness constraint, which could lead to duplicate template names. ###### Why this matters Multiple templates with the same name could cause confusion for users and potential issues in template selection/management. ###### Suggested change ∙ *Feature Preview* Add unique constraint to the name field: ```python name = Column(String(250), nullable=False, unique=True) ``` ###### Provide feedback to improve future suggestions [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/99378ad2-9043-4d37-a228-8097f2509252/upvote) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/99378ad2-9043-4d37-a228-8097f2509252?what_not_true=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/99378ad2-9043-4d37-a228-8097f2509252?what_out_of_scope=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/99378ad2-9043-4d37-a228-8097f2509252?what_not_in_standard=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/99378ad2-9043-4d37-a228-8097f2509252) </details> <sub> 💬 Looking for more details? Reply to this comment to chat with Korbit. </sub> <!--- korbi internal id:309c9b3e-47ae-40ae-8b02-8f4e674ecf6e --> [](309c9b3e-47ae-40ae-8b02-8f4e674ecf6e) ########## superset/report_templates/models.py: ########## @@ -0,0 +1,25 @@ +from __future__ import annotations + +from flask_appbuilder import Model +from sqlalchemy import Column, Integer, String, Text, ForeignKey +from sqlalchemy.orm import relationship + +from superset.models.helpers import AuditMixinNullable +from superset.connectors.sqla.models import SqlaTable + + +class ReportTemplate(Model, AuditMixinNullable): + """Template for generating reports based on datasets.""" Review Comment: ### Insufficient model docstring context <sub></sub> <details> <summary>Tell me more</summary> ###### What is the issue? The class docstring is too generic and doesn't explain the purpose, use case, or key attributes of the ReportTemplate model. ###### Why this matters Without clear context about how this template is used and what its key attributes represent, other developers may misuse the model or struggle to maintain it. ###### Suggested change ∙ *Feature Preview* """A model representing report templates that define how to generate reports from datasets. This template stores the mapping between a dataset and its report generation template file, allowing consistent report generation based on predefined layouts and data sources. Attributes: name: The unique name identifying this template template_path: Path to the template file used for report generation dataset: The source dataset used to populate the report""" ###### Provide feedback to improve future suggestions [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/3bbd3f9b-0f4e-4564-97f9-07e5bcc0c61c/upvote) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/3bbd3f9b-0f4e-4564-97f9-07e5bcc0c61c?what_not_true=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/3bbd3f9b-0f4e-4564-97f9-07e5bcc0c61c?what_out_of_scope=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/3bbd3f9b-0f4e-4564-97f9-07e5bcc0c61c?what_not_in_standard=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/3bbd3f9b-0f4e-4564-97f9-07e5bcc0c61c) </details> <sub> 💬 Looking for more details? Reply to this comment to chat with Korbit. </sub> <!--- korbi internal id:11612328-9d4e-4407-a271-5ea1795a27e5 --> [](11612328-9d4e-4407-a271-5ea1795a27e5) ########## superset-frontend/src/features/reportTemplates/UploadReportTemplateModal.tsx: ########## @@ -0,0 +1,107 @@ +import { useState } from 'react'; +import { t, SupersetClient } from '@superset-ui/core'; +import { Modal, Input, Button } from '@superset-ui/core/components'; +import DatasetSelect from 'src/dashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigForm/DatasetSelect'; + +interface UploadReportTemplateModalProps { + show: boolean; + onHide: () => void; + onUpload: () => void; + addDangerToast: (msg: string) => void; + addSuccessToast: (msg: string) => void; +} + +export default function UploadReportTemplateModal({ + show, + onHide, + onUpload, + addDangerToast, + addSuccessToast, +}: UploadReportTemplateModalProps) { + const [name, setName] = useState(''); + const [dataset, setDataset] = useState<{ label: string; value: number } | null>(null); + const [description, setDescription] = useState(''); + const [file, setFile] = useState<File | null>(null); + + const reset = () => { + setName(''); + setDataset(null); + setDescription(''); + setFile(null); + }; + + const handleUpload = () => { + if (!file || !name || !dataset) { + addDangerToast(t('All fields are required')); + return; + } + const form = new FormData(); + form.append('template', file); + form.append('name', name); + form.append('dataset_id', String(dataset.value)); + if (description) form.append('description', description); + SupersetClient.post({ + endpoint: '/api/v1/report_template/', + postPayload: form, + stringify: false, + }) + .then(() => { + addSuccessToast(t('Template uploaded')); + reset(); + onUpload(); + onHide(); + }) + .catch(err => { + addDangerToast(t('Failed to upload template: %s', err.message)); + }); + }; + + return ( + <Modal + show={show} + title={t('Add report template')} + onHide={onHide} + footer={ + <div> + <Button buttonStyle="secondary" onClick={onHide}> + {t('Cancel')} + </Button> + <Button buttonStyle="primary" onClick={handleUpload}> + {t('Upload')} + </Button> + </div> + } + > + <div className="field"> + <label htmlFor="template-name">{t('Name')}</label> + <Input + id="template-name" + value={name} + onChange={e => setName(e.target.value)} + /> + </div> + <div className="field"> + <label htmlFor="template-dataset">{t('Dataset')}</label> + <DatasetSelect + onChange={(opt: { label: string; value: number }) => setDataset(opt)} + value={dataset || undefined} + /> + </div> + <div className="field"> + <label htmlFor="template-desc">{t('Description')}</label> + <Input.TextArea + id="template-desc" + value={description} + onChange={e => setDescription(e.target.value)} + /> + </div> + <div className="field"> + <input + type="file" + accept=".odt" + onChange={e => setFile(e.target.files?.[0] || null)} Review Comment: ### Uncontrolled File Size Upload <sub></sub> <details> <summary>Tell me more</summary> ###### What is the issue? Direct file handling in onChange without size validation or file preprocessing can lead to performance issues with large files. ###### Why this matters Large files can cause UI freezes and memory issues if loaded directly into state without validation or preprocessing. ###### Suggested change ∙ *Feature Preview* Add file size validation and consider using FileReader with chunks for large files. Example: ```typescript const handleFileChange = (e: React.ChangeEvent<HTMLInputElement>) => { const file = e.target.files?.[0]; if (file && file.size > MAX_FILE_SIZE) { addDangerToast(t('File size exceeds limit')); return; } setFile(file); }; ``` ###### Provide feedback to improve future suggestions [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/2468eac0-8eb3-45b2-a354-feef9f3c94e6/upvote) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/2468eac0-8eb3-45b2-a354-feef9f3c94e6?what_not_true=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/2468eac0-8eb3-45b2-a354-feef9f3c94e6?what_out_of_scope=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/2468eac0-8eb3-45b2-a354-feef9f3c94e6?what_not_in_standard=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/2468eac0-8eb3-45b2-a354-feef9f3c94e6) </details> <sub> 💬 Looking for more details? Reply to this comment to chat with Korbit. </sub> <!--- korbi internal id:2cd40b10-6f27-414e-bc8e-2014b43afae8 --> [](2cd40b10-6f27-414e-bc8e-2014b43afae8) ########## superset-frontend/src/features/reportTemplates/UploadReportTemplateModal.tsx: ########## @@ -0,0 +1,107 @@ +import { useState } from 'react'; +import { t, SupersetClient } from '@superset-ui/core'; +import { Modal, Input, Button } from '@superset-ui/core/components'; +import DatasetSelect from 'src/dashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigForm/DatasetSelect'; + +interface UploadReportTemplateModalProps { + show: boolean; + onHide: () => void; + onUpload: () => void; + addDangerToast: (msg: string) => void; + addSuccessToast: (msg: string) => void; +} + +export default function UploadReportTemplateModal({ + show, + onHide, + onUpload, + addDangerToast, + addSuccessToast, +}: UploadReportTemplateModalProps) { + const [name, setName] = useState(''); + const [dataset, setDataset] = useState<{ label: string; value: number } | null>(null); + const [description, setDescription] = useState(''); + const [file, setFile] = useState<File | null>(null); + + const reset = () => { + setName(''); + setDataset(null); + setDescription(''); + setFile(null); + }; + + const handleUpload = () => { + if (!file || !name || !dataset) { + addDangerToast(t('All fields are required')); + return; + } + const form = new FormData(); + form.append('template', file); + form.append('name', name); + form.append('dataset_id', String(dataset.value)); + if (description) form.append('description', description); + SupersetClient.post({ + endpoint: '/api/v1/report_template/', + postPayload: form, + stringify: false, + }) + .then(() => { + addSuccessToast(t('Template uploaded')); + reset(); + onUpload(); + onHide(); + }) + .catch(err => { + addDangerToast(t('Failed to upload template: %s', err.message)); + }); + }; + + return ( + <Modal + show={show} + title={t('Add report template')} + onHide={onHide} + footer={ + <div> + <Button buttonStyle="secondary" onClick={onHide}> + {t('Cancel')} + </Button> + <Button buttonStyle="primary" onClick={handleUpload}> + {t('Upload')} + </Button> + </div> + } + > + <div className="field"> + <label htmlFor="template-name">{t('Name')}</label> + <Input + id="template-name" + value={name} + onChange={e => setName(e.target.value)} + /> + </div> + <div className="field"> + <label htmlFor="template-dataset">{t('Dataset')}</label> + <DatasetSelect + onChange={(opt: { label: string; value: number }) => setDataset(opt)} + value={dataset || undefined} + /> + </div> + <div className="field"> + <label htmlFor="template-desc">{t('Description')}</label> + <Input.TextArea + id="template-desc" + value={description} + onChange={e => setDescription(e.target.value)} + /> + </div> + <div className="field"> + <input + type="file" + accept=".odt" + onChange={e => setFile(e.target.files?.[0] || null)} Review Comment: ### Insufficient File Upload Validation <sub></sub> <details> <summary>Tell me more</summary> ###### What is the issue? File upload lacks proper file validation beyond simple extension checking. ###### Why this matters Relying only on file extension could allow upload of malicious files masquerading as .odt files, potentially leading to security vulnerabilities. ###### Suggested change ∙ *Feature Preview* ```typescript const validateFile = (file: File) => { const maxSize = 5 * 1024 * 1024; // 5MB example return file.size <= maxSize && file.type === 'application/vnd.oasis.opendocument.text'; }; const handleFileChange = (e: React.ChangeEvent<HTMLInputElement>) => { const selectedFile = e.target.files?.[0]; if (selectedFile && validateFile(selectedFile)) { setFile(selectedFile); } else { addDangerToast(t('Invalid file type or size')); e.target.value = ''; } }; <input type="file" accept=".odt" onChange={handleFileChange} /> ``` ###### Provide feedback to improve future suggestions [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/957bd3d8-db16-4ae8-9813-a905d7fd06a1/upvote) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/957bd3d8-db16-4ae8-9813-a905d7fd06a1?what_not_true=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/957bd3d8-db16-4ae8-9813-a905d7fd06a1?what_out_of_scope=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/957bd3d8-db16-4ae8-9813-a905d7fd06a1?what_not_in_standard=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/957bd3d8-db16-4ae8-9813-a905d7fd06a1) </details> <sub> 💬 Looking for more details? Reply to this comment to chat with Korbit. </sub> <!--- korbi internal id:3e0fe39f-ef38-498e-b690-7163ff0fcc53 --> [](3e0fe39f-ef38-498e-b690-7163ff0fcc53) ########## superset-frontend/src/features/reportTemplates/UploadReportTemplateModal.tsx: ########## @@ -0,0 +1,107 @@ +import { useState } from 'react'; +import { t, SupersetClient } from '@superset-ui/core'; +import { Modal, Input, Button } from '@superset-ui/core/components'; +import DatasetSelect from 'src/dashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigForm/DatasetSelect'; + +interface UploadReportTemplateModalProps { + show: boolean; + onHide: () => void; + onUpload: () => void; + addDangerToast: (msg: string) => void; + addSuccessToast: (msg: string) => void; +} + +export default function UploadReportTemplateModal({ + show, + onHide, + onUpload, + addDangerToast, + addSuccessToast, +}: UploadReportTemplateModalProps) { + const [name, setName] = useState(''); + const [dataset, setDataset] = useState<{ label: string; value: number } | null>(null); + const [description, setDescription] = useState(''); + const [file, setFile] = useState<File | null>(null); + + const reset = () => { + setName(''); + setDataset(null); + setDescription(''); + setFile(null); + }; + + const handleUpload = () => { + if (!file || !name || !dataset) { + addDangerToast(t('All fields are required')); + return; + } + const form = new FormData(); + form.append('template', file); + form.append('name', name); + form.append('dataset_id', String(dataset.value)); + if (description) form.append('description', description); + SupersetClient.post({ + endpoint: '/api/v1/report_template/', + postPayload: form, + stringify: false, + }) Review Comment: ### Missing Upload Progress and Retry Logic <sub></sub> <details> <summary>Tell me more</summary> ###### What is the issue? File upload lacks upload progress tracking and doesn't implement retry logic for failed uploads. ###### Why this matters Large file uploads can fail due to network issues, and without progress tracking, users have no feedback during long uploads. ###### Suggested change ∙ *Feature Preview* Implement upload progress tracking and retry logic using XMLHttpRequest or fetch with a custom upload handler: ```typescript const uploadWithProgress = async (form: FormData) => { const xhr = new XMLHttpRequest(); xhr.upload.onprogress = (event) => { const progress = (event.loaded / event.total) * 100; // Update progress state }; // Add retry logic and return promise }; ``` ###### Provide feedback to improve future suggestions [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/cfc72b93-ecb6-496e-a01f-35ccefa374bd/upvote) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/cfc72b93-ecb6-496e-a01f-35ccefa374bd?what_not_true=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/cfc72b93-ecb6-496e-a01f-35ccefa374bd?what_out_of_scope=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/cfc72b93-ecb6-496e-a01f-35ccefa374bd?what_not_in_standard=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/cfc72b93-ecb6-496e-a01f-35ccefa374bd) </details> <sub> 💬 Looking for more details? Reply to this comment to chat with Korbit. </sub> <!--- korbi internal id:2e48d3d8-2878-421a-bd8c-f9ed4732ce65 --> [](2e48d3d8-2878-421a-bd8c-f9ed4732ce65) ########## superset/migrations/versions/2025-06-07_00-00_b4757fa23b89_add_report_templates_table.py: ########## @@ -0,0 +1,52 @@ +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. +"""add report templates table + +Revision ID: b4757fa23b89 +Revises: 363a9b1e8992 +Create Date: 2025-06-07 00:00:00.000000 + +""" + +import sqlalchemy as sa +from alembic import op + +from superset.migrations.shared.utils import create_table + +# revision identifiers, used by Alembic. +revision = "b4757fa23b89" +down_revision = "363a9b1e8992" + + +def upgrade() -> None: + create_table( + "report_templates", + sa.Column("id", sa.Integer(), nullable=False), + sa.Column("created_on", sa.DateTime(), nullable=True), + sa.Column("changed_on", sa.DateTime(), nullable=True), + sa.Column("name", sa.String(length=250), nullable=False), + sa.Column("description", sa.Text(), nullable=True), + sa.Column("dataset_id", sa.Integer(), sa.ForeignKey("tables.id"), nullable=False), + sa.Column("template_path", sa.String(length=1024), nullable=False), + sa.Column("changed_by_fk", sa.Integer(), nullable=True), + sa.Column("created_by_fk", sa.Integer(), nullable=True), Review Comment: ### Missing Foreign Key Constraints for User References <sub></sub> <details> <summary>Tell me more</summary> ###### What is the issue? The foreign key relationships for changed_by_fk and created_by_fk columns are not properly defined with ForeignKey constraints to the users table. ###### Why this matters Without proper foreign key constraints, data integrity cannot be maintained, potentially leading to orphaned references when users are deleted or invalid user IDs being stored. ###### Suggested change ∙ *Feature Preview* Add proper foreign key constraints for the user reference columns: ```python sa.Column("changed_by_fk", sa.Integer(), sa.ForeignKey("ab_user.id"), nullable=True), sa.Column("created_by_fk", sa.Integer(), sa.ForeignKey("ab_user.id"), nullable=True), ``` ###### Provide feedback to improve future suggestions [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/678bd359-195a-4305-8fe4-b16382055017/upvote) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/678bd359-195a-4305-8fe4-b16382055017?what_not_true=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/678bd359-195a-4305-8fe4-b16382055017?what_out_of_scope=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/678bd359-195a-4305-8fe4-b16382055017?what_not_in_standard=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/678bd359-195a-4305-8fe4-b16382055017) </details> <sub> 💬 Looking for more details? Reply to this comment to chat with Korbit. </sub> <!--- korbi internal id:08d61846-4f51-408a-ac9a-ba2f74872f1f --> [](08d61846-4f51-408a-ac9a-ba2f74872f1f) ########## superset-frontend/src/features/reportTemplates/UploadReportTemplateModal.tsx: ########## @@ -0,0 +1,107 @@ +import { useState } from 'react'; +import { t, SupersetClient } from '@superset-ui/core'; +import { Modal, Input, Button } from '@superset-ui/core/components'; +import DatasetSelect from 'src/dashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigForm/DatasetSelect'; + +interface UploadReportTemplateModalProps { + show: boolean; + onHide: () => void; + onUpload: () => void; + addDangerToast: (msg: string) => void; + addSuccessToast: (msg: string) => void; +} + +export default function UploadReportTemplateModal({ + show, + onHide, + onUpload, + addDangerToast, + addSuccessToast, +}: UploadReportTemplateModalProps) { + const [name, setName] = useState(''); + const [dataset, setDataset] = useState<{ label: string; value: number } | null>(null); + const [description, setDescription] = useState(''); + const [file, setFile] = useState<File | null>(null); + + const reset = () => { + setName(''); + setDataset(null); + setDescription(''); + setFile(null); + }; + + const handleUpload = () => { + if (!file || !name || !dataset) { + addDangerToast(t('All fields are required')); + return; + } Review Comment: ### Misleading Form Validation Message <sub></sub> <details> <summary>Tell me more</summary> ###### What is the issue? The validation message states 'All fields are required' but the description field is not included in the validation check, despite being part of the form. ###### Why this matters Users might be confused when they see 'All fields are required' but can actually submit the form without a description. This inconsistency between the message and actual validation requirements could lead to user confusion. ###### Suggested change ∙ *Feature Preview* Either update the validation message to be more specific about required fields or include description in the validation if it should be required: ```typescript if (!file || !name || !dataset) { addDangerToast(t('Name, dataset, and file are required')); return; } ``` ###### Provide feedback to improve future suggestions [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/76b93c90-a8c0-457f-9273-9b4c6d2d6a81/upvote) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/76b93c90-a8c0-457f-9273-9b4c6d2d6a81?what_not_true=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/76b93c90-a8c0-457f-9273-9b4c6d2d6a81?what_out_of_scope=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/76b93c90-a8c0-457f-9273-9b4c6d2d6a81?what_not_in_standard=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/76b93c90-a8c0-457f-9273-9b4c6d2d6a81) </details> <sub> 💬 Looking for more details? Reply to this comment to chat with Korbit. </sub> <!--- korbi internal id:c911a241-04ad-4830-a9c1-666d371227b1 --> [](c911a241-04ad-4830-a9c1-666d371227b1) ########## superset/migrations/versions/2025-06-07_00-00_b4757fa23b89_add_report_templates_table.py: ########## @@ -0,0 +1,52 @@ +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. +"""add report templates table + +Revision ID: b4757fa23b89 +Revises: 363a9b1e8992 +Create Date: 2025-06-07 00:00:00.000000 + +""" Review Comment: ### Migration purpose not documented <sub></sub> <details> <summary>Tell me more</summary> ###### What is the issue? The migration docstring only states what the migration does without explaining the purpose or impact of adding the report templates table. ###### Why this matters Without understanding the purpose of the report templates feature, future developers may have difficulty maintaining or modifying this schema. ###### Suggested change ∙ *Feature Preview* """add report templates table Creates a table to store reusable report templates that can be associated with specific datasets. These templates define the structure and formatting of generated reports. Revision ID: b4757fa23b89 Revises: 363a9b1e8992 Create Date: 2025-06-07 00:00:00.000000 """ ###### Provide feedback to improve future suggestions [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/912e847d-e51d-416d-ad13-c9be473379cd/upvote) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/912e847d-e51d-416d-ad13-c9be473379cd?what_not_true=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/912e847d-e51d-416d-ad13-c9be473379cd?what_out_of_scope=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/912e847d-e51d-416d-ad13-c9be473379cd?what_not_in_standard=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/912e847d-e51d-416d-ad13-c9be473379cd) </details> <sub> 💬 Looking for more details? Reply to this comment to chat with Korbit. </sub> <!--- korbi internal id:962091a0-dfd8-42ce-a579-d2c39f8c5f22 --> [](962091a0-dfd8-42ce-a579-d2c39f8c5f22) -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
