PDF Downloader downloads HTML source code instead if no access by binsu-kth · Pull Request #7474 · JabRef/jabref

binsu-kth · 2021-02-28T15:48:18Z

Adds ignore parameter to mimeType and notify the user if the downloaded file is of HTML type, fixes #7452.

In the method ExternalFileTypes::getExternalFileTypeByMimeType, we added a feature to remove the parameters part
of the mimeType, this is allowed in mimeType-specification link. The previous code didn't consider the parameter part of the mimeType, thus not conforming to the correct enum type in StandardExternalFileType.java.

We also added a feature to notify the user if the downloaded link is a HTML file, via the "status bar", because the user might think they have downloaded a PDF while this isn't the case.

Change in CHANGELOG.md described in a way that is understandable for the average user (if applicable)
Tests created for changes (if applicable)
Manually tested changed features in running JabRef (always required)
Screenshots added in PR description (for UI changes)
Checked documentation: Is the information available and up to date? If not created an issue at https://github.com/JabRef/user-documentation/issues or, even better, submitted a pull request to the documentation repository.

Screenshot 1: The downloaded file from the issue example is downloaded as a HTML file instead of a HTML-file disguised as a PDF-file.

Screenshot of importing the article in the issue

Screenshot of notification/warning that the link is downloaded as an HTML file after the import is complete

Screenshot showing that the file downloaded is now a local HTML file

Siedlerchr · 2021-03-01T16:16:36Z

Thanks for your contribution, this is a good first step. Please have a look at the checkstlye/reviewdog style checker.

Adds ignore parameter to mime type and notify the user if the file downloaded is HTML.

Use Localization when writing messages in the status bar.

replace StringUtils::substringBefore with String::substring

Add test ensuring the UI warns the user if they download a linked HTML file (i.e. a web page).

The test checks the resulting file type when downloading a HTML file.

Add check to only process the mimeType if an ';' exists inside the string

Tests that mime type with parameter value is parsed correctly to exclude the parameter.

Sets a new system-wide cookie manager if there is none, and sets the cookie policy to ACCEPT_NONE after each test.

Clean up code styling according to JabRef style guidelines.

Siedlerchr

Thanks for your contribution and also for the tests, just a minor remark regarding the preferences mocking

…ed in testing (JabRef#7452)

binsu-kth · 2021-03-06T09:35:22Z

Thanks for a great first time experience regarding open source. Our course is now over and the project has been presented. I hope this patch has been helpful in order to improve your open source project and i wish you good luck in the future! From @binsu-kth , @grundb , @kittytinythai , @keivanm & @kaniyi from KTH (Royal Institute of Technology) course DD2480 .

Siedlerchr · 2021-03-06T13:53:21Z

    // Structure taken from deleteWhenDialogCancelledReturnsFalseAndDoesNotRemoveFile method
    @Test
    void downloadHtmlFileCausesWarningDisplay() throws MalformedURLException {
-        Globals.prefs = JabRefPreferences.getInstance(); // required for task execution


You don't need that assignment

Siedlerchr · 2021-03-06T13:55:43Z

-        ExternalFileTypes externalFileTypes = ExternalFileTypes.getInstance();
-        Optional<ExternalFileType> test = externalFileTypes.getExternalFileTypeByMimeType("text/html; charset=UTF-8");
+        Globals.prefs = jabRefPreferences.getInstance(); // required for task execution
+        ExternalFileTypes externalFileTypesInstance = externalFileType.getInstance();


same as above and you don't need to call getInstance , the mock already creates an instance for you

Siedlerchr · 2021-03-06T13:56:27Z

Thanks for your contribution, it would be really nice if you could address the small changes in the test so that we can merge.

…roup-22/jabref into fix-for-issue-7452

calixtus

Hi folks, thank you for your contribution so far. I have one more important question and bit nitpicking about code / comment style.

Good look with your studies, I hope you like working with JabRef. JabRef is a project of some open source enthusiasts, who like to share their interest in open source with other people and especially to help students learn about free software. We are always happy, if someone decides to put some time and energy in our common project.

calixtus · 2021-03-08T09:18:16Z

+                    dialogService.notify(Localization.lang("Downloaded website as an HTML file."));
+                    LOGGER.debug("Downloaded website {} as an HTML file at {}", linkedFile.getLink(), destination);


Is this really the error message, a user can understand?
As I understand this PR, the user tries to download a not accessible PDF file. This is not possible, so JabRef downloads the website showing "not allowed" or something.
But the user does not know this, but is only shown: "Downloaded website as an HTML file."
I guess the right error message would be something like: "Problem accessing PDF file, unexpected download result. Please try manually."

The thing is you can also link a html file and really want to download the html. Thus it is not always a mistake to download html. Maybe it's ok to ignore this problematic (except if the user sets the file type to HTML)?

Apart from this, I also like the suggestion of @Krzmbrzl in #7452

My suggestion for a mitigation would be to check the downloaded file and if it starts with plain text , then assume the download has failed and remove the "PDF" again (restoring the file link with the download URL in JabRef).

to keep the link in case the download produced a html file. In this case, the user can activate his university vpn/proxy and simply try again.

calixtus · 2021-03-08T09:34:32Z

+        Pattern warningPattern = Pattern.compile(".*HTML.*", Pattern.CASE_INSENSITIVE);
+        verify(dialogService, atLeastOnce()).notify(matches(warningPattern));


Can't this be simplified somehow to a test for a static string (I believe the tests are running in english localization by default) instead of matching a regex? Millisecs do not count if it's just one test, but we have some more. 😅

calixtus · 2021-03-08T09:41:42Z

+    // Structure taken from deleteWhenDialogCancelledReturnsFalseAndDoesNotRemoveFile method
+    @Test
+    void downloadHtmlFileCausesWarningDisplay() throws MalformedURLException {
+        // From BibDatabaseContextTest::setUp
+        when(filePreferences.shouldStoreFilesRelativeToBib()).thenReturn(true);
+        when(filePreferences.getFileNamePattern()).thenReturn("[citationkey]"); // used in other tests in this class
+        when(filePreferences.getFileDirectoryPattern()).thenReturn("[entrytype]"); // used in movesFileWithFileDirPattern at MoveFilesCleanupTest.java


If you like to comment something for the reviewer, you can comment on your own PRs in the github-ui. But please check twice, if a comment made in the source really has some long lasting use. Otherwise it's just polluting the codebase.
These comments here seem to be superfluous.

calixtus · 2021-03-08T09:46:45Z

+    // Tests if added parameters to mimeType gets parsed to correct format.
+    @Test
+    void mimeTypeStringWithParameterIsReturnedAsWithoutParameter() {


The comment here seems just to describe in other words, what the name of the test method implies, so probably unnecessary too.

Siedlerchr · 2021-03-12T20:33:57Z

It would be really nice if you could address the small comments so we can finally merge this

grundb mentioned this pull request Feb 28, 2021

PDF Downloader downloads HTML source code instead if no access #7452

Closed

1 task

grundb mentioned this pull request Mar 3, 2021

Add screenshot of UI changes to PR DD2480-2021-group-22/jabref#13

Closed

binsu-kth marked this pull request as ready for review March 4, 2021 15:26

binsu-kth marked this pull request as draft March 4, 2021 15:27

binsu-kth and others added 13 commits March 4, 2021 19:05

Ignore mime type params (JabRef#7452)

c6e95c7

Adds ignore parameter to mime type and notify the user if the file downloaded is HTML.

Update status bar message (JabRef#7452)

a99708f

Use Localization when writing messages in the status bar.

Add debug message (JabRef#7452)

6f4ca6b

Replace apache StringUtils (JabRef#7452)

a4c3008

replace StringUtils::substringBefore with String::substring

Add UI test (JabRef#7452)

24669c8

Add test ensuring the UI warns the user if they download a linked HTML file (i.e. a web page).

Add unit test for HTML file (JabRef#7452)

8867b5b

The test checks the resulting file type when downloading a HTML file.

Fix mime type parsing bug (JabRef#7452)

647c1b1

Add check to only process the mimeType if an ';' exists inside the string

Add unit test for mime type parsing (JabRef#7452)

a55b2bc

Tests that mime type with parameter value is parsed correctly to exclude the parameter.

Add changes to changelog (JabRef#7452)

2f536a3

Clarify changes (JabRef#7452)

85e8f8d

Reset cookie policy in test (JabRef#7452)

0f5a819

Sets a new system-wide cookie manager if there is none, and sets the cookie policy to ACCEPT_NONE after each test.

Add test for when a linked file points to a PDF url (JabRef#7452)

e2f4c6f

Clean up code style (JabRef#7452)

5ec83b6

Clean up code styling according to JabRef style guidelines.

grundb force-pushed the fix-for-issue-7452 branch from 08486f2 to 5ec83b6 Compare March 4, 2021 18:16

Remove duplicate changelog entry (JabRef#7452)

40fa1dd

binsu-kth marked this pull request as ready for review March 4, 2021 18:32

Siedlerchr reviewed Mar 4, 2021

View reviewed changes

Comment thread src/test/java/org/jabref/gui/fieldeditors/LinkedFileViewModelTest.java Outdated

Siedlerchr requested changes Mar 4, 2021

View reviewed changes

Siedlerchr added the status: changes-required Pull requests that are not yet complete label Mar 4, 2021

Refactor LinkedFileViewModelTest adding mock for JabRefPreferences us…

32b9433

…ed in testing (JabRef#7452)

Siedlerchr reviewed Mar 6, 2021

View reviewed changes

Refactor LinkedFileViewModelTest removing redundant code (JabRef#7452)

263178a

binsu-kth added 2 commits March 6, 2021 15:29

Refactor LinkedFileViewModelTest removing redundant code (JabRef#7452)

b873b33

Merge branch 'fix-for-issue-7452' of https://github.com/DD2480-2021-g…

3a03b5c

…roup-22/jabref into fix-for-issue-7452

Siedlerchr approved these changes Mar 7, 2021

View reviewed changes

Siedlerchr added status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers and removed status: changes-required Pull requests that are not yet complete labels Mar 7, 2021

calixtus requested changes Mar 8, 2021

View reviewed changes

tobiasdiez added the status: changes-required Pull requests that are not yet complete label Mar 10, 2021

Siedlerchr merged commit 7bd9c0d into JabRef:master Mar 14, 2021

		dialogService.notify(Localization.lang("Downloaded website as an HTML file."));
		LOGGER.debug("Downloaded website {} as an HTML file at {}", linkedFile.getLink(), destination);

		Pattern warningPattern = Pattern.compile(".HTML.", Pattern.CASE_INSENSITIVE);
		verify(dialogService, atLeastOnce()).notify(matches(warningPattern));

Uh oh!

Conversation

binsu-kth commented Feb 28, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Screenshot 1: The downloaded file from the issue example is downloaded as a HTML file instead of a HTML-file disguised as a PDF-file.

Screenshot of importing the article in the issue

Screenshot of notification/warning that the link is downloaded as an HTML file after the import is complete

Screenshot showing that the file downloaded is now a local HTML file

Uh oh!

Siedlerchr commented Mar 1, 2021

Uh oh!

Uh oh!

Siedlerchr left a comment

Choose a reason for hiding this comment

Uh oh!

binsu-kth commented Mar 6, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Siedlerchr commented Mar 6, 2021

Uh oh!

calixtus left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Siedlerchr commented Mar 12, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

binsu-kth commented Feb 28, 2021 •

edited

Loading