Conversation
|
@decidim/lot-core this PR is ready to review 😄 |
|
@rbngzlv could you add some screenshots too? |
|
@oriolgual I'will added |
oriolgual
left a comment
There was a problem hiding this comment.
Please add system specs for the admin
| private | ||
|
|
||
| def partner | ||
| return block unless model.link.presence |
There was a problem hiding this comment.
Maybe some of this view logic should go to the template, not inside the cell? It feels weird to me having all the markup here instead of the template.
| def initialize(form, current_user, conference) | ||
| @form = form | ||
| @current_user = current_user | ||
| @conference = conference |
There was a problem hiding this comment.
Shouldn't the conference be part of the form too?
There was a problem hiding this comment.
why? We are doing the same with speakers, conference admins, assembly admins, assembly members...
There was a problem hiding this comment.
I think it's cleaner (you don't have to merge attributes from the form) but maybe I didn't review it correctly in other PRs.
There was a problem hiding this comment.
I still think that it is better to continue with the same system, for the whole decidim. Not in different ways according to the command...
| # conference_partner - The ConferencePartner to update | ||
| def initialize(form, conference_partner) | ||
| @form = form | ||
| @conference_partner = conference_partner |
There was a problem hiding this comment.
Same as the creation form, if I'd move this inside the form so the command doesn't need to check anything else.
There was a problem hiding this comment.
In this case, is conference_partner, not conference.
|
|
||
| def edit | ||
| @partner = collection.find(params[:id]) | ||
| enforce_permission_to :update, :partner, speaker: @partner |
There was a problem hiding this comment.
Shouldn't this be partner instead of speaker?
|
|
||
| def update | ||
| @partner = collection.find(params[:id]) | ||
| enforce_permission_to :update, :partner, speaker: @partner |
There was a problem hiding this comment.
Shouldn't this be partner instead of speaker?
|
|
||
| def destroy | ||
| @partner = collection.find(params[:id]) | ||
| enforce_permission_to :destroy, :partner, speaker: @partner |
There was a problem hiding this comment.
Shouldn't this be partner instead of speaker?
| def index | ||
| enforce_permission_to :index, :partner | ||
|
|
||
| @query = params[:q] |
There was a problem hiding this comment.
nowhere, deleted!
|
@oriolgual requested changes applied! |
|
Merging! Tests look like they are not passing, but they really passed: https://circleci.com/workflow-run/a6af249c-e498-48fd-b200-077d298d00e4 |
🎩 What? Why?
This PR adds the new functionality of Conference Partners. Is part of the #3709
📌 Related Issues
📋 Subtasks
CHANGELOGentry📷 Screenshots (optional)