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.

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

Feature/add time to medication alerts #1946

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

marthaerm
Copy link

Description

First approach to add time support for medication alerts

More Details

I created the db/migrate/20201229031222_add_time_to_medication_daily_reminders.rb migration 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:

  • Refactor the way to add the 6 inputs and checkbox for each day
  • Add a default value to the time inputs
  • Store the time on the database on the correspondent field
  • Modify the email sender to send it on the time selected
  • Create a migration to add a time for each day on the current data (and decide if to add a default value or let the user chose it).

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

image


Reviewing this pull request? Check out our Code Review Practices guide if you haven't already!

@welcome
Copy link

welcome bot commented Dec 29, 2020

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 julianguyen self-requested a review January 15, 2021 00:49
Copy link
Member

@julianguyen julianguyen left a comment

Choose a reason for hiding this comment

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

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.

@@ -87,15 +87,15 @@ def hidden_props(field, value)
def medication_weekly_dosage
{
type: 'checkboxGroup',
checkboxes: days_checkbox,
# checkboxes: days_checkbox,
Copy link
Member

Choose a reason for hiding this comment

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

Why is this being commented out?

@@ -0,0 +1,11 @@
class AddTimeToMedicationDailyReminders < ActiveRecord::Migration[6.0]
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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
Copy link
Member

Choose a reason for hiding this comment

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

Looks like a typo, with the word saturday.

@marthaerm
Copy link
Author

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 :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants