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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

DEV: adds event hook when add/remove user to group #8038

Merged
merged 5 commits into from Sep 10, 2019

Conversation

@jjaffeux
Copy link
Contributor

commented Aug 25, 2019

This is needed for a project of mine 馃 ... and also I think it makes sense to have this hook in core.

@jjaffeux jjaffeux requested a review from eviltrout Aug 25, 2019

@discoursebot

This comment has been minimized.

Copy link

commented Aug 25, 2019

You've signed the CLA, jjaffeux. Thank you! This pull request is ready for review.

@SamSaffron

This comment has been minimized.

Copy link
Member

commented Aug 26, 2019

I am fine to merge this provided you are comfortable with this not triggering when some automatic group membership is triggered.

Also I think we should probably add a spec here (just tack on to existing specs) cause this will ensure we have this will not break.

@jjaffeux

This comment has been minimized.

Copy link
Contributor Author

commented Aug 26, 2019

I am fine to merge this provided you are comfortable with this not triggering when some automatic group membership is triggered.

Also I think we should probably add a spec here (just tack on to existing specs) cause this will ensure we have this will not break.

Cool thanks, I did 馃憤

From my tests, it seems like it's triggering for automatic group membership too, which makes sense giving the automatic job does group.add(user).

We might want to change method definition to reflect this:
from def add(user, notify: false) to def add(user, notify: false, automatic: false)
It's probably a good info to have in the hook to know if it's coming from manual or automatic action.

@discoursereviewbot

This comment has been minimized.

Copy link

commented Aug 26, 2019

Joffrey JAFFEUX posted:

@ZogStriP @SamSaffron I have asked additional questions on github which are not showing here

@discoursereviewbot

This comment has been minimized.

Copy link

commented Aug 26, 2019

R茅gis Hanol posted:

Yeah, I don't think we handle post edits. cc @danielwaterworth

@eviltrout
Copy link
Member

left a comment

LGTM

@discoursereviewbot

This comment has been minimized.

Copy link

commented Sep 4, 2019

Joffrey JAFFEUX posted:

Did you see the automatic suggestion too? it鈥檚 not part of the PR yet, will add it if no one is against it.

@discoursereviewbot

This comment has been minimized.

Copy link

commented Sep 4, 2019

Robin Ward posted:

The suggestion seems good to me. However there are situations where I suspect it won't fire, like seeding a database for example.

@discoursereviewbot

This comment has been minimized.

Copy link

commented Sep 4, 2019

Joffrey JAFFEUX posted:

Guess Im fine with that 馃し鈥嶁檪锔

@jjaffeux jjaffeux merged commit a258699 into discourse:master Sep 10, 2019

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@jjaffeux jjaffeux deleted the jjaffeux:on-group-add-remove-member-event branch Sep 10, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
5 participants
You can鈥檛 perform that action at this time.