Object Permissions

[loneil]

Description

Allow users to set object permissions on files they have manage permissions for. The "Add User" part is on an upcoming ticket.

Don't show the actions on a file that you don't have the corresponding permission for.

Additionally a fix for
https://apps.nrs.gov.bc.ca/int/jira/browse/SHOWCASE-3012

Types of changes

New feature (non-breaking change which adds functionality)

Checklist

• I have read the CONTRIBUTING doc
• I have checked that unit tests pass locally with my changes
• I have added tests that prove my fix is effective or that my feature works
• I have added necessary documentation (if appropriate)

Further comments

There's probably some efficiencies that can be done here. This is mostly the same code as the bucket permissions screen and table, but with enough changes I couldn't quickly figure out how to generalize it. There's some differences between the 2 things.
Possibly could be refactored to a common component and/or composable, but need to be sure the behavior would stay the same.

At the very least the component SCSS should be extracted out so that it's not duplicated. Would need to decide if above path would be looked at, if not, move to global scss for this type of checkbox table.

[github-actions]

Coverage Report (Frontend)

Totals
Statements:	43.68% ( 38 / 87 )
Methods:	25% ( 6 / 24 )
Lines:	53.85% ( 28 / 52 )
Branches:	36.36% ( 4 / 11 )

[github-actions]

Coverage Report (Application)

Totals
Statements:	75% ( 51 / 68 )
Methods:	62.5% ( 5 / 8 )
Lines:	82.61% ( 38 / 46 )
Branches:	57.14% ( 8 / 14 )

[codeclimate]

Code Climate has analyzed commit 39051a3 and detected 11 issues on this pull request.

Here's the issue category breakdown:

Category	Count
Complexity	1
Duplication	10

The test coverage on the diff in this pull request is 100.0% (50% is the threshold).

This pull request will bring the total coverage in the repository to 67.0% (-15.5% change).

View more on Code Climate.

[loneil]

So if you directly navigate to a object list landing page, you will have your perms for that bucket populated.

[jujaga]

Semantically looks good. Just a few comments for consideration.

[jujaga]

Two thoughts:

1. If we're only using the values and don't need the key/value pairing, we should probably be using Object.values() instead.

for (const value of Object.values(Permissions)) {

2. Since this looks like a general async dispatcher pattern, it may be better to wrap this into a Promise.all() for synchronization purposes:

await Promise.all(
  Object.values(Permissions)
    .map(perm => objectService.deletePermission(bucketId, {
      userId,
      permCode: value,
    }));
);

[loneil]

Used the same logic as in existing bucketStore removeBucketUser.
Think there's possibly a way to take in the service method and have this permission logic extracted to a common util function (for buckets and objects). So someone could take a look at that at some point and implement your logic above for both (ideally in one palce)

[TimCsaky]

a lot of good stuff in here!

[TimCsaky]

i think in line 77 we are removing the perms and then adding them again here in line 91. not sure if that could be optimized.

[loneil]

I think the line 77 being referred to is just stashing the specific IDs from the permissions to fetch the objects? So not removing them?
(file has changed since so line 77 might be something else from original comment)

[TimCsaky]

my point was: we are looping through permResponse in line 77, the perms can be added there instead of doing another loop. only mention this because for a bucket full of objects this could impact the speed

[TimCsaky]

just ran this in the browser and it's looking good.
couple more minor things we will probably have to look at later:

• i was able to remove myself (remove user) from the list of users with permissions. But then i still see the file in objects table. i think i still had the read permission before i clicked the remove button.
• there's some kind of flickering when the objects table is refreshed, i wonder if there's a black or missing css background somewhere.

update:

• re: removing myself, it's probably an issue with inconsistent data state. not concerned right now
• re: flickering table, it's just the 'loading overlay' from primeview (p.datatable-loading-overlay)
  this feels resolved