fixed shouldOverwrite is never called when rename target exists

[ephes]

References

Fix #12541

Code changes

The main fix is to not look at error.message to determine if the error was cause by an already existing notebook with this name but error.response.status.

I also added 3 tests to make sure shouldOverwrite is called when error.response.status is 409. And that other errors are re-thrown and don't cause shouldOverwrite to be called.

I'm a javascript newbie and had trouble to mock shouldOverwrite. Therefore I added a callback parameter to renameFile to be able to pass a mock callback in the tests. But probably somebody with more javascript experience should have a look at the tests, there's probably an easier way.

User-facing changes

Nothing really changed besides the missing dialog is now shown.

Backwards-incompatible changes

---

Edited to link the issue

[jupyterlab-probot]

Thanks for making a pull request to jupyterlab!
To try out this branch on binder, follow this link:

[welcome]

Thanks for submitting your first pull request! You are awesome! 🤗

If you haven't done so already, check out Jupyter's Code of Conduct. Also, please make sure you followed the pull request template, as this will help us review your contribution more quickly.

You can meet the other Jovyans by joining our Discourse forum. There is also a intro thread there where you can stop by and say Hi! 👋

Welcome to the Jupyter community! 🎉

[fcollonval]

Thanks @ephes

I added some pointers on how to use proper Jest mock (the JS test library used by JupyterLab).
Let me know if you need more help.

[fcollonval]

We are using Jest for testing. The documentation for mocking is there.

Something like the following should to the trick

Suggested change

	import { DocumentManager, renameFile } from '../src';
	function shouldOverwriteFalse(path: string): Promise<boolean> {
	return Promise.resolve(false);
	}
	import { DocumentManager, renameFile } from '../src';
	jest.mock('../src', () => {
	const originalModule = jest.requireActual('../src');
	//Mock the shouldOverwrite
	return {
	__esModule: true,
	...originalModule,
	shouldOverwrite: jest.fn().mockReturnValue(false),
	};
	});

[ephes]

Thanks, I didn't know about mocking partials, which seems to be very useful. But in this case it doesn't work, because it only mocks shouldOverwrite if you call it from the test. But it's called from within dialog.ts and stays unmocked there. A lot of people seem to have run into this (even found a comment from myself 2 years ago in this thread 😬).

I tried option 1 but ended up with the typescript error message TypeError: Cannot redefine property: shouldOverwrite.

[fcollonval]

Thanks for trying that path @ephes

I look a bit more at the code of shouldOverwrite. And as this is triggering a dialog, you could make use of the test helper dimissDialog (as you are emulating a rejection). See that example:

jupyterlab/packages/apputils/test/sessioncontext.spec.ts

Line 578 in 43f784a

const dismiss = dismissDialog();

Something like the following code should do the trick:

        await Promise.all([dismissDialog(), renameFile(
          manager,
          'foo.ipynb',
          'bar.ipynb'
        )]).catch(error => {
          // error should be 'File not renamed'
          expect(error).not.toBe(alreadyExistsError);
        });

[ephes]

Oh wow, this is much better.

[fcollonval]

Thanks @ephes

[welcome]

Congrats on your first merged pull request in this project! 🎉

Thank you for contributing, we are very proud of you! ❤️

[fcollonval]

@meeseeksdev please backport to 3.4.x

[github-actions]

Benchmark report

The execution time (in milliseconds) are grouped by test file, test type and browser.
For each case, the following values are computed: min <- [1st quartile - median - 3rd quartile] -> max.

The mean relative comparison is computed with 95% confidence.

Results table

Test file	large_code_notebook	large_md_notebook
open
chromium
actual	11335 <- [11551 - 11594 - 11650] -> 11991	2342 <- [2409 - 2442 - 2472] -> 2731
expected	11413 <- [11520 - 11544 - 11571] -> 11756	2312 <- [2409 - 2429 - 2460] -> 2640
Mean relative change	0.6% ± 0.2%	0.5% ± 0.7%
switch-from
chromium
actual	490 <- [518 - 529 - 539] -> 563	385 <- [411 - 420 - 430] -> 1032
expected	499 <- [518 - 531 - 540] -> 642	394 <- [409 - 422 - 431] -> 999
Mean relative change	-0.4% ± 0.9%	-2.5% ± 6.8%
switch-to
chromium
actual	293 <- [384 - 394 - 403] -> 429	245 <- [261 - 267 - 273] -> 297
expected	343 <- [369 - 391 - 402] -> 431	235 <- [260 - 265 - 271] -> 283
Mean relative change	0.5% ± 1.6%	0.7% ± 1.0%
close
chromium
actual	920 <- [951 - 961 - 976] -> 1054	402 <- [423 - 435 - 442] -> 494
expected	914 <- [939 - 958 - 978] -> 1048	404 <- [423 - 433 - 440] -> 517
Mean relative change	0.5% ± 0.7%	0.0% ± 1.2%

Changes are computed with expected as reference.

[ephes]

Thanks @ephes

Thank you too @fcollonval :)