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

potpotkettle · 2023-07-06T03:29:27Z

Pull Request Checklist

Have you read "How to write the perfect pull request"?
Have you read the contributing guidelines?
Have you added tests for any changed functionality?
Have you added the Jira issue number
as the first thing in your pull request title (e.g. AO3-1234 Fix thing)
Do you have fewer than 5 pull requests already open? If not, please wait
until they are reviewed and merged before creating new pull requests.

Issue

https://otwarchive.atlassian.net/browse/AO3-3997

Purpose

Fix additional issues found in AO3-3997, after #4390

Testing Instructions

As a user with the ability to import works, try importing with an ill-formatted email address. ("Archiving for others") It should show an error message (where no error was shown and importing succeeded).
As a user with the ability to send invitations, try sending an invitation to an ill-formatted email address. It should show an error message (where it was missing previously).

References

Comments in the Jira link above, particularly https://otwarchive.atlassian.net/browse/AO3-3997?focusedCommentId=360179 .

Credit

Potpotkettle (they/them)

hound · 2023-07-06T03:41:12Z

spec/models/story_parser_spec.rb

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


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

potpotkettle · 2023-07-06T08:41:31Z

app/models/story_parser.rb

+
+      # 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?
+
+        external_author_name = ExternalAuthorName.find_by(name: name, external_author_id: external_author.id) ||


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.

Poking @ariana-paris who knows the story parser best

tickinginstant · 2023-07-07T00:55:17Z

app/models/story_parser.rb

      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?


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.

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.

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.)

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.

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.

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?)

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.

Understood. Thanks for explaining.

sarken · 2023-07-07T02:40:05Z

app/views/invitations/_invitation.html.erb

@@ -10,8 +10,9 @@
    <dd><%= invitation.invitee_email %></dd>
    <% else %>
    <dd>
+      <%= error_messages_for @invitation %>


I poked around a bit to see where we usually put this, and it looks like most of the time it's either after the page heading (which we don't have here) or the first or second thing inside the form. Can we move this inside the form_for?

You might also be able to use invitation rather than @invitation here.

sarken · 2023-07-07T02:42:12Z

app/views/invitations/_invitation.html.erb

      <%= form_for(invitation) do |f| %>
-        <p><label for="invitee_email">Enter an email address:</label> <%= f.text_field :invitee_email %></p>
+        <p><%= label_tag "invitation[invitee_email]", t('.email_address_label') %>: <%= f.text_field :invitee_email %></p>


f.label :invitee_email should work here, and it would be more consistent since we're using f.text_field :invitee_email. If you could also use double quotes around the translation key name and remove the colon (we've been slowly eliminating them on our form labels, plus taking it out of the actual label means it doesn't get the red highlight when there's an error), that would be great!

Just to make sure, do you want the "Enter an email address" label to be in red as well, like this?

Yup, that's fine!

sarken · 2023-07-07T02:46:32Z

features/other_a/invite_request.feature

+      And an admin grants the request
+      And I try to invite a friend from my user page
+    When all emails have been delivered
+      And I fill in "invitation[invitee_email]" with "test@"


When possible, we prefer the user-facing label text.

sarken · 2023-07-07T02:53:14Z

features/other_a/invite_request.feature

@@ -62,6 +62,18 @@ 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 fail to send an invitation to an invalid email address
+
+    Given invitations are required


Looking at Scenario: A user can manage their invitations below, I think we can simplify this:

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 etc

potpotkettle · 2023-07-07T11:15:42Z

features/other_a/invite_request.feature

@@ -62,16 +62,14 @@ 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 fail to send an invitation to an invalid email address
+  Scenario: User can see an error after trying to invide an invalid email address


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

hound · 2023-07-08T06:36:45Z

spec/models/story_parser_spec.rb

+
+    it "raises an exception when the external author name is invalid and email is blank" do
+      expect do
+        @sp.parse_author("", "!!!!", "")


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

hound · 2023-07-08T06:36:45Z

spec/models/story_parser_spec.rb

+
+    it "raises an exception when the external author name is blank and email is invalid" do
+      expect do
+        @sp.parse_author("", "", "not_email")


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

hound · 2023-07-08T06:36:45Z

spec/models/story_parser_spec.rb

+
+    it "raises an exception when the external author name and email are invalid" do
+      expect do
+        @sp.parse_author("", "!!!!", "not_email")


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

hound · 2023-07-08T06:36:46Z

spec/models/story_parser_spec.rb

+    end
+
+    it "ignores invalid letters in the external author name" do
+      results = @sp.parse_author("", "Author!! Name!!", "[email protected]")


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

hound · 2023-07-08T06:36:46Z

spec/models/story_parser_spec.rb

+    end
+
+    it "ignores the external author name when it is invalid" do
+      results = @sp.parse_author("", "!!!!", "[email protected]")


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

hound · 2023-07-08T06:36:46Z

spec/models/story_parser_spec.rb

+    it "gives the same external author object for the same email" do
+      res1 = @sp.parse_author("", "Author Name", "[email protected]")
+      res2 = @sp.parse_author("", "Author Name Second", "[email protected]")
+      res3 = @sp.parse_author("", "Author!! Name!!", "[email protected]")


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

hound · 2023-07-08T06:36:46Z

spec/models/story_parser_spec.rb

+
+    it "gives the same external author object for the same email" do
+      res1 = @sp.parse_author("", "Author Name", "[email protected]")
+      res2 = @sp.parse_author("", "Author Name Second", "[email protected]")


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

hound · 2023-07-08T06:36:46Z

spec/models/story_parser_spec.rb

+    end
+
+    it "gives the same external author object for the same email" do
+      res1 = @sp.parse_author("", "Author Name", "[email protected]")


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

I think it's possible to replace the instance variable with let. Not sure if it's a good idea to do it now, because that would be an unrelated refactoring that would touch most of the methods here.

github-actions bot added the Awaiting Review label Jul 6, 2023

potpotkettle force-pushed the fix-3997 branch from 1f47f26 to 9764d5d Compare July 6, 2023 03:41

hound bot reviewed Jul 6, 2023

View reviewed changes

sarken added the Priority: High - Broken on Test Merge immediately after approval label Jul 6, 2023

AO3-3997 Catch email validation failure in import

112ae8c

potpotkettle force-pushed the fix-3997 branch from 9764d5d to 112ae8c Compare July 6, 2023 08:22

potpotkettle commented Jul 6, 2023

View reviewed changes

AO3-3997 Add missing error message in invitation page

2de6037

potpotkettle force-pushed the fix-3997 branch from 58a0a07 to 2de6037 Compare July 6, 2023 09:01

potpotkettle marked this pull request as ready for review July 6, 2023 09:22

tickinginstant reviewed Jul 7, 2023

View reviewed changes

sarken reviewed Jul 7, 2023

View reviewed changes

sarken added Reviewed: Action Needed and removed Awaiting Review labels Jul 7, 2023

github-actions bot added Coder Has Actioned Review and removed Reviewed: Action Needed labels Jul 7, 2023

potpotkettle force-pushed the fix-3997 branch from 03f26d8 to 30bcfdd Compare July 7, 2023 04:09

AO3-3997 more fixes for invite

7885c14

potpotkettle force-pushed the fix-3997 branch from 30bcfdd to 7885c14 Compare July 7, 2023 08:04

potpotkettle commented Jul 7, 2023

View reviewed changes

potpotkettle changed the title ~~AO3-3997 Fix for #4390~~ AO3-3997 Fix for email address validation #4390 Jul 7, 2023

AO3-3997 more fixes for import

fa60bde

hound bot reviewed Jul 8, 2023

View reviewed changes

AO3-3997 more fixes for import (2)

04c0878

tickinginstant approved these changes Jul 11, 2023

View reviewed changes

ariana-paris approved these changes Jul 11, 2023

View reviewed changes

ariana-paris merged commit ff9f334 into otwcode:master Jul 11, 2023

potpotkettle deleted the fix-3997 branch July 11, 2023 09:33

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

potpotkettle commented Jul 6, 2023 •

edited

Loading

hound bot Jul 6, 2023

potpotkettle Jul 6, 2023

sarken Jul 6, 2023

tickinginstant Jul 7, 2023

potpotkettle Jul 7, 2023 •

edited

Loading

tickinginstant Jul 8, 2023

potpotkettle Jul 8, 2023

tickinginstant Jul 8, 2023

potpotkettle Jul 9, 2023

tickinginstant Jul 9, 2023

potpotkettle Jul 10, 2023

sarken Jul 7, 2023

sarken Jul 7, 2023

potpotkettle Jul 7, 2023

sarken Jul 7, 2023

sarken Jul 7, 2023

sarken Jul 7, 2023

potpotkettle Jul 7, 2023

hound bot Jul 8, 2023

hound bot Jul 8, 2023

hound bot Jul 8, 2023

hound bot Jul 8, 2023

hound bot Jul 8, 2023

hound bot Jul 8, 2023

hound bot Jul 8, 2023

hound bot Jul 8, 2023

potpotkettle Jul 8, 2023

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

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

Conversation

potpotkettle commented Jul 6, 2023 • edited Loading

Pull Request Checklist

Issue

Purpose

Testing Instructions

References

Credit

hound bot Jul 6, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

potpotkettle Jul 7, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hound bot Jul 8, 2023

Choose a reason for hiding this comment

hound bot Jul 8, 2023

Choose a reason for hiding this comment

hound bot Jul 8, 2023

Choose a reason for hiding this comment

hound bot Jul 8, 2023

Choose a reason for hiding this comment

hound bot Jul 8, 2023

Choose a reason for hiding this comment

hound bot Jul 8, 2023

Choose a reason for hiding this comment

hound bot Jul 8, 2023

Choose a reason for hiding this comment

hound bot Jul 8, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

potpotkettle commented Jul 6, 2023 •

edited

Loading

potpotkettle Jul 7, 2023 •

edited

Loading