search

devict / votelocal

A web app for receiving local voting information via SMS
15 stars 16 forks source link

Allow for SMS or Twitter to be selected as targets for messages (#62) #65

Closed JustinDDavis closed 4 years ago

JustinDDavis commented 5 years ago

Working to implement #62

I needed to create a migration to include two new database columns for tracking the SMS or Twitter selections. I then modified the new and update controller to map to the new column, but included a validation to catch that at least one checkbox is selected. I don't 100% like the way the error message comes across, but would like some feedback if that needs to be implemented within this PR.

I also modified the SendScheduledMessages command so it will respect the target selection as it loops through the messages that need sent.

This enhancement was a big learning for me, so any guidance on changes/additions/removals would be appreciated.

sethetter commented 4 years ago

@JustinDDavis I feel terrible for not getting around to reviewing this sooner! We're ramping things back up right now and I plan on checking this out by the weekend. Let me know if you think you can find time to resolve the conflicts, or if you'd prefer to hand off the PR.

sethetter commented 4 years ago

@JustinDDavis I went ahead and fixed the conflicts and got this merged. Works like a charm! Thank you for the contribution ❤️