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

Allow groups to access queries #36

Merged
merged 22 commits into from Sep 11, 2019

Conversation

@markvanlan
Copy link
Contributor

commented Aug 29, 2019

The thread describing this feature can be found here.

lib/queries.rb Outdated Show resolved Hide resolved
plugin.rb Outdated Show resolved Hide resolved

@markvanlan markvanlan marked this pull request as ready for review Sep 2, 2019

@markvanlan markvanlan requested a review from SamSaffron Sep 6, 2019


before do
controller
.stubs(:params)

This comment has been minimized.

Copy link
@SamSaffron

SamSaffron Sep 9, 2019

Member

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

This comment has been minimized.

Copy link
@markvanlan

markvanlan Sep 9, 2019

Author Contributor

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.

@SamSaffron

This comment has been minimized.

Copy link
Member

commented Sep 9, 2019

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

This comment has been minimized.

Copy link
Member

commented Sep 9, 2019

@eviltrout can you have a look as well?

@eviltrout
Copy link
Member

left a comment

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

markvanlan and others added 4 commits Sep 9, 2019
return and return -> return render
Co-Authored-By: Robin Ward <robin.ward@gmail.com>
@markvanlan

This comment has been minimized.

Copy link
Contributor Author

commented Sep 9, 2019

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

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


checkForReports() {
return ajax(`/g/${this.group.name}/reports`).then(response => {
return this.set("showReportsTab", response.queries.length > 0);

This comment has been minimized.

Copy link
@SamSaffron

SamSaffron Sep 10, 2019

Member

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)

This comment has been minimized.

Copy link
@markvanlan

markvanlan Sep 10, 2019

Author Contributor

@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.

image
image

This comment has been minimized.

Copy link
@SamSaffron

SamSaffron Sep 10, 2019

Member

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

This comment has been minimized.

Copy link
@discoursereviewbot

discoursereviewbot Sep 10, 2019

Joffrey JAFFEUX posted:

AFAIK octane should mostly land in 3.14 https://blog.emberjs.com/2019/08/15/octane-release-plan.html and this is when we should start to have solutions to this.

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

plugin.rb Outdated Show resolved Hide resolved
plugin.rb Outdated Show resolved Hide resolved
@discoursereviewbot

This comment has been minimized.

Copy link

commented Sep 10, 2019

Mark VanLandingham posted:

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

@eviltrout
Copy link
Member

left a comment

This looks pretty good to me now!

Update assets/javascripts/discourse/components/group-reports-nav-item…
….js.es6

Co-Authored-By: Robin Ward <robin.ward@gmail.com>
@SamSaffron

This comment has been minimized.

Copy link
Member

commented Sep 11, 2019

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

@eviltrout eviltrout merged commit 30fe928 into discourse:master Sep 11, 2019

@eviltrout

This comment has been minimized.

Copy link
Member

commented Sep 11, 2019

Merged, let's do it!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
5 participants
You can’t perform that action at this time.