Skip to content
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

Only Mark Museum Mode if User Has Role #306

Merged
merged 3 commits into from Dec 3, 2019
Merged

Only Mark Museum Mode if User Has Role #306

merged 3 commits into from Dec 3, 2019

Conversation

@wgranger
Copy link
Collaborator

wgranger commented Nov 27, 2019

Fixes #303

Invision Mock-ups:

Describe your changes.

  • Optimize the way we set museum mode. Instead of a call for all project_roles on every project in the mobile app, we make a single call for all projects where the current user has the museum role, but only if logged in.
  • For those projects with the museum role, the in_museum_mode flag is triggered if they are in the app.

When reviewing, you can test this out with the Project With All Mobile Workflow Types, which I've promoted to Beta Review and should be available to both logged in and anonymous users.

Review Checklist

  • Does it work in Android and iOS?
  • Can you rm -rf node_modules/ && npm install and app works as expected?
  • Are tests passing?
@chelseatroy chelseatroy self-requested a review Nov 29, 2019
})
export const tagMuseumRoleForProjects = projects => {
return apiClient.type('projects')
.get({ current_user_roles: 'museum' })

This comment has been minimized.

Copy link
@chelseatroy

chelseatroy Nov 29, 2019

Member

This looks great! I wish I had known that current_user_roles was an option when I was writing this the first time. Is there some documentation or another way that I can figure out things like this in the future?

This comment has been minimized.

Copy link
@wgranger

wgranger Dec 2, 2019

Author Collaborator

It should probably be in the Apiary, but doesn't appear to be there. We're also using current_user_roles at the top of this script, but I'm not sure where/if documentation on this lives anywhere.

This comment has been minimized.

Copy link
@srallen

srallen Dec 2, 2019

Member

Unfortunately it's an undocumented Panoptes feature. It works on any controlled resource, so projects, organizations, and collections for an authenticated requests. The filter should include an array of the user roles you want to filter against. Here's its code if that is at all helpful: https://github.com/zooniverse/Panoptes/blob/4cfe0e6e8bf7b3edd98490416caedb116207753a/app/controllers/concerns/filter_by_current_user_roles.rb

And an example of its use on PFE:
https://github.com/zooniverse/Panoptes-Front-End/blob/21cf42485929a62938112d5e2d4bca1a6702b00e/app/pages/home-for-user/recent-collections.jsx#L48

This comment has been minimized.

Copy link
@chelseatroy

This comment has been minimized.

Copy link
@camallen

camallen Dec 16, 2019

Member

Unfortunately it's an undocumented Panoptes feature.

This is unfortunate, looks like we failed to add the docs after this came in, zooniverse/Panoptes#864

FWIW - apiary got bought by oracle a while back, i'm not sure we've still got access to update it either :( - though feel free to try and update https://github.com/zooniverse/Panoptes/blob/7d8a941fd7bc190e8dc4646da20b11d8b0a251a1/apiary.apib#L1122 to add the filter examples in. If that merges in theory the docs will update on Apiary.

FYI - The plan is to shift to our own documentation scheme in zooniverse/Panoptes#3050 but we haven't had any traction on porting those docs.

It works on any controlled resource, so projects, organizations, and collections for an authenticated requests.

And to clarify - it only works on projects, organizations, and collections.

@chelseatroy

This comment has been minimized.

Copy link
Member

chelseatroy commented Nov 29, 2019

Love the tests! Made one small change to the tests. Also asked a question above, but will merge next week.

@chelseatroy chelseatroy merged commit 44c6e65 into master Dec 3, 2019
3 checks passed
3 checks passed
Hound No violations found. Woof!
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.