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

Add some helper methods to be able to get a scim resource #99

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

DeRRudi77
Copy link

In some cases it is useful to get a scim_resource based on a patch hash and not have it directly patched on the Active Record model itself. Therefor i've introduced two methods to make this possible. Let me know what you think of this.

self.class.scim_resource_type.new(self.scim_hash_from_patch(patch_hash:))
end

def scim_hash_from_patch(patch_hash:)
Copy link
Member

@pond pond Jan 17, 2024

Choose a reason for hiding this comment

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

This needs a method comment explaining what it does, in RDoc format, as with all other methods. See lines above line 418 for what's already there. It probably needs to reference the documentation of the #from_scim_patch! method as a "see also" and/or vice versa. Use bundle exec rake rerdoc to rebuild RDoc content and view it locally in a web browser to make sure the markup is correct.

I do not understand scim_resource_from_patch. A patch payload contains an alteration, not the entire resource. It can often contain just a single operation for a single attribute. The method implemented would produce an invalid resource with only patched fields present and other things missing. There are already better ways to retrieve a resource instance and patch it.

Copy link
Author

@DeRRudi77 DeRRudi77 Jan 17, 2024

Choose a reason for hiding this comment

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

This needs a method comment explaining what it does

Will add it

I do not understand scim_resource_from_patch

Will add documentation for it, but in essence it creates a complete scim resource based on the original AR object and applies the patches, this is how the from_scim_patch! internally works. I've tested this and always get a complete resource with the patches applied.

There are already better ways to retrieve a resource instance and patch it.

Would love to see those, can you point me in the right direction? If we have those, we don't need these extractions, I just couldn't find them, nor are they documented.

Copy link
Author

Choose a reason for hiding this comment

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

@pond if possible could you point me towards this?

There are already better ways to retrieve a resource instance and patch it.

If I know how to do this, I can possibly close this MR.

Copy link
Member

@pond pond Jan 18, 2024

Choose a reason for hiding this comment

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

@DeRRudi77 Sorry for delay. I got a bit sick and had no sleep into the week, so just wasn't firing on all cylinders (and am still not, but I'll do my best).

I am wrong about the resource recovery. I had forgotten that the old #from_scim_patch ! does take a full SCIM representation of the existing model then apply patch operations on top, so the reworked code in the PR isn't just getting the patch parts only.

In that case, all we need here is comments in RDoc format on the 80 column wrap limit as elsewhere in this file and then the code itself is good. Tests, however, should be added, please - are you OK doing that? It is hopefully not too hard to take some of the existing mixin #from_scim_patch ! and do a bit of copy-paste pulling out of subset parts of the tests to cover the new methods, I would think.

@pond
Copy link
Member

pond commented Mar 13, 2024

@DeRRudi77 I was cycling back onto some Scimitar work and hoping to merge this PR but the requested changes are still I think waiting for a push?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants