search

zopefoundation / Products.PluggableAuthService

Pluggable Zope authentication / authorization framework
Other
9 stars 18 forks source link

Add events for PrincipalAddedToGroup and PrincipalRemovedFromGroup #21

Closed ezvirtual closed 4 years ago

ezvirtual commented 5 years ago

Hi there, I have gone ahead and added the PAS events for principals being added and removed from groups. This was discussed in this issue in April: https://github.com/zopefoundation/Products.PluggableAuthService/issues/17

I was hoping for a bit of advice in regards to the following:

I am intending to implement these missing events in master as well but added them in the Plone 4 compatible version first as that is where we have the immediate need for them.

Any constructive suggestions would be appreciated.

icemac commented 5 years ago

@ezvirtual wrote: […]

I was hoping for a bit of advice in regards to the following:

  • Is there any process I need to follow to make a PR here? Contributor agreement etc?

The contributor agreement is here: http://docs.zope.org/developer/becoming-a-committer.html

  • What's the policy around versioning? Did I do the right thing by incrementing the version to 1.12.0?

A version 1.12 should not be in the 1.11.x branch. (It should only contain bugfixes for 1.11.) I just created a 1.x branch. Please move the target of your PR to this new branch.

  • Should the name of the branch be changed too?

The name of the branch in your repository does not matter.

[…]

I am intending to implement these missing events in master as well but added them in the Plone 4 compatible version first as that is where we have the immediate need for them.

That's okay.

Any constructive suggestions would be appreciated.

I haven't looked into the code change at all, yet.

ezvirtual commented 5 years ago

I think that I've made all of the suggested changes & added event notification from the ZODBGroupManager plugin.

Thanks for adding a new branch and updating it. The versioning stuff all makes perfect sense in hindsight!

Any further suggestions welcome

ezvirtual commented 5 years ago

Thanks for that suggestion. I've committed that change along with a small bug fix in events.userCredentialsUpdatedHandler

Please let me know if there's anything else required from my end.

Cheers

tseaver commented 5 years ago

Merge blocked pending resolution of contributor agreement (needs a reference from an existing committer).

dataflake commented 5 years ago

Has the committer agreement been submitted/accepted?

Robynstar commented 5 years ago

The agreement had been submitted, but the reference's credentials had been lost! However a new reference has been found so hopefully things can be moving along soon :smiley:

sgeulette commented 5 years ago

Hi all, Could this pr finally merged ? Regards

dataflake commented 5 years ago

The author still doesn't show as having signed the contributor agreement.

ezvirtual commented 5 years ago

Hi everyone, I sent an email on March 27th saying that I'd contacted @thet & that he had said he was happy to be a referee for my contributor agreement. This was sent to @jimfulton @tseaver @icemac & the contributor-agreement address [at]zope.org

I'm not sure what else I need to do to get this merged?

Cheers

jugmac00 commented 5 years ago

@ezvirtual I am sorry that you have to wait so long.

Please re-send your email to contributor-admin@zope.org

Also, I will be at the Zope sprint tomorrow, where also @icemac takes part. I'll talk to him how we could improve the contributor onboarding experience a bit.

ezvirtual commented 5 years ago

Hi @jugmac00 I have resent that now. Appreciate that follow-up, it's probably a good thing to make easier for people :)

Cheers,

@ezvirtual I am sorry that you have to wait so long.

Please re-send your email to contributor-admin@zope.org

Also, I will be at the Zope sprint tomorrow, where also @icemac takes part. I'll talk to him how we could improve the contributor onboarding experience a bit.

jugmac00 commented 5 years ago

HI @ezvirtual - quick update. @MatthewWilkes made an update about the merge of Zope and Plone Foundation which willl - in future - result only in my agreement to sign (which you obviously did already for Plone). But he also announced you may sign the Plone agreement once more but only swap out the project name. Please also read https://github.com/plone/mr.roboto/pull/74#issuecomment-490570576

Looks like a lot is going on in the moment - but I am very positive it will work out great.

MatthewWilkes commented 5 years ago

@ezvirtual Sorry you've been having trouble here. We are very much in a state of flux right now. I wouldn't say I "announced" that there's a new process, as we're still working on details. If you want to do as @jugmac00 suggests and follow the Plone procedure but enter "Zope" as the program that should be sufficient to meet the legal obligations.

ezvirtual commented 5 years ago

I've (finally) managed to complete all of the herculean tasks required and am a signed up contributor.

Can we now talk about merging this small commit?

Cheers

jugmac00 commented 5 years ago

Your changes got approved and you (finally) got a commiter agreement signed - so go ahead :-) You are able to merge the changes yourself.

Update Wait, the checks are failing :-(

Please have a look at the tests - they have to pass.

dataflake commented 5 years ago

Stop stop stop. The reason for the failing tests needs to be investigated first.

dataflake commented 5 years ago

The code as it stands doesn't work. The tests fail. The tests on the source branch 1.x do not fail.

Please fix your code and then we'll be happy to include it.

If you ever intend to move to PluggableAuthService 2.0 you should also create a PR against master with your changes.

ezvirtual commented 5 years ago

Hello everyone, Can someone please sanity check the following:

Thanks :)

icemac commented 5 years ago

Is there a new status for this PR? What needs to be done as the next step?

sgeulette commented 4 years ago

I have tested this PR without problem. Can be merged... ?

ale-rt commented 4 years ago

It looks to me @ezvirtual contributor agreement has been accepted https://github.com/orgs/zopefoundation/people?utf8=%E2%9C%93&query=ezvirtual To prove that @ezvirtual should press the merge button. Anyway I am requesting some changes.

sgeulette commented 4 years ago

agreed with you for parameters names and tests

dataflake commented 4 years ago

I have created a new PR #58 to replace this PR because I want this finished and don't want to go through the hoops of getting access to a fork. Please direct all further comments there. I believe I made all changes necessary to address the remaining concerns. I am closing this PR.