gabotorresruiz commented on code in PR #35832:
URL: https://github.com/apache/superset/pull/35832#discussion_r2479843464
##########
superset-frontend/src/features/alerts/components/NotificationMethod.tsx:
##########
@@ -249,84 +215,105 @@ export const NotificationMethod:
FunctionComponent<NotificationMethodProps> = ({
}
};
- const fetchSlackChannels = async ({
- searchString = '',
- types = [],
- exactMatch = false,
- force = false,
- }: {
- searchString?: string | undefined;
- types?: string[];
- exactMatch?: boolean | undefined;
- force?: boolean | undefined;
- } = {}): Promise<JsonResponse> => {
- const queryString = rison.encode({
- searchString,
- types,
- exactMatch,
- force,
- });
- const endpoint = `/api/v1/report/slack_channels/?q=${queryString}`;
- return SupersetClient.get({ endpoint });
- };
+ const fetchSlackChannels = async (
+ search: string,
+ page: number,
+ pageSize: number,
+ ): Promise<{
+ data: { label: string; value: string }[];
+ totalCount: number;
+ }> => {
+ try {
+ const cacheKey = `${search}:${page}`;
+
+ if (dataCache.current[cacheKey] && !forceRefreshRef.current) {
+ return dataCache.current[cacheKey];
+ }
- const updateSlackOptions = async ({
- force,
- }: {
- force?: boolean | undefined;
- } = {}) => {
- setIsSlackChannelsLoading(true);
- fetchSlackChannels({ types: ['public_channel', 'private_channel'], force })
- .then(({ json }) => {
- const { result } = json;
- const options: SlackOptionsType = mapChannelsToOptions(result);
-
- setSlackOptions(options);
-
- if (isFeatureEnabled(FeatureFlag.AlertReportSlackV2)) {
- // for edit mode, map existing ids to names for display if slack v2
- // or names to ids if slack v1
- const [publicOptions, privateOptions] = options;
- if (
- method &&
- [
- NotificationMethodOption.SlackV2,
- NotificationMethodOption.Slack,
- ].includes(method)
- ) {
- setSlackRecipients(
- mapSlackValues({
- method,
- recipientValue,
- slackOptions: [
- ...publicOptions.options,
- ...privateOptions.options,
- ],
- }),
- );
- }
- }
- })
- .catch(e => {
- // Fallback to slack v1 if slack v2 is not compatible
- setUseSlackV1(true);
- })
- .finally(() => {
- setMethodOptionsLoading(false);
- setIsSlackChannelsLoading(false);
+ const cursor = page > 0 ? cursorRef.current[cacheKey] : null;
+
+ const params: Record<string, any> = {
+ types: ['public_channel', 'private_channel'],
+ limit: pageSize,
+ };
+
+ if (page === 0 && forceRefreshRef.current) {
+ params.force = true;
+ forceRefreshRef.current = false;
+ }
+
+ if (search) {
+ params.search_string = search;
+ }
+
+ if (cursor) {
+ params.cursor = cursor;
+ }
+
+ const queryString = rison.encode(params);
+ const endpoint = `/api/v1/report/slack_channels/?q=${queryString}`;
+ const response = await SupersetClient.get({ endpoint });
+
+ const { result, next_cursor, has_more } = response.json;
+
+ if (next_cursor) {
+ cursorRef.current[`${search}:${page + 1}`] = next_cursor;
+ }
+
+ const options = result.map((channel: SlackChannel) => ({
+ label: channel.name,
+ value: channel.id,
+ }));
+
+ const totalCount = has_more
+ ? (page + 1) * pageSize + 1
+ : page * pageSize + options.length;
+
+ const responseData = {
+ data: options,
+ totalCount,
+ };
+
+ dataCache.current[cacheKey] = responseData;
+
+ return responseData;
+ } catch (error) {
+ console.error('Failed to fetch Slack channels:', error);
Review Comment:
You're absolutely right. Let me update this to use `logging.error`
##########
superset-frontend/src/features/alerts/components/NotificationMethod.tsx:
##########
@@ -249,84 +215,105 @@ export const NotificationMethod:
FunctionComponent<NotificationMethodProps> = ({
}
};
- const fetchSlackChannels = async ({
- searchString = '',
- types = [],
- exactMatch = false,
- force = false,
- }: {
- searchString?: string | undefined;
- types?: string[];
- exactMatch?: boolean | undefined;
- force?: boolean | undefined;
- } = {}): Promise<JsonResponse> => {
- const queryString = rison.encode({
- searchString,
- types,
- exactMatch,
- force,
- });
- const endpoint = `/api/v1/report/slack_channels/?q=${queryString}`;
- return SupersetClient.get({ endpoint });
- };
+ const fetchSlackChannels = async (
+ search: string,
+ page: number,
+ pageSize: number,
+ ): Promise<{
+ data: { label: string; value: string }[];
+ totalCount: number;
+ }> => {
+ try {
+ const cacheKey = `${search}:${page}`;
+
+ if (dataCache.current[cacheKey] && !forceRefreshRef.current) {
+ return dataCache.current[cacheKey];
+ }
- const updateSlackOptions = async ({
- force,
- }: {
- force?: boolean | undefined;
- } = {}) => {
- setIsSlackChannelsLoading(true);
- fetchSlackChannels({ types: ['public_channel', 'private_channel'], force })
- .then(({ json }) => {
- const { result } = json;
- const options: SlackOptionsType = mapChannelsToOptions(result);
-
- setSlackOptions(options);
-
- if (isFeatureEnabled(FeatureFlag.AlertReportSlackV2)) {
- // for edit mode, map existing ids to names for display if slack v2
- // or names to ids if slack v1
- const [publicOptions, privateOptions] = options;
- if (
- method &&
- [
- NotificationMethodOption.SlackV2,
- NotificationMethodOption.Slack,
- ].includes(method)
- ) {
- setSlackRecipients(
- mapSlackValues({
- method,
- recipientValue,
- slackOptions: [
- ...publicOptions.options,
- ...privateOptions.options,
- ],
- }),
- );
- }
- }
- })
- .catch(e => {
- // Fallback to slack v1 if slack v2 is not compatible
- setUseSlackV1(true);
- })
- .finally(() => {
- setMethodOptionsLoading(false);
- setIsSlackChannelsLoading(false);
+ const cursor = page > 0 ? cursorRef.current[cacheKey] : null;
+
+ const params: Record<string, any> = {
+ types: ['public_channel', 'private_channel'],
+ limit: pageSize,
+ };
+
+ if (page === 0 && forceRefreshRef.current) {
+ params.force = true;
+ forceRefreshRef.current = false;
+ }
+
+ if (search) {
+ params.search_string = search;
+ }
+
+ if (cursor) {
+ params.cursor = cursor;
+ }
+
+ const queryString = rison.encode(params);
+ const endpoint = `/api/v1/report/slack_channels/?q=${queryString}`;
+ const response = await SupersetClient.get({ endpoint });
+
+ const { result, next_cursor, has_more } = response.json;
+
+ if (next_cursor) {
+ cursorRef.current[`${search}:${page + 1}`] = next_cursor;
+ }
+
+ const options = result.map((channel: SlackChannel) => ({
+ label: channel.name,
+ value: channel.id,
+ }));
+
+ const totalCount = has_more
+ ? (page + 1) * pageSize + 1
+ : page * pageSize + options.length;
+
+ const responseData = {
+ data: options,
+ totalCount,
+ };
+
+ dataCache.current[cacheKey] = responseData;
+
+ return responseData;
+ } catch (error) {
+ console.error('Failed to fetch Slack channels:', error);
+ setUseSlackV1(true);
+ onMethodChange({
+ label: NotificationMethodOption.Slack,
+ value: NotificationMethodOption.Slack,
});
+ throw error;
+ }
};
- useEffect(() => {
- const slackEnabled = options?.some(
- option =>
- option === NotificationMethodOption.Slack ||
- option === NotificationMethodOption.SlackV2,
- );
- if (slackEnabled && !slackOptions[0]?.options.length) {
- updateSlackOptions();
+ const handleRefreshSlackChannels = async () => {
+ setIsRefreshing(true);
+
+ try {
+ forceRefreshRef.current = true;
+ cursorRef.current = {};
+ dataCache.current = {};
+
+ setSlackRecipients([]);
+
+ if (onUpdate && setting) {
+ onUpdate(index, {
+ ...setting,
+ recipients: '',
+ } as NotificationSetting);
+ }
+
+ await fetchSlackChannels('', 0, 100);
+
+ setRefreshKey(prev => prev + 1);
+ } catch (error) {
+ console.error('Error refreshing channels:', error);
Review Comment:
Same as the comment above!
##########
superset-frontend/src/features/alerts/components/NotificationMethod.tsx:
##########
@@ -249,84 +215,105 @@ export const NotificationMethod:
FunctionComponent<NotificationMethodProps> = ({
}
};
- const fetchSlackChannels = async ({
- searchString = '',
- types = [],
- exactMatch = false,
- force = false,
- }: {
- searchString?: string | undefined;
- types?: string[];
- exactMatch?: boolean | undefined;
- force?: boolean | undefined;
- } = {}): Promise<JsonResponse> => {
- const queryString = rison.encode({
- searchString,
- types,
- exactMatch,
- force,
- });
- const endpoint = `/api/v1/report/slack_channels/?q=${queryString}`;
- return SupersetClient.get({ endpoint });
- };
+ const fetchSlackChannels = async (
Review Comment:
Good catch! Let me take a look
--
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]
