-
-
Notifications
You must be signed in to change notification settings - Fork 429
Assembly members #3008
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
Assembly members #3008
Conversation
d2d76e4
to
0f424f6
Compare
Codecov Report
@@ Coverage Diff @@
## master #3008 +/- ##
=========================================
Coverage ? 97.52%
=========================================
Files ? 1688
Lines ? 40348
Branches ? 0
=========================================
Hits ? 39349
Misses ? 999
Partials ? 0 |
3b2d991
to
3292bbb
Compare
18ed118
to
bda7e91
Compare
0578433
to
5e9a8b5
Compare
Hi! I'm introducing a custom reusable component that uses My idea is to add a I'm almost finished (not code uploaded yet) but I wanted to ask if there is any any concern about this. |
72d64dd
to
e0b710f
Compare
Hi, I need some opinions/guidance/help in my implementation of a autocomplete select with React. Can do a fast review on this commit, please? e0b710f I need to implement specs, pass linters, etc. but in the meanwhile are the pieces in the right place? /cc @decidim/lot-core @deivid-rodriguez Edit: wrong commit link |
format.json do | ||
render json: [] && return if params.fetch(:term, "").empty? | ||
|
||
query = current_organization.users&.order(name: :asc) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This code is very similar to another PR, maybe we should extract this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@oriolgual What do you propose? Extract it to a concern? It's a good fit for an API endpoint, but it has no authentication, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, we would have to add the api
as dependency for the admin
... 😕
include Decidim::Traceable | ||
include Decidim::Loggable | ||
|
||
GENDERS = %w(man woman).freeze |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@decidim/product do we really need to save the gender? I'm not an expert but in terms of LOPD isn't this considered a different level of data?
Also, if we really want to know the gender of the members, maybe we should consider an extra field with a free text option.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @oriolgual, this is a specific requirement from Barcelona Participation Regulation, which states that the composition of assemblies must be proportional in terms of age, gender and origin.
That said, I think a drop-down field with binary option is not a good solution. As the gender issue can be quite tricky (see the 71 gender options offered by Facebook) I would leave this field as an open text field.
In the end, it should serve to make parity issues visible. I understand that using an open text field is the simplest change.
create_table :decidim_assembly_members do |t| | ||
t.references :decidim_assembly, index: true | ||
t.references :decidim_user, index: true | ||
t.integer :weight, null: false, default: 0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add an index here since it's the column used as default sorting.
designation_mode: Designation mode | ||
full_name: Full name | ||
gender: Gender | ||
origin: Origin |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@decidim/product what do we actually need to save here? Where the member is native from? See https://www.wordnik.com/words/origin
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@oriolgual sorry I forgot to answer this one. Yes, this refers to place of birth (again, data required from Barcelona Participation Regulations)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe birthplace
would be a better naming?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll be working on this today, I'll change this field to birthplace
.
@rbngzlv Sorry I don't have time to review your work at the moment but at first glance it seems like a fantastic piece of job that's very interesting for other places of decidim as well, and helps up better integrating react in our workflows. Love what you're building 💪 |
@rbngzlv @isaacmg410 this PR is expected to be merged this week to enter v0.12. Do you expect to have it finished soon? |
# Conflicts: # decidim-assemblies/app/models/decidim/assemblies/abilities/admin/admin_ability.rb # decidim-assemblies/app/models/decidim/assemblies/abilities/admin/assembly_admin_ability.rb # decidim-assemblies/app/models/decidim/assemblies/abilities/admin_ability.rb # decidim-assemblies/app/models/decidim/assemblies/abilities/assembly_role_ability.rb # decidim-assemblies/app/models/decidim/assemblies/abilities/everyone_ability.rb # decidim-assemblies/app/views/layouts/decidim/admin/assembly.html.erb # decidim-assemblies/lib/decidim/assemblies/test/factories.rb
@decidim/lot-core updated with master and conflicts fixed. @mrcasals can you check my implementation for the new permissions system? |
@rbngzlv yup, the permissions commit looks good to me! I'll check the rest of the PR too and try to merge it 😄 |
helper_method :collection | ||
|
||
def index | ||
redirect_to "/404" if members.none? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should raise ActionController::RoutingError, "No members for this assembly"
here. In development we'd see the error, in production we'd be redirected to /404
.
Thoughts?
module Abilities | ||
module Admin | ||
# Defines the abilities for an admin user. Intended to be used with `cancancan`. | ||
class AdminAbility < Decidim::Abilities::AdminAbility |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These ability classes should be removed
module Assemblies | ||
module Abilities | ||
# Defines the abilities for an admin user. Intended to be used with `cancancan`. | ||
class AdminAbility < Decidim::Abilities::AdminAbility |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be removed
module Abilities | ||
# Defines the abilities for any user that can manage assemblies (whatever their role). | ||
# Intended to be used with `cancancan`. | ||
class AssemblyRoleAbility |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be removed
module Abilities | ||
# Defines the base abilities related to assemblies for any user. Guest users | ||
# will use these too. Intended to be used with `cancancan`. | ||
class EveryoneAbility < Decidim::Abilities::EveryoneAbility |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be removed
@@ -0,0 +1,29 @@ | |||
<% member_presenter = Decidim::AssemblyMemberPresenter.new(assembly_member) %> | |||
|
|||
<div class="column"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Move this to a cell
, maybe? 😄
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure about this, for now we're not going to reuse this view.
Anyway, if we want to have this moved to a cell
, I can make the commitment to do it in a future PR, but not now. We need this feature to be merged before the end of this week, and we have two more related PR's pending. What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds reasonable to me, 👍
module Admin | ||
# Defines the abilities for an assembly admin user. Intended to be used | ||
# with `cancancan`. | ||
class AssemblyAdminAbility < Decidim::Assemblies::Abilities::Admin::AssemblyRoleAbility |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This one should be removed too
@rbngzlv you need to update the test after adding the exception 😄 |
783aed9
to
bc1aa2f
Compare
* Add AssemblyMember entity * Show/Hide and Disable/Enable position other input * Implement admin log * Add seeds for assembly members * Basic public UI * Add weight attribute to order members * A member can be ceased * Allow to filter and search members in admin * Apply final design * Add specs * Rename origin to birthplace * Change gender from select to text field * Add changelog entry * Fix locales, rubocop and remove unnecessary specs * Add birthplace to factories * Implement new permissions sytem * Remove cancancan abilities * raise ActionController::RoutingError instead of redirect * Update spec
🎩 What? Why?
An assembly can have members (president, vice president, ...)
📌 Related Issues
📋 Subtasks
CHANGELOG
entry📷 Screenshots