Feature/add time to medication alerts#1946
Conversation
|
Thank you for opening this pull request with us! Be sure to follow our Pull Request Practices. Let us know if you have any questions on Slack. |
julianguyen
left a comment
There was a problem hiding this comment.
Thanks for taking this on Martha 🎉 This is a good start! Let me know if you need any help or want to do any pairing.
For the Codeclimate issues you're getting, you can also run rubocop -a in your dev environment to catch and auto-fix some of those errors.
| { | ||
| type: 'checkboxGroup', | ||
| checkboxes: days_checkbox, | ||
| # checkboxes: days_checkbox, |
There was a problem hiding this comment.
Why is this being commented out?
| @@ -0,0 +1,11 @@ | |||
| class AddTimeToMedicationDailyReminders < ActiveRecord::Migration[6.0] | |||
There was a problem hiding this comment.
You'll have to run a migration (rake db:migrate) in order for the database schema to be updated (https://github.com/ifmeorg/ifme/blob/main/db/schema.rb). This why when you run our RSpec tests you get the following error:
ActiveRecord::PendingMigrationError:
Migrations are pending. To resolve this issue, run:
rails db:migrate RAILS_ENV=test
# ./spec/spec_helper.rb:24:in `<top (required)>'
No examples found.
There was a problem hiding this comment.
I think the changes you made to app/helpers/medications_form_helper.rb will result in failing tests that need to be updated. You should run rspec in your dev environment!
| add_column :medications, :wednesday, :time | ||
| add_column :medications, :thursday, :time | ||
| add_column :medications, :friday, :time | ||
| add_column :medications, :staurday, :time |
There was a problem hiding this comment.
Looks like a typo, with the word saturday.
|
Thanks for the comments @julianguyen ! I'm a bit busier now, so I'll not be able to make the changes quickly, I'll let you know as soon as I can work on them, but If meanwhile, someone else wants to work on them, t also would be great! Thank you so much :) |
Description
First approach to add time support for medication alerts
More Details
I created the
db/migrate/20201229031222_add_time_to_medication_daily_reminders.rbmigration that adds a column for each day to add the time for daily reminders. Also, the frontend time inputs were added after the daily checkbox.The related issue is not completely solved yet but I would like to have a review on the approach.
I believe the next steps to close the issue are:
I would like to keep working on this, I'm not sure if I'll be able to finish it but, I'll keep you updated
Corresponding Issue
#1601.
Screenshots
Reviewing this pull request? Check out our Code Review Practices guide if you haven't already!