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.

Sign up for GitHub

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

Jump to bottom

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
Open

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

Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
e4d6754
Add some helper methods to be able to get a scim resource based on a …
DeRRudi77 Jan 16, 2024
3f0dcdf
Merge branch 'main' into add-helper-methods-in-resource-mixin
pond Mar 28, 2024
b25da5a
Fix mistakes I made in the conflict resolution
pond Mar 28, 2024
a6108c7
Update mixin.rb
pond Mar 28, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 14 additions & 2 deletions app/models/scimitar/resources/mixin.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -441,6 +441,19 @@ def from_scim!(scim_hash:, with_clearing: true)
# Call ONLY for PATCH. For POST and PUT, see #from_scim!.
#
def from_scim_patch!(patch_hash:)
ci_scim_hash = self.scim_hash_from_patch(patch_hash: patch_hash)

self.from_scim!(scim_hash: ci_scim_hash, with_clearing: false)
return self
end

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

def scim_hash_from_patch(patch_hash:)
Copy link
Member

@pond pond Jan 17, 2024 •
edited
Loading

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 •
edited
Loading

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 •
edited
Loading

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.

frozen_ci_patch_hash = patch_hash.with_indifferent_case_insensitive_access().freeze()
ci_scim_hash = self.to_scim(location: '(unused)').as_json().with_indifferent_case_insensitive_access()
operations = frozen_ci_patch_hash['operations']
Expand Down Expand Up @@ -517,8 +530,7 @@ def from_scim_patch!(patch_hash:)
end
end

self.from_scim!(scim_hash: ci_scim_hash, with_clearing: false)
return self
ci_scim_hash
end

private # (...but note that we're inside "included do" within a mixin)
Expand Down
Loading