Hide the "respond" UI items when not logged in #824
Conversation
|
Please critique my Vue and JS testing, as it's been since whenever I last touched JS on this project that I wrote anything in Vue or tried to test JS. I'm particularly interested in cleaner ways of expressing the tests due to not remembering all the methods that are available |
There was a problem hiding this comment.
LGTM! Made some vue-testing suggestions as requested, though nothing that necessarily needs changed.
Not specific to this PR but i'm not a fan of using big chunks of json fixture data in specs. Among other issues, they often change drastically between commits and it's very difficult to know what part of the diff were germane to the code/spec changes and what is coincidental.
Thanks!
I agree with those negatives. I'd still some way to verify that the data that we expect Rails to emit is then consumed properly by by the JS. Do you have any suggestions for that? |
Co-authored-by: lasitha <exbinary@gmail.com>
h-m-m
force-pushed
the
conditionally-hide-contribution-response
branch
from
35792b2
to
2498c31
Compare
| @@ -16,6 +16,7 @@ This changelog also serves to acknowledge the incredible people who've contribut | |||
| * Allow custom text on landing page to override default text #769, #780 | |||
| * Add Rubocop #793 | |||
| * Show the "Contributions" link in the navbar for P2P users even if they aren't logged in | |||
| * Hide the respond button on contributions when nobody is logged in. (We'll add more functionality later for the peer-to-peer scneario) #819 | |||
| @@ -1,16 +1,36 @@ | |||
| import {assert} from 'chai' | |||
There was a problem hiding this comment.
why isn't assert needed from chai anymore?
There was a problem hiding this comment.
I think that's because our setup.js file brings it in, and that setup file is explicitly included in
Line 50 in 8086ba8
Why
This addresses one of the items in #819
We'll probably want more logic involved later. For now, it's okay hide all "respond" UI items when not logged in. The only time they should show up for users not logged in is when the system is in peer-to-peer mode
Pre-Merge Checklist
What
Also, I added some tests, pulling some test data from my dev environment
When not logged in, viewing tiles:
When not logged in, viewing the list:
When logged in viewing tiles:
When logged in viewing the list:
How
The two key changes are
Testing
Next Steps
Outstanding Questions, Concerns and Other Notes
Accessibility
This should not change accessibility as it makes an existing UI element entirely hidden conditionally
Security
This is pretty reasonable, security-wise. The decision to expose the information is being made server-side, so there's no way a user could spy on the info only from the client. That, and the URLs that are being exposed have authorization controls around them, so there's not much risk if they were exposed.
Meta