Scope resources under organization resource #250
Conversation
| <li class="<%= "active" if current_page?(inquiries_path) %>"> | ||
| <%= link_to inquiries_path do %> | ||
| <li class="<%= "active" if current_page?(organization_inquiries_path(organization_id: @organization.id)) %>"> | ||
| <%= link_to organization_inquiries_path(organization_id: @organization.id) do %> |
There was a problem hiding this comment.
Should be possible to directly use the shorter form organization_inquiries_path(@organization).
|
👍 Testeo un poco |
|
is there any special reason why we need this scoping? |
| @@ -1,5 +1,11 @@ | |||
| class HomeController < ApplicationController | |||
|
|
|||
| # TODO: what happens when the user doesn't pertain to any organization? | |||
There was a problem hiding this comment.
I would redirect them to a page where we tell them to join some organization. Do we have something alike @sseerrggii ?
There was a problem hiding this comment.
ping @sseerrggii 😬
| raise not_found unless @organization | ||
|
|
||
| @organization | ||
| end |
There was a problem hiding this comment.
We could achive the same behaviour without needing this code if we rescued ActiveRecord::RecordNotFound in a rescue_from block. Furthermore, this would work for any model.
There was a problem hiding this comment.
Yep, sure. We need to move somewhere else and DRY it a bit firts though.
| class OffersController < PostsController | ||
| def model | ||
| Offer | ||
| end | ||
|
|
||
| def dashboard |
| def load_organization | ||
| @organization = Organization.find_by_id(params[:organization_id]) | ||
|
|
||
| raise not_found unless @organization |
| def resources | ||
| controller_name | ||
| end | ||
|
|
||
| def set_user_id(p) | ||
| if current_user.manages?(current_organization) | ||
| if current_user.manages?(@organization) |
There was a problem hiding this comment.
we could avoid lots of these ivar changes by implemanting the getter method in the application controller as:
def current_organization
@organization
endThis would reduce the number of changes considerably and increase the readability of this PR.
There was a problem hiding this comment.
Yep this is part of the abstracion part, moving the loading/authorizing of the organization somewhere else and DRY it.
| @@ -34,11 +46,11 @@ def toggle_active | |||
| private | |||
|
|
|||
| def find_member | |||
| @member ||= current_organization.members.find(params[:id]) | |||
| @member ||= @organization.members.find(params[:id]) | |||
There was a problem hiding this comment.
find_by_id or rescue meee 🎤
There was a problem hiding this comment.
Uff this repo is full of this kind of stuff :(
Yep, will do. but maybe once I finish to move the resources under the /organizations scope.
@sauloperez basically we already do some kind of scoping storing the current organization in the browser session... let's use the URL instead. EDIT: I added link to discussion in community in PR description. |
|
Tener en cuenta que los links de ofertas/demandas en los mails también tienen que cambiar Estaría bien que los links viejos, tipo https://www.timeoverflow.org/offers/19721 sigan funcionando, ya que al saber el id de post (oferta/demanda) podemos saber la organización |
enricostano
force-pushed
the
feature/scope-resources-under-org
branch
from
23ea2f0
to
86cc250
Compare
enricostano
force-pushed
the
feature/scope-resources-under-org
branch
from
912a2f1
to
a56dab6
Compare
|
Is this still in progress? cc @enricostano @Morantron |
|
Yes, I need to work on it ASAP. We're prioritizing push notifications. |
This is a first attempt at scoping all the resources related to an organization under the organization scope.
For instance
/organization/27/offer/12.More info in community thread: http://community.coopdevs.org/t/scope-todos-los-recursos-de-organization-bajo-organizations/25
This is still a work in progress, but @sseerrggii should be able to test it
TODO: