View contributions publicly when peer-to-peer is on #810
Conversation
|
|
||
| def contribution_blueprint_options | ||
| options = { respond_path: ->(id) { respond_contribution_path(id)} } | ||
| options[:view_path] = ->(id) { contribution_path(id) } if SystemSetting.current_settings&.peer_to_peer? |
There was a problem hiding this comment.
Line 62 above references @system_setting. Can we do that here as well?
| before do | ||
| sign_in user unless scenario =~ /logged out/ | ||
| allow_any_instance_of(SystemSetting).to receive(:peer_to_peer?).and_return(true) if scenario =~ /p2p is enabled/ | ||
| end |
There was a problem hiding this comment.
In general, i don't love repeating buckets of examples across multiple contexts.
There are of course circumstances when that's appropriate, eg for cross-cutting concerns. But in many cases, it can quickly become difficult to know which examples really care about being tested in all contexts and which just happened to be lumped in out of convenience. This then can hinder future refactoring.
In this case, iirc the key thing to test between the three contexts is whether /contributions is accessible. I'd probably make that clear in a separate describe 'access' or similar block where that's the only thing being asserted. The examples below deal with details of the page which i don't think need to be re-run across contexts that are about authorization.
All that said, this is just my gut feeling -- i'm certainly fine with leaving it as is.
There was a problem hiding this comment.
I've updated the tests as you've suggested
Co-authored-by: exbinary <exbinary@gmail.com>
- Previous was running authenticate_user! on all actions except respond and thank_you
h-m-m
force-pushed
the
p2p-contribution-changes
branch
from
c36db18
to
153cc13
Compare
h-m-m
force-pushed
the
p2p-contribution-changes
branch
from
153cc13
to
001326a
Compare
|
Discussed with @maebeale and @svileshina, and I think we're okay with merging this in now, and then putting the "TODO: changed the ContributionsController#respond action because the view for that page currently has a lot of stuff in it that we don't want publicly-facing" part in a separate PR As long as we don't cut a release before fixing that part of things, we're good |
|
WOO HOOOOOO! |
Why
This is one of the required steps of the peer-to-peer matchmaking workflow. This makes the list of available contributions generally public if the peer-to-peer setting is on.
Next Steps
The next step after this will be to change what the "Respond" button does for people who aren't logged in. I'd prefer to keep that next step in a separate PR, but can add it on to here if people would rather keep them together.
Also, the permission logic here should be refactored out to a Pundit object once we're bringing those in. @maebeale talked about adding features later that allow users to mark their contributions as "for admin eyes only" or something similar. Implementation of those features would be a good time to refactor here.
Pre-Merge Checklist
What
ContributionsController#respondaction because the view for that page currently has a lot of stuff in it that we don't want publicly-facingAccessibility
I expect this change will have no net effect on accessibility excepting any of the security concerns below.
Security
There is a security risk here of inadvertently allowing information to be public.
I feel like it's acceptable to continue with this work, understanding that the above security concerns are things we can address once we confirm mutual aid groups are happy with the basic functionality