10 configuration webhook

[jkva]

Thanks Hans, this looks good - easy enough fix in general. What happens when a webhook with the name "Drupal" already exists but with another callback url? I can't remember if I covered the case for that in my earlier work on this functionality.

For the final PR you're free to drop the first two commits and force-push to the branch.

[catorghans]

Ehh, good point. What should happen? Add another one? or give a warning?

I can imagine an organisation with 2 drupal sites. Or a company/foundation with 2 musea, which probably then would be 2 organisations.

[jkva]

Ehh, good point. What should happen? Add another one? or give a warning?

I can imagine an organisation with 2 drupal sites. Or a company/foundation with 2 musea, which probably then would be 2 organisations.

I think it would be best to show an informative message - e.g., "Failed to create resource group: A group with the name "Drupal" but different configuration already exists."

It's an edge case, really — but this gives a clear message for any administrator on how to proceed.

Alternatively in the case of a group without expected URL not existing already, I've considered generating:

Drupal-[hash] where hash is substr(sha1(callback_url), 0, 5) ensuring a unique name for that callback url. But that might be overkill.

[catorghans]

Ok, another edge case: What if an admin changes the callback url in coyote admin after the group id is stored in Drupal? I think that is allowed, right? Drupal just notices: I already have a group Id, so I trust the admin knows what he is doing?

[jkva]

Agreed on the admin needing to know what they're doing. If someone "outside the loop" modifies a callback URL — while we could be detecting that Drupal-side — I don't think it's our responsibility. If they run into unexpected problems they can always verify things on the Drupal side of things.

[catorghans]

OK, what happens now is: it tries to create a Drupal group, but fails without warning. So a warning should be added.

Next edge case (which I think could happen): when you change Organization, the group stays the same! Which does not belong anymore to the stored Organization. So, when Organization changes, the group should be cleared.

[jkva]

OK, what happens now is: it tries to create a Drupal group, but fails without warning. So a warning should be added.

Next edge case (which I think could happen): when you change Organization, the group stays the same! Which does not belong anymore to the stored Organization. So, when Organization changes, the group should be cleared.

Agreed to both points.

[jkva]

It would be good to run the resource group check automatically when the organization changes.

[catorghans]

yeah, we now do that with buildForm, so it should also do it at validateForm IF organizationID is changed.

[catorghans]

OK, so CreateResourceGroupRequest::perform() just logs the error and returns null, it does not give the specific Error back. So when I see it gives a null back I will create the warning on the Form, which theoretically could be the wrong warning for the wrong issue.

[jkva]

OK, so CreateResourceGroupRequest::perform() just logs the error and returns null, it does not give the specific Error back. So when I see it gives a null back I will create the warning on the Form, which theoretically could be the wrong warning for the wrong issue.

Good call. I've created an issue on the ApiClient side.

[catorghans]

OK, I will simply add "Failed to create resource group" for now. When other issue is resolved I can adjust to specific error.

[catorghans]

OK, I made it slightly better. Warning says: "Resource group 'Drupal' could not be created." So that should already give a lot of info that it might be that a Drupal group is already available.

And resource group is also reset when token or endpoint is (succesfully) changed.

[jkva]

@catorghans this is now merged; I ended up rebasing the PR to drop the first two commits, as well as removing the unsupported shape annotation as a boyscout commit on this branch.