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

Add process groups #94

Open
wants to merge 7 commits into
base: develop
Choose a base branch
from

Conversation

eliegaboriau
Copy link

This PR allows admins to add constraints about process groups.

It could closes #75 and it is based on those specs

@paulinebessoles
Copy link

@ahukkanen I tested Elie's branch and it works for me in local 😁

Copy link

@carlobeltrame carlobeltrame left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code looks much safer now! Maybe @paulinebessoles can test again whether everything still works as expected?

@paulinebessoles
Copy link

Sorry for the late reply! I tested again and it works for me 😁 @eliegaboriau could you update your branch so it matches the main branch recent commits?
@ahukkanen do you think you can review and merge this contribution in the next days?

@eliegaboriau
Copy link
Author

It should be okay now !

Copy link
Contributor

@sinaeftekhar sinaeftekhar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the contributions.
Overal looks good.

Copy link
Collaborator

@ahukkanen ahukkanen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great work and thank you very much for your great contribution @eliegaboriau!

I have tested this locally and it worked great. I could edit the strings in the participatory group page and when scoping the translation set only to a participatory group through the constraints.

However, there are some issues making tight coupling with the decidim-participatory_processes module with this one. Could you please have another look and see if there is a way to abstract that away, to make the module also usable when the decidim-participatory_processes module is not installed. We have one of those instances we are hosting ourselves.

I would like to keep this module separate from the decidim-participatory_processes to avoid tight coupling.

Another issue I had was with the ApplicationHelper being included everywhere, so please also see that comment below.

app/models/decidim/term_customizer/constraint.rb Outdated Show resolved Hide resolved
lib/decidim/term_customizer/context/base.rb Outdated Show resolved Hide resolved
lib/decidim/term_customizer/context/controller_context.rb Outdated Show resolved Hide resolved
lib/decidim/term_customizer/resolver.rb Outdated Show resolved Hide resolved
@eliegaboriau
Copy link
Author

Hello @ahukkanen, thanks for your review !
I've just changed a few things following your requests :

  • I've moved the manifests method in the module methods and remove all calls to the helper.
  • I've removed the participatory_process_group argument i was working with, and used the space instead. It seems to work well.
    Tell me if it seems okay !

@eliegaboriau eliegaboriau requested a review from ahukkanen March 27, 2023 15:09
@paulinebessoles
Copy link

@ahukkanen Is this solution right for you?

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.

5 participants