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

BUG: CSV Export history should ONLY show exports from the logged in user #2601

Closed
2 of 5 tasks
rowasc opened this issue Mar 14, 2018 · 19 comments · Fixed by #3161
Closed
2 of 5 tasks

BUG: CSV Export history should ONLY show exports from the logged in user #2601

rowasc opened this issue Mar 14, 2018 · 19 comments · Fixed by #3161

Comments

@rowasc
Copy link
Contributor

rowasc commented Mar 14, 2018

Expected behaviour

  • When I see my exports history, I only see the ones that belong to me

Actual behaviour

  • When I see my exports history as an admin, I see all the exports

Steps to reproduce the behavior/error

Where
@rowasc rowasc added this to the Cycle 1 milestone Mar 14, 2018
@rowasc rowasc self-assigned this Mar 14, 2018
@kinstella kinstella modified the milestones: Cycle 1, Cycle 2: Untangle SMS Mar 20, 2018
@rowasc
Copy link
Contributor Author

rowasc commented Mar 27, 2018

@Angamanga I couldn't reproduce this.

@caharding
Copy link
Contributor

@Angamanga can you see if you can recreate this?

@Angamanga
Copy link
Contributor

Angamanga commented Apr 20, 2018

@rowasc @caharding Unfortunately, yes I can reproduce it on .io. I created a new user and went to the data-export in settings and started an export. In the export-history-tab I see all export-jobs that is finished from all users.

I also see all the export-jobs in the response from the api so its not a caching-issue.

@willdoran
Copy link
Contributor

willdoran commented May 8, 2018

Spec:

  • Export Job History should show only export jobs from the currently logged in user, when user has bulk data permissions but is not an admin
  • Export Job Repository should filter export jobs by current user when user has bulk data permissions but is not an admin
  • Admin users should see all jobs and see who each job is created by

@Angamanga
Copy link
Contributor

@willdoran Looks good, moving to ready for dev!

@jrtricafort
Copy link
Contributor

Needs a new spec. @willdoran

@jrtricafort jrtricafort modified the milestones: Cycle 4: Comrades and HDX [Anna], Cycle 6: Finish COMRADES May 29, 2018
@rowasc
Copy link
Contributor Author

rowasc commented Jun 21, 2018

@jrtricafort do you remember why this was flagged as "needs a new spec" ?

@jrtricafort
Copy link
Contributor

@rowasc That comment was to capturing something from a conversation I had with @willdoran , but I don't recall the specifics. Will do you remember? If not, I think we can probably ignore that for now, since we have prioritized this as P2 so not as high a priority.

@rowasc
Copy link
Contributor Author

rowasc commented Jun 21, 2018

Thanks @jrtricafort. I'd prefer to move this to cycle 7 if it's really a P2 for us , since cycle 6 is pretty full at the moment and we're even behind on specs :/

@jrtricafort
Copy link
Contributor

@rowasc Done

@crcommons
Copy link
Contributor

@willdoran Do you recall your conversation with Juan about why this needed new spec?

@willdoran
Copy link
Contributor

@crcommons I think though I failed to write this down it's because admin users should not see all jobs but only their own. But may be double check that with @jrtricafort

@jrtricafort
Copy link
Contributor

@willdoran @crcommons That's correct. Admin users should only see their jobs.

There's some ambiguity here, because we do tend to give admin users lots of superpowers over lots of their own deployments. But seeing all exports with the username (as was originally spec'd for this issue back in May) is more like an audit of other users' behavior, and at no other point in the UX do we support that. If there is ever a use-case where someone actually needs to see who is exporting what, that is more of a privacy workflow and that should be treated as a separate special thing (which doesn't currently exist outside of hitting up the DB directly on the backend).

So long story short, yep, even admins should just see their own exports in export history.

@crcommons
Copy link
Contributor

@jrtricafort I've implemented this fix in develop, however, with Kohana (which is in production) we have it so that users who have all manage settings can't even access exports at all, let alone their own exports. Is this something we need to change now in production or can we wait until lumen is released?

Screenshot shows a logged in user with manage settings, posts, and users. Clearly this user cannot do those things.

Screen Shot 2018-07-17 at 8.55.17 AM.png

@rowasc
Copy link
Contributor Author

rowasc commented Jul 18, 2018

@jrtricafort @crcommons I added an issue about Manage Settings not allowing some of the permissions it states in the docs that we allow. #3169
Manage settings is not supposed to allow a CSV download as far as I know but I realized that we also are not giving users with Manage Settings the permission to see / edit categories .

@crcommons
Copy link
Contributor

UPDATE:

  • Changes in master have been merged in client and API and waiting to deploy;
  • changes in develop have been merged in API and waiting to deploy;
  • changes in develop in client are waiting for review

@crcommons
Copy link
Contributor

Update: changes in client have been merged into develop.

@rowasc rowasc assigned Eve-wanderi and unassigned crcommons Jul 24, 2018
@Eve-wanderi
Copy link

@rowasc This has passed for QA.

image

image

@rowasc rowasc modified the milestones: Cycle 7, Cycle 8 Jul 31, 2018
@Eve-wanderi
Copy link

Confirmed in http://targetedsurveytesting.v3-qa.ush.zone/ works as expected.

@rowasc rowasc closed this as completed Aug 9, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

8 participants