Allow groups to access queries (PR #36)

The thread describing this feature can be found here.

GitHub

be sure to install prettier locally and always run your files through it. I use IDE integration it is magical.

comment a bit confusing

This will not cache … its not really a cachable route, one consideration here is preload store, may make sense to use it. or amend serializer to include groups

if it is just going to be blank everywhere is there any point including it? just rely on default at the consumer.

This bypasses security, probably not mega risky as it is admin only, but why not rely on existing group api?

Yeah, agreed.

Using existing group api now

I think a lot of this can be amended to stop all this .get and .set, cause we this.selectedItem is already assumed to be there in the return. So this.selectedItem.group_ids && this.selectedItem.group_ids.length < 1 and so on in this method make sense.

isn’t model a given? this.model.id?

a lot of set here, I don’t think we need this anymore this.showResults = false should do

I much prefer no stubs here, can this be avoided?

Overall this looks great, I also love the UX and positioning! great job

General pattern though it is a bit hard for me to review every line of the spec file, but can you confirm the new public controller actions all have an integration test to ensure users who are not allowed have no access and one’s allowed do.

@eviltrout can you have a look as well?

Thanks for the big piece of work. I’ve provided feedback.

Trivial but you could avoid the p1 variable here by returning ajax(...).then()

1 Like

Rather than mapping through then running an includes, you could do it in one go with something like: if (this.currentUser.groups.some(g => g.id === this.group.id)

1 Like

I don’t like using the same variable to represent two different actions. Rather than using a magic -1 value to represent this I would far prefer if you added a second parameter that indicates “remove all”.

1 Like

The indentation here is odd. Should be two spaces.

Another trivial suggestion but this could be one line. groups.forEach(g => groupNames[g.id] = g.name)