fixes #12894 - remove 'users' from org update params

[thomasmckay]

No description provided.

[jlsherrill]

Do you even remember the point of this?

[thomasmckay]

I suggested that users be removed from the attr_accessible on the taxonomy but apparently a usage of Organization.new() is required. Trying to update an org w/ a users param fails with the error. The katello UI gets org json, modifies some of it, then PUTs it back. Foreman UI doesn't use API so they don't encounter this.

[jlsherrill]

ahhh when we update the cdn url in the UI i see, I should have read the issue!

[daviddavis]

Wondering if we should add a comment so that we can remove this line when foreman moves to strong_params.

[jlsherrill]

I'm still getting an error:

"ERF42-6434 [Foreman::Exception]: You cannot set an organization's parent_id. This feature is disabled."

[jlsherrill]

I wonder if it would make sense to just update the cdn url here: https://github.com/Katello/katello/blob/master/engines/bastion_katello/app/assets/javascripts/bastion_katello/subscriptions/manifest/manifest-import.controller.js#L152

instead of PUT'ing the whole object.

[thomasmckay]

The parent_id is likely from this PR theforeman/foreman#3094
I think you're right that it's better to just trim down the info in the caller. I had not wanted to do that but it's more likely to work long term.

[thomasmckay]

@jlsherrill @daviddavis - updated to remove json fields on GET /organizations and added comment there.

[jlsherrill]

would you be opposed to building up what we need (and calling Organization.update()) rather than deleting what we don't?

[thomasmckay]

I originally was just going to modify the org update from the manifests page to just send redhat_repository_url. However, any bastion page that updates an org would end up sending the GET values and would also break (as I discovered in one of my local plugins).

To be honest, I first tried to fix this in foreman itself (not allow 'users') but that was nack'ed. The parent params should also not be present in katello.

Thus! This seemed like the safest thing all around.

[jlsherrill]

right, walden hit this very similarly with hosts recently and I liked the approach he ended up taking much better. Editing the object in the GET seems really hacky (or even more hacky) and means if we ever actually need to use users, parent_id or parent_name, we can't.

https://github.com/Katello/katello/blob/master/engines/bastion_katello/app/assets/javascripts/bastion_katello/content-hosts/details/content-host-details.controller.js#L60-L72

I don't mean to be a stickler, but this just seems like the wrong approach.

[thomasmckay]

@jlsherrill - That looks good, I'll change PR to that pattern.

[jlsherrill]

In the controller all it would be would be something like changing the line here:
https://github.com/Katello/katello/blob/master/engines/bastion_katello/app/assets/javascripts/bastion_katello/subscriptions/manifest/manifest-import.controller.js#L152

from

organization.$update(function (response) {

to

Organization.update({'id': organization.id, 'redhat_cdn_url': organization.redhat_cdn_url}, function() {

It may be slightly more complex than that, but not much

[thomasmckay]

@jlsherrill - So I updated the call to Organization.update() call to just send up the id and redhat_repository_url. This results, however, in

 "error": {"id":3,"errors":{"environments":["you cannot remove environments that are used by hosts or inherited."],"domains":["you cannot remove domains that are used by hosts or inherited."]},"full_messages":["Environments you cannot remove environments that are used by hosts or inherited.","Domains you cannot remove domains that are used by hosts or inherited."]}

You'll notice that in the controller the redhat_repository_url is handled specially and then 'super' is called. Perhaps when we switch to PATCH this will work?

Thoughts?

[jlsherrill]

@thomasmckay can you paste your code? What you're seeing doesn't make a whole lot of sense given the foreman controller just uses .update_attributes.

I tried this myself doing:

PUT /api/v2/organizations/1/
with:
{"name": "default_org1"}

and it seemed to work fine.

[thomasmckay]

@jlsherrill - You're right! That error was due to the org having "mismatches" for domains and environments. I'll update this PR shortly.

[jlsherrill]

[test]

[jlsherrill]

ACK