Failed or Canceled Export Retry Button #2975

krishnaglick · 2022-02-15T20:56:03Z

Change description

Description here

Checklists

Development

Application changes have been tested appropriately

Impact

Code follows company security practices and guidelines
Security impact of change has been considered
Performance impact of change has been considered
Possible migration needs considered (model migrations, config migrations, etc.)

Please explain any security, performance, migration, or other impacts if relevant:

Code review

Pull request has a descriptive title and context useful to a reviewer. Screenshots or screencasts are attached where applicable.
Relevant tags have been added to the PR (bug, enhancement, internal, etc.)

krishnaglick

The list and the detail view have different ways to trigger the same thing. Could replicate the icon method on the detail view, or try to fit the button on the list.

krishnaglick · 2022-02-15T20:57:04Z

core/src/modules/ops/export.ts

@@ -175,7 +175,7 @@ export namespace ExportOps {
    saveExports = true
  ) {
    const where: WhereOptions<Export> = {
-      state: "failed",
+      state: { [Op.in]: ["failed", "canceled"] },


@pedroslopez This is a change in existing functionality, I'm unsure if it's a problem.
I can move this logic out so retried canceled exports tell it explicitly to run them.

Yeah, this could affect the existing "retry failed" functionality. After #2818, we are now canceling older exports if there are multiple for the same record/destination (test case here). This means that these intentionally canceled ones would also be retried when doing it by date range.

Seeing this and the date range thing makes me think that perhaps this is a different method, ExportOps.retryExportById or similar. To ensure this new one and the existing one are retrying in the same way we could break out the actual updating of the export into a separate function and share that.

krishnaglick · 2022-02-15T20:57:12Z

core/src/modules/cls.ts

@@ -31,15 +31,15 @@ export namespace CLS {
   * Wrap an Async function f in such a way that all enqueued afterCommit / enqueueTasks during invocation will be run afterwords
   * Returns the return value of function f
   */
-  export interface CLSWrapMethod {
+  export interface CLSWrapMethod<T = any> {


krishnaglick · 2022-02-15T20:59:03Z

ui/ui-components/client/client.ts

+  public async requestAction<A extends Action>(
+    verb: Method = "get",
+    path: string,
+    data: Partial<ParamsFrom<A>>, // TODO: Make ParamsFrom handle required


Once this handles required we can use this to type input params in the Client!

We can use this to handle type inputs on the client anyway, but it'll be better.

krishnaglick · 2022-02-15T20:59:20Z

ui/ui-components/components/export/List.tsx

+import EnterpriseLink from "../GrouparooLink";
+import LoadingTable from "../LoadingTable";
+import Pagination from "../Pagination";
+import { ExportGroupsDiff, ExportRecordPropertiesDiff } from "./Diff";


Sorted imports, no negative effects seen.

krishnaglick · 2022-02-15T21:00:30Z

ui/ui-components/components/export/List.tsx

+                        style={{ cursor: "pointer" }}
+                        onClick={() => retryExport(_export, index)}
+                      />
+                    ) : null}


@bleonard If you get a chance to look over things, this is what this looks like.

krishnaglick · 2022-02-15T21:03:19Z

ui/ui-components/pages/export/[id]/edit.tsx

+            Retry Export
+          </LoadingButton>,
+        ]}
+      >


Moved to Edmundo's managed card 👌

ui/ui-components/components/export/List.tsx

ui/ui-components/utils/apiData.ts

edmundito · 2022-02-15T21:36:53Z

ui/ui-components/pages/export/[id]/edit.tsx

+  const [_export, setExport] = useState(props._export);
+  const [loading, setLoading] = useState(false);
+
+  const retryExport = useCallback(async () => {


A lot of this logic seems to be repeated in two files. I wonder if we could encapsulate part, if not all of this logic, maybe in a hook?

I think the best option may be to extract the meat of the function and just export that. Otherwise you'll end up passing in all the hook setters, etc. I'll move it, lemme know what you think!

ui/ui-components/components/export/List.tsx

pedroslopez · 2022-02-15T22:35:29Z

core/src/modules/ops/export.ts

@@ -175,7 +175,7 @@ export namespace ExportOps {
    saveExports = true
  ) {
    const where: WhereOptions<Export> = {
-      state: "failed",
+      state: { [Op.in]: ["failed", "canceled"] },


Yeah, this could affect the existing "retry failed" functionality. After #2818, we are now canceling older exports if there are multiple for the same record/destination (test case here). This means that these intentionally canceled ones would also be retried when doing it by date range.

Seeing this and the date range thing makes me think that perhaps this is a different method, ExportOps.retryExportById or similar. To ensure this new one and the existing one are retrying in the same way we could break out the actual updating of the export into a separate function and share that.

…exports. Additionally, adds some new functionality to client to type input parameters.

edmundito

👍🏼

edmundito · 2022-02-16T15:47:43Z

core/src/actions/exports.ts

+  }: {
+    params: ParamsFrom<ExportsRetryFailedById>;
+  }) {
+    console.log(params);


console.log

krishnaglick · 2022-02-16T15:39:54Z

core/src/models/Export.ts

@@ -314,7 +314,7 @@ export class Export extends CommonModel<Export> {
    }
  }

-  static async retryFailed(
+  static retryFailed(


Doesn't need to be async since we're not awaiting anything.

krishnaglick · 2022-02-16T15:40:33Z

ui/ui-components/client/client.ts

+  public async requestAction<A extends Action>(
+    verb: Method = "get",
+    path: string,
+    data: Partial<ParamsFrom<A>>, // TODO: Make ParamsFrom handle required


We can use this to handle type inputs on the client anyway, but it'll be better.

core/src/config/routes.ts

core/src/modules/ops/export.ts

edmundito · 2022-02-16T22:16:00Z

@krishnaglick I feel that this PR is now doing too much: Both the feature change and the action cleanup.

krishnaglick · 2022-02-16T22:18:39Z

@krishnaglick I feel that this PR is now doing too much: Both the feature change and the action cleanup.

I can pull it out, but it did catch a bug. This is definitely a larger PR but I'm pretty sure it didn't break anything since it's just a type-inference fix.

pedroslopez

Looks good, just one suggestion on keeping the names consistent.

core/src/config/routes.ts

ui/ui-components/client/client.ts

krishnaglick requested review from edmundito and pedroslopez February 15, 2022 20:56

krishnaglick added the enhancement New feature or request label Feb 15, 2022

krishnaglick commented Feb 15, 2022

View reviewed changes

edmundito reviewed Feb 15, 2022

View reviewed changes

pedroslopez suggested changes Feb 15, 2022

View reviewed changes

Krishna Glick added 4 commits February 16, 2022 10:38

This adds a retry button to the exports page for failed or cancelled …

6013e8b

…exports. Additionally, adds some new functionality to client to type input parameters.

Canceled now retried

573a254

Don't need to catch.

edca916

Edmundo & Pedro Code Review

c42a06e

krishnaglick force-pushed the 180997068-failed-or-cancelled-export-retry-button branch from b9d4e0f to c42a06e Compare February 16, 2022 15:39

edmundito reviewed Feb 16, 2022

View reviewed changes

core/src/actions/exports.ts Outdated

}: {

params: ParamsFrom<ExportsRetryFailedById>;

}) {

console.log(params);

Copy link

Contributor

edmundito Feb 16, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

console.log

Cleanup

43020ab

krishnaglick commented Feb 16, 2022

View reviewed changes

krishnaglick requested review from pedroslopez and edmundito February 16, 2022 16:02

Krishna Glick added 3 commits February 16, 2022 14:01

Upgrading Actionhero

dc99b25

as const all inputs

d33b03c

A bug!

da57f04

pedroslopez reviewed Feb 16, 2022

View reviewed changes

core/src/config/routes.ts Outdated Show resolved Hide resolved

core/src/modules/ops/export.ts Outdated Show resolved Hide resolved

core/src/modules/ops/export.ts Outdated Show resolved Hide resolved

Added tests, changed endpoints

490ecd5

krishnaglick requested a review from pedroslopez February 16, 2022 22:04

pedroslopez reviewed Feb 17, 2022

View reviewed changes

core/src/config/routes.ts Outdated Show resolved Hide resolved

ui/ui-components/client/client.ts Show resolved Hide resolved

ui/ui-components/client/client.ts Outdated Show resolved Hide resolved

pedroslopez approved these changes Feb 17, 2022

View reviewed changes

Cleanup/CR

5deb489

krishnaglick merged commit c07f674 into main Feb 17, 2022

krishnaglick deleted the 180997068-failed-or-cancelled-export-retry-button branch February 17, 2022 16:30

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Failed or Canceled Export Retry Button #2975

Failed or Canceled Export Retry Button #2975

krishnaglick commented Feb 15, 2022 •

edited

krishnaglick left a comment

krishnaglick Feb 15, 2022

pedroslopez Feb 15, 2022 •

edited

krishnaglick Feb 15, 2022

krishnaglick Feb 15, 2022

krishnaglick Feb 16, 2022

krishnaglick Feb 15, 2022

krishnaglick Feb 15, 2022

krishnaglick Feb 15, 2022

edmundito Feb 15, 2022

krishnaglick Feb 16, 2022

pedroslopez Feb 15, 2022 •

edited

edmundito left a comment

edmundito Feb 16, 2022

krishnaglick Feb 16, 2022

krishnaglick Feb 16, 2022

edmundito commented Feb 16, 2022

krishnaglick commented Feb 16, 2022

pedroslopez left a comment

Failed or Canceled Export Retry Button #2975

Failed or Canceled Export Retry Button #2975

Conversation

krishnaglick commented Feb 15, 2022 • edited

Change description

Checklists

Development

Impact

Code review

krishnaglick left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pedroslopez Feb 15, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pedroslopez Feb 15, 2022 • edited

Choose a reason for hiding this comment

edmundito left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

edmundito commented Feb 16, 2022

krishnaglick commented Feb 16, 2022

pedroslopez left a comment

Choose a reason for hiding this comment

krishnaglick commented Feb 15, 2022 •

edited

pedroslopez Feb 15, 2022 •

edited

pedroslopez Feb 15, 2022 •

edited