Enable cancellation for Lift exports

[andracc]

Resolves #3254

This change is 

[codecov]

Codecov Report

Attention: Patch coverage is 88.23529% with 6 lines in your changes missing coverage. Please review.

Project coverage is 72.85%. Comparing base (06ada98) to head (fd07849).
Report is 1 commits behind head on master.

Files with missing lines	Patch %	Lines
Backend/Services/PermissionService.cs	0.00%	3 Missing ⚠️
src/components/ProjectExport/ExportButton.tsx	71.42%	2 Missing ⚠️
Backend/Controllers/LiftController.cs	93.75%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #3664      +/-   ##
==========================================
+ Coverage   72.83%   72.85%   +0.01%     
==========================================
  Files         287      287              
  Lines       10701    10726      +25     
  Branches     1330     1331       +1     
==========================================
+ Hits         7794     7814      +20     
- Misses       2512     2517       +5     
  Partials      395      395              

Flag	Coverage Δ
backend	81.84% <90.00%> (+<0.01%)	⬆️
frontend	65.80% <81.81%> (+0.01%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[imnasnainaec]

I know this tooltip will never be active when the reset button is also active, but it feels odd to have it around the reset button too, especially since we might want to give the reset button its own tooltip.

[hahn-kev]

@imnasnainaec the Dictionary class is not thread safe and the more you use it the more likely you are to run into trouble. I would swap it out for Concurrent Dictionary which is thread safe.

[imnasnainaec]

@hahn-kev Ah, thanks for pointing that out @hahn-kev. Might we also need to use a lock if two different threads are checking then updating the same dictionary entry?

[hahn-kev]

You could do that instead, but it would be much simpler to use the correct type. It's also much more reliable as you have to be careful with locks to not access the object outside of the lock.

[imnasnainaec]

Reviewed 2 of 9 files at r2, 3 of 7 files at r5, all commit messages.
Reviewable status: 5 of 14 files reviewed, 4 unresolved discussions (waiting on @andracc)

---

Backend/Controllers/LiftController.cs line 289 at r5 (raw file):

        /// <summary> Cancels project export </summary>
        /// <returns> ProjectId, if cancel successful </returns>
        [HttpGet("cancelExport", Name = "CancelLiftExport")]

Use all-lowercase urls:"cancelExport" -> "cancelexport"

---

Backend/Controllers/LiftController.cs line 377 at r5 (raw file):

                    await _notifyService.Clients.All.SendAsync(CombineHub.DownloadReady, userId);
                }
                return true;

return proceed; might make more sense, even if we're not using that return value.

---

Backend/Services/LiftService.cs line 155 at r5 (raw file):

                {
                    _liftExports.TryAdd(userId, (InProgress, exportId));
                }

I think you could use .AddOrUpdate (rather than .TryRemove and .TryAdd) to avoid a lock in SetExportInProgress (and possibly elsewhere).

[imnasnainaec]

Reviewed all commit messages.
Reviewable status: 5 of 17 files reviewed, 3 unresolved discussions (waiting on @andracc)

---

Backend/Services/LiftService.cs line 138 at r6 (raw file):

        /// <summary> Store status that a user's export is cancelled. </summary>
        public void SetCancelExport(string userId)

Since SetCancelExport isn't setting anything, something like CancelExport or ClearExportInProgress would be more accurate.

---

Backend/Services/LiftService.cs line 144 at r6 (raw file):

        /// <summary> Store status that a user's export is in-progress. </summary>
        public void SetExportInProgress(string userId, bool isInProgress, string exportId)

Now that we have a function to cancel exports, that can be used instead of this with isInProgress = false, so we can drop that middle bool param (and add a check to throw an error if exportId is empty).

[imnasnainaec]

Dismissed @hahn-kev from a discussion.
Reviewable status: 5 of 17 files reviewed, 3 unresolved discussions (waiting on @andracc)

---

src/components/ProjectExport/Redux/ExportProjectReduxTypes.ts line 2 at r9 (raw file):

export enum ExportStatus {
  Canceling = "CANCELING",

I'm not sure why/whether we need this added status option for the Redux state or if it's sufficient to handle the canceling state within the ExportButton component.

---

Backend/Services/PermissionService.cs line 144 at r9 (raw file):

                return "";
            }
            return exportId;

Can simplify to return request.TraceIdentifier ?? "";

---

src/components/ProjectExport/ExportButton.tsx line 25 at r9 (raw file):

export default function ExportButton(props: ExportButtonProps): ReactElement {
  const dispatch = useAppDispatch();
  const [canceling, setCanceling] = useState(false);

This component state canceling should only be necessary if we get rid of the ExportStatus.Canceling in the Redux state. If we keep that, then you should be able to use status === ExportStatus.Canceling in place of canceling.

[imnasnainaec]

Reviewed 2 of 10 files at r7, 4 of 11 files at r10, all commit messages.
Reviewable status: 8 of 17 files reviewed, 4 unresolved discussions (waiting on @andracc)

---

src/backend/index.ts line 278 at r10 (raw file):

/** Tell the backend to cancel the LIFT file export. */
// revisit

// revisit revisited yet?

---

src/components/ProjectExport/Redux/ExportProjectActions.ts line 45 at r11 (raw file):

export function asyncCancelExport(projectId: string) {
  return async () => {
    await cancelExport(projectId);

After cancelling the export, this needs to dispatch the resetExportAction

---

src/components/ProjectExport/Redux/ExportProjectActions.ts line 14 at r10 (raw file):

// Action Creation Functions

Please re-add this removed newline. The comment applies to multiple function, not just the next one .

---

Backend/Services/LiftService.cs line 122 at r10 (raw file):

        /// A dictionary shared by all Projects for storing and retrieving paths to in-process imports.
        /// </summary>
        private readonly Dictionary<string, string> _liftImports;

Let's also make _liftImports a ConcurrentDictionary. And then the StoreImport function below could be

        public void StoreImport(string userId, string filePath)
        {
            _liftImports.AddOrUpdate(userId, filePath, (_, _) => filePath);
        }

[imnasnainaec]

Reviewed 1 of 6 files at r9, 1 of 11 files at r10, 1 of 2 files at r11, 2 of 5 files at r12, all commit messages.
Reviewable status: 13 of 18 files reviewed, 1 unresolved discussion (waiting on @andracc)

---

Backend/Controllers/LiftController.cs line 294 at r13 (raw file):

        {
            var userId = _permissionService.GetUserId(HttpContext);
            return CancelLiftExport(userId);

To get ...ResponseType(StatusCodes.Status200OK..., this should be
return Ok(CancelLiftExport(userId));

[imnasnainaec]

Reviewed 1 of 5 files at r12.
Reviewable status: 14 of 18 files reviewed, 5 unresolved discussions (waiting on @andracc)

---

src/components/ProjectExport/ExportButton.tsx line 36 at r13 (raw file):

  async function cancelExport(): Promise<void> {
    setCanceling(true);
    await dispatch(asyncCancelExport());

Because asyncCancelExport updates the Redux state before awaiting anything, there's no appreciable delay between setCanceling(true) and when status is ExportStatus.Exporting again. We can getting rid of this component's canceling state and not even worry about disabling the export button after it's clicked.

Just in case of future changes that would give time for a second click, I artificially added a delay to the cancel function so I could click the button multiple times, and that didn't cause any error.

---

src/components/ProjectExport/ExportButton.tsx line 44 at r13 (raw file):

  useEffect(() => {
    console.log("status: ", status);

Don't forget to remove this console.log when you're done with it.

---

src/components/ProjectExport/ExportButton.tsx line 78 at r13 (raw file):

        </span>
      </Tooltip>
      {status == ExportStatus.Exporting && (

This should be ===

---

src/components/ProjectExport/ExportButton.tsx line 79 at r13 (raw file):

      </Tooltip>
      {status == ExportStatus.Exporting && (
        <Tooltip title="Cancel export">

This tooltip text needs to be localized, something like projectExport.cancelExport with the associated entry added to public/locales/en/translation.json

Also, it'd be good to use our IconButtonWithTooltip for this cancel button to help us maintain uniform button styling:

      {status == ExportStatus.Exporting && (
        <IconButtonWithTooltip
          disabled={canceling}
          icon={<Cancel />}
          onClick={cancelExport}
          textId={"projectExport.cancelExport"}
        />
      )}

[imnasnainaec]

Reviewable status: 14 of 18 files reviewed, 5 unresolved discussions (waiting on @andracc)

---

Backend/Controllers/LiftController.cs line 294 at r13 (raw file):

Previously, imnasnainaec (D. Ror.) wrote…

To get ...ResponseType(StatusCodes.Status200OK..., this should be
return Ok(CancelLiftExport(userId));

Rather, it can return Ok(_liftService.CancelRecentExport(userId)); and you can delete private bool CancelLiftExport(string userId), since that's not used elsewhere.

[imnasnainaec]

Reviewed 1 of 5 files at r12, 5 of 5 files at r14, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @andracc)