columns filtering #15

Neyaz · 2016-12-03T04:31:10Z

Hello,
it's my solution task on cultofmartians.

houndci-bot · 2016-12-03T04:31:16Z

spec/integrations/triggers_spec.rb

+    end
+  end
+end
+


Style/TrailingBlankLines: 1 trailing blank lines detected.

houndci-bot · 2016-12-03T04:31:16Z

spec/integrations/triggers_spec.rb

+          .to include("rating" => 22)
+
+      expect(post.log_data.versions.first.changes)
+          .not_to include("title" => "Triggers", "active" => true)


Style/DotPosition: Place the . on the previous line, together with the method call receiver.
Style/MultilineMethodCallIndentation: Use 2 (not 4) spaces for indenting an expression spanning multiple lines.

houndci-bot · 2016-12-03T04:31:16Z

spec/integrations/triggers_spec.rb

+      expect(post.reload.log_version).to eq 6
+      expect(post.log_size).to eq 4
+      expect(post.log_data.versions.first.changes)
+          .to include("rating" => 22)


Style/DotPosition: Place the . on the previous line, together with the method call receiver.
Style/MultilineMethodCallIndentation: Use 2 (not 4) spaces for indenting an expression spanning multiple lines.

houndci-bot · 2016-12-03T04:31:16Z

spec/integrations/triggers_spec.rb

+          .to include("rating" => 10)
+
+      expect(post.log_data.versions.first.changes)
+          .not_to include("title" => "Triggers", "active" => true)


Style/DotPosition: Place the . on the previous line, together with the method call receiver.
Style/MultilineMethodCallIndentation: Use 2 (not 4) spaces for indenting an expression spanning multiple lines.

houndci-bot · 2016-12-03T04:31:16Z

spec/integrations/triggers_spec.rb

+      expect(post.reload.log_version).to eq 5
+      expect(post.log_size).to eq 4
+      expect(post.log_data.versions.first.changes)
+          .to include("rating" => 10)


Style/DotPosition: Place the . on the previous line, together with the method call receiver.
Style/MultilineMethodCallIndentation: Use 2 (not 4) spaces for indenting an expression spanning multiple lines.

houndci-bot · 2016-12-03T04:31:17Z

spec/integrations/triggers_spec.rb

+          .to include("title" => "Triggers", "active" => true)
+
+      expect(post.log_data.versions.first.changes)
+          .not_to include("rating" => 20)


Style/DotPosition: Place the . on the previous line, together with the method call receiver.
Style/MultilineMethodCallIndentation: Use 2 (not 4) spaces for indenting an expression spanning multiple lines.

houndci-bot · 2016-12-03T04:31:17Z

spec/integrations/triggers_spec.rb

+      expect(post.reload.log_version).to eq 6
+      expect(post.log_size).to eq 4
+      expect(post.log_data.versions.first.changes)
+          .to include("title" => "Triggers", "active" => true)


Style/DotPosition: Place the . on the previous line, together with the method call receiver.
Style/MultilineMethodCallIndentation: Use 2 (not 4) spaces for indenting an expression spanning multiple lines.

houndci-bot · 2016-12-03T04:31:17Z

spec/integrations/triggers_spec.rb

+          .to include("title" => "Triggers", "active" => true)
+
+      expect(post.log_data.versions.first.changes)
+          .not_to include("rating" => 10)


Style/DotPosition: Place the . on the previous line, together with the method call receiver.
Style/MultilineMethodCallIndentation: Use 2 (not 4) spaces for indenting an expression spanning multiple lines.

houndci-bot · 2016-12-03T04:31:17Z

spec/integrations/triggers_spec.rb

+      expect(post.reload.log_version).to eq 5
+      expect(post.log_size).to eq 4
+      expect(post.log_data.versions.first.changes)
+          .to include("title" => "Triggers", "active" => true)


Style/DotPosition: Place the . on the previous line, together with the method call receiver.
Style/MultilineMethodCallIndentation: Use 2 (not 4) spaces for indenting an expression spanning multiple lines.

houndci-bot · 2016-12-03T04:31:17Z

spec/generators/model_generator_spec.rb

+          let(:args) { ["user", "--except='active,age'", "--only='active,age'"] }
+
+          it "creates trigger with except and only" do
+              is_expected.to exist


Style/IndentationWidth: Use 2 (not 4) spaces for indentation.

houndci-bot · 2016-12-03T04:31:17Z

spec/generators/model_generator_spec.rb

+      end
+
+      context "with except and only" do
+          let(:args) { ["user", "--except='active,age'", "--only='active,age'"] }


Style/IndentationWidth: Use 2 (not 4) spaces for indentation.

houndci-bot · 2016-12-03T04:31:18Z

spec/generators/model_generator_spec.rb

+          let(:args) { ["user", "--limit=5", "--except='active,age'", "--only='active,age'"] }
+
+          it "creates trigger with limit and except and only" do
+              is_expected.to exist


Style/IndentationWidth: Use 2 (not 4) spaces for indentation.

houndci-bot · 2016-12-03T04:31:18Z

spec/generators/model_generator_spec.rb

+      end
+
+      context "with limit and except and only" do
+          let(:args) { ["user", "--limit=5", "--except='active,age'", "--only='active,age'"] }


Style/IndentationWidth: Use 2 (not 4) spaces for indentation.

houndci-bot · 2016-12-03T04:31:18Z

spec/generators/model_generator_spec.rb

+          let(:args) { ["user", "--limit=5", "--only='active,age'"] }
+
+          it "creates trigger with limit and only" do
+              is_expected.to exist


Style/IndentationWidth: Use 2 (not 4) spaces for indentation.

houndci-bot · 2016-12-03T04:31:18Z

spec/generators/model_generator_spec.rb

+      end
+
+      context "with limit and only" do
+          let(:args) { ["user", "--limit=5", "--only='active,age'"] }


Style/IndentationWidth: Use 2 (not 4) spaces for indentation.

houndci-bot · 2016-12-03T04:31:18Z

spec/generators/model_generator_spec.rb

+          let(:args) { ["user", "--limit=5", "--except='active,age'"] }
+
+          it "creates trigger with limit and except" do
+              is_expected.to exist


Style/IndentationWidth: Use 2 (not 4) spaces for indentation.

palkan

Overall, great work! Thanks!

palkan · 2016-12-04T11:21:47Z

lib/generators/logidze/install/templates/migration.rb.erb

+          IF blacklist <> 'null' THEN
+            filtered_array := logidze_exclude_keys(data, array['log_data']::text[] || string_to_array(blacklist, ','));
+          ELSIF blacklist = 'null' AND whitelist <> 'null' THEN
+            filtered_array := logidze_include_keys(data, string_to_array(whitelist, ','));


I'd prefer to leave the only one function for filtering, logidze_exclude_keys.
Then we have to calculate required keys beforehand, smth like:

keys_to_exclude := select array(select unnest(jsonb_object_keys(data)) except select unnest(whitelist)); filtered_array := logidze_exclude_keys(data, keys_to_exclude);

palkan · 2016-12-04T11:22:54Z

lib/generators/logidze/install/templates/migration.rb.erb

@@ -16,17 +16,26 @@ class <%= @migration_class_name %> < ActiveRecord::Migration
    end

    execute <<-SQL
-      CREATE OR REPLACE FUNCTION logidze_version(v bigint, data jsonb) RETURNS jsonb AS $body$
+      CREATE OR REPLACE FUNCTION logidze_version(v bigint, data jsonb, blacklist text DEFAULT '', whitelist text DEFAULT '') RETURNS jsonb AS $body$


Maybe we can use here blacklist text[] to avoid string_to_array for every operation?

Yes, i used blacklist text[] , but got problems with how to transform ruby string to pl pgSQL array and decided to use strings

palkan · 2016-12-04T11:25:12Z

lib/generators/logidze/install/templates/migration.rb.erb

        BEGIN
+          IF blacklist <> 'null' THEN
+            filtered_array := logidze_exclude_keys(data, array['log_data']::text[] || string_to_array(blacklist, ','));


Why not {"log_data"}?

When i tried this notation I was getting "PG::SyntaxError: ERROR: syntax error at or near "{"".
But today i found what it should be (data, '{log_data}').

palkan · 2016-12-04T11:29:00Z

spec/integrations/triggers_spec.rb

+
+  let(:params) { { title: 'Triggers', rating: 10, active: false } }
+
+  describe "backfill" do


Please, do not duplicate existing tests. Only test the feature you're working on.
If you want to check that the new options don't break existing functionality, just add them to the existing migrations.

temanmd · 2016-12-04T15:54:23Z

good job! 👍

Neyaz · 2016-12-04T16:46:26Z

@palkan Thank you for review.

palkan · 2016-12-27T07:53:53Z

@Neyaz Hi! How is it going?)

palkan · 2017-01-13T08:56:17Z

Closed by #16

columns filtering

8c8512a

houndci-bot reviewed Dec 3, 2016

View reviewed changes

spec/integrations/triggers_spec.rb

end

end

end

Copy link

houndci-bot Dec 3, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Style/TrailingBlankLines: 1 trailing blank lines detected.

houndci-bot reviewed Dec 3, 2016

View reviewed changes

palkan requested changes Dec 4, 2016

View reviewed changes

palkan closed this Jan 13, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

columns filtering #15

columns filtering #15

Neyaz commented Dec 3, 2016

houndci-bot Dec 3, 2016

houndci-bot Dec 3, 2016

houndci-bot Dec 3, 2016

houndci-bot Dec 3, 2016

houndci-bot Dec 3, 2016

houndci-bot Dec 3, 2016

houndci-bot Dec 3, 2016

houndci-bot Dec 3, 2016

houndci-bot Dec 3, 2016

houndci-bot Dec 3, 2016

houndci-bot Dec 3, 2016

houndci-bot Dec 3, 2016

houndci-bot Dec 3, 2016

houndci-bot Dec 3, 2016

houndci-bot Dec 3, 2016

houndci-bot Dec 3, 2016

palkan left a comment

palkan Dec 4, 2016

palkan Dec 4, 2016

Neyaz Dec 4, 2016

palkan Dec 4, 2016

Neyaz Dec 4, 2016

palkan Dec 4, 2016

temanmd commented Dec 4, 2016

Neyaz commented Dec 4, 2016

palkan commented Dec 27, 2016

palkan commented Jan 13, 2017


		let(:params) { { title: 'Triggers', rating: 10, active: false } }

		describe "backfill" do

columns filtering #15

columns filtering #15

Conversation

Neyaz commented Dec 3, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

palkan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

temanmd commented Dec 4, 2016

Neyaz commented Dec 4, 2016

palkan commented Dec 27, 2016

palkan commented Jan 13, 2017