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

Add validation to Github name validation #259 #352

Open
wants to merge 9 commits into
base: master
Choose a base branch
from

Conversation

thefenry
Copy link
Contributor

Issue #259 , Remove conditional in the model validation. Now Github Name will get validated before submission.

Now
image

Remove conditional in the model validation.
@@ -13,7 +13,7 @@ class Organization < ActiveRecord::Base
attr_accessor :is_public_submission

validates_presence_of :name
validates_presence_of :github_org, if: :is_public_submission
validates_presence_of :github_org

Choose a reason for hiding this comment

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

Prefer the new style validations validates :column, presence: value over validates_presence_of.

@@ -83,7 +83,7 @@
end

describe '#related_projects' do
let(:organization) { Organization.create!(name: 'CodeMontage') }
let(:organization) { Organization.create!(name: 'CodeMontage', github_org: 'codeMontage') }

Choose a reason for hiding this comment

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

Line is too long. [95/80]
Prefer double-quoted strings unless you need single quotes to avoid extra backslashes for escaping.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is ready to be merged as it passes the travisci test

@@ -83,7 +83,8 @@
end

describe '#related_projects' do
let(:organization) { Organization.create!(name: 'CodeMontage') }
let(:organization) { Organization.create!(name: 'CodeMontage',
github_org: 'codeMontage') }

Choose a reason for hiding this comment

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

Align the elements of a hash literal if they span more than one line.
Expression at 87, 66 should be on its own line.
Prefer double-quoted strings unless you need single quotes to avoid extra backslashes for escaping.

@thefenry
Copy link
Contributor Author

thefenry commented Mar 2, 2015

@DBNess This is ready to be merged.

@courte
Copy link
Member

courte commented Mar 5, 2015

Thank you for this change, @thefenry! I really appreciate all the improvements you've been taking on over the last few months. 👏 🙇

Unfortunately, there are some instances where we have organizations in the database that do not have relevant GitHub profiles (e.g. sponsors, partners), so we can't make it a requirement for new orgs in all cases.

Buuuut, it looks like the client-side validation gem we're using has an option for input-based validations. Check out the end of this: https://github.com/DavyJonesLocker/client_side_validations-simple_form#usage.

If you have time, would you adjust this fix to use that method instead? I haven't attempted it before, so let me know if you hit any snags. 😎

@thefenry
Copy link
Contributor Author

thefenry commented Mar 5, 2015

Thanks for the feedback @courte. I have some time today to get this fixed. I have a few questions about the requirements for this validation to exist. I understand that some organizations do not have github profiles which makes sense but is there anyway in which you would know that they have one based on this form. I am not even sure that you need to require this field if some organizations do not have one. Let me know your thoughts on this. Thanks

@courte
Copy link
Member

courte commented Mar 5, 2015

That's a great question, @thefenry. This form is for submitting a new project, and projects can only belong to organizations that have GitHub orgs due how we create GitHub links and a host of other things for projects. So we can't accept a project if we didn't also get the org to which its repo belongs, or it wouldn't display properly.

(On a non-technical level, it's a built-in confirmation that submitted projects, which may eventually be approved for the platform, have the potential for longevity created by an organized, collaborative effort.)

@thefenry
Copy link
Contributor Author

thefenry commented Mar 5, 2015

Hmm, @courte I will admit I am a bit confused. What I got from your explanation in the previous comment is what i thought I was doing with the validations. If you are submitting a project you need to have a github organization. The github organization is the github username field that is used for the link when the project is approved. So I am not sure what we are missing on this. Sorry for being confused.

@courte
Copy link
Member

courte commented Mar 6, 2015

No need to apologize! I wasn't very clear.

Yes, we Users/Visitors who are submitting a project must include a :github_org. But Admins who use the administrative or console layers to create organizations, need to be able to create organizations that don't have a :github_org in certain cases.

Validations in the model impact both, but the form-based validation (from the gem docs I linked yesterday) will hopefully only impact user/visitor submissions.

Does that make more sense?

@@ -1,6 +1,7 @@
class OrganizationsController < ApplicationController
def new
@organization = Organization.new
@organization.is_public_submission = current_user.is_admin unless current_user.nil?

Choose a reason for hiding this comment

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

Line is too long. [87/80]

@@ -1,6 +1,8 @@
class OrganizationsController < ApplicationController
def new
@organization = Organization.new
@organization.is_public_submission =
current_user.is_admin unless current_user.nil?

Choose a reason for hiding this comment

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

Indent the first parameter one step more than the previous line.

@thefenry
Copy link
Contributor Author

thefenry commented Mar 9, 2015

@courte Yes that makes more sense. I think i got it. I made a pull request and everything seems to pass all the tests and function correctly. I tested as an admin and a regular user. They both work correctly and I was able to add a organization using the console as well. Let me know if there are any other issues.

@DBNess
Copy link
Member

DBNess commented Mar 9, 2015

@thefenry you're a champion! 🏆 🎉 thank you! 😃

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants