nytai commented on a change in pull request #11937:
URL:
https://github.com/apache/incubator-superset/pull/11937#discussion_r538857515
##########
File path: superset-frontend/src/views/CRUD/alert/AlertList.tsx
##########
@@ -118,49 +105,17 @@ function AlertList({
refreshData();
}, [isReportEnabled]);
+ const gotoExecutionLog = (id: number, type: string) => {
+ window.location.href = `/${type.toLowerCase()}/${id}/log`;
Review comment:
We should use react-router for this so we don't have to wait for a full
page reload.
##########
File path: superset-frontend/src/views/CRUD/alert/ExecutionLog.tsx
##########
@@ -0,0 +1,181 @@
+/**
+ * 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 React, { useMemo, useEffect, useCallback, useState } from 'react';
+import { useParams, Link, useHistory } from 'react-router-dom';
+import moment from 'moment';
+import { styled, t, SupersetClient } from '@superset-ui/core';
+
+import AlertStatusIcon from 'src/views/CRUD/alert/components/AlertStatusIcon';
+import ListView from 'src/components/ListView';
+import SubMenu from 'src/components/Menu/SubMenu';
+import getClientErrorObject from 'src/utils/getClientErrorObject';
+import withToasts from 'src/messageToasts/enhancers/withToasts';
+import { fDuration } from 'src/modules/dates';
+import { useListViewResource } from 'src/views/CRUD/hooks';
+
+import { LogObject } from './types';
+
+const PAGE_SIZE = 25;
+
+interface ExecutionLogProps {
+ addDangerToast: (msg: string) => void;
+ addSuccessToast: (msg: string) => void;
+ isReportEnabled: boolean;
+}
+
+function ExecutionLog({ addDangerToast, isReportEnabled }: ExecutionLogProps) {
+ const { alertId }: any = useParams();
+ const {
+ state: { loading, resourceCount: LogCount, resourceCollection: logs },
+ fetchData,
+ } = useListViewResource<LogObject>(
+ `report/${alertId}/log`,
+ t('log'),
+ addDangerToast,
+ false,
+ );
+ const [alertName, setAlertName] = useState<string>('');
+ const [executionLogType, setExecutionLogType] = useState<'Report' | 'Alert'>(
+ 'Report',
+ );
+ const StyledHeader = styled.div`
+ display: flex;
+ flex-direction: row;
+
+ a,
+ Link {
+ margin-left: 16px;
+ font-size: 12px;
+ font-weight: normal;
+ text-decoration: underline;
+ }
+ `;
+
+ let hasHistory = true;
+
+ try {
+ useHistory();
+ } catch (err) {
+ // If error is thrown, we know not to use <Link> in render
+ hasHistory = false;
Review comment:
When would we encounter this? Off the top of my head, I can't think of a
scenario where this page is not rendered in a router context as it's part of
the crud_views package which has react-router.
##########
File path: superset-frontend/src/views/CRUD/alert/AlertList.tsx
##########
@@ -219,14 +174,17 @@ function AlertList({
Cell: ({ row: { original } }: any) => {
const handleEdit = () => {}; // handleAnnotationEdit(original);
const handleDelete = () => {}; //
setAlertCurrentlyDeleting(original);
+ const hanldGotoExecutionLog = () =>
Review comment:
typo? `hanldGotoExecutionLog` =>`handleGoToExecutionLog`
##########
File path: superset-frontend/src/views/CRUD/alert/ExecutionLog.tsx
##########
@@ -0,0 +1,181 @@
+/**
+ * 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 React, { useMemo, useEffect, useCallback, useState } from 'react';
+import { useParams, Link, useHistory } from 'react-router-dom';
+import moment from 'moment';
+import { styled, t, SupersetClient } from '@superset-ui/core';
+
+import AlertStatusIcon from 'src/views/CRUD/alert/components/AlertStatusIcon';
+import ListView from 'src/components/ListView';
+import SubMenu from 'src/components/Menu/SubMenu';
+import getClientErrorObject from 'src/utils/getClientErrorObject';
+import withToasts from 'src/messageToasts/enhancers/withToasts';
+import { fDuration } from 'src/modules/dates';
+import { useListViewResource } from 'src/views/CRUD/hooks';
+
+import { LogObject } from './types';
+
+const PAGE_SIZE = 25;
+
+interface ExecutionLogProps {
+ addDangerToast: (msg: string) => void;
+ addSuccessToast: (msg: string) => void;
+ isReportEnabled: boolean;
+}
+
+function ExecutionLog({ addDangerToast, isReportEnabled }: ExecutionLogProps) {
+ const { alertId }: any = useParams();
+ const {
+ state: { loading, resourceCount: LogCount, resourceCollection: logs },
Review comment:
nit: regular variables shouldn't be capitalized
##########
File path: superset-frontend/src/views/CRUD/alert/ExecutionLog.tsx
##########
@@ -0,0 +1,181 @@
+/**
+ * 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 React, { useMemo, useEffect, useCallback, useState } from 'react';
+import { useParams, Link, useHistory } from 'react-router-dom';
+import moment from 'moment';
+import { styled, t, SupersetClient } from '@superset-ui/core';
+
+import AlertStatusIcon from 'src/views/CRUD/alert/components/AlertStatusIcon';
+import ListView from 'src/components/ListView';
+import SubMenu from 'src/components/Menu/SubMenu';
+import getClientErrorObject from 'src/utils/getClientErrorObject';
+import withToasts from 'src/messageToasts/enhancers/withToasts';
+import { fDuration } from 'src/modules/dates';
+import { useListViewResource } from 'src/views/CRUD/hooks';
+
+import { LogObject } from './types';
+
+const PAGE_SIZE = 25;
+
+interface ExecutionLogProps {
+ addDangerToast: (msg: string) => void;
+ addSuccessToast: (msg: string) => void;
+ isReportEnabled: boolean;
+}
+
+function ExecutionLog({ addDangerToast, isReportEnabled }: ExecutionLogProps) {
+ const { alertId }: any = useParams();
+ const {
+ state: { loading, resourceCount: LogCount, resourceCollection: logs },
+ fetchData,
+ } = useListViewResource<LogObject>(
+ `report/${alertId}/log`,
+ t('log'),
+ addDangerToast,
+ false,
+ );
+ const [alertName, setAlertName] = useState<string>('');
+ const [executionLogType, setExecutionLogType] = useState<'Report' | 'Alert'>(
+ 'Report',
+ );
+ const StyledHeader = styled.div`
+ display: flex;
+ flex-direction: row;
+
+ a,
+ Link {
+ margin-left: 16px;
+ font-size: 12px;
+ font-weight: normal;
+ text-decoration: underline;
+ }
+ `;
+
+ let hasHistory = true;
+
+ try {
+ useHistory();
+ } catch (err) {
+ // If error is thrown, we know not to use <Link> in render
+ hasHistory = false;
+ }
+
+ const fetchAlert = useCallback(
+ async function fetchAlert() {
+ try {
+ const response = await SupersetClient.get({
+ endpoint: `/api/v1/report/${alertId}`,
+ });
+ setAlertName(response.json.result.name);
+ setExecutionLogType(response.json.result.type);
+ } catch (response) {
+ await getClientErrorObject(response).then(({ message }: any) => {
+ addDangerToast(message || t('Sorry, An error occurred'));
+ });
+ }
+ },
+ [alertId],
+ );
Review comment:
we could probably replace this with a `useSingleViewResource` call from
the `CRUD/hooks.ts` file and save a few lines.
##########
File path: superset-frontend/src/views/CRUD/alert/components/AlertStatusIcon.tsx
##########
@@ -0,0 +1,75 @@
+/**
+ * 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 React from 'react';
+import { t, styled } from '@superset-ui/core';
+
+import Icon, { IconName } from 'src/components/Icon';
+import { Tooltip } from 'src/common/components/Tooltip';
+
+const StatusIcon = styled(Icon)<{ status: string }>`
+ color: ${({ status, theme }) => {
+ switch (status) {
+ case 'Working':
Review comment:
You might want to make this into an enum to avoid any potential typos in
the future. Something like
```typescript
enum AlertState {
Working = 'Working',
Error = 'Error',
Success = 'Success',
}
```
I believe @dpgaspar recently added another state, so having an enum is good
documentation for all the different states.
##########
File path: superset-frontend/src/views/CRUD/alert/types.ts
##########
@@ -44,3 +44,13 @@ export type AlertObject = {
recipients?: recipients;
type?: string;
};
+
+export type LogObject = {
+ end_dttm: string;
+ error_message: string;
+ id: number;
+ scheduled_dttm: string;
+ start_dttm: string;
+ state: string;
Review comment:
can we type these out too?
```suggestion
state: 'Success' | 'Working' | 'Error';
```
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
