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

Iqss/9183 terms of access validation fix #9184

Merged
merged 8 commits into from
Apr 6, 2023

Conversation

haarli
Copy link
Contributor

@haarli haarli commented Nov 23, 2022

What this PR does / why we need it:
Prevents that an dataset is saved twice (which might result in subsequent OptimisticLockingError problems), when the two hidden input fields for Terms of Access validation are not available (e.g. due to a customized terms page)

Which issue(s) this PR closes:

Closes #9183

@haarli haarli changed the title Iqss/9183 terms of access validation dix Iqss/9183 terms of access validation fix Nov 23, 2022
Copy link
Member

@qqmyers qqmyers left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Interesting issue - fixing a bug that only appears in a fork. W.r.t. the code in the PR, it makes sense to move the code into the try block - it should have no effect for the community version but is reasonable code clean-up - if anything is changed here in the future such that another way to have an error is introduced, the current code could end up doing a double save, as in the fork, and the PR prevents that possibility.

@pdurbin
Copy link
Member

pdurbin commented Jan 3, 2023

@haarli can you please explain how to reproduce the problem? In the issue you said you removed some input fields? Which ones? Is there some other way to reproduce it?

@haarli
Copy link
Contributor Author

haarli commented Jan 4, 2023

As you already mentioned, this is a very special behaviour which only takes effect when you mess around with the dataset-license-terms page. So this PR is just a code cleanup.
Here are the steps to reproduce:

Prerequesites:
The hidden input fields with ids "termsofAccessHiddenLT" and "fileAccessRequestHiddenLT" must be removed in dataset-license-terms.xhtml

<o:inputHidden id="termsofAccessHiddenLT" value="#{termsOfUseAndAccess.termsOfAccess}" />
<o:inputHidden id="fileAccessRequestHiddenLT" value="#{termsOfUseAndAccess.fileAccessRequest}" />

(in our case, we just wanted to display the licenses list and the terms of use field, and removed everything else)

Steps:

  1. Create a new dataset
  2. Go to Terms Tab, edit the terms (e.g. changing the license) and click on save => The dataset is saved twice due to the javascript code.
    At first in the catch-clause, which is called because the hidden fields do not exist:
    datasetSaveCommand();

    and then in the further code:
    datasetSaveCommand();

    The DatasetPage Scope keeps the version after the first save, the database contains the newer version of the second save.
  3. Directly click on Publish afterwards -> OptimisticLockingError because the versions of the page scope and the database differ.

@pdurbin pdurbin added Type: Suggestion an idea Component: Code Infrastructure formerly "Feature: Code Infrastructure" Size: 3 A percentage of a sprint. 2.1 hours. and removed Help Wanted: Cannot Reproduce labels Jan 4, 2023
@mreekie
Copy link

mreekie commented Jan 10, 2023

Prio meeting with Stefano.

  • Moved from Community Backlog to ordered backlog

@mreekie mreekie added this to New in deleteMeAfterTesting via automation Feb 23, 2023
@mreekie mreekie removed this from New in deleteMeAfterTesting Feb 23, 2023
@mreekie mreekie added this to New in deleteMeAfterTesting via automation Feb 23, 2023
@mreekie mreekie removed this from New in deleteMeAfterTesting Feb 23, 2023
@mreekie mreekie added this to New in deleteMeAfterTesting via automation Feb 23, 2023
@mreekie mreekie removed this from New in deleteMeAfterTesting Feb 23, 2023
@scolapasta scolapasta moved this from ▶ SPRINT READY to This Sprint 🏃‍♀️ 🏃 in IQSS/dataverse (TO BE RETIRED / DELETED in favor of project 34) Mar 29, 2023
@scolapasta scolapasta moved this from This Sprint 🏃‍♀️ 🏃 to Ready for Review ⏩ in IQSS/dataverse (TO BE RETIRED / DELETED in favor of project 34) Mar 29, 2023
IQSS/dataverse (TO BE RETIRED / DELETED in favor of project 34) automation moved this from Ready for Review ⏩ to Ready for QA ⏩ Apr 3, 2023
@kcondon kcondon self-assigned this Apr 4, 2023
@kcondon
Copy link
Contributor

kcondon commented Apr 5, 2023

Hi @haarli , would you please refresh this branch from develop since we've since update the version and this prevents us from building and testing. Thanks!

@kcondon kcondon merged commit 42f81fe into IQSS:develop Apr 6, 2023
IQSS/dataverse (TO BE RETIRED / DELETED in favor of project 34) automation moved this from QA ✅ to Done 🚀 Apr 6, 2023
@pdurbin pdurbin added this to the 5.14 milestone May 10, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Component: Code Infrastructure formerly "Feature: Code Infrastructure" Size: 3 A percentage of a sprint. 2.1 hours. Type: Suggestion an idea
Projects
Status: No status
Development

Successfully merging this pull request may close these issues.

Bug: Dataset saved twice when updating terms
7 participants