FEATURE: introduce data-explorer tables (PR #61)

Instead of using PluginStoreRow we should use plugin-specific models like DataExplorer::Query and DataExplorer::QueryGroup

@tgxworld I didn’t create that controller, I was just thinking that extracting that to separate file would make sense. However, I had to make small adjustments to use separate models instead of PluginStoreRows

GitHub

requires_plugin is already setting a before_action to check that the plugin has not been disabled.

:thinking: Are we using these anywhere?

Let us make this method private since they are not controller actions.

Let us make this method private since they are not controller actions.

find will raise an error if the params[:id] is invalid. Do we want to raise an error here? Otherwise, it’ll probably be better to raise a Discourse::InvalidParameters error or maybe Discourse::NotFound.

I think we can do this check much earlier to avoid all the work above if database_queries_ids.include?(query.id) returns true.

Generally I prefer to move this to the top of the controller so that we can see all the callbacks in a single place.

This will raise an error if params[:id] is invalid.

I think the plugin is only accessible to admin so we need proper access control here.

Just curious, how is this being used because performance might be bad if a site has alot of groups. Also, do we need to filter out automatic groups or implement some sort of access control here?

Ahhh looks like there is an admin constraint on the route so this is OK.

I’m not quite sure why we need this, is this safe to remove?

          .joins("INNER JOIN data_explorer_query_groups 

We should execute all the queries in a single transaction so that one query failing does not leave us in a weird state.

This will raise an error on invalid params[:id] too.

    ActiveRecord::Base.transaction do
      query.update!(params.require(:query).permit(:name, :sql, :description).merge(hidden: false))

      group_ids = params.require(:query)[:group_ids]
      DataExplorer::QueryGroup.where.not(group_id: group_ids).delete_all
      group_ids&.each do |group_id|
        query.query_groups.find_or_create_by!(group_id: group_id)
      end
    end

Hmm this doesn’t make sense to me because this is a destroy action. If the params[:id] is not valid, we shouldn’t have to create an instance of the object.

Not a fan of using a blanket rescue nil because it swallows all errors.

Not a fan of using a blanket rescue nil because it swallows all errors.