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
Merged
Changes from 16 commits
Commits
Show all changes
22 commits
Select commit Hold shift + click to select a range
9b3acca
[WIP] group ids saving on new reports
mark-vanlandingham-roostify Aug 21, 2019
2132983
Add groups to default queries, and added tab connector
mark-vanlandingham-roostify Aug 22, 2019
3d1b0b3
group_ids set to empty array for default queries
mark-vanlandingham-roostify Aug 22, 2019
cb18b5b
group reports route (in & and) action
mark-vanlandingham-roostify Aug 24, 2019
14f8617
[WIP] created group reports show route/controller
mark-vanlandingham-roostify Aug 29, 2019
06a58b5
Find correct query in show route
mark-vanlandingham-roostify Aug 29, 2019
35e2df1
Removed empty array for group_ids in query file
mark-vanlandingham-roostify Aug 30, 2019
6ab3c60
Add report show view, where users can run queries
mark-vanlandingham-roostify Aug 31, 2019
e11a079
Removed unneeded commas from queries.rb
mark-vanlandingham-roostify Aug 31, 2019
99a788e
Allow non-admin group members to access reports
mark-vanlandingham-roostify Sep 2, 2019
667d239
query-result component dynamic download url based on location
mark-vanlandingham-roostify Sep 2, 2019
29e405d
Removed accidental changes, and corrected tab size
mark-vanlandingham-roostify Sep 2, 2019
e3fd54b
Group members can add params to queries
mark-vanlandingham-roostify Sep 4, 2019
c639670
Specs for new QueryController actions
mark-vanlandingham-roostify Sep 6, 2019
acf7213
remove "Inlude query plan" from group reports
mark-vanlandingham-roostify Sep 6, 2019
8f96cdb
Run prettier
mark-vanlandingham-roostify Sep 6, 2019
0a6cc6e
return and return -> return render
markvanlan Sep 9, 2019
13a78fa
[WIP] changes from review
mark-vanlandingham-roostify Sep 9, 2019
47d6854
Remove weird [-1] group_ids logic, for a simply check for [] in query…
mark-vanlandingham-roostify Sep 9, 2019
6fb16a6
Added integration tests for group report access
mark-vanlandingham-roostify Sep 9, 2019
cc482e0
Using guardian for securing endpoints, and much improved specs
mark-vanlandingham-roostify Sep 10, 2019
3e1707d
Update assets/javascripts/discourse/components/group-reports-nav-item…
markvanlan Sep 10, 2019
File filter...
Filter file types
Jump to…
Jump to file or symbol
Failed to load files and symbols.

Always

Just for now

@@ -0,0 +1,23 @@
import { ajax } from "discourse/lib/ajax";

export default Ember.Component.extend({
group: null,
showReportsTab: false,

checkForReports() {
const p1 = ajax(`/g/${this.group.name}/reports`);
This conversation was marked as resolved by markvanlan

This comment has been minimized.

Copy link
@eviltrout

eviltrout Sep 9, 2019

Member

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

return p1.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.

});
},

init(args) {
this.set("group", args.group);
let usersGroupIds = this.currentUser.groups.map(g => g.id);
This conversation was marked as resolved by markvanlan

This comment has been minimized.

Copy link
@eviltrout

eviltrout Sep 9, 2019

Member

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)

This comment has been minimized.

Copy link
@markvanlan

markvanlan Sep 9, 2019

Author Contributor

Agreed!

if (usersGroupIds.includes(this.group.id)) {
// User is apart of the group. Now check if the group has reports
This conversation was marked as resolved by markvanlan

This comment has been minimized.

Copy link
@eviltrout

eviltrout Sep 10, 2019

Member
Suggested change
// User is apart of the group. Now check if the group has reports
// User is a part of the group. Now check if the group has reports
this.checkForReports();
}
this._super(args);
}
});
@@ -146,6 +146,12 @@ const QueryResultComponent = Ember.Component.extend({
return this.site.get("categoriesById")[id];
},

download_url() {
return this.group
? `/g/${this.group.name}/reports/`
: "/admin/plugins/explorer/queries/";
},

downloadResult(format) {
// Create a frame to submit the form in (?)
// to avoid leaving an about:blank behind
@@ -161,7 +167,7 @@ const QueryResultComponent = Ember.Component.extend({
form.setAttribute(
"action",
Discourse.getURL(
"/admin/plugins/explorer/queries/" +
this.download_url() +
this.get("query.id") +
"/run." +
format +
@@ -54,6 +54,22 @@ export default Ember.Controller.extend({
return item || NoQuery;
},

@computed("selectedItem", "editing")
selectedGroupNames(selectedItem) {
const groupIds = this.selectedItem.group_ids || [];
const groupNames = groupIds.map(id => {
return this.groupOptions.find(groupOption => groupOption.id == id).name;
});
return groupNames.join(", ");
},

@computed("groups")
groupOptions(groups) {
return groups.arrangedContent.map(g => {
return { id: g.id.toString(), name: g.name };
});
},

@computed("selectedItem", "selectedItem.dirty")
othersDirty(selectedItem) {
return !!this.model.find(q => q !== selectedItem && q.dirty);
@@ -81,6 +97,15 @@ export default Ember.Controller.extend({
this.set("loading", true);
This conversation was marked as resolved by markvanlan

This comment has been minimized.

Copy link
@SamSaffron

SamSaffron Sep 9, 2019

Member

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.

This comment has been minimized.

Copy link
@markvanlan

markvanlan Sep 9, 2019

Author Contributor

All of this is unnecessary, and has been removed.

if (this.get("selectedItem.description") === "")
this.set("selectedItem.description", "");

// group_ids must not be an empty array, as the param will not be sent the server.
This conversation was marked as resolved by markvanlan

This comment has been minimized.

Copy link
@eviltrout

eviltrout Sep 9, 2019

Member

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

This comment has been minimized.

Copy link
@markvanlan

markvanlan Sep 9, 2019

Author Contributor

Great idea. I will do that.

// By setting group_ids to [-1], the user can remove the all groups
if (
!this.get("selectedItem.group_ids") ||
this.get("selectedItem.group_ids").length < 1
)
this.set("selectedItem.group_ids", [-1]);

return this.selectedItem
.save()
.then(() => {
@@ -183,6 +208,8 @@ export default Ember.Controller.extend({
.then(result => {
const query = this.get("selectedItem");
query.setProperties(result.getProperties(Query.updatePropertyNames));
if (!query.group_ids || !Array.isArray(query.group_ids))
query.set("group_ids", []);
query.markNotDirty();
this.set("editing", false);
})
@@ -0,0 +1,10 @@
import Query from "discourse/plugins/discourse-data-explorer/discourse/models/query";
import { ajax } from "discourse/lib/ajax";
import {
default as computed,
observes
} from "ember-addons/ember-computed-decorators";

export default Ember.Controller.extend({
queries: Ember.computed.alias("model.queries")
This conversation was marked as resolved by markvanlan

This comment has been minimized.

Copy link
@eviltrout

eviltrout Sep 9, 2019

Member

The indentation here is odd. Should be two spaces.

});
@@ -0,0 +1,46 @@
import Query from "discourse/plugins/discourse-data-explorer/discourse/models/query";
import { popupAjaxError } from "discourse/lib/ajax-error";
import { ajax } from "discourse/lib/ajax";
import {
default as computed,
observes
} from "ember-addons/ember-computed-decorators";

export default Ember.Controller.extend({
showResults: false,
explain: false,
loading: false,
results: Ember.computed.alias("model.results"),
hasParams: Ember.computed.gt("model.param_info.length", 0),

actions: {
run() {
this.setProperties({ loading: true, showResults: false });
ajax(`/g/${this.get("group.name")}/reports/${this.get("model.id")}/run`, {
type: "POST",
data: {
params: JSON.stringify(this.get("model.params")),
This conversation was marked as resolved by markvanlan

This comment has been minimized.

Copy link
@SamSaffron

SamSaffron Sep 9, 2019

Member

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

explain: this.explain
}
})
.then(result => {
this.set("results", result);
if (!result.success) {
this.set("showResults", false);
return;
}

this.set("showResults", true);
This conversation was marked as resolved by markvanlan

This comment has been minimized.

Copy link
@SamSaffron

SamSaffron Sep 9, 2019

Member

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

})
.catch(err => {
this.set("showResults", false);
if (err.jqXHR && err.jqXHR.status === 422 && err.jqXHR.responseJSON) {
this.set("results", err.jqXHR.responseJSON);
} else {
popupAjaxError(err);
}
})
.finally(() => this.set("loading", false));
}
}
});
@@ -0,0 +1,9 @@
export default {
resource: "group",

map() {
this.route("reports", function() {
this.route("show", { path: "/:query_id" });
});
}
};
@@ -23,7 +23,7 @@ const Query = RestModel.extend({
this.resetParams();
},

@observes("name", "description", "sql")
@observes("name", "description", "sql", "group_ids")
markDirty() {
this.set("dirty", true);
},
@@ -85,6 +85,7 @@ Query.reopenClass({
"sql",
"created_by",
"created_at",
"group_ids",
"last_run_at"
]
});
@@ -4,19 +4,36 @@ export default Discourse.Route.extend({
controllerName: "admin-plugins-explorer",

model() {
const p1 = this.store.findAll("query");
This conversation was marked as resolved by markvanlan

This comment has been minimized.

Copy link
@SamSaffron

SamSaffron Sep 10, 2019

Member

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

const p1 = this.store.findAll("group");
const p2 = ajax("/admin/plugins/explorer/schema.json", { cache: true });
return p1
.then(model => {
model.forEach(query => query.markNotDirty());
const p3 = this.store.findAll("query");

return p1
.then(groups => {
let group_names = {};
This conversation was marked as resolved by markvanlan

This comment has been minimized.

Copy link
@eviltrout

eviltrout Sep 9, 2019

Member

In Javascript code we use camelCase not snake_case for variables.

groups.forEach(g => {
This conversation was marked as resolved by markvanlan

This comment has been minimized.

Copy link
@eviltrout

eviltrout Sep 9, 2019

Member

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

This comment has been minimized.

Copy link
@markvanlan

markvanlan Sep 9, 2019

Author Contributor

@eviltrout Prettier is forcing it onto multiple line

group_names[g.id] = g.name;
});
return p2.then(schema => {
return { model, schema };
return p3.then(model => {
model.forEach(query => {
query.markNotDirty();
query.set(
"group_names",
query.group_ids
.map(id => group_names[id])
.filter(n => n)
.join(", ")
);
});
return { model, schema, groups };
});
});
})
.catch(() => {
p2.catch(() => {});
return { model: null, schema: null, disallow: true };
p3.catch(() => {});
return { model: null, schema: null, disallow: true, groups: null };
});
},

@@ -0,0 +1,39 @@
import { ajax } from "discourse/lib/ajax";

export default Discourse.Route.extend({
controllerName: "group-reports-index",

model() {
const group = this.modelFor("group");
const p1 = ajax(`/g/${group.name}/reports`);
return p1
This conversation was marked as resolved by markvanlan

This comment has been minimized.

Copy link
@SamSaffron

SamSaffron Sep 10, 2019

Member

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

.then(queries => {
return {
model: queries,
group: group
};
})
.catch(() => {
this.transitionTo("group.members", group);
});
},
afterModel(model) {
if (
!model.group.get("is_group_user") &&
!(this.currentUser && this.currentUser.admin)
) {
this.transitionTo("group.members", group);
}
},

setupController(controller, model) {
controller.setProperties(model);
},

actions: {
refreshModel() {
this.refresh();
return false;
}
}
});
@@ -0,0 +1,31 @@
import { ajax } from "discourse/lib/ajax";

export default Discourse.Route.extend({
controllerName: "group-reports-show",

model(params) {
const group = this.modelFor("group");
const p1 = ajax(`/g/${group.name}/reports/${params.query_id}`);
return p1
.then(response => {
return {
model: Object.assign({ params: {} }, response.query),
group: group
};
})
.catch(err => {
this.transitionTo("group.members", group);
});
},

setupController(controller, model) {
controller.setProperties(model);
},

actions: {
refreshModel() {
this.refresh();
return false;
}
}
});
@@ -38,6 +38,7 @@
<div class="desc">
{{textarea value=selectedItem.description placeholder=(i18n "explorer.description_placeholder")}}
</div>

{{else}}
<div class="name">
{{d-button action=(action "goHome") icon="chevron-left" class="previous"}}
@@ -52,6 +53,20 @@
</div>
{{/if}}

<div class="groups">
<span class="label">Allow groups to acess this query</span>
<span>
{{ multi-select values=selectedItem.group_ids content=groupOptions }}
This conversation was marked as resolved by markvanlan

This comment has been minimized.

Copy link
@eviltrout

eviltrout Sep 9, 2019

Member
Suggested change
{{ multi-select values=selectedItem.group_ids content=groupOptions }}
{{multi-select values=selectedItem.group_ids content=groupOptions}}
</span>
{{#if runDisabled }}
This conversation was marked as resolved by markvanlan

This comment has been minimized.

Copy link
@eviltrout

eviltrout Sep 9, 2019

Member
Suggested change
{{#if runDisabled }}
{{#if runDisabled}}
{{#unless editing}}
<span class='setting-controls'>
{{d-button class="ok" action=(action "save") icon="check"}}
{{d-button class="cancel" action=(action "discard") icon="times"}}
</span>
{{/unless}}
{{/if}}
</div>
{{! the SQL editor will show the first time you }}
{{#if everEditing}}
<div class="query-editor {{if hideSchema "no-schema"}}">
@@ -158,6 +173,11 @@
{{directory-toggle field="username" labelKey="explorer.query_user" order=order asc=asc}}
</div>
</th>
<th class='col heading group-names'>
<div class='group-names-header'>
{{i18n "explorer.query_groups"}}
</div>
</th>
<th class="col heading created-at">
<div class="heading-toggle" {{action "sortByProperty" "last_run_at"}}>
{{directory-toggle field="last_run_at" labelKey="explorer.query_time" order=order asc=asc}}
@@ -181,6 +201,11 @@
</a>
{{/if}}
</td>
<td class="query-group-names">
{{#if query.group_names}}
<medium>{{query.group_names}}</medium>
{{/if}}
</td>
<td class="query-created-at">
{{#if query.last_run_at}}
<medium>
@@ -0,0 +1,9 @@
{{#if showReportsTab}}
<ul class ='nav-pills'>
This conversation was marked as resolved by markvanlan

This comment has been minimized.

Copy link
@eviltrout

eviltrout Sep 9, 2019

Member

Again quite weird indentation here.

<li>
{{#link-to 'group.reports'}}
{{i18n 'group.reports'}}
{{/link-to}}
</li>
</ul>
{{/if}}
@@ -0,0 +1 @@
{{ group-reports-nav-item group = this.group }}
This conversation was marked as resolved by markvanlan

This comment has been minimized.

Copy link
@eviltrout

eviltrout Sep 9, 2019

Member
Suggested change
{{ group-reports-nav-item group = this.group }}
{{group-reports-nav-item group = this.group}}

This comment has been minimized.

Copy link
@eviltrout

eviltrout Sep 9, 2019

Member

Does this work properly? You shouldn't need to use this in a hbs template.

This comment has been minimized.

Copy link
@markvanlan

markvanlan Sep 9, 2019

Author Contributor

It does work, but I can certainly remove this.

@@ -1,6 +1,6 @@
<div class="result-info">
{{d-button action=(action "downloadResultJson") icon="download" label="explorer.download_json"}}
{{d-button action=(action "downloadResultCsv") icon="download" label="explorer.download_csv"}}
{{d-button action=(action "downloadResultJson") icon="download" label="explorer.download_json" group=group}}
{{d-button action=(action "downloadResultCsv") icon="download" label="explorer.download_csv" group=group}}
</div>

<div class="result-about">
@@ -0,0 +1,25 @@
<section class='user-content'>
<table class='group-reports'>
<thead>
<th>
{{i18n "explorer.report_name"}}
</th>
<th>
{{i18n "explorer.query_description"}}
</th>
<th>
{{i18n "explorer.query_time"}}
</th>
</thead>
<tr></tr>
<tbody>
{{#each queries as |query|}}
<tr>
<td><a href="reports/{{query.id}}">{{query.name}}</a></td>
This conversation was marked as resolved by markvanlan

This comment has been minimized.

Copy link
@eviltrout

eviltrout Sep 9, 2019

Member

Can you use {{link-to}} here? If not this link will need to be aware of subfolder installations of Discourse. (usually a call to Discourse.getURL

This comment has been minimized.

Copy link
@markvanlan

markvanlan Sep 9, 2019

Author Contributor

We can!

<td>{{query.description}}</td>
<td>{{bound-date query.last_run_at}}</td>
</tr>
{{/each}}
</tbody>
</table>
</section>
ProTip! Use n and p to navigate between commits in a pull request.
You can’t perform that action at this time.