Skip to content

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.

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.

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

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?

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]

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