Organizations: minor code cleanup #355
Conversation
markets
force-pushed
the
orgs_minor_refactor
branch
from
96fa377 to
fdb3120
Compare
| @organizations = Organization.all | ||
| end | ||
| end | ||
| before_filter :load_resource, except: [:new, :index, :create] |
There was a problem hiding this comment.
what about using only instead? It will feel a bit more under control. Loading it by default in any new action seems dangerous to me.
|
|
||
| def new | ||
| @organization = Organization.new | ||
| end | ||
|
|
||
| def index | ||
| @organizations = @organizations.matching(params[:q]) if params[:q].present? |
There was a problem hiding this comment.
why are we removing the q param? That will break the search, right?
There was a problem hiding this comment.
I deleted it becasue I can't found any point of the app using this search.
spec/models/organization_spec.rb
Outdated
| expect(organization).to be_valid | ||
| expect(organization.web).to eq "http://www.casa.com" | ||
| end | ||
| it "with http" do |
There was a problem hiding this comment.
can we add some spacing between text examples, now that we are at it?
| } | ||
|
|
||
| validates :name, uniqueness: true | ||
| validates :name, presence: true, uniqueness: true |
There was a problem hiding this comment.
let's make sure we also have null: false and a unique index at db level 🙏
There was a problem hiding this comment.
agree @sauloperez I'll review the schema and in case this is missing, I'll create a migration to fix that
There was a problem hiding this comment.
@markets can you please check this, thanks!!! (no rush of course 😄 )
There was a problem hiding this comment.
Oh, I see you did it already here b9d9b96 right?
|
@sauloperez code review changes pushed! |
There was a problem hiding this comment.
Nice, thanks @markets !
Would you please mind to add some test note in the description? So who'll pick it to test it will know what to look for. Thanks!
|
Hi @markets I tested to create a organization, without name now it isn't possible 👏 . Is it enough test? May be you can help with other improvements in active admin, if you want I can create the issues:
😘 😘 😘 |
|
Hi @sseerrggii thanks a lot! 👏 yes, that test should be enough, as it's the only behavior change here, the rest is just some code cleanup and minor refactors! About the other couple of points, yes no problem, I can take a look, but in a separated branch. So if you can create a new issue with them, would be 👌 |
|
Thanks @sseerrggii @markets 🎉
Remember that we have a prioritized list of stuff to do in Trello, I would rather start from there 🙏 |
|
@sseerrggii @markets can we give this PR as tested and merge it? |
|
@markets sorry, would you mind to rebase it from the latest |
|
@enricostano updated! |
This is just a minor code cleanup 🔨 (about Organizations):
validates :name, presence: trueshow_error_messages!(@organization)to print error messages in formWe need to properly define and implement an
OrganizationPolicy, but this would be a different PR (we should define an access control policy first), this branch only contains some minor refactors, which clarify/simplify code and remove 💀 methods. What do you think guys? The only behavior change is the validation on name presence, which is probably a sane change.