-
Notifications
You must be signed in to change notification settings - Fork 8
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
Added method to update person data #84
Conversation
because they are unnecessary
@DumbergerL I am not sure what to do with the failed Doc-Generator check "Inconsistent Docs. Please run DocGenerator and check in changed Files.". I suspect that the added code blocks in the When I run |
@Naitsirch Thank's for your contribution. I have thought of this "writing to api"-topic quite a lot. But i think it's not that big thing because ChurchTools don't offer many PATCH-Routes. So i think your approach is great! We should consider to add some Code-Checks to validate if all MODIFIABLE_ATTRIBUTES are contained in the Model-Class. This could also include to check if all Model-Attributes have Setters and Getter. |
Can you explain your last paragraph a little bit more? |
…te modifiableAttributes-getter in ExtractData-Trait
@Naitsirch Yes you are right! If the modifiableAttributes contains a key that does not exists in the Class it won't be exposed to the REST-API. I wrote a Unit-Test for this case: 73db4f4 |
@Naitsirch I refactored your implementation a little bit. Can you check if it still faces your needs? Does it work for you this way? If yes i would merge the PR. |
I hope I'll have time in the next days. Then I will test it and give feedback. |
@DumbergerL I have tested it now and it works fine :-) |
Currently there is no way to update a person's data with this adapter. This PR adds a new
update
method to thePersonRequest
andPersonRequestBuilder
classes.This PR also contains user documentation and unit tests.
This is how the method can be used:
In the doc you'll find some more explanations.
I'll be happy to see this getting merged, soon :-)