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

Thumbnail setting inconsistent between loading from Datafile and Uploading from file #10821

Closed
stevenwinship opened this issue Sep 3, 2024 · 5 comments · Fixed by #10823
Closed
Labels
Size: 3 A percentage of a sprint. 2.1 hours. Type: Bug a defect
Milestone

Comments

@stevenwinship
Copy link
Contributor

stevenwinship commented Sep 3, 2024

What steps does it take to reproduce the issue?

  1. Create a Dataset
  2. Publish
  3. Add datafile (image)
  4. Collection page should show both dataset versions with the Datafile Image as thumbnail
  5. Select Draft version
  6. Edit Dataset -> Thumbnail -> Remove image -> Save changes
  7. Collection page should show both dataset versions with the Default Image as thumbnail
  8. Select Draft Version
  9. Edit Dataset -> Thumbnail -> Upload Image -> Save changes
  10. Collection page still shows default image for both versions (should show new thumbnail for both version)
  11. Select published version, Dataset page still shows default image (should show new thumbnail)
  12. Select Draft version, Dataset page shows correct image

If the image is selected via the Select Thumbnail button instead of Upload button all images show correctly.

  • When does this issue occur? Every time

  • Which page(s) does it occurs on? Collection / Dataset

  • What happens? Thumbnails not showing correctly

  • To whom does it occur (all users, curators, superusers)? All

  • What did you expect to happen? Should match functionality of selecting thumbnail image from Datafile. See above

Which version of Dataverse are you using? 6.3

Any related open or closed issues to this bug report?

Screenshots:
Thumbnails selected by Upload Image button from Draft version
image

image

image

Thumbnails selected by Select Thumbnail button from Draft version
image

@stevenwinship stevenwinship added Type: Bug a defect Size: 10 A percentage of a sprint. 7 hours. labels Sep 3, 2024
@stevenwinship stevenwinship moved this to SPRINT READY in IQSS Dataverse Project Sep 3, 2024
@landreev
Copy link
Contributor

landreev commented Sep 3, 2024

@stevenwinship
Yeah, I can reproduce the behavior. It appears that removing a DataFile-based thumbnail and selecting the default icon is the key. If you skip this step:
6. Edit Dataset -> Thumbnail -> Remove image -> Save changes
and proceed to upload an image, then it works properly.
In other words, it looks like uploading the thumbnail image does not unset dataset.usegenericthumbnail. To make matters worse, this combination of dataset.usegenericthumbnail == true and the presence of the uploaded logo image is interpreted differently by the dataset and dataverse pages, and/or for published and draft versions on the dataset page.

@landreev
Copy link
Contributor

landreev commented Sep 3, 2024

@stevenwinship
I believe that the important thing is to test the behavior of the thumbnail image upload API. If it's similarly broken/inconsistent there, we should fix it, because that's what we'll be using in the long term. My guess is that it IS similarly broken in the API, because both places appear to be using the same UpdateDatasetThumbnailCommand. But we need to confirm.
This may also be as simple as adding a missing
dataset.setUseGenericThumbnail(false);
line to the method DatasetServiceBean.setNonDatasetFileAsThumbnail().

@landreev
Copy link
Contributor

landreev commented Sep 3, 2024

@stevenwinship
It really looks like that one missing line fixes it. See the draft PR tagged.

I know I shouldn't be spending any time on this, but can't help myself.

@stevenwinship
Copy link
Contributor Author

@landreev I approved the draft PR. I tested it as well and the issue goes away. Nice fix

@landreev landreev added Size: 30 A percentage of a sprint. 21 hours. (formerly size:33) Size: 3 A percentage of a sprint. 2.1 hours. and removed Size: 10 A percentage of a sprint. 7 hours. Size: 30 A percentage of a sprint. 21 hours. (formerly size:33) labels Sep 4, 2024
@landreev
Copy link
Contributor

landreev commented Sep 4, 2024

I'll mention it during standup or on slack and explain how it materialized on the board bypassing the formal process. I have downgraded it to "size: 3" also.

stevenwinship pushed a commit that referenced this issue Sep 4, 2024
@pdurbin pdurbin added this to the 6.4 milestone Sep 4, 2024
bjosel added a commit to TIK-NFL/dataverse that referenced this issue Nov 19, 2024
* missing empty watermark entry

* fix capitalization

* Changed: dataverse image_url Solr property set on SearchServiceBean

* Changed: do not modify existing JSF logic

* remove unused imports IQSS#10517

* add test to assert capitalizataion of Dataset and Software IQSS#10517

* add details to error messages (IQSS#10813)

* Fix addDataverse expected request body structure (IQSS#10802)

* Fixed: MetadataBlockServiceBean to check for not excluded fields in input levels

* Changed: using queries for obtaining dataset field types based on displaying conditions

* Refactor: json printer method for MetadataBlock

* Added: IT test case for list metadata blocks testing field with include=false and displayOnCreate=true property

* Fixed: removed condition in MetadataBlockServiceBean

* Added: release notes for IQSS#10741

* Fixed: displayOnCreate query logic

* Fixed: excluding conditionally required fields when display-on-create is true

* Fixed: query predicate for required-in-dataverse field condition

* Fixed: addDataverse API facetIds field json structure

* Added: docs IQSS#10800

* A one line fix for IQSS#10821 - ? (IQSS#10823)

* Add thumbnail for featured dataverses (IQSS#10433)

* Add thumbnail for futured dataverses

* Add documentation

* Release note snippet

* New flyway namming

* Update doc/release-notes/10433-release-notes.md

Co-authored-by: Philip Durbin <[email protected]>

* Release note snippet update
Add new recommandations (HTML preview + "for more information ...")

* Update SQL file name after 6.2 release

* renamed sql file

---------

Co-authored-by: Philip Durbin <[email protected]>
Co-authored-by: Ludovic DANIEL <[email protected]>
Co-authored-by: Philip Durbin <[email protected]>

* bump sql script version IQSS#10517

* JDD Metrics: Label KO IQSS#10123 (IQSS#10124)

* remove parentheses

* Correction of the parenthesis display

* conditional INSERT of dataset type IQSS#10517

* Add logic to suppress query tool display for non-public files.

* typo

* fix test

* fix labels when cvoc is used

* doc tweaks for versioned base images: making releases IQSS#10827

* iterate on "supported image tags" section IQSS#10827

* Added: setting imageUrl in SearchServiceBean for datasets and files

* simplify now that everything is inside the try

* update tests - added one field in citation block

* reworked controlled vocab language keys

* fixing key to lowercase

* fixing key to lowercase

* release note

* undo changes

* Update doc/release-notes/10810-search-api-payload-extensions.md

Co-authored-by: Philip Durbin <[email protected]>

* Added: note about upcoming change to image_url field in docs

* fixing language list

* fixing language list

* fixing language list

* fixing language list

* fixing language list

* fixing language list

* changes per review comments

* changes per review comments

* support no pubIdType for URLs

* direct people to the log for failures - they aren't in the response

* bug - the _target url isn't being set elsewhere

* avoid failing when the entity is null for error statuses

* don't update unpublished files - no need and it will fail

the updateIdentifier call is checking for the findable metadata which is
not available before publication.
(We don't update DataCite after dataset edits, so unpublished datasets
don't go through here, but unpublished files on published datasets would
hit this code)

* lower logging, add null check on relatedIdentifier

* Change to use POST for all

* Documentation and updated release note

* changes per review comments

* test fix - number of fields

* Remap oai_dc fields dc:type and dc:date (IQSS#10737)

* Remap oai_dc fields dc:type, dc:date, and dc:rights IQSS#8129.

The `oai_dc` export and harvesting format has had the following fields remapped:

- dc:type was mapped to the field "Kind of Data". Now it is hard-coded to the word "Dataset".
- dc:date was mapped to the field "Production Date" when available and otherwise to "Publication Date". Now it is mapped only to the field "Publication Date".
- dc:rights was not mapped to anything. Now it is mapped (when available) to terms of use, restrictions, and license.

* add tests for export and citation date IQSS#8129

* map dc:date to pub date or field for citation date  IQSS#8129

* back out of any changes to dc:rights IQSS#8129

* remove OAI-PMH changes from API changelog (also in release note) IQSS#8129

* tweak release note, mention backward incompatibility, reexport IQSS#8129

* update release note

* check for ROR in grantAgency field too

* 10527 404static.xhtml has an old date in the footer (2023 is hard-coded) and update URLs (IQSS#10535)

* Javascript updates the year automatically and the URLs are customised.

* add taps

* only change year and and all the other errors are still there.

* adopt using CDI, fix funderIdentifier element per schema

* datasetTypes test fix

* release note/changelog changes

* chore(ct): update base image wait4x to 2.14.2 for stdlib update

* chore(ct): update base image wait4x to 2.14.2 for stdlib update IQSS#10844

* don't send contributors w/o contributorType

* relatedIdentifierType is required

* flip to prefer identifier over url

seeing cases at QDR where the type is DOI, the identifier is the doi and
the URL is a non-DOI reference (e.g. at pubmed).

* Handle case where type is set but there's no identifier

* map non-standard contributors to Other, remove unused imports

* Treat missing contrib type as Other

* chore(ci): delete duplicate action after renaming

* docs(ct): update base image tag policy from latest discussion IQSS#10827

As discussed during the 2024-09-12 containerization working group meeting (see ct.gdcc.io) and on https://dataverse.zulipchat.com/#narrow/stream/375812-containers/topic/change.20version.20scheme.20base.20image.3F/near/469884104

* style,docs(ct): minor tweaks to base image policy as per @pdurbin

* feat(ct): switch latest to unstable in base image flow

As per latest discussion, we want to keep the unstable tag around.
It shall still point to the latest from develop as it has been done
before our revised tagging policy.

Latest will be used for production images, much more aligned with the
Bitnami policy

* style,docs(ct): minor tweaks to base image policy as per @pdurbin

* fix(ct): remove auth for revision action

Maybe we won't run into a rate limit for now, as the limit of 180/s * 100 = 18000 tags/s seems to leave us some headroom for now.

* fix(ct): try to avoid shell substitution in base image flow

We might need more backslashes to avoid telling the shell to replace what we want to be a maven property

* fix(ct): avoid shell substitutions going awry for base image release tag

Instead of using a Maven reference and dealing with escaping of $ chars, override the suffix with an empty string

* add trailing / after hostname for perma base-url IQSS#10775

* ci(ct): remove out of scope actions for IQSS#10478

We will deal with shipping the updated application container images separately as part of issue IQSS#10618.

Adding some comments about why some stuff is still around.

* ci(ct): reorganise tags for develop branch IQSS#10478

Now adding the "upcoming" tag during the develop branch run.

Also some reorganizing to ship the tag options using outputs, not env vars.
Moving the common Maven option to enable tag overrides to the command instead of the options building.

* chore(parent): remove OSS plugin snapshot repo after upgrade to DMP 0.45

* build(ct): make application image use new base image flexi stack IQSS#10478

* style(ct): remove stale comment from base module pom

* chore(ct): add comment about apt sec updates detection via list hack

For now, we stick to not alter the image more than necessary. Only packages we installed will be upgraded, as these are not part of the normal Java base images.

The Java base images receive regular updates and undergo testing. It might be unwise to just install all the security updates we could get.

Leaving the option here for later saves the trouble to dig up the solution again.

* ci(ct): use new setup-maven action in base image push workflow

Simplify setup following DRY principle

* ci(ct): replace logic in base image push workflow

Using the same actions and steps as done in the maintenance action should work for this workflow in case of a push event, too.

* test(ct): temporarily set dev branch in base image push flow to feature branch

* ci(ct): use setup-maven action in app image push workflow to simplify setup

Following DRY principle, reuse the steps defined

* test(ct): temporarily enable app image push flow execution in all forks

* avoid spurious log warning for others e.g. isbn

these were going through the default check for URLs and failing (not a
url) leading to a warning. The new code should try URL parsing for URLs,
try PID and URL parsing for ones with no type specified, and send the
rest of the identifiers w/o any additional (optional) attributes.

* update doc

* ci(ct): use an optional base image ref for app image push flow

We need to transfer the determined base image name we might have just built from the calling workflow into this flow.
As we provide a default value, this is picked up for pull_requests.

* ci(setup-maven): try to auto-detect git ref

It's not so easy to determine the right git ref for different scenarios like PR, etc. Unless explicitly given a ref, try to autodetect the right one to go with.

* chore(ct): add notes in flows about adding a path filter

We want to avoid duplicate runs which might trigger race conditions for image shipments.

* chore(ct): add note about missing triggers for base push flow

This is out of scope for IQSS#10827, but should be addressed at a later point to avoid duplicated runs with potential race conditions. Also it enables proper rebuilds for preview images when someone is just trying to create a base image change, but which should obviously be tested full chain.

* fix,ci(ct): don't trigger the base push flow for backports

Obviously these are meant for the maintenance workflow, not the push flow!

* ci(ct): trigger app flow from base push flow

We detect the tag we have been using in a finalizing step to hand a proper base image ref to the app image workflow to make it work on the images we just pushed to the registry.

* changes per review

* Apply suggestions from code review

Co-authored-by: Philip Durbin <[email protected]>

* cleaner formatting

* minor doc tweak IQSS#10632

* standardize image url

* No longer needed with use of CDI.current() in XMLMetadataTemplate

* no longer used and CrossRef ended up using it's own.

* add more info about the scope of changes.

* doc changes

* doc changes

* ci(ct): reshape maintenance workflow into external matrix script

Unfortunately, matrix jobs logs and outputs cannot be aggregated in Github Actions.

The only way to work around the limitations of GHA is by using a custom build script that create a similar matrix like experience.

This commit introduces these scripts, probably also making some custom actions we added obsolete.

* ci(ct): remove obsolete actions for revisions and parent image changes detection

* ci(ct): re-enable forced build for maintenance workflow

* doc changes

* doc changes

* ci(ct): add outputs to maintenance matrix job

Can be picked up by other jobs, e.g. to create textblocks for docs or a job matrix.

* ci(ct): remove draft of building app images in maintenance matrix job

* style,ci(ct): reword the maintenance build workflow name

* ci(ct): make the maintenance workflow push the hub description for the base image IQSS#10478

* style(ct): fix simple typo in base image README

* fix(ct): remove bug from package upgrade detection in maintenance workflow

We did not correctly compare the status code of the grep command, breaking the update detection

* update query per review comments to handle all cases

* docs,style(ct): small rewording about immutable tags for base image

* feat,ci(ct): add immutable tags to list of base image tags in maintenance job

As discussed during community meeting on 2024-09-19.

* style,ci(ct): add some more verbosity about progress in maintenance job

* refactor,ci(ct): finishing touches for IQSS#10478

Re-enable and change everything necessary to reference the upstream IQSS context as of now.

* add docs for disable-dataset-thumbnail-autoselect IQSS#10819 IQSS#10820

* create 6.4 release notes and add about half the updates IQSS#10853

* add second half of snippets IQSS#10853

* fix,ci(ct): only add base image Maven option when the input is defined

Without this in case of the pull_request event the input is null and the build fails because we have base image defined at all.

Simply not adding the option if the input is undefined means we stick to what is defined within the POM.

* add highlights and upgrade steps IQSS#10853

* docs(ct): add release note for maintenance workflow IQSS#10478

* style(ct): add comment explaining what a flavor is in base image Maven props

Co-authored-by: Philip Durbin <[email protected]>

* A potential simple fix for IQSS#10667 ?

* Update 6.4-release-notes.md

corrected the schema.xml instructions

* add blurb for tagged base images IQSS#10853

* doc how to handle develop into develop PRs IQSS#9508

* remove extra line

* dont return image_url if there is none

* Fix NPE using CVOC

* set dataset type before registering pid (which needs the type)

* add release note

* adding fix from review comment

* remove tabs to make reviewdog happy. woof! IQSS#10623

* globus doc tweaks IQSS#10623

* add more and better DataCite export IQSS#10853

* add blur for listing feature flags IQSS#10853

* add CVOC bug fix IQSS#10853
-6.4-release-notes

* update image_url IQSS#10853

* Updated the docs to reflect the new name of a JVM option (IQSS#10623)

* I fixed anchor links IQSS#10876 (IQSS#10877)

* improve release note IQSS#10623

* add globus async IQSS#10853

* add cvoc update IQSS#10853

* add new globus settings under settings section IQSS#10853

* various tweaks IQSS#10853

* typo IQSS#10853

* put features before bug fixes IQSS#10853

* reword

* datacite title

* croissant update IQSS#10853

* bump version to 6.4 IQSS#10852 (IQSS#10871)

* displayOnCreate set to true for depositor and dateOfDeposit in Citation metadata block (IQSS#10884)

* Changed: displayOnCreate set to true for depositor and dateOfDeposit in citation.tsv

* Changed: MetadataBlocksIT test assertion for new total number of displayOnCreate fields

* Added: release notes for IQSS#10850

* Added: minor tweak to release notes

* IQSS#10853 fix typo version number

* Add release note change for fields depositor and dateOfDeposit in the citation.tsv

* remove old release note

* formatting fix

fixed formatting of the shell block in the upgrade instruction

* tweak depositor and dateOfDeposit IQSS#10853

* fixed update-fields.sh url 

(it had "9.4.1" in it; and we probably don't want to get it from the master branch either)

* reindex instruction

* removed a superfluous command line

* temp dir cleanup

* typo

* docs: update release notes from IQSS#10343

* tweaks IQSS#10343

* Upgrade to upstream version 6.4

* Merge upstream v6.4 into branch properties

* Sync with upstream

* Fix merge of properties

* bugfix: metadataFragment.xhtml

* fix relationType display value bug

---------

Co-authored-by: Jim Myers <[email protected]>
Co-authored-by: GPortas <[email protected]>
Co-authored-by: Philip Durbin <[email protected]>
Co-authored-by: landreev <[email protected]>
Co-authored-by: jeromeroucou <[email protected]>
Co-authored-by: Philip Durbin <[email protected]>
Co-authored-by: Ludovic DANIEL <[email protected]>
Co-authored-by: sbondka <[email protected]>
Co-authored-by: Stephen Kraffmiller <[email protected]>
Co-authored-by: Steven Winship <[email protected]>
Co-authored-by: Benedikt Kruse <[email protected]>
Co-authored-by: Oliver Bertuch <[email protected]>
Co-authored-by: Oliver Bertuch <[email protected]>
Co-authored-by: qqmyers <[email protected]>
Co-authored-by: paulboon <[email protected]>
Co-authored-by: ofahimIQSS <[email protected]>
Co-authored-by: Florian Fritze <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Size: 3 A percentage of a sprint. 2.1 hours. Type: Bug a defect
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants