Change file destroy error message

[mtrezza]

Improving the error message when deleting an unnamed file.

The current error message mentions an "unsaved" file, while checking for a file name.
To be more specific, the error message should mention an "unnamed" file.

As discussed in parse-community/docs#791 (comment).

[codecov]

Codecov Report

Merging #1257 (8397e51) into master (c9699e2) will not change coverage.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master    #1257   +/-   ##
=======================================
  Coverage   99.98%   99.98%           
=======================================
  Files          57       57           
  Lines        5407     5409    +2     
  Branches     1204     1204           
=======================================
+ Hits         5406     5408    +2     
  Misses          1        1           

Impacted Files	Coverage Δ
src/ParseError.js	100.00% <100.00%> (ø)
src/ParseFile.js	100.00% <100.00%> (ø)

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c9699e2...8397e51. Read the comment docs.

[davimacedo]

There is a failing test. You also need to change the message in the test case.

[dplewis]

@mtrezza can you fix the test and update from master for the CI?

[mtrezza]

Fails and I cannot see any output, is that a known issue?

[davimacedo]

Could you check again? I can now see. The Github Actions logs are little bit buggy. Sometimes I had to refresh the page to see the data.

[dplewis]

@mtrezza you can run the test locally and find the failing test.

[mtrezza]

Thanks, I can see the log output in the CI here now too.

The error was that the error message was different in the test vs. in the method. That confirms that it could be beneficial to consolidate the Parse Error codes and messages into their own repo at some point in the future.

Back to the test here, it throws just an Error, there is no dedicated ParseError for this:

Parse-SDK-JS/src/ParseFile.js

Line 323 in f50b94e

throw new Error('Cannot delete an unsaved ParseFile.');

On the other hand in the docs we describe a ParseError that is not defined in the SDK and the error code and message are not referenced anywhere in Parse JS SDK or Parse Server:

FileDeleteError | 131 | File could not be deleted.

The blame line shows it has been added 6 years ago, so it was also not a recently added feature that may not have been merged yet.

What do you think about adding the 131 error code definition to the JS SDK and throwing this Parse Error instead of the current error?

[dplewis]

I would prefer some consistency. Now that you mention there are a few TypeError being thrown instead of Parse.Error.

[mtrezza]

I would prefer some consistency. Now that you mention there are a few TypeError being thrown instead of Parse.Error.

By consistency you mean we should throw a proper Parse.Error and use the 131 error? Or should we introduce a new Parse.Error specifically for this case with message Cannot delete an unnamed file., I am leaning towards that but undecided.

Edit: actually I think for the sake of proper testing we'd have to introduce a new Parse Error, otherwise we wouldn't know if a test fails because of unnamed file or another reason.

[mtrezza]

I found the missing Parse Error with error number 153 instead of 131 as documented:

Parse-SDK-JS/src/ParseError.js

Lines 338 to 344 in f24bd99

	/**
	* Error code indicating an error deleting a file.
	*
	* @property {number} FILE_DELETE_ERROR
	* @static
	*/
	ParseError.FILE_DELETE_ERROR = 153;

And it's thrown in Parse Sever when deleting a file fails for an unspecified reason, as in this test case:

Parse-SDK-JS/integration/test/ParseFileTest.js

Lines 107 to 117 in 7cf413e

	it('can handle delete file error', async () => {
	const parseLogo = 'https://raw.githubusercontent.com/parse-community/parse-server/master/.github/parse-server-logo.png';
	const file = new Parse.File('parse-server-logo', { uri: parseLogo });
	try {
	await file.destroy();
	assert.equal(false, true);
	} catch (e) {
	assert.equal(e.code, Parse.Error.FILE_DELETE_ERROR);
	}
	});
	});

Since the error is thrown for unspecified errors when deleting a file, it makes sense to introduce a new Parse Error for this PR, unless there is a contra argument.

[mtrezza]

Incorrect error number fixed in parse-community/docs#803

[mtrezza]

I would prefer some consistency. Now that you mention there are a few TypeError being thrown instead of Parse.Error.

I have opened #1269 for this.

[mtrezza]

Ready for review, added new Parse Error for when trying to delete an un-named file.

[davimacedo]

LGTM!