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

Columns filtering: blacklisting or whitelisting #16

Merged
merged 3 commits into from Jan 13, 2017

Conversation

Projects
None yet
3 participants
@charlie-wasp
Copy link
Collaborator

commented Jan 12, 2017

Hello there,

It's my attempt to resolve #13

I implemented two mutually exclusive options, --whitelist and --blacklist, both accepts arrays, like this: --blacklist=title created_at or --whitelist active body

I will be glad to fix any problems with this code and discuss any suggestions!

@@ -6,6 +6,11 @@ def successfully(command)
.to eq(true), "'#{command}' was unsuccessful"
end

def unsuccessfully(command)
expect(system("RAILS_ENV=test #{command}"))
.to eq(false), "'#{command}' was successful"

This comment has been minimized.

Copy link
@houndci-bot

houndci-bot Jan 12, 2017

Style/DotPosition: Place the . on the previous line, together with the method call receiver.

@@ -6,6 +6,11 @@ def successfully(command)
.to eq(true), "'#{command}' was unsuccessful"
end

def unsuccessfully(command)
expect(system("RAILS_ENV=test #{command}"))
.to eq(false), "'#{command}' was successful"

This comment has been minimized.

Copy link
@houndci-bot

houndci-bot Jan 12, 2017

Style/DotPosition: Place the . on the previous line, together with the method call receiver.

end

This comment has been minimized.

Copy link
@houndci-bot

houndci-bot Jan 12, 2017

Style/EmptyLinesAroundBlockBody: Extra empty line detected at block body end.

end

This comment has been minimized.

Copy link
@houndci-bot

houndci-bot Jan 12, 2017

Style/EmptyLinesAroundBlockBody: Extra empty line detected at block body end.

include_context "cleanup migrations"

before(:all) do
@whitelist = ["title", "rating"]

This comment has been minimized.

Copy link
@houndci-bot

houndci-bot Jan 12, 2017

Style/WordArray: Use %w or %W for an array of words.

include_context "cleanup migrations"

before(:all) do
@whitelist = ["title", "rating"]

This comment has been minimized.

Copy link
@houndci-bot

houndci-bot Jan 12, 2017

Style/WordArray: Use %w or %W for an array of words.

post.update!(rating: 5)
post.reload.undo!
before(:all) do
@blacklist = ["created_at", "active"]

This comment has been minimized.

Copy link
@houndci-bot

houndci-bot Jan 12, 2017

Style/WordArray: Use %w or %W for an array of words.

post.update!(rating: 5)
post.reload.undo!
before(:all) do
@blacklist = ["created_at", "active"]

This comment has been minimized.

Copy link
@houndci-bot

houndci-bot Jan 12, 2017

Style/WordArray: Use %w or %W for an array of words.

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", "rating" => 22, "active" => true)

This comment has been minimized.

Copy link
@houndci-bot

houndci-bot Jan 12, 2017

Style/DotPosition: Place the . on the previous line, together with the method call receiver.

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", "rating" => 22, "active" => true)

This comment has been minimized.

Copy link
@houndci-bot

houndci-bot Jan 12, 2017

Style/DotPosition: Place the . on the previous line, together with the method call receiver.

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", "rating" => 10, "active" => true)

This comment has been minimized.

Copy link
@houndci-bot

houndci-bot Jan 12, 2017

Style/DotPosition: Place the . on the previous line, together with the method call receiver.

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", "rating" => 10, "active" => true)

This comment has been minimized.

Copy link
@houndci-bot

houndci-bot Jan 12, 2017

Style/DotPosition: Place the . on the previous line, together with the method call receiver.

end

after(:all) { @old_post.destroy! }
context 'without blacklisting'do

This comment has been minimized.

Copy link
@houndci-bot

houndci-bot Jan 12, 2017

Style/SpaceAroundKeyword: Space before keyword do is missing.

end

after(:all) { @old_post.destroy! }
context 'without blacklisting'do

This comment has been minimized.

Copy link
@houndci-bot

houndci-bot Jan 12, 2017

Style/SpaceAroundKeyword: Space before keyword do is missing.

@@ -2,7 +2,7 @@
require 'spec_helper'
require 'generators/logidze/model/model_generator'

describe Logidze::Generators::ModelGenerator, type: :generator do
describe Logidze::Generators::ModelGenerator, type: :generator do

This comment has been minimized.

Copy link
@houndci-bot

houndci-bot Jan 12, 2017

Style/TrailingWhitespace: Trailing whitespace detected.

@@ -2,7 +2,7 @@
require 'spec_helper'
require 'generators/logidze/model/model_generator'

describe Logidze::Generators::ModelGenerator, type: :generator do
describe Logidze::Generators::ModelGenerator, type: :generator do

This comment has been minimized.

Copy link
@houndci-bot

houndci-bot Jan 12, 2017

Style/TrailingWhitespace: Trailing whitespace detected.

def columns_blacklist
array = if !options[:whitelist]
options[:blacklist]
else

This comment has been minimized.

Copy link
@houndci-bot

houndci-bot Jan 12, 2017

Style/TrailingWhitespace: Trailing whitespace detected.

def columns_blacklist
array = if !options[:whitelist]
options[:blacklist]
else

This comment has been minimized.

Copy link
@houndci-bot

houndci-bot Jan 12, 2017

Style/TrailingWhitespace: Trailing whitespace detected.

@charlie-wasp

This comment has been minimized.

Copy link
Collaborator Author

commented Jan 12, 2017

@palkan it seems, that travis has some problems with Postgres, not with a code, am I right? :)

@palkan
Copy link
Owner

left a comment

Also, let's remove this line from .travis.yml – sudo apt-get -y remove --purge postgresql-9.1.
Looks like Travis doesn't support it anymore.

Btw, maybe it already has PostgreSQL 9.5 support?

@@ -16,7 +16,7 @@ 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 '{}') RETURNS jsonb AS $body$

This comment has been minimized.

Copy link
@palkan

palkan Jan 12, 2017

Owner

When upgrading we have to drop previously defined function, 'cause we change the signature.
So, we should add here DROP FUNCTION IF EXISTS ... in case of upgrade (use update? helper).

@@ -36,12 +36,12 @@ class <%= @migration_class_name %> < ActiveRecord::Migration
$body$
LANGUAGE plpgsql;

CREATE OR REPLACE FUNCTION logidze_snapshot(item jsonb) RETURNS jsonb AS $body$
CREATE OR REPLACE FUNCTION logidze_snapshot(item jsonb, blacklist text[] DEFAULT '{}') RETURNS jsonb AS $body$

This comment has been minimized.

Copy link
@palkan

palkan Jan 12, 2017

Owner

The same goes here.

This comment has been minimized.

Copy link
@charlie-wasp

charlie-wasp Jan 13, 2017

Author Collaborator

My bad, didn't think about it. Added DROP FUNCTION... statement 👍 Don't know why only one comment now is marked as outdated, though.

@palkan

This comment has been minimized.

Copy link
Owner

commented Jan 12, 2017

@charlie-wasp Yep.

It's my attempt to resolve #13

Pretty good attempt) 👍

@charlie-wasp

This comment has been minimized.

Copy link
Collaborator Author

commented Jan 13, 2017

@palkan about travis: I deleted postgres 9.1 related instruction, so build now goes further, but now travis gives metype "jsonb" does not exist, when it tries to create users table.

I am sorry, I've got no prior experience with Travis, so it may be not easy for me to figure things out :)

About 9.5 support - I found this long conversation travis-ci/travis-ci#4264, it seems that there are some problems, but maybe it's not our case. There was a link https://docs.travis-ci.com/user/database-setup/#Using-a-different-PostgreSQL-Version, it's said there that for some Linux builds 9.5 is supported, but I don't know, which one we are using :(

@palkan

This comment has been minimized.

Copy link
Owner

commented Jan 13, 2017

@charlie-wasp That's the way to use specific dist – link.

I've updated .travis.yml in master, rebase, please.

@charlie-wasp charlie-wasp force-pushed the charlie-wasp:columns_filtering branch from 2981df4 to 86db7e9 Jan 13, 2017

@charlie-wasp

This comment has been minimized.

Copy link
Collaborator Author

commented Jan 13, 2017

@palkan rebased!

@palkan

palkan approved these changes Jan 13, 2017

@palkan

This comment has been minimized.

Copy link
Owner

commented Jan 13, 2017

Great work, thanks!

@palkan palkan merged commit dc088b8 into palkan:master Jan 13, 2017

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
hound No violations found. Woof!

@palkan palkan referenced this pull request Jan 13, 2017

Closed

columns filtering #15

@charlie-wasp

This comment has been minimized.

Copy link
Collaborator Author

commented Jan 13, 2017

@palkan my pleasure! Should I update the docs next?

@palkan

This comment has been minimized.

Copy link
Owner

commented Jan 13, 2017

@charlie-wasp Yep, please, open a new PR with readme and changelog updates.

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