Allow groups to access queries (PR #36)

@eviltrout Prettier is forcing it onto multiple line

We can!

All of this is unnecessary, and has been removed.

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.

@SamSaffron I have added integration tests (before I had filters, and tested those, but never actually hitting the endpoint that use the filters), and removed the stubbing.

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

@eviltrout I addressed all your comments. Thanks for the thorough review!

There are lots of places in this PR that use .set no need really.

this.showReportsTab = response.queries.length > 0; reads much better (there are quite a few similar places in the PR)

probably worth renaming this to queryPromise and so on … p1,p2,p3 is somewhat confusing.

no need to define p1 here, return ajax(...

Our general pattern is to use a guardian eg: that should take care of current_user check and everything.

a guardian check at the entry is what we want here, no to scope it down to json only. the html should 404 as well.

@SamSaffron I agree it is cleaner, but I kept getting errors when trying to set without calling this.set. I got this error when I tried to not use set.

:cry: I wonder if there is a workaround here for components @eviltrout / @jjaffeux ? Seems like a tricky rule to remember.

AFAIK octane should mostly land in 3.14 Ember.js - Octane is coming in v3.14 and this is when we should start to have solutions to this. (3.14 will be huge :heart_eyes::heart_eyes::heart_eyes:, with modifiers in components, access to native events through ˋon` very simply…)

Can’t find it in this blog post post, but I remember reading something with @tracked which should make this super explicit.

2 Likes

@SamSaffron I pushed up another commit that uses guardian to secure the public endpoints, as well as addressing your other comments.

1 Like
      // User is a part of the group. Now check if the group has reports

This looks pretty good to me now!

2 Likes

Looking good, @eviltrout probably best if you merge this in your morning.

Merged, let’s do it!

1 Like