-
-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Add new password field #998
Conversation
OscarPay
commented
Oct 6, 2017
- New Field::Password
- More tests for the new field
- Modify the Customer Dashboard to show new field
- Add field information in Customizing Dashboards documentation
SHOW_PAGE_ATTRIBUTES = ATTRIBUTE_TYPES.keys - [:name] | ||
FORM_ATTRIBUTES = [ | ||
:name, | ||
:email, | ||
:email_subscriber, | ||
:kind, | ||
:country, | ||
:password |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Put a comma after the last item of a multiline array.
} | ||
|
||
COLLECTION_ATTRIBUTES = ATTRIBUTE_TYPES.keys | ||
COLLECTION_ATTRIBUTES = ATTRIBUTE_TYPES.keys - [:created_at, :updated_at] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use %i or %I for an array of symbols.
@@ -12,16 +12,18 @@ class CustomerDashboard < Administrate::BaseDashboard | |||
kind: Field::Select.with_options(collection: Customer::KINDS), | |||
country: Field::BelongsTo. | |||
with_options(primary_key: :code, foreign_key: :country_code), | |||
password: Field::Password |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Put a comma after the last item of a multiline hash.
lib/administrate/field/password.rb
Outdated
end | ||
end | ||
end | ||
end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Final newline missing.
lib/administrate/field/password.rb
Outdated
end | ||
|
||
def character | ||
options.fetch(:character, '•') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Prefer double-quoted strings unless you need single quotes to avoid extra backslashes for escaping.
spec/lib/fields/password_spec.rb
Outdated
end | ||
|
||
it "different to the default character" do | ||
password = password_with_options(lorem(30), character: '*') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Prefer double-quoted strings unless you need single quotes to avoid extra backslashes for escaping.
spec/lib/fields/password_spec.rb
Outdated
end | ||
|
||
it "defaults to displaying up to 50 characters" do | ||
short = Administrate::Field::Password.new(:short_password, lorem(30), :show) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Line is too long. [82/80]
spec/lib/fields/password_spec.rb
Outdated
|
||
expect(path).to eq("/fields/password/#{page}") | ||
end | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Extra empty line detected at block body end.
spec/lib/fields/password_spec.rb
Outdated
include FieldMatchers | ||
|
||
describe "#to_partial_path" do | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Extra empty line detected at block body beginning.
spec/example_app/db/seeds.rb
Outdated
@@ -28,6 +28,7 @@ | |||
name: name, | |||
email: Faker::Internet.safe_email(name), | |||
country: countries.sample, | |||
password: Faker::Internet.password |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Put a comma after the last item of a multiline hash.
spec/lib/fields/password_spec.rb
Outdated
end | ||
end | ||
|
||
it {should_permit_param(:foo, for_attribute: :foo)} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Space missing inside {.
Space missing inside }.
} | ||
|
||
COLLECTION_ATTRIBUTES = ATTRIBUTE_TYPES.keys | ||
COLLECTION_ATTRIBUTES = ATTRIBUTE_TYPES.keys - %i[created_at, updated_at] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Within %i/%I, ':' and ',' are unnecessary and may be unwanted in the resulting symbols.
spec/lib/fields/password_spec.rb
Outdated
end | ||
end | ||
|
||
it {should_permit_param(:foo, for_attribute: :foo)} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Space missing inside {.
Space missing inside }.
} | ||
|
||
COLLECTION_ATTRIBUTES = ATTRIBUTE_TYPES.keys | ||
COLLECTION_ATTRIBUTES = ATTRIBUTE_TYPES.keys - %i[created_at, updated_at] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Within %i/%I, ':' and ',' are unnecessary and may be unwanted in the resulting symbols.
Hi @OscarPay! Thanks for contributing this PR! I'm a bit apprehensive about adding additional fields as we've got our plugin system, but I think this is a reasonable default field to have. Would you be able to rebase so that I can go ahead and merge this? |
Hey @nickcharlton - Rebase and merge pull requests should be |
06a858f
to
709b614
Compare
* New Field::Password * Modify the Customer Dashboard to show new field * Add field information in Customizing Dashboards documentation
709b614
to
348a6d1
Compare