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

Starting to enable supervolunteer role for #455 #476

Merged
merged 16 commits into from
Mar 15, 2018
Merged

Conversation

schuyler1d
Copy link
Collaborator

@schuyler1d schuyler1d commented Jan 25, 2018

wip for #455

Things to do (for hiding things, it's dependent on the supervolunteer, but an admin should still see it):

  • hide archive buttons
  • hide disallowed campaign sections
  • hide campaign add button
  • hide export data/archive buttons in campaign-started view
  • see if top nav menu needs to change
  • add SuperVolunteer as a role in the People admin menu (ideally by just using the list in src/lib/permissions.js)
  • filter admin menu items
  • allow access on backend

@schuyler1d schuyler1d added the S-work in progress (WIP) Status: PR label for work that is not yet ready to be reviewed label Jan 25, 2018
@schuyler1d schuyler1d closed this Feb 9, 2018
@schuyler1d schuyler1d changed the base branch from master to main February 9, 2018 15:29
@schuyler1d
Copy link
Collaborator Author

reopening

const { roles } = this.props.data.currentUser

// HACK: Setting params.adminPerms helps us hide non-supervolunteer functionality
params.adminPerms = hasRole('ADMIN', roles || [])
Copy link
Collaborator Author

@schuyler1d schuyler1d Mar 12, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is an icky icky hack, but drastically simplifies the implementation here. I'm not sure if there is a way to do this better, but if not, I think it would be worth the hackiness to keep the rest of the code cleaner.

The reason this helps, is AdminDashboard is the wrapping route component for the other Admin pages, including side Nav, all of which need the property of whether the user is a full admin or not for the organization. The other pages, then just test against this.props.params.adminPerms -- much easier than, e.g. all calling the same query in mapQueriesToProps (event if it's cached, that's a lot of code for a single property).

@schuyler1d schuyler1d changed the title wip: starting to enable supervolunteer role for #455 Starting to enable supervolunteer role for #455 Mar 12, 2018
@schuyler1d
Copy link
Collaborator Author

I think this completes the feature, though we should discuss the hackiness issue.

@schuyler1d schuyler1d added S-qa staging round Status (ADMINS ONLY): PR label for being tested on stage (next step: testing in producgtion) and removed help wanted labels Mar 12, 2018
@shakalee14
Copy link
Contributor

shakalee14 commented Mar 12, 2018

Hey @schuyler1d - reviewing this PR. Thanks for finishing it off.

Bugs found:

  1. I visit http://localhost:3000/admin/1/people - when I try to click on the drop down menu for roles for a user and select Supervolunteer it doesn't allow me to select it. I can still select other roles for the user.

  2. I changed the role for the user in the backend to SUPERVOLUNTEER. Now when I visit http://localhost:3000/admin/1/campaigns I can see the Campaigns, Incoming Messages, Settings and Switch to Texter

    • I think incoming messages should be hidden (not a bug but definitely an opinion - incoming
      messages displays phone numbers
    • I can't click on any campaigns (started or not started) as a supervolunteer
  3. When I switch back to being my owner account (this person has permissions to be a texter, admin and owner), I can't create a new campaign. I can load in Basics info but uploading contacts, assigning texters and adding interactions steps is not saving.

In the backend, I get this error:

 Error: Undefined binding(s) detected when compiling FIRST query: select count(*)
11:22:55 AM server.1              |  >   as "count" from "user_organization" whe
11:22:55 AM server.1              |  >  re "user_id" = ? and "organization_id" =
11:22:55 AM server.1              |  >   ? and "role" in (?, ?) limit ?
11:22:55 AM server.1              |      at QueryCompiler_PG.toSQL (/Users/sh
11:22:55 AM server.1              |  >  aka/moveon/Spoke/node_modules/knex/lib/q
11:22:55 AM server.1              |  >  uery/compiler.js:131:13)
11:22:55 AM server.1              |      at Builder.toSQL (/Users/shaka/moveo
11:22:55 AM server.1              |  >  n/Spoke/node_modules/knex/lib/query/buil
11:22:55 AM server.1              |  >  der.js:115:44)
11:22:55 AM server.1              |      at /Users/shaka/moveon/Spoke/node_mo
11:22:55 AM server.1              |  >  dules/knex/lib/runner.js:56:32
11:22:55 AM server.1              |      at tryOnImmediate (timers.js:543:15)
11:22:55 AM server.1              |      at processImmediate [as _immediateCa
11:22:55 AM server.1              |  >  llback] (timers.js:523:5)
11:22:55 AM server.1              |  From previous event:
11:22:55 AM server.1              |      at Runner.run (/Users/shaka/moveon/S
11:22:55 AM server.1              |  >  poke/node_modules/knex/lib/runner.js:51:
11:22:55 AM server.1              |  >  31)
11:22:55 AM server.1              |      at Builder.Target.then (/Users/shaka
11:22:55 AM server.1              |  >  /moveon/Spoke/node_modules/knex/lib/inte
11:22:55 AM server.1              |  >  rface.js:35:43)
11:22:55 AM server.1              |      at /Users/shaka/moveon/Spoke/node_mo
11:22:55 AM server.1              |  >  dules/core-js/modules/es6.promise.js:141
11:22:55 AM server.1              |  >  :16
11:22:55 AM server.1              |      at flush (/Users/shaka/moveon/Spoke/
11:22:55 AM server.1              |  >  node_modules/core-js/modules/_microtask.
11:22:55 AM server.1              |  >  js:18:9)
11:22:55 AM server.1              |      at _combinedTickCallback (internal/p
11:22:55 AM server.1              |  >  rocess/next_tick.js:67:7)
11:22:55 AM server.1              |      at process._tickDomainCallback (inte
11:22:55 AM server.1              |  >  rnal/process/next_tick.js:122:9)

@schuyler1d
Copy link
Collaborator Author

thanks for review/testing! I'll take a look (this evening or tomorrow evening when I have my volunteer hat on :-)

As to your incoming messages in 2.a -- the original request was that they see incoming messages: https://github.com/MoveOnOrg/Spoke/issues/455 -- even though I agree that it's sensitive info, I assume the idea is that it will relate to their supervolunteer role where they may review/monitor that queue the same way our supportcorps does for emails coming in.

@schuyler1d schuyler1d removed the S-work in progress (WIP) Status: PR label for work that is not yet ready to be reviewed label Mar 13, 2018
@schuyler1d
Copy link
Collaborator Author

schuyler1d commented Mar 13, 2018

Ok, @shakalee14 check out the current updates.

  1. fixed with 9e89174 which also fixes On admin people screen, cannot select text #447
  2. Should be fixed with: MoveOnOrg@797c495 however, my guess is we'll need to QA the full supervolunteer set of features and make sure we have the permissions correct everywhere.
  3. I think this is fixed by: MoveOnOrg@34962c5 -- the scary part here, is this suggests that we are not doing access control on these methods at the moment (i.e. they always succeed). We should probably document when/where/why to use campaign.organization_id vs. campaign.organizationId

@shakalee14
Copy link
Contributor

shakalee14 commented Mar 13, 2018

@schuyler1d

  1. Fixed
  2. Ok - that's fine
  3. The paneling looks great - it looks clear on the super volunteer side on what I cannot do. But when I switch back to the admin account, I still cannot upload contacts. I upload the contact csv. It correctly tells me the number and info about fields. I click save and then it takes me to the next tab (for assigning texters) and the # of contacts is 0 and the contacts tab is still yellow.

@schuyler1d
Copy link
Collaborator Author

hm. for 3. do you get an error server-side, or do you see anything odd about the graphql request? It's working for me with a test file. maybe we can pair at some point and see what's going on.

@shakalee14
Copy link
Contributor

shakalee14 commented Mar 13, 2018

error when i attempt to upload:

5:50:03 PM server.1               |  Error: Undefined binding(s) detected when compiling FIRST query: select count(*) as "count" from "user_organization" where "user_id" = ? and "organization
5:50:03 PM server.1               |  >  _id" = ? and "role" in (?, ?) limit ?
5:50:03 PM server.1               |      at QueryCompiler_PG.toSQL (/Users/shaka/moveon/Spoke/node_modules/knex/lib/query/compiler.js:131:13)
5:50:03 PM server.1               |      at Builder.toSQL (/Users/shaka/moveon/Spoke/node_modules/knex/lib/query/builder.js:115:44)
5:50:03 PM server.1               |      at /Users/shaka/moveon/Spoke/node_modules/knex/lib/runner.js:56:32
5:50:03 PM server.1               |      at runCallback (timers.js:672:20)
5:50:03 PM server.1               |      at tryOnImmediate (timers.js:645:5)
5:50:03 PM server.1               |      at processImmediate [as _immediateCallback] (timers.js:617:5)
5:50:03 PM server.1               |  From previous event:
5:50:03 PM server.1               |      at Runner.run (/Users/shaka/moveon/Spoke/node_modules/knex/lib/runner.js:51:31)
5:50:03 PM server.1               |      at Builder.Target.then (/Users/shaka/moveon/Spoke/node_modules/knex/lib/interface.js:35:43)

@schuyler1d - can you tell me the level of permissions you have for the admin user (where it works)? for example, the acct i'm using has the roles as 'OWNER', 'ADMIN', and 'TEXTER'.

@schuyler1d schuyler1d removed the S-qa staging round Status (ADMINS ONLY): PR label for being tested on stage (next step: testing in producgtion) label Mar 14, 2018
@shakalee14
Copy link
Contributor

ok - i no longer see the error anymore - and it's working as expected!

@shakalee14 shakalee14 mentioned this pull request Mar 14, 2018
@shakalee14 shakalee14 merged commit 0f0cde2 into main Mar 15, 2018
@shakalee14 shakalee14 deleted the super-volunteer branch May 14, 2018 13:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants