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

FEATURE: Ship default queries with the Data Explorer #26

Merged
merged 10 commits into from Oct 10, 2018

Conversation

@rishabhnambiar
Copy link
Member

rishabhnambiar commented Sep 28, 2018

How it works:
-Queries are added to the Data Explorer from json
-These default queries are saved in the db IF they are run
-If changes are made to the json file, the new values are updated whenever we run a query.

  • Reads default queries from json and adds to serializer
  • Hides edit/delete buttons for default queries
  • Gives a persistent id to each default query
  • Updates last run at for default queries
  • Choose which default queries to ship and make a post on meta for selection
@@ -33,6 +34,7 @@ export default Ember.Controller.extend({
const id = parseInt(this.get('selectedQueryId'));
const item = this.get('content').find(q => q.get('id') === id);
!isNaN(id) ? this.set('showRecentQueries', false) : this.set('showRecentQueries', true);
if (id<0) this.set('editDisabled', true);

This comment has been minimized.

Copy link
@ZogStriP

ZogStriP Sep 29, 2018

Member

Did you run the linter on this file? Seems like this wouldn't pass.

This comment has been minimized.

Copy link
@rishabhnambiar

rishabhnambiar Oct 1, 2018

Author Member

Prettier wants to obliterate this file 😅 Can I fix all the linting/prettier issues in a separate PR?

(I did fix that line though^)

@rishabhnambiar rishabhnambiar force-pushed the rishabhnambiar:ship_default_queries branch 2 times, most recently from 811c4eb to df861d3 Sep 29, 2018
@rishabhnambiar rishabhnambiar force-pushed the rishabhnambiar:ship_default_queries branch from df861d3 to 82446b9 Sep 29, 2018
@rishabhnambiar rishabhnambiar force-pushed the rishabhnambiar:ship_default_queries branch from e76962c to 477eff6 Oct 1, 2018
@rishabhnambiar rishabhnambiar requested a review from techAPJ Oct 1, 2018
@techAPJ

This comment has been minimized.

Copy link
Member

techAPJ commented Oct 3, 2018

@rishabhnambiar PR looks good to me, but..

These default queries are saved in the db IF they are run

I am not sure if we should save default query in database (on run) as any change we make to that query will not be updated on Discourse instances that had run that query.

Let's wait for @SamSaffron's opinion here.

@rishabhnambiar

This comment has been minimized.

Copy link
Member Author

rishabhnambiar commented Oct 3, 2018

NOTE: I'm saving them because I need to store last_run_at somewhere.
One possibility is that we can read from json and update the query on every run in save_default_query.

@techAPJ

This comment has been minimized.

Copy link
Member

techAPJ commented Oct 5, 2018

One possibility is that we can read from json and update the query on every run in save_default_query.

Sounds good to me. Let's do this.

@rishabhnambiar

This comment has been minimized.

Copy link
Member Author

rishabhnambiar commented Oct 5, 2018

Done!
The changes now are now read and saved on every run. 👍

@techAPJ
techAPJ approved these changes Oct 5, 2018
Copy link
Member

techAPJ left a comment

Looks good to me. 👍

@SamSaffron

This comment has been minimized.

Copy link
Member

SamSaffron commented Oct 5, 2018

This is minor, but totally worth changing. Instead of using JSON and parsing a JSON file for the queries use Ruby, it is far easier on the eye:

query = Query.new(
  id: -1,
  name: "top 50 quality users",
  description: "some awesome description"
)

query.sql = <<~SQL
    SELECT something
    FROM table
SQL

@queries << query

plenty of ways to play with this, but the newline story in JSON is really not good at all ... makes it hard to review and add queries

@techAPJ

This comment has been minimized.

Copy link
Member

techAPJ commented Oct 5, 2018

@rishabhnambiar while you are at it make sure to squash all commits into a single commit for better git commit history.

lib/queries.rb Show resolved Hide resolved
plugin.rb Show resolved Hide resolved
@rishabhnambiar rishabhnambiar force-pushed the rishabhnambiar:ship_default_queries branch 4 times, most recently from c15286e to eedb827 Oct 9, 2018
lib/queries.rb Outdated Show resolved Hide resolved
@rishabhnambiar rishabhnambiar force-pushed the rishabhnambiar:ship_default_queries branch from eedb827 to e5905fc Oct 10, 2018
@rishabhnambiar rishabhnambiar force-pushed the rishabhnambiar:ship_default_queries branch from e5905fc to 7839bfd Oct 10, 2018
@rishabhnambiar rishabhnambiar merged commit b352e74 into discourse:master Oct 10, 2018
@rishabhnambiar rishabhnambiar deleted the rishabhnambiar:ship_default_queries branch Oct 10, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
4 participants
You can’t perform that action at this time.