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

Jump to bottom

Updated load_drains rake task. #198

Closed
wants to merge 6 commits into from
Closed

Updated load_drains rake task. #198

wants to merge 6 commits into from

Conversation

squidarth
Copy link
Contributor

@jszwedko @jasonlally Finally got around to this!

Broader context

@jasonlally and I discussed at the last civic hack night that we want to start syncing the drains in the database w/ the data set of drains on the open data website.

Changes

The main change I had to make here was delete all the drains in the DB that no longer exist in the dataset. We discussed the product implications of this (if you adopted a drain that doesn't exist, it'll disappear), and decided it's not that big a deal.

Fixed a couple other bugs--mainly gsub! actually returns nil, so we weren't properly doing updates.

Let me know what you think!

@squidarth
Updated load_drains rake task.
431115f 
Made the following changes:

* Delete all drains from DB that are no longer in the
dataset.

* Update properties on existing drains
@coveralls
Copy link

Coverage Status

Coverage decreased (-94.6%) to 3.125% when pulling 431115f on squidarth:sid-shanker/update-rake-task into c39d24d on sfbrigade:production.

@jszwedko
Copy link
Member

Awesome, thank you @squidarth! This looks good, but would you mind addressing the rubocop failures?

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.1%) to 97.561% when pulling 66562b7 on squidarth:sid-shanker/update-rake-task into c39d24d on sfbrigade:production.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.1%) to 97.561% when pulling 9ae7bc4 on squidarth:sid-shanker/update-rake-task into c39d24d on sfbrigade:production.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.3%) to 97.967% when pulling 544c8f0 on squidarth:sid-shanker/update-rake-task into c39d24d on sfbrigade:production.

@squidarth
Copy link
Contributor Author

@jszwedko all done!

@jszwedko
Copy link
Member

jszwedko commented Nov 22, 2016 •
edited
Loading

Awesome, thank you! This looks great.

One functional question for @jeanwalshie -- this implementation would delete drains that are no longer in the PUC database. Is that OK?

@squidarth for safety, in case the the PUC database gives an accidentally gives an incorrect response (e.g. an empty one), what would you think about adding a deleted column to the things table and just flagging it? This would allow us to quickly restore if all drains were deleted by accident (restoring the adoption status of the drain). We could add deleted: false to the default scope.

On a related note, when we go to add this as a schedule rake task it'd be cool to send an e-mail if any drains are added/removed.

Thank you again for getting the ball rolling on this!

@alocalsewerhistorian
Copy link

I'd recommend having some flag to show any drains adopted that have been deleted or even leave adopted drains which might be deleted by the update operation. We'd be happy to take a feedback loop (if possible). One idea would be to have a "data steward" contact field in the app and a "data quality" feedback, both for users who find items which are missing or not located properly and for this kind of update. Maybe some email auto-notification loop on update including listing any adopted drains which are deleted by update.

@jasonlally
Copy link
Contributor

@jszwedko
I think a flag is a good idea. We would also want to make sure that if a record does show back up that the update sets the record's deleted attribute back to false.

@alocalsewerhistorian
These are good suggestions for feedback loops. I think for this PR, we should get the syncing working first and then can "hang" new features off of the updated data model. Once we have our first successful sync run we'll have data to work with to develop new features and it'll be less abstract.

I'll create an issue for discussion on feedback loops and maybe we do a design session to work through different alternatives and make sure we're.

@jasonlally jasonlally mentioned this pull request Nov 24, 2016
Open
@squidarth
Copy link
Contributor Author

Cool--so sounds like the main action item for this PR is adding a deleted_at column to Thing and soft delete the drains instead of mass deleting them (I've used this gem in the past for this, but i think it might be overkill for this: https://github.com/rubysherpas/paranoia)

Should I also add an email that notifies the maintainers when drains are added/removed?

@jszwedko
Copy link
Member

@alocalsewerhistorian I agree with @jasonlally that they are a out-of-scope for the initial implementation, but those are great ideas.

@squidarth I think I've used that gem in the past -- I'm fine with using it here too (it's not too heavy and I'm sure they've thought of edge cases that we haven't).

If you could e-mail users marked as admin if any drains are added/removed, I think that would be pretty useful as a stop-gap until dedicated users are designated as part of @alocalsewerhistorian's suggestions. The mailer is already wired up.

@coveralls
Copy link

Coverage Status

Coverage decreased (-94.8%) to 2.954% when pulling bdc3f48 on squidarth:sid-shanker/update-rake-task into c39d24d on sfbrigade:production.

@squidarth
Improved drain deletion.
7aa3911 
* Send email on deleted drain
* Soft-delete the removed rains
@coveralls
Copy link

Coverage Status

Coverage increased (+0.3%) to 98.016% when pulling 7aa3911 on squidarth:sid-shanker/update-rake-task into 30070d5 on sfbrigade:production.

@squidarth
Copy link
Contributor Author

squidarth commented Dec 5, 2016 •
edited
Loading

@jszwedko Updated the PR w/ those two changes (admin email) and soft-deletion. Lemme know what you think & sorry for the late response on this.

My test of the admin email sending is not super comprehensive (I would have change the fixture data to incorporate ALL of the drains created by the test fixtures, which is kinda annoying), but I think it's good enough for our purposes

@chaunceygraves
Copy link

chaunceygraves commented Dec 5, 2016 via email

@chaunceygraves
Copy link

chaunceygraves commented Dec 5, 2016 via email

@jasonlally
Copy link
Contributor

jasonlally commented Dec 9, 2016 •
edited
Loading

@squidarth
Thanks for your work on this! Things got a little busy with the demo night last week for the Brigade, but I'm going to dig back in here.

One question, if the drain is restored in the remote dataset, will the rake task handle restoring it in the Adopt a Drain database? The reason I ask, is that could finish a closed feedback loop:

  1. Drain is deleted from remote dataset
  2. Drain is soft deleted from adopt a drain database
  3. Notice sent to admin
  4. Admin restores drain in remote dataset (through the course of regular edits, or in response to the notice)
  5. Adopt a drain app restores record

@jszwedko @squidarth
We may want to modify the way admins are updated. For example, we may want a slightly different message for when an adopted drain is deleted.

I think it could be as simple as sending a different subject. Admins can create email filters to parse these out.

Subject: An adopted drain has been deleted

Body: Just notifying you that drain with PUC_Maximo_Asset_ID: <%= @thing.city_id %>, and rails id <% @thing.id %>, has been removed from the database.

Thoughts?

@chaunceygraves @alocalsewerhistorian
Once this feature is complete, if we set your accounts to admins on the adopt a drain application, you'll receive an email every time a drain record is deleted.

Per #200 we can tease out other designs for soliciting feedback from users. I just want to keep this PR as tightly defined around syncing and notifying admins.

@jszwedko
Copy link
Member

Regarding the email, depending on the number of drains we expected to be removed / added on a regular basis, I might opt for a "report" style e-mail. Something like:

Subject: Adopt-a-Drain import (n adopted drains removed, n drains added, n removed)
Body: A table of the added and removed drains (highlighting adopted drains that are removed)

If we don't expect this to happen often, than I'm fine with an individual email for each drain.

@squidarth
Copy link
Contributor Author

One question, if the drain is restored in the remote dataset, will the rake task handle restoring it in the Adopt a Drain database? The reason I ask, is that could finish a closed feedback loop:

Ah, I haven't considered this case yet. The way I've done this right now, if we try to re-create a deleted drain, it won't succeed, because there's a unique index on the city_id. There are two options for resolving this:

  1. Reify the deleted drain object
  2. Change the DB index on drains to be a composite index of ['deleted_at', 'city_id'].

Let me know what you think!

Regarding the email, depending on the number of drains we expected to be removed / added on a regular basis, I might opt for a "report" style e-mail.

Yeah, this sounds good, happy to do that too

@alocalsewerhistorian
Copy link

Yes, there is a chance that deleted drains might be re-activated. We've gotten reports from the field about drain locations (or the lack thereof) which are incorrect, probably because they thought they were on a different street. We usually use street view or other backup information (construction drawings are first choice) to substantiate but sometimes that's limited. It is likely to be rare however.

The number of drains deleted on a weekly basis will be small, probably 1-2 max, however sometimes we'll do some focused field study and typically add 15+ and move 5-10 and delete a couple once in a while.

@jszwedko
Copy link
Member

👍 thank you for the additional context @alocalsewerhistorian -- give that expected magnitude of adding/removal of drains, I don't think it is imperative that we do a report style e-mail, but I feel like that would still be the preferable format.

@squidarth if a drain is undeleted, I think we'd want to revive the existing drain since, e.g., we might be undeleting a drain that a user has adopted and would like their adoption status of the drain to persist.

Thank you both!

@squidarth
Copy link
Contributor Author

@squidarth if a drain is undeleted, I think we'd want to revive the existing drain since, e.g., we might be undeleting a drain that a user has adopted and would like their adoption status of the drain to persist.

Cool, sounds good, will get this done by end of the weekend

@coveralls
Copy link

Coverage Status

Coverage decreased (-94.8%) to 2.954% when pulling 3f47eb2 on squidarth:sid-shanker/update-rake-task into 30070d5 on sfbrigade:production.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.3%) to 98.024% when pulling 526a597 on squidarth:sid-shanker/update-rake-task into 5a32412 on sfbrigade:production.

@coveralls
Copy link

Coverage Status

Coverage decreased (-94.9%) to 2.772% when pulling 563f009 on squidarth:sid-shanker/update-rake-task into 5a32412 on sfbrigade:production.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.5%) to 98.141% when pulling b9815c9 on squidarth:sid-shanker/update-rake-task into 5a32412 on sfbrigade:production.

@squidarth
Copy link
Contributor Author

Alright, updated the code to reify deleted drains if they are reintroduced, and to include an report-style email.

Had to do a pretty heavy refactor of load_drains to get the report-style email working.

adopt-a-drain_import_report_-sshanker220_gmail_com-_gmail

Let me know what you think. Really sorry about the delay on this

@coveralls
Copy link

Coverage Status

Coverage increased (+0.5%) to 98.141% when pulling b9815c9 on squidarth:sid-shanker/update-rake-task into 5a32412 on sfbrigade:production.

@squidarth
Copy link
Contributor Author

/cc @jszwedko @jasonlally

jszwedko
jszwedko reviewed Dec 22, 2016
View reviewed changes
app/views/thing_mailer/drain_deleted_notification.html.erb
@@ -0,0 +1,3 @@
Hi Adopt a drain admin!
Copy link
Member

Choose a reason for hiding this comment

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

Is this still used?

@jszwedko
Copy link
Member

@squidarth this looks great, I can see that it required substantial refactoring so kudos for taking that on.

I'll give others a chance to comment, but I think this is ready to go.

@jszwedko jszwedko mentioned this pull request Jan 5, 2017
Closed
@jszwedko
Copy link
Member

jszwedko commented Jan 5, 2017

I'll merge and roll this out this weekend if there are no objections

@squidarth
Copy link
Contributor Author

Sounds good, thanks!

@jasonlally
Copy link
Contributor

@jszwedko - will you handle QA looking at pre- and post- import on a test DB with a copy of the real data? Do you want any help with that or a second set of eyes? I'm around this weekend.

@jszwedko jszwedko mentioned this pull request Jan 9, 2017
Merged
@jasonlally jasonlally mentioned this pull request Feb 10, 2017
Closed
@jszwedko
Copy link
Member

Closing in lieu of #217

@jszwedko jszwedko closed this Apr 24, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jszwedko jszwedko jszwedko left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@squidarth @coveralls @jszwedko @alocalsewerhistorian @jasonlally @chaunceygraves