[Fixes #10290] complete ISO contact roles per ressource base with multiplicity

[mwallschlaeger]

This is a work in progress PR to give an overview and first feedback from as the changes getting a little out of hand.

What the current state is aware of:

• showing all contact roles via the rest API v2
• handling contact roles in metadata editor for dataset, I guess i would fully adopt this the same way for documents, maps and geoapps. I started working on this already (see changes).
• adopted resource base model to handle all contact roles and be able to set multiple authors for one role

I know that further work in this topic at least requires changes to the following sections/files:

There is even more files to change apart from the ones already changed an the ones listed above. Further tests have to be added to this PR.

I'm willing to further work on this issue the upcoming weeks, but I think it is good to get some advice from the PSC to see if the direction this is going fits the ideas of the committee members.

Checklist

Reviewing is a process done by project maintainers, mostly on a volunteer basis. We try to keep the overhead as small as possible and appreciate if you help us to do so by completing the following items. Feel free to ask in a comment if you have troubles with any of them.

For all pull requests:

• Confirm you have read the contribution guidelines
• You have sent a Contribution Licence Agreement (CLA) as necessary (not required for small changes, e.g., fixing typos in the documentation)
• Make sure the first PR targets the master branch, eventual backports will be managed later. This can be ignored if the PR is fixing an issue that only happens in a specific branch, but not in newer ones.

The following are required only for core and extension modules (they are welcomed, but not required, for contrib modules):

• There is a ticket in https://github.com/GeoNode/geonode/issues describing the issue/improvement/feature (a notable exemption is, changes not visible to end-users)
• The issue connected to the PR must have Labels and Milestone assigned
• PR for bug fixes and small new features are presented as a single commit
• Commit message must be in the form "[Fixes #<issue_number>] Title of the Issue"
• New unit tests have been added covering the changes, unless there is an explanation on why the tests are not necessary/implemented
• This PR passes all existing unit tests (test results will be reported by travis-ci after opening this PR)
• This PR passes the QA checks: flake8 geonode
• Commits changing the settings, UI, existing user workflows, or adding new functionality, need to include documentation updates
• Commits adding new texts do use gettext and have updated .po / .mo files (without location infos)

Submitting the PR does not require you to check all items, but by the time it gets merged, they should be either satisfied or inapplicable.

[lgtm-com]

This pull request introduces 2 alerts and fixes 2 when merging 09fc233 into 477c4bb - view on LGTM.com

new alerts:

• 1 for Except block handles 'BaseException'
• 1 for Unused import

fixed alerts:

• 2 for Unused local variable

---

Heads-up: LGTM.com's PR analysis will be disabled on the 5th of December, and LGTM.com will be shut down ⏻ completely on the 16th of December 2022. Please enable GitHub code scanning, which uses the same CodeQL engine ⚙️ that powers LGTM.com. For more information, please check out our post on the GitHub blog.

[MalteIwanicki]

This PR also needs to be merged when we wanna use a list instead of a single contact for point of contact.

[MalteIwanicki]

I took care of the contacts in the geonode_metadata_full.html. Once I can commit to your branch it's there

[gannebamm]

@MalteIwanicki @mwallschlaeger please take a peak at #10521
I think the mentioned changes could improve this draft PR

[giohappy]

@giohappy i will check this again. but what I recognized when I applied those changes to an existing Geonode Instance is that there are migrations required for the already uploaded resources

which migrations @mwallschlaeger? I don'ìt see migrations in your PR

[mwallschlaeger]

@giohappy i will check this again. but what I recognized when I applied those changes to an existing Geonode Instance is that there are migrations required for the already uploaded resources

which migrations @mwallschlaeger? I don'ìt see migrations in your PR

Hey the migrations are not included yet. but I tested this again they are not required. Regarding your issue. I cannot confirm that multiple users are not set correctly. Actually there is also a test for this case in geonode.layers.tests.test_resource_form_is_valid_multiple_user_contact_role_as_queryset and geonode.layers.tests.test_patch_processor and so on are tests setting multiple users for one contact role.

Can you please doublecheck that you have the right commit?

[giohappy]

@mwallschlaeger ok, I will doublecheck, BTW your latest merges seem to have broken the tests again.

[giohappy]

@MalteIwanicki I confirm it works as expected now. I think this can be merged as soon as the tests are fixed.

[ridoo]

@MalteIwanicki I confirm it works as expected now. I think this can be merged as soon as the tests are fixed.

@giohappy please note that the PR of Malte was superseded by another PR which already had been merged. See my comment here: #10367 (comment)

[MalteIwanicki]

@giohappy @ridoo I'm totally out of the loop for this PR, not knowing what it was about. It has been over a year. Do you need any action from my side?

[ridoo]

@giohappy @ridoo I'm totally out of the loop for this PR, not knowing what it was about. It has been over a year. Do you need any action from my side?

@MalteIwanicki no problem at all :)

[giohappy]

@mwallschlaeger FYI we're investigating several problems that were reported with the management of contacts in metadata. It's broken in many ways. We will open an issue soon but I was curious to know if you did notice the problems it's giving, like:

• contacts are not saved correctly or they're not saved at all
• the layout for documents is broken

[mwallschlaeger]

@giohappy hey i have noticed one issue I have fixed in our fork. i can at least play the one I have fixed back to core in the next days

[giohappy]

@mwallschlaeger do you mean that editing of contacts works fine (except the issue you mentioned)?
If you try on the stable demo you'll see that the changes to contacts are not saved.

[giohappy]

@mwallschlaeger I've seen your PR but tests are failing. You're using getlist on a dict object.
I wonder how much your fork has diverged from this.

If it cannot be easily fixed we will be forced to revert the commit the commits. I have built a Docker image at the commit of your PR and it seems they weren't working even at that time. Something must be changed between my tests of your PR and its merge.

[gannebamm]

@vineetasharma105

Please check this in our dev instance.

[gannebamm]

@giohappy If this is not easily fixable, than my +1 for reverting the changes for now.

[mwallschlaeger]

@giohappy I guess i figured it out now. The test which are failing are providing the form data using the data argument, which passes and empty dict to the not filled contact roles, where getlist cannot be executed on.

As in e.g.

geonode/geonode/layers/views.py

Line 532 in 75edcdb

dataset_form.fields[role].initial = [p.username for p in layer.__getattribute__(role)]

they get filled via initial.

Another problem I came across investigating this, was that contact roles couldn't be removed also this was related to the change of the expected object type in widgets.py.

I'm gonna run the tests now locally and commit the changes to the PR, so you can check it, too.

[giohappy]

@mwallschlaeger noticed that we see different behavior between stable demo (4.2.x) and master demo.

The latter is even more worrying because changes are applied when a user is selected for a role, but the resulting saved user is different from the one that was selected.