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

Added a flag for forced person creation #107

Merged
merged 7 commits into from
Sep 15, 2022

Conversation

stollr
Copy link
Collaborator

@stollr stollr commented Sep 10, 2022

The force flag has to be set to true, if there is already another person with the same name (yes, in churches with a larger number of members it may happen that there are different persons with the same name ;-)) and the person should be created anyways. Otherwise the API responds with an error.

The 'force' flag has to be set to true, if there is already another person with the same name (yes, in churches with a larger number of members it may happen that there are different persons with the same name ;-)) and the person should be created anyways. Otherwise the API response with an error.
@DumbergerL
Copy link
Contributor

@Naitsirch That is a important point!

Can you add the flag in the example code for the docs and also describe the purpose in the docs?

public function testCreatePerson()

@stollr
Copy link
Collaborator Author

stollr commented Sep 11, 2022

I'll look into it ;-)

The 'force' flag has to be set to true, if there is already another person with the same name (yes, in churches with a larger number of members it may happen that there are different persons with the same name ;-)) and the person should be created anyways. Otherwise the API response with an error.
@stollr
Copy link
Collaborator Author

stollr commented Sep 12, 2022

Done. I hope I have done it correctly ^^

@stollr
Copy link
Collaborator Author

stollr commented Sep 12, 2022

Thank you for your trust to let me commit to your repository. I am not sure whether I'll do that, because it is your project and my coding style differs from yours. So I'll prefer to create PRs and wait for your response, if this is okay for you.

Btw. there won't be many more PRs from me, because the feature set, that I need is mostly complete. The only thing I still need, is the possibility to synchronize person photos. If this works, I'll stop annoying you :-D

@DumbergerL
Copy link
Contributor

Feel free to contribute however you like! I am grateful for your support!

Your update for the doc looks great!
I had to adjust the logic for creating new models because of the Absence API. Here the approach with the setters didn't work, because the Absence-Model expected an AbsenceReason-Model as property. Maybe you can take a look at the code again and check if this works for you?

if (method_exists($model, "fillWithData")) {
$model->fillWithData($responseData);
} else {
foreach ($responseData as $prop => $value) {
$setter = 'set' . ucfirst($prop);
if (method_exists($model, $setter) && 'meta' !== $prop) {
$model->$setter($value);
}
}
}

@DumbergerL
Copy link
Contributor

I would be happy if we stay in contact. If you want you can send me an email to: lukas.dumberger@gmail.com

I would be very interested in what use-case you are using the ChruchTools API for at your church. Maybe we can exchange ideas sometime! I am always interested in new project ideas.

@DumbergerL
Copy link
Contributor

Hi @Naitsirch,
can you approve i my changes for the code-base works for you?

Your update for the doc looks great! I had to adjust the logic for creating new models because of the Absence API. Here the approach with the setters didn't work, because the Absence-Model expected an AbsenceReason-Model as property. Maybe you can take a look at the code again and check if this works for you?

if (method_exists($model, "fillWithData")) {
$model->fillWithData($responseData);
} else {
foreach ($responseData as $prop => $value) {
$setter = 'set' . ucfirst($prop);
if (method_exists($model, $setter) && 'meta' !== $prop) {
$model->$setter($value);
}
}
}

@stollr
Copy link
Collaborator Author

stollr commented Sep 15, 2022

Sorry, I did not had the time, to check it, yet. But if it works for you, you can merge it and if I'll experience any errors I'd fix it vor create an issue ;-)

Otherwise I plan to go in with it at the weekend.

@DumbergerL
Copy link
Contributor

Thanks for your reply. I close the PR.

By the way: You said you would need to upload avatar of persons? I added the file-api today. I hope this helps you: #114

@DumbergerL DumbergerL merged commit 736effd into 5pm-HDH:master Sep 15, 2022
@stollr
Copy link
Collaborator Author

stollr commented Sep 15, 2022

I have tested it now, and it works fine ;-)
Thanks for working on the file API. I'll try to test it at the weekend and give you feedback.

@stollr
Copy link
Collaborator Author

stollr commented Sep 15, 2022

By the way, here is the project where I am using your library: https://github.com/naitsirch/ecgpb-memberlist.

This is the interactive command that I use to run the synchronization process:
https://github.com/naitsirch/ecgpb-memberlist/blob/master/src/Command/ChurchtoolsSyncInteractiveCommand.php

And this is the service class, that is responsible for the business logic:
https://github.com/naitsirch/ecgpb-memberlist/blob/master/src/Service/ChurchTools/Synchronizer.php

I'll contact you per mail for more details ;-)

@stollr stollr deleted the force_create branch October 6, 2022 20:01
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