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.

Sign up for GitHub

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

AO3-3997 Fix for email address validation #4390 #4565

Merged
merged 5 commits into from
Jul 11, 2023
Merged

AO3-3997 Fix for email address validation #4390 #4565

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
112ae8c
AO3-3997 Catch email validation failure in import
potpotkettle Jul 5, 2023
2de6037
AO3-3997 Add missing error message in invitation page
potpotkettle Jul 4, 2023
7885c14
AO3-3997 more fixes for invite
potpotkettle Jul 7, 2023
fa60bde
AO3-3997 more fixes for import
potpotkettle Jul 7, 2023
04c0878
AO3-3997 more fixes for import (2)
tickinginstant Jul 10, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 5 additions & 1 deletion app/models/story_parser.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -421,8 +421,12 @@ def parse_author_common(email, name)
name = name.to_ascii.gsub(/[^\w[ \-@.]]/u, "")
external_author = ExternalAuthor.find_or_create_by(email: email)
external_author_name = external_author.default_name

# if the name and email don't exist in the DB tables, add it
unless name.blank?
external_author_name = ExternalAuthorName.where(name: name, external_author_id: external_author.id).first ||
raise Error, external_author.errors.full_messages.join(" ") if external_author.invalid?
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this line is in the wrong place. If someone enters !!!!! as the name, and also uses an invalid email address, then the exclamation points in the name will be stripped down so that name is blank. So even though it's an invalid email, I don't think it will reach this line and show the error. I'm pretty sure this line needs to be outside of the unless name.blank? block.

Copy link
Contributor Author

@potpotkettle potpotkettle Jul 7, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Thank you for your comment. In that scenario (and for the scenario where the name is blank and the email is invalid), should the error just say "invalid email"? Or should it alert about the name and the email?

EDIT: Actually, it looks the name with invalid characters only (like "!!!!") is not supposed to cause an error, or at least that's the status quo. So my comment above is more for the blank name and invalid email case.

Copy link
Contributor

Choose a reason for hiding this comment

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

It'd be more difficult to handle that case, since the ExternalAuthor currently doesn't get created if either the name or the email is blank, so it never runs through the validator. Since it's not producing a 500 error, I'd be tempted to let it slide for the moment, but someone who's more familiar with the Open Doors committee might have a better answer for you.

(Really, I think the entire function should be rewritten, so that there are proper error messages for everything instead of silently fixing invalid names, and it's easier to follow the code. But IDK if that's in scope, and IDK if that's what Open Doors wants.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I also reorganized parser_author_common in a way that makes sense to me. And to avoid breaking something, I added tests and confirmed the behavior didn't change (as far as I can see) except for the email address validation, too.

Copy link
Contributor

Choose a reason for hiding this comment

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

If you're going to rewrite it for clarity reasons, I think it'd be cleaner to consolidate most of the name-related code. As is, redacted_name is set, and then there are a bunch of lines involving errors & emails, and then redacted_name is finally used. But there's no reason for it to be set that early, and it's easier to keep track of the variable's purpose if it's set right before it's used.

Similarly, the two different possible values of external_author_name are set in two different if blocks, so it's hard to see at a glance what the function is supposed to return. It'd be cleaner to set it equal to external_author.default_name in the else of the if redacted_name.present? block.

With some other cleanup, that could get you something like this:

  def parse_author_common(email, name)
    errors = []

    errors << "No author name specified" if name.blank?

    if email.present?
      external_author = ExternalAuthor.find_or_create_by(email: email)
      errors += external_author.errors.full_messages
    else
      errors << "No author email specified"
    end

    raise Error, errors.join("\n") if errors.present?

    # convert to ASCII and strip out invalid characters (everything except alphanumeric characters, _, @ and -)
    redacted_name = name.to_ascii.gsub(/[^\w[ \-@.]]/u, "")
    if redacted_name.present?
      external_author.names.find_or_create_by(name: redacted_name)
    else
      external_author.default_name
    end
  end

Ideally, it'd also be nice to move some or all of the error-checking code here into the models, but that's almost certainly out of scope.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks a lot for the rewrite. Do you think .save is unneccesary? I thought the method makes persistent changes to the databases so that author names can be retrieved by querying emails later. (Or maybe it's implicitly saved somehow?)

Copy link
Contributor

Choose a reason for hiding this comment

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

find_or_create_by calls create under the hood, and create calls save under the hood. So both the external author and the external author name are getting saved to the database if they don't already exist, because they're both fetched/created with find_or_create_by in the modified code.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Understood. Thanks for explaining.


external_author_name = ExternalAuthorName.find_by(name: name, external_author_id: external_author.id) ||
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Does this comment look accurate? I didn't understand the intention of this part of the code first, and the comment I inserted was my best guess.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Poking @ariana-paris who knows the story parser best

ExternalAuthorName.new(name: name)
external_author.external_author_names << external_author_name
external_author.save
Expand Down
3 changes: 2 additions & 1 deletion app/views/invitations/_invitation.html.erb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,8 @@
<% else %>
<dd>
<%= form_for(invitation) do |f| %>
<p><label for="invitee_email">Enter an email address:</label> <%= f.text_field :invitee_email %></p>
<%= error_messages_for invitation %>
<p><%= f.label :invitee_email, t(".email_address_label") %> <%= f.text_field :invitee_email %></p>
<p><%= hidden_field_tag :user_id, @user.try(:login) %></p>
<p class="submit actions"><%= f.submit %></p>
<% end %>
Expand Down
3 changes: 3 additions & 0 deletions config/locales/views/en.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -428,6 +428,9 @@ en:
home:
donate:
page_title: Donate or Volunteer
invitations:
invitation:
email_address_label: Enter an email address
invite_requests:
invite_request:
date: 'At our current rate, you should receive an invitation on or around: %{date}.'
Expand Down
10 changes: 10 additions & 0 deletions features/other_a/invite_request.feature
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,16 @@ Feature: Invite requests
And I should not see "Sorry, you have no unsent invitations right now."
And I should see "You have 2 open invitations and 0 that have been sent but not yet used."

Scenario: User can see an error after trying to invide an invalid email address
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oops. I think I have some more things to fix anyway, and will fix the typo along them later.


Given I am logged in as "user1"
And "user1" has "1" invitation
And I am on user1's manage invitations page
When I follow the link for "user1" first invite
And I fill in "Enter an email address" with "test@"
And I press "Update Invitation"
Then I should see "Invitee email should look like an email address"

Scenario: User can send out invites they have been granted, and the recipient can sign up

Given invitations are required
Expand Down
6 changes: 6 additions & 0 deletions spec/models/story_parser_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -209,6 +209,12 @@ class StoryParser
@sp.parse_author("", nil, nil)
}.to raise_exception(StoryParser::Error) { |e| expect(e.message).to eq("No author name specified\nNo author email specified") }
end

it "raises an exception when the external author email is invalid" do
expect do
@sp.parse_author("", "Author Name", "not_email")
Copy link

Choose a reason for hiding this comment

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

RSpec/InstanceVariable: Avoid instance variables – use let, a method call, or a local variable (if possible).

end.to raise_exception(StoryParser::Error) { |e| expect(e.message).to eq("Email should look like an email address.") }
end
end

# Let the test get at external sites, but stub out anything containing certain keywords
Expand Down