-
Notifications
You must be signed in to change notification settings - Fork 3
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
Remove historic whitespace from strings/text in Contacts data #372
base: master
Are you sure you want to change the base?
Remove historic whitespace from strings/text in Contacts data #372
Conversation
BATCH_SIZE = 1000 | ||
# Process in batches set by the batch size 1000 is default. | ||
Contact.find_in_batches(batch_size: BATCH_SIZE) do |contact| | ||
puts contact.size |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we have 5 100 records we will have:
1000
1000
1000
1000
100
We can just have some ids output like:
12,13,14,15....
to show the progress
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a progress meter in the previous one. I just made this super simple. Maybe too simple. @go8soft what about failures? There should be none, but how do you propose to display those?
This was why in the closed PR I wrote a sql statement to execute so we knew that only records that needed updating are targeted. It should in theory be a small subset of the total number of records (hopefully)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
about failures
a good point, thanks! Can we do save!
instead of save
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@go8soft Actioned as requested
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 👍 minor output log comment, but not mandatory
@userman123 please double check we have backup before exec on prod
@go8soft Good call on the backup prior to execution. I should have put that in the PR description at the top. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please log the id before save
and use save!
instead of save
. Thanks!
6faa4f7
to
4f88e28
Compare
ebfe0aa
to
c54aa1e
Compare
Happy with this - I want to test it locally and on staging before I merge in though - given it's not going to cause any conflicts, there is no rush to merge in. Will set aside some time in the new year. |
8078af3
to
e7fe187
Compare
e7fe187
to
ccd153b
Compare
Trello 86 - whitespace removal - CONTACTS
Simplified version.
Will close PR 366
Tested with same data as previous PR
All data and names here are entirely fictitious and are not in any way related to real people. On the odd chance they were, these were made up out of the authors imagination!
Valid Test Data
Invalid Test Data
Output - local testing
Output - WITH errors
Output - ZERO errors