korbit-ai[bot] commented on code in PR #32969: URL: https://github.com/apache/superset/pull/32969#discussion_r2024004503
########## superset/migrations/versions/2025-03-25_21-03_bd200ee2fd0c_adding_publishing_filed_to_chart.py: ########## @@ -0,0 +1,42 @@ +# 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. +"""adding_publishing_filed_to_chart + +Revision ID: bd200ee2fd0c +Revises: 32bf93dfe2a4 +Create Date: 2025-03-25 21:03:10.362333 + +""" + +# revision identifiers, used by Alembic. +revision = "bd200ee2fd0c" +down_revision = "32bf93dfe2a4" + +from alembic import op +import sqlalchemy as sa +from sqlalchemy.dialects import postgresql Review Comment: ### Unused Import <sub></sub> <details> <summary>Tell me more</summary> ###### What is the issue? Unused import 'postgresql' from sqlalchemy.dialects. ###### Why this matters Unused imports make the code less clean and can mislead developers about dependencies actually needed by the module. ###### Suggested change ∙ *Feature Preview* Remove the line: ```python from sqlalchemy.dialects import postgresql ``` ###### Provide feedback to improve future suggestions [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/c5c7d955-23f7-4a2d-8a76-4a51c4415be7/upvote) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/c5c7d955-23f7-4a2d-8a76-4a51c4415be7?what_not_true=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/c5c7d955-23f7-4a2d-8a76-4a51c4415be7?what_out_of_scope=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/c5c7d955-23f7-4a2d-8a76-4a51c4415be7?what_not_in_standard=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/c5c7d955-23f7-4a2d-8a76-4a51c4415be7) </details> <sub> 💬 Looking for more details? Reply to this comment to chat with Korbit. </sub> <!--- korbi internal id:438c1758-5072-4335-a891-0c3e0631897d --> [](438c1758-5072-4335-a891-0c3e0631897d) ########## superset/migrations/versions/2025-03-25_21-03_bd200ee2fd0c_adding_publishing_filed_to_chart.py: ########## @@ -0,0 +1,42 @@ +# 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. +"""adding_publishing_filed_to_chart + +Revision ID: bd200ee2fd0c +Revises: 32bf93dfe2a4 +Create Date: 2025-03-25 21:03:10.362333 + +""" + +# revision identifiers, used by Alembic. +revision = "bd200ee2fd0c" +down_revision = "32bf93dfe2a4" + +from alembic import op +import sqlalchemy as sa +from sqlalchemy.dialects import postgresql + + +def upgrade(): + op.add_column( + "slices", sa.Column("published", sa.Boolean(), nullable=True, default=False) + ) + op.execute("UPDATE slices SET published=false") Review Comment: ### Inefficient Two-Pass Table Operation <sub></sub> <details> <summary>Tell me more</summary> ###### What is the issue? The column addition and data update are executed as separate operations, requiring two passes over the table. ###### Why this matters On large tables, performing separate column addition and update operations can significantly impact migration performance and lock the table for a longer duration. ###### Suggested change ∙ *Feature Preview* Combine the column addition and default value setting in a single operation using server_default: ```python op.add_column( "slices", sa.Column("published", sa.Boolean(), nullable=True, server_default=sa.false()) ) ``` ###### Provide feedback to improve future suggestions [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/1fbf66a4-c0ab-4df9-ba51-8cb973f95e12/upvote) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/1fbf66a4-c0ab-4df9-ba51-8cb973f95e12?what_not_true=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/1fbf66a4-c0ab-4df9-ba51-8cb973f95e12?what_out_of_scope=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/1fbf66a4-c0ab-4df9-ba51-8cb973f95e12?what_not_in_standard=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/1fbf66a4-c0ab-4df9-ba51-8cb973f95e12) </details> <sub> 💬 Looking for more details? Reply to this comment to chat with Korbit. </sub> <!--- korbi internal id:ded671b2-d760-492e-a6c3-03a3e5626110 --> [](ded671b2-d760-492e-a6c3-03a3e5626110) ########## superset/migrations/versions/2025-03-25_21-03_bd200ee2fd0c_adding_publishing_filed_to_chart.py: ########## @@ -0,0 +1,42 @@ +# 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. +"""adding_publishing_filed_to_chart Review Comment: ### Typo in Migration Name <sub></sub> <details> <summary>Tell me more</summary> ###### What is the issue? There is a typo in the migration name where 'filed' should be 'field'. ###### Why this matters Migration names are used for documentation and tracking purposes. A typo in the name could cause confusion when other developers need to reference or understand this migration later. ###### Suggested change ∙ *Feature Preview* ```python "adding_publishing_field_to_chart" ``` ###### Provide feedback to improve future suggestions [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/2a6f20b6-f813-40fd-9a80-37d1d60961b1/upvote) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/2a6f20b6-f813-40fd-9a80-37d1d60961b1?what_not_true=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/2a6f20b6-f813-40fd-9a80-37d1d60961b1?what_out_of_scope=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/2a6f20b6-f813-40fd-9a80-37d1d60961b1?what_not_in_standard=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/2a6f20b6-f813-40fd-9a80-37d1d60961b1) </details> <sub> 💬 Looking for more details? Reply to this comment to chat with Korbit. </sub> <!--- korbi internal id:e7befd60-49c4-4dd4-9bf9-79aeb0f1697e --> [](e7befd60-49c4-4dd4-9bf9-79aeb0f1697e) ########## superset/migrations/versions/2025-03-25_21-03_bd200ee2fd0c_adding_publishing_filed_to_chart.py: ########## @@ -0,0 +1,42 @@ +# 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. +"""adding_publishing_filed_to_chart + +Revision ID: bd200ee2fd0c +Revises: 32bf93dfe2a4 +Create Date: 2025-03-25 21:03:10.362333 + +""" + +# revision identifiers, used by Alembic. +revision = "bd200ee2fd0c" +down_revision = "32bf93dfe2a4" + +from alembic import op +import sqlalchemy as sa +from sqlalchemy.dialects import postgresql + + +def upgrade(): + op.add_column( + "slices", sa.Column("published", sa.Boolean(), nullable=True, default=False) + ) + op.execute("UPDATE slices SET published=false") Review Comment: ### Raw SQL Execution in Migration <sub></sub> <details> <summary>Tell me more</summary> ###### What is the issue? Direct SQL execution without parameterization in a migration script ###### Why this matters While this specific UPDATE statement doesn't use external input and sets a fixed value, using raw SQL execution can establish poor security patterns. In migrations involving user data or variables, this pattern could lead to SQL injection vulnerabilities. ###### Suggested change ∙ *Feature Preview* ```python with op.get_bind() as conn: conn.execute(sa.text("UPDATE slices SET published=:value"), {"value": False}) ``` ###### Provide feedback to improve future suggestions [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/ac01ecca-2185-4964-8ebc-6761e02e0948/upvote) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/ac01ecca-2185-4964-8ebc-6761e02e0948?what_not_true=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/ac01ecca-2185-4964-8ebc-6761e02e0948?what_out_of_scope=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/ac01ecca-2185-4964-8ebc-6761e02e0948?what_not_in_standard=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/ac01ecca-2185-4964-8ebc-6761e02e0948) </details> <sub> 💬 Looking for more details? Reply to this comment to chat with Korbit. </sub> <!--- korbi internal id:c5e0d140-950b-4f82-ad2d-0c7f8d3ab354 --> [](c5e0d140-950b-4f82-ad2d-0c7f8d3ab354) ########## superset-frontend/src/explore/components/ExploreChartPublish/index.tsx: ########## @@ -0,0 +1,95 @@ +/** + * 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. + */ +import { useState } from 'react'; +import { SupersetClient, t } from '@superset-ui/core'; +import { Tooltip } from 'src/components/Tooltip'; +import { PublishedLabel } from 'src/components/Label'; + +export type ChartPublishedStatusType = { + sliceId: number; + userCanOverwrite: boolean; + isPublished: boolean; +}; + +const draftButtonTooltip = t( + 'This chart is in draft. This indicated this chart is a work in progress.', +); Review Comment: ### Grammatical Error in UI Message <sub></sub> <details> <summary>Tell me more</summary> ###### What is the issue? There's a typo in the draft tooltip message that affects user understanding ('indicated' should be 'indicates'). ###### Why this matters Incorrect grammar in UI messages reduces professionalism and can confuse users, especially non-native English speakers. ###### Suggested change ∙ *Feature Preview* Correct the tooltip message: ```typescript const draftButtonTooltip = t( 'This chart is in draft. This indicates this chart is a work in progress.', ); ``` ###### Provide feedback to improve future suggestions [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/32ab0294-efd8-4721-8208-f1b008adc998/upvote) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/32ab0294-efd8-4721-8208-f1b008adc998?what_not_true=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/32ab0294-efd8-4721-8208-f1b008adc998?what_out_of_scope=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/32ab0294-efd8-4721-8208-f1b008adc998?what_not_in_standard=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/32ab0294-efd8-4721-8208-f1b008adc998) </details> <sub> 💬 Looking for more details? Reply to this comment to chat with Korbit. </sub> <!--- korbi internal id:12ebe4f0-a97e-4c4d-a3b7-59ea992d94e3 --> [](12ebe4f0-a97e-4c4d-a3b7-59ea992d94e3) -- 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: notifications-unsubscr...@superset.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org --------------------------------------------------------------------- To unsubscribe, e-mail: notifications-unsubscr...@superset.apache.org For additional commands, e-mail: notifications-h...@superset.apache.org
