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

RapidPro: Add support to remove from all groups if left blank #163

Merged
merged 3 commits into from
Feb 21, 2025

Conversation

geoo89
Copy link
Contributor

@geoo89 geoo89 commented Feb 14, 2025

If the main argument ("message_text") is left blank in a remove_from_group row, the user will be removed from all groups.

fixes #161

@geoo89 geoo89 requested a review from fagiothree February 14, 2025 17:23
@fagiothree
Copy link
Member

I see how this is the most "elegant" way to implement it (also considering the json has an empty list for groups), but I wondering if this is maybe a bit dangerous from an authoring perspective? Could we maybe have a warning?

@geoo89
Copy link
Contributor Author

geoo89 commented Feb 15, 2025

I added a warning, please check.

We could also consider adding a special keyword like ALL (in all caps) that will also remove from all groups, but suppress the warning (in addition or instead of leaving the cell blank).

@fagiothree
Copy link
Member

@geoo89 the code did run fine and created the node as expected. The warning is included in the logs, but I am wondering if maybe it should also be in the terminal?
Happy also to have "ALL" and suppress the warning in that case, if that's easy to implement

@geoo89
Copy link
Contributor Author

geoo89 commented Feb 17, 2025

If I remember, warnings are by design only written to the log, while critical errors stop execution (and show an error in the terminal). There is the error level in between, does that have the behavior you propose? (Do you know of any errors that are shown in the terminal but do not stop execution?)

@fagiothree
Copy link
Member

mm yeah, you are right, I don't think there are. So maybe the safest would be to have the "ALL" keyword?

@geoo89
Copy link
Contributor Author

geoo89 commented Feb 17, 2025

Ok, I'll implement that then.
Should I elevate the blank cell to an error then and require the ALL? Only downside would be if you happen to have a group named "ALL".

@geoo89
Copy link
Contributor Author

geoo89 commented Feb 19, 2025

I added support for the ALL keyword, while leaving the warning in case the cell is empty.

@fagiothree fagiothree merged commit 73c4d22 into main Feb 21, 2025
12 checks passed
@fagiothree fagiothree deleted the iss161 branch February 21, 2025 12:03
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.

Support row type to remove users from all the groups they are in
2 participants