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

4423 format account request org websites #4427

Merged

Conversation

Gabe-Torres
Copy link
Contributor

Resolves #4423

Description

This pr implements adding validation for theorganization_website attribute within the account_request model and enforces a URL format that will reduce the account acceptance process for admins. It also adds an example placeholder in the account request form to reduce confusion.

Type of change

  • New feature (non-breaking change which adds functionality)

How Has This Been Tested?

organization_websites attribute validation is now being tested within the account_request model spec and enforces valid URL formats and will not validate www.example.com formats
it { should allow_value(Faker::Internet.url).for(:organization_website) }
it { should_not allow_value("www.example.com").for(:organization_website) }

Screenshots

Before

Screenshot 2024-06-04 at 2 27 57 PM

Screenshot 2024-06-04 at 2 27 29 PM

After

Screenshot 2024-06-04 at 2 01 31 PM

Screenshot 2024-06-04 at 2 02 02 PM

cielf
cielf previously requested changes Jun 5, 2024
Copy link
Collaborator

@cielf cielf left a comment

Choose a reason for hiding this comment

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

Thank you! This looks pretty good from a functional p.o.v. -- one small textual change, then over to @dorner to see if there's anything we want modified from a technical/testing p.o.v.

@@ -21,6 +21,7 @@ class AccountRequest < ApplicationRecord
validates :email, presence: true, uniqueness: true
validates :request_details, presence: true, length: { minimum: 50 }
validates :email, format: { with: URI::MailTo::EMAIL_REGEXP }
validates :organization_website, format: { with: URI::DEFAULT_PARSER.make_regexp, message: "it should look like 'http://www.example.com'" }, allow_blank: true
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please remove the "it" at the beginning of the message. Thanks.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed the "it" from the message

Copy link
Collaborator

@cielf cielf left a comment

Choose a reason for hiding this comment

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

LGTM

@cielf cielf dismissed their stale review June 6, 2024 02:42

Changes look good!

@cielf cielf requested a review from dorner June 6, 2024 02:42
Copy link
Collaborator

@dorner dorner left a comment

Choose a reason for hiding this comment

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

Looks good, but I'd like to change the messaging to prompt for https as the default.

@@ -21,6 +21,7 @@ class AccountRequest < ApplicationRecord
validates :email, presence: true, uniqueness: true
validates :request_details, presence: true, length: { minimum: 50 }
validates :email, format: { with: URI::MailTo::EMAIL_REGEXP }
validates :organization_website, format: { with: URI::DEFAULT_PARSER.make_regexp, message: "should look like 'http://www.example.com'" }, allow_blank: true
Copy link
Collaborator

Choose a reason for hiding this comment

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

The default should be https, not http. Pretty much every website in existence is on httpsnow.

@@ -75,7 +75,7 @@
</div>

<div class="form-inputs">
<%= f.input :organization_website %>
<%= f.input :organization_website, placeholder: "http://www.example.com" %>
Copy link
Collaborator

Choose a reason for hiding this comment

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

Ditto

@Gabe-Torres
Copy link
Contributor Author

Looks good, but I'd like to change the messaging to prompt for https as the default.

Good point. I'll change that up!

Majority of websites are now https
@Gabe-Torres
Copy link
Contributor Author

Updated 👍🏽

@Gabe-Torres Gabe-Torres requested a review from dorner June 12, 2024 02:08
@dorner dorner merged commit 46eef9f into rubyforgood:main Jun 14, 2024
19 checks passed
@dorner
Copy link
Collaborator

dorner commented Jun 14, 2024

Thanks!

@Gabe-Torres
Copy link
Contributor Author

Yeah, thank y'all as well!

Copy link
Contributor

@Gabe-Torres: Your PR 4423 format account request org websites is part of today's Human Essentials production release: 2024.06.16.
Thank you very much for your contribution!

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.

Account requests and org websites
3 participants