[GitHub] [superset] villebro commented on a diff in pull request #23170: feat(alert/reports): Pdf as attachment for alert and report

["villebro (via GitHub)" <gi...@apache.org>]

villebro commented on code in PR #23170:
URL: https://github.com/apache/superset/pull/23170#discussion_r1118476589


##########
superset-frontend/src/views/CRUD/alert/AlertReportModal.tsx:
##########
@@ -1451,6 +1449,23 @@ const AlertReportModal: FunctionComponent<AlertReportModalProps> = ({
                 </StyledCheckbox>
               </div>
             )}
+            {dashboardFormatOptionEnabled && (
+              <>
+                <div className="inline-container">
+                  <StyledRadioGroup
+                    onChange={onFormatChange}
+                    value={reportFormat}
+                  >
+                    <StyledRadio value="PNG">
+                      {TRANSLATIONS.SEND_AS_PNG_TEXT}
+                    </StyledRadio>
+                    <StyledRadio value="PDF">
+                      {TRANSLATIONS.SEND_AS_PDF_TEXT}
+                    </StyledRadio>
+                  </StyledRadioGroup>
+                </div>
+              </>
+            )}

Review Comment:
   This is duplicating the chart specific formatting logic on 1418-1439 (=increasing code maintenance debt). I think we should consolidate these two.



##########
superset-frontend/src/views/CRUD/alert/AlertReportModal.tsx:
##########
@@ -490,6 +491,10 @@ const AlertReportModal: FunctionComponent<AlertReportModalProps> = ({
     contentType === 'chart' &&
     (isFeatureEnabled(FeatureFlag.ALERTS_ATTACH_REPORTS) || isReport);
 
+  const dashboardFormatOptionEnabled =
+    contentType === 'dashboard' &&
+    (isFeatureEnabled(FeatureFlag.ALERTS_ATTACH_REPORTS) || isReport);
+

Review Comment:
   If we really want to introduce a new variable `dashboardFormatOptionEnabled` we really should rename the old one on line 490 to `chartFormatOptionEnabled`.



##########
superset/reports/notifications/base.py:
##########
@@ -28,7 +28,8 @@
 class NotificationContent:
     name: str
     header_data: HeaderDataType  # this is optional to account for error states
-    csv: Optional[bytes] = None  # bytes for csv file
+    data: Optional[bytes] = None  # bytes for data attachment
+    data_format: Optional[str] = None  # data attachment format (csv, xlsx, pdf, etc)

Review Comment:
   I think we should use an `Enum` or `Literal` as the type here to make it detectable by the linter if a `data_format` is valid.



-- 
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-unsubscribe@superset.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org