Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow users to upload a file to use as a dataset thumbnail #3559

Closed
28 tasks done
djbrooke opened this issue Jan 6, 2017 · 31 comments
Closed
28 tasks done

Allow users to upload a file to use as a dataset thumbnail #3559

djbrooke opened this issue Jan 6, 2017 · 31 comments

Comments

@djbrooke
Copy link
Contributor

djbrooke commented Jan 6, 2017

Currently, users can select one of the files in a dataset to serve as a thumbnail for a dataset. This is selected by editing the metadata for a file.

screen shot 2017-01-05 at 11 28 18 pm

Instead of selecting a file that's part of the dataset, allow a user to upload a file to serve as the thumbnail.

To do list

To anyone who picks up items on the to do list below, please ensure that all the tests in SearchIT.java continue to pass. The code is in the 3559-dataset-thumbnail branch.

  • Switch from canIssue UpdateDatasetThumbnailCommand to Permission.ViewUnpublishedDataset or remove the canUpdateThumbnail check entirely. Removed in 63e40d0.
  • Double check performance on search cards (see Performance degradation investigation #3631). An extra lookup per dataset were added to the SearchServiceBean. Removed the extra lookup in bd280ad.
  • Disallow restricted files as thumbnails. Enforce in getThumbnail, getThumbnailCandidates, and but not EditFilesPage because we are trying not to touch that old workflow. We can't write API tests around restricted files because it's a GUI-only feature (API: add endpoint for restricting files #2497). Fixed in fb12e02.
  • Consolidate the dataset.isReleased logic from Datasets to DatasetUtil. Fixed in ceb629b.
  • Add back in the concept of selecting a thumbnail automatically at runtime, if available, when a dataset logo hasn't been uploaded and when thumbnailfile_id is null. Fixed in 36e359f but not that the method will not select a restricted file.
  • There are two DataFileConverter.java classes. Rename either src/main/java/edu/harvard/iq/dataverse/dataaccess/DataFileConverter.java (existing) or src/main/java/edu/harvard/iq/dataverse/DataFileConverter.java (new in this branch) because this is a code smell: http://stackoverflow.com/questions/6647510/bad-practice-to-have-two-classes-of-the-same-name-in-different-packages/6647540#6647540 . Renamed in 36e359f.
  • When thumbnailfile_id is null, run the automatic thumbnail selection logic and persist the value of the file id to thumbnailfile_id. Progress in a66fce2. We decided not to to this after all.
  • File size limit validation to UI
  • Move sizeLimit="500000" from UI to a method on SystemConfig and write a failing test in SearchIT to exercise it.
  • Add a way to test with an automated test what image shows on a search card. Progress in f590ede. Good enough as of a56829b.
  • Make sure staging file is never left in the dataset directory after navigating away from the page or whatever. No extra cruft left behind. Probably put the staging file in a temp directory rather than the dataset directory. Fixed in 743c5b8.
  • Filenames are missing from "Select Available File" as of a56829b, switched to a number for now. Fixed in dec5546.
  • update the file upload tool tip to take a dynamic maximum size
  • Hard code 500 KB as a size limit in a new method in SystemConfig. Plan to re-use the method for the Dataverse logo upload. Done as of 27c5e3b.
  • Revert code having to do with thumbnail sizes. Done in c687022.
  • Select Available File popup doesn't reflect that a file in that table is already selected (see screenshot above). Fixed in 7553fdb.
  • Updates to User Guide > Dataset + File Management. Consider documenting "rules" above.
  • Review accepted file types (.jpg vs. jpeg), make them consistent with logo at Dataverse level
  • Capture save logic in DatasetWidgetsPage in command similar to UpdateDataverseThemeCommand or in a service bean to be re-used by the API. Done in db1c03e.
  • UpdateDatasetThumbnailCommand remove staging path and use only input stream. Fixed in 771a4b8.
  • Make sure EditDatafilesPage uses new UpdateDatasetThumbnailCommand. Done in 2f8f9cb.
  • When you upload a file that's too big you see text squished together like "Invalid file sizecoffeeshop.png 531.2 KB". It should say "Invalid file size coffeeshop.png 531.2 KB" with a space between "size" and the filename.
  • Fix NullPointerException from DataFile.getLabel when switching from one DataFile to another. Fixed in 87b728c. Not sure why this started happening.
  • Rather than generating "data:image/png;base64," + imageDataBase64 on the fly, generate thumbnails based on imageDataBase64 and save them to disk. Use dataset_logo.thumb48 as the filename. Fixed in e7a429c.
  • Create an API endpoint to return the thumbnail (we used to have this but it broke and image_url from Search API results no longer yields a downloadable image #3616 was opened to address it some day). Started in 56c33cf. Will be secured as part of an item below. It's only for datasets, not dataverses or files.
  • For datasets but not dataverses or files, switch from <h:graphicImage value=" to `<h:graphicImage url=" and use the new API endpoint for datasets that returns the thumbnail. Fixed in 671bb9b.
  • Save original uploaded resolution, respecting the file size limit. This is for someday supporting more than just the 48x48 resolution. (We decided not to do this.) Currently it becomes a zero byte file after it's converted to a thumbnail. Either delete the file or save the original properly (doesn't matter). Dataset logo (original file uploaded) is being deleted after a thumbnail has been created as of 74fba36.
  • Secure newly added API endpoints (make similar to permissionsWrapper.canIssueCommand(dataset, UpdateDatasetCommand.class)). Write tests to ensure security. Done as of b043eb8.
  • Switch from <h:graphicImage value=" to <h:graphicImage url=" on dataset page. Fixed in ebfe4da.

QUESTIONS

Out of scope:

@TaniaSchlatter
Copy link
Member

Is there background info for this request?

@pdurbin
Copy link
Member

pdurbin commented Jan 6, 2017

@pdurbin pdurbin added the SBGrid label Jan 6, 2017
@djbrooke
Copy link
Contributor Author

djbrooke commented Jan 6, 2017

Disciplines that will be supported as part of the SBGrid grant require that the collections of files in a dataset remain intact and untouched by any process. If a structural biologist wants to add a cool thumbnail to show off the nature of her dataset, she's currently not able to do so because adding that file for thumbnail purposes would change the nature of the dataset.

@djbrooke djbrooke added the ready label Jan 6, 2017
@mheppler
Copy link
Contributor

We can quickly create mockups to get this ready for development by looking at the Theme + Widgets page for a dataverse, and applying that to this new workflow for a dataset.

screen shot 2017-01-11 at 11 19 24 am

@pdurbin
Copy link
Member

pdurbin commented Jan 25, 2017

I just mentioned to @pameyer that I would expect the image_url field in the Search API to continue to be populated for datasets that have the new alternative thumbnail (once it's developed). Here's how it looks:

"image_url":"https://demo.dataverse.org/api/access/dsCardImage/2"

That's from http://guides.dataverse.org/en/4.6/api/search.html#advanced-search-example

@djbrooke
Copy link
Contributor Author

Some tasks we identified in sprint planning today:

  • Determine the entry point for thumbnail updating
  • Determine where the file lives
  • Change the logic for uploading vs. file selection for thumbnails - we want these to both be available
  • Validate the file types (we don't want .exes)

@pdurbin pdurbin added in progress and removed ready labels Jan 31, 2017
@pdurbin pdurbin self-assigned this Jan 31, 2017
@mheppler
Copy link
Contributor

mheppler commented Feb 1, 2017

Discuss this feature quickly with @pdurbin and @pameyer to review a mocked up workflow (see attached) that is based off how we upload logos for a dataverse. Here are some decisions we've come to about it.

  • Entry point will be through the dataset's Edit button, where the link for Widgets will now read "Thumbnail + Widgets"
  • The Thumbnail Image upload component will live in a tabbed pg, which is currently just Dataset Widgets, just like the dataverse has a Theme + Widgets pg
  • The validation and sizing and all of that will be based on the existing dataverse logo and data file preview thumbnail logic, and we'll discuss any pitfalls as they arise in development
  • The current default functionality for generating dataset thumbnails through data file uploaded to the dataset will not change, data owners will still be able go to the Edit Files pg and set the thumbnail, even if they have manually uploaded a new thumbnail image
  • When a default thumbnail is being used by a dataset, it will be displayed in the new Thumbnail Image upload component (with no Remove button), and the user can manually upload a new image to overwrite it

Question to be answered:

  • Can this Thumbnail Image component also be added to the add dataset workflow, like we currently do with the data file upload component?

screen shot 2017-02-01 at 12 15 12 pm

@pdurbin
Copy link
Member

pdurbin commented Feb 2, 2017

@mheppler I added some wireframes in 9c9025a but I could use some help cleaning up the UI as well as thinking about all the implications on the bundle properties and the names of xhtml files. Here's a screeshot of what I have so far. The branch is "3559-dataset-thumbnail".

screen shot 2017-02-02 at 1 23 58 pm

@pdurbin
Copy link
Member

pdurbin commented Feb 7, 2017

First some terminology:

  • dataset file: A dataset file is part of the dataset and a thumbnail for both the file and its dataset may be created from the dataset file automatically if the file is an image, for example. This is existing functionality.
  • dataset logo: A dataset logo is not one of the files in the dataset. The thumbnail for your dataset can be created based on the dataset logo you upload rather than a dataset file. This issue is about adding the ability to upload a dataset logo.

After standup yesterday, @scolapasta @landreev @mheppler @kcondon and I met and decided the following:

  • Instead of the altthumbnail field on the dataset table idea I've been demoing, we'll store multiple resolutions of the dataset logo on the filesystem under, for example, /usr/local/glassfish4/glassfish/domains/domain1/files/10.5072/FK2/ZQJXKL in multiple resolutions with filenames like this:
    • dataset_logo_thumbnail.jpg
    • dataset_logo_thumbnail.jpg.thumb48
    • dataset_logo_thumbnail.jpg.thumb64
    • dataset_logo_thumbnail.jpg.thumb400

In addition, Mike and I have been talking about wanting something along these lines:

  • On the new "Dataset Thumbnail + Widgets" page:
    • Indicate if a thumbnail has already been set from a dataset file. Make sure the user understands that by uploading a dataset logo the exisiting thumbnail from a dataset file, if any, will no longer be used. Working well as of d4bbcff.
  • On the "Edit Files" page:
    • If a dataset logo is in place and the user clicks "Set Thumbnail" on a dataset file, a confirmation should be shown that the dataset logo will be deleted. Fixed in 08c660e.

I've already uploaded a screenshot of a early "Dataset Thumbnail + Widgets" prototype at #3559 (comment)

Here's how the existing "Set Dataset Thumbnail" looks on the "Edit Files" page:

screen shot 2017-02-06 at 2 56 18 pm

pdurbin added a commit that referenced this issue Feb 9, 2017
mheppler added a commit that referenced this issue Feb 9, 2017
…humbnail popup on the Thumbnail + Widgets pg. [ref #3559]
@mheppler
Copy link
Contributor

Mockup for new Select Dataset Thumbnail popup.

screen shot 2017-02-10 at 10 16 21 am

pdurbin added a commit that referenced this issue Feb 10, 2017
@landreev
Copy link
Contributor

@sekmiller
I'm done with the review (submitted it in the PR)

@pdurbin
Copy link
Member

pdurbin commented Mar 22, 2017

@kcondon heads up that I just opened #3671 with @sekmiller looking over my shoulder as I took screenshots and added it to the description of this issue as out of scope.

pdurbin added a commit that referenced this issue Mar 22, 2017
@pdurbin
Copy link
Member

pdurbin commented Mar 22, 2017

I added some more logging in c7aba71 to DatasetUtil and now I'm seeing this error on dataverse-internal: UnirestException caught attempting to GET https://dataverse-internal.iq.harvard.edu/api/datasets/336/thumbnail and exception was: com.mashape.unirest.http.exceptions.UnirestException: javax.net.ssl.SSLHandshakeException: sun.security.validator.ValidatorException: PKIX path building failed: sun.security.provider.certpath.SunCertPathBuilderException: unable to find valid certification path to requested target

When the error above is thrown, thumbnails do not appear on any methods that call DatasetUtil.getThumbnailImageString (search cards and dataset page).

pdurbin added a commit that referenced this issue Mar 22, 2017
Avoid "UnirestException: javax.net.ssl.SSLHandshakeException" by not
using Unirest (or any Java HTTP client) from any backing beans.
@pdurbin
Copy link
Member

pdurbin commented Mar 22, 2017

After discussing with @scolapasta in 9ddd0a4 I went ahead and ripped out the slightly-odd concept of having backing beans call new Dataverse API. No more Unirest, so no more UnirestException. I doubt @sekmiller will have a problem with this since he and I already discussed how we already have the dataset in our hands on the dataset page. Likewise, @landreev and I talked about how there's already a lookup of a dataset happening in the new thumbnail API, so in that last commit we are moving that lookup to the SearchServiceBean. @scolapasta points out that there's the potential for a performance hit in adding the lookup to the SearchServiceBean in the sense that around 4.2 we introduced a multi-phase/multi-pass approach to showing thumbnails and now the dataset thumbnails will have the expense of looking up the dataset in phase 1 rather than phase 2. I suspect the performance will be good enough but we can revisit if necessary.

@kcondon I already ran a build on dataverse-internal. Thanks for finding that bug. Dataset thumbnails no longer care about "siteUrl" as of this fix I just made.

@kcondon
Copy link
Contributor

kcondon commented Mar 23, 2017

Found a couple issues:

  • Default thumbnail from available files appears on dataset page next to title when draft, disappears/ reverts to default icon when publish dataset, though dataset search card and thumbnail/widgets page shows it as thumbnail. I think this may be due to default thumbnail not identified as selected thumbnail when you edit file metadata. Fixed in 57331c4.
  • Image files which were too large to generate thumbnails or do not have thumbnails still appear in the select file list but with just their name. Since these are not really meaningful, maybe they should be filtered out? Fixed in 0a2b503.

pdurbin added a commit that referenced this issue Mar 23, 2017
Also document that the following config options are in bytes:

- dataverse.dataAccess.thumbnail.image.limit
- dataverse.dataAccess.thumbnail.pdf.limit
pdurbin added a commit that referenced this issue Mar 23, 2017
Return quickly to decide to render the "Select Available File" button.
pdurbin added a commit that referenced this issue Mar 23, 2017
@kcondon kcondon closed this as completed Mar 23, 2017
@djbrooke djbrooke added this to the 4.6.2 - Tabular Mapping milestone Mar 28, 2017
@pdurbin pdurbin added the Type: Feature a feature request label May 18, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

10 participants