Skip to content
This repository has been archived by the owner on Mar 27, 2022. It is now read-only.

Invalid birthdates should be set to nil rather than raise exceptions #679

Merged
merged 1 commit into from Jul 9, 2017

Conversation

AndyGauge
Copy link
Contributor

…during Import

For #678

@AndyGauge
Copy link
Contributor Author

Oh, I didn't have a CSV to test this with so I'm not sure it actually works 😊

@seven1m
Copy link
Owner

seven1m commented Jul 6, 2017

@AndyGauge current functionality is to show an error to the user. I don't get an exception when I use a bad date.

screen shot 2017-07-05 at 10 27 08 pm

Are you seeing actual Ruby exceptions?

@seven1m
Copy link
Owner

seven1m commented Jul 6, 2017

SORRY! I just now noticed you linked to #678. This is specifically to catch the " / / " issue then? I wonder if we could catch that specific case a little more precisely, perhaps by using our String#digits_only method, like this:

string = nil if string.digits_only.nil?

That way, if there are digits, we can show an error to the user if the date is invalid, but if there are only slashes (or dashes or other punctuation), then we can treat it like a blank value.

Thoughts?

@AndyGauge
Copy link
Contributor Author

That's a great idea, I'll refactor it tomorrow.

@AndyGauge
Copy link
Contributor Author

Sorry about the word exception. I didn't have a CSV to test this feature with. When I was in the console I got an exception (parsing with Time, not using the built in features) and it stuck in my head. Funny story, I was actually going to work on 557, but after reading the issues I accidentally did this one. 😃

@AndyGauge
Copy link
Contributor Author

Just noticed share_birthday is a boolean, not a date. I removed those and rebased. I think this is good to test with an actual CSV to ensure functionality.

@seven1m
Copy link
Owner

seven1m commented Jul 7, 2017

@AndyGauge I think this looks good. Can you make it so this applies to custom date fields also? If you don't have time, I'll just merge and work on that separately. Thanks.

@AndyGauge
Copy link
Contributor Author

I've tried finding the custom dates in the UI but haven't stumbled upon it. Is it something only in the backend? The only custom fields I find are the fields1, fields2, ... but I shouldn't apply the same logic to them because they may be alphabetic characters. Am I missing something?

@seven1m
Copy link
Owner

seven1m commented Jul 9, 2017

@AndyGauge let me play around with this and get back with you. I might have misunderstood the issue, so I need to test it.

I'll go ahead and merge this as-is. We can make another commit if there are issues with custom fields. Thanks!

@seven1m seven1m merged commit 1fe2d9c into seven1m:master Jul 9, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants