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

Problem changing publisher in dataset (affects perhaps other referenced properties too) #4298

Open
stefan-korn opened this issue Sep 26, 2024 · 2 comments
Assignees
Labels

Comments

@stefan-korn
Copy link
Contributor

Current Behavior

As far as I can see there is a problem with changing publishers via the dataset edit form.

The publishers in the dataset edit form are managed through the "list" widget with type "autocomplete". This widget is creating a new publisher if it does not exist (by typing the new publisher name and clicking it, see also #4056). This is working and you can also choose a publisher created like this for another dataset without problems.

Publishers (and other referenced elements too) can have additional properties. Currently this is only subOrganizationOf property for publishers, but in theory can be more as well. Now If you add something to these other properties, i. e. in default DKANv2 schema something to subOrganizationOf property, this will make problems if you switch a dataset from one publisher to another. The problem will occur if the publishers that are to be switched differ in their other properties.
So the problem will not occur as long as you do not edit publishers other properties. That is maybe the cause why this issue was not discovered yet. (You can of course only edit the properties if you edit the publisher directly, but this seems to be a valid operation as far as I can see).

The problem now is as follows:
If you change the publisher from one to another while saving the dataset, the Referencer will check if the publisher already exists and this check is based on a hash of the properties of the publisher. But the data for checking in case of a switch of publisher is the name of the new publisher and the value of the other properties of the old publisher (subOrganizationOf by default schema). Now if the new publisher does not have the same value for the other properties as the old publisher, a new publisher gets created with the name of the new publisher and the other properties from the old publisher.
This is really a problem if you plan to make use of the other properties, especially if you add more properties or if you use other references the same way publisher is used.

One could maybe avoid this, if one doesn't use the list widget with autocomplete, but currently that's the only way to create new publishers via the UI, see #4056 about that.

I am currently experimenting with injecting some additional code in the Referencer check that checks for the publisher other than via hash to avoid this problem. Will come up with more infos if I have something working.

Expected Behavior

Changing the publisher should just switch the reference and not create a new publisher.

Steps To Reproduce

  • Install DKAN with Demo Content
  • Edit the publishers and add something (different) to subOrganizationOf for each of the 3 publishers
  • Switch the publisher in one dataset
  • Check that a new publisher is created in that case, consisting of name from one publisher and subOrganizationOf of the other publisher

Relevant log output (optional)

No response

Anything else?

No response

@stefan-korn
Copy link
Contributor Author

I have created a module DKAN reference fix hat should fix this issue.

This module just needs to be enabled and it has to be taken care that the patch from the module's composer.json is applied to DKAN.

I needed to do:

  • composer require drupal/dkan_reference_fix
  • composer install

Only after composer install the patch is applied.

The patch is just changing some methods in DKAN from private to protected to enable the module to override them. The patch is not changing DKAN functionally. All the functionally changes are made in the DKAN reference fix module.

If you do the steps to reproduce and enable the DKAN reference fix module after installing DKAN, the issue should be fixed.

I am open for discussion how to fix it in DKAN directly. The module is only meant as a temporary solution as we are really in need to have this issue fixed before we start editing real data in our DKAN2 setup.

I would also gladly provide a MR to change the private methods in Referencer and HelperTrait into protected methods. Just tell me if that is okay and if you rather have all methods changed to protected or only the ones affected from this fix. Imho it would be more consistent to change them all to protected.

@janette
Copy link
Member

janette commented Oct 4, 2024

@stefan-korn I think it would be fine to change all private methods in Referencer and HelperTrait to protected. 👍

@dafeder dafeder self-assigned this Nov 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants