Changed Content-Disposition header to download Contact Photo with correct extension

[jacobneplokh]

Now prompts to download as ContactPhoto.png cross-browser

Signed-off-by: Jacob Neplokh me@jacobneplokh.com

--
Fixes#20143
@jancborchardt@skjnldsv

The Content-Disposition header now also has the file name (set to ContactPhoto.png — if this needs to be changed, let me know) to make sure it is downloaded with the correct file extension, and across browsers.

[jacobneplokh]

Hey @rullzer and @skjnldsv ,

I made some changes using the suggestions. As this is a "good first issue," I chose this to get some practice and learn some more :)

So let me know if there is something to change/more to do. Also, I noticed the PHP Code-Standards check failed, but am having a little trouble understanding where specifically the error is.

Thank you!

[skjnldsv]

@jneplokh please also rebase and squash your commits into one :)

[skjnldsv]

There is a failure on the carddav integration test build/integration/features/carddav.feature:53
Expected value 'attachment' for header 'Content-Disposition', got 'attachment; filename=bjoern.vcf.jpg' (Exception)

The two other failures are false positive :)

[jacobneplokh]

Ok, I fixed the error on the carddav integration test by changing the expected header. To clarify, those 3 other failures (nodb, postgres11-php7.2, and integration-auth) are false positives correct?

Also, looking at the drone page, jsunit seemed to have failed because of a failed connection to codecov.io?

[skjnldsv]

1. OCA\DAV\Tests\unit\CardDAV\ImageExportPluginTest::testCard with data set Name the repository "server" #1 (null, true)
   Undefined index: imgtype

/drone/src/apps/dav/lib/CardDAV/ImageExportPlugin.php:106
/drone/src/apps/dav/tests/unit/CardDAV/ImageExportPluginTest.php:198

2. OCA\DAV\Tests\unit\CardDAV\ImageExportPluginTest::testCard with data set Use shields for IRC links #3 (32, true)
   Undefined index: imgtype

/drone/src/apps/dav/lib/CardDAV/ImageExportPlugin.php:106
/drone/src/apps/dav/tests/unit/CardDAV/ImageExportPluginTest.php:198

This seems related though

[jacobneplokh]

Hmm, true. Maybe something with not getting the full mimetype but just the extension. I'll see what is going on there.

[jacobneplokh]

Hey,

Sorry for the late comment here, got busy with something for a few weeks. I went over the PHPUnit docs, but I am honestly lost on what to change in the test. I got some great help from the Contacts channel, but am still confused on how to make the test work with the changes.

What should the next course of action be? If it is closing the PR, I understand.

Thank you for all the help, though, during this PR!

[skjnldsv]

What should the next course of action be? If it is closing the PR, I understand.

No no! :)
Next step is to fix the step and get this in 😉

Look at the line of the test

server/apps/dav/tests/unit/CardDAV/ImageExportPluginTest.php

Lines 177 to 182 in 2a054e6

	$this->response->expects($this->at(3))
	->method('setHeader')
	->with('Content-Type', 'imgtype');
	$this->response->expects($this->at(4))
	->method('setHeader')
	->with('Content-Disposition', 'attachment');

Since you changed the headers, the unit tests needs to reflect those changes too :)
You can run your test locally by running phpunit --bootstrap apps/dav/tests/unit/bootstrap.php apps/dav/tests/unit/CardDAV/ImageExportPluginTest.php

[jacobneplokh]

No no! :)
Next step is to fix the step and get this in 😉

Awesome!

Since you changed the headers, the unit tests needs to reflect those changes too

Ok yeah, I thought this was happening but was not exactly sure where to change it. Thanks for the testing command too.

I'll keep working on it :)

[jacobneplokh]

Since you changed the headers, the unit tests needs to reflect those changes too

Ok, I just pushed changes which update the headers with a .jpg extension. Do I need to create test cases for all the file extensions?

Also, are those two failures false positives?

[faily-bot]

🤖 beep boop beep 🤖

Here are the logs for the failed build:

Status of 32690: failure

mariadb10.4-php7.3

Show full log

There were 2 warnings:
1) Test\Files\ViewTest::testRenameFailDeleteTargetKeepSource
Trying to configure method "writeStream" which cannot be configured because it does not exist, has not been specified, is final, or is static
2) Test\Files\ViewTest::testCopyFailDeleteTargetKeepSource
Trying to configure method "writeStream" which cannot be configured because it does not exist, has not been specified, is final, or is static
--
There was 1 failure:
1) OCA\Files_Sharing\Tests\SharedMountTest::testPermissionMovedGroupShare with data set #30 ('folder', 3, 17)
Failed asserting that false is true.
/drone/src/apps/files_sharing/tests/SharedMountTest.php:367

mysql5.6-php7.2

Show full log

There was 1 error:
1) Test\Comments\ManagerTest::testGetTreeNoReplies
InvalidArgumentException: IDs must be translatable to a number in this implementation.
/drone/src/lib/private/Comments/Manager.php:248
/drone/src/lib/private/Comments/Manager.php:305
/drone/src/tests/lib/Comments/ManagerTest.php:174
--
There were 2 warnings:
1) Test\Files\ViewTest::testRenameFailDeleteTargetKeepSource
Trying to configure method "writeStream" which cannot be configured because it does not exist, has not been specified, is final, or is static
2) Test\Files\ViewTest::testCopyFailDeleteTargetKeepSource
Trying to configure method "writeStream" which cannot be configured because it does not exist, has not been specified, is final, or is static

[skjnldsv]

Also, are those two failures false positives?

Looks like it :)

[jacobneplokh]

Looks like it :)

Awesome! So is it effectively ready to be merged?

[welcome]

Thanks for your first pull request and welcome to the community! Feel free to keep them coming! If you are looking for issues to tackle then have a look at this selection: https://github.com/nextcloud/server/issues?q=is%3Aopen+is%3Aissue+label%3A%22good+first+issue%22

[skjnldsv]

Thank you @jneplokh !!

[jacobneplokh]

No problem! Definitely learned, too.

And thanks for the help during it! :)