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

Add Practical Support Mode! #3050

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

Conversation

elimbaum
Copy link
Contributor

I rule and have completed some work on Case Manager that's ready for review!

This pull request makes the following changes:

Screenshot 2023-08-25 at 13 32 46 Screenshot 2023-08-25 at 13 42 06

Currently, there is no valid for marking a patient complete, as there is for fulifilling a pledge; this could be changed.

DRAFT until tests are added.

It relates to the following issue #s:

For reviewer:

  • Adjust the title to explain what it does for the notification email to the listserv.
  • Tag this PR:
    • feature if it contains a feature, fix, or similar. This is anything that contains a user-facing fix in some way, such as frontend changes, alterations to backend behavior, or bug fixes.
    • dependencies if it contains library upgrades or similar. This is anything that upgrades any dependency, such as a Gemfile update or npm package upgrade.
  • If it contains neither, no need to tag this PR.

Copy link
Member

@colinxfleming colinxfleming left a comment

Choose a reason for hiding this comment

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

this is fabulous stuff, thank you @elimbaum ! Very clean effective rails and you've hit the vast majority of the spots where we'd need to do this. I think we'll also want to add this to exports and the accounting panel but both of those can be done in separate PRs if you think this one is too busy. I'll pull down and acid test more extensively once tests are in place if that's cool with you, let me know whether there's anything in particular you'd like my opinion on.

@@ -308,7 +315,7 @@ def validate_time_zone

def validate_yes_or_no
# allow yes or no, to be nice (technically only yes is considered)
options.last =~ /\A(yes|no)\z/i
validate_singleton and options.last =~ /\A(yes|no)\z/i
Copy link
Member

Choose a reason for hiding this comment

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

great rails

@@ -22,7 +22,8 @@ class Config < ApplicationRecord
hide_budget_bar: 'Enter "yes" to hide the budget bar display.',
aggregate_statistics: 'Enter "yes" to show aggregate statistics on the budget bar.',
hide_standard_dropdown_values: 'Enter "yes" to hide standard dropdown values. Only custom options (specified on this page) will be used.',
time_zone: "Time zone to use for displaying dates. Default is Eastern. Valid options are Eastern, Central, Mountain, Pacific, Alaska, Hawaii, Arizona, Indiana (East), or Puerto Rico."
time_zone: "Time zone to use for displaying dates. Default is Eastern. Valid options are Eastern, Central, Mountain, Pacific, Alaska, Hawaii, Arizona, Indiana (East), or Puerto Rico.",
practical_support_mode: 'Enter "yes" to enable practical-support-only mode. Pledges will be disabled.'
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
practical_support_mode: 'Enter "yes" to enable practical-support-only mode. Pledges will be disabled.'
practical_support_mode: 'Enter "yes" to enable practical-support-only mode. Pledges will be disabled, in in favor of marking people complete/incomplete.'

app/views/dashboards/index.html.erb Show resolved Hide resolved
@elimbaum
Copy link
Contributor Author

Thanks for review @colinxfleming! am moving this week (and starting school...) but once I get a dev environment spun back up I'll close this out!

Comment on lines +19 to +34
def mark_complete_button
content_tag :span, class: 'btn btn-primary btn-lg submit-btn btn-block',
aria: { hidden: true },
id: 'submit-pledge-button' do
t('patient.menu.mark_complete')
end
end

def mark_incomplete_button
content_tag :span, class: 'btn btn-warning btn-lg cancel-btn btn-block',
aria: { hidden: true },
id: 'submit-pledge-button' do
t('patient.menu.mark_incomplete')
end
end

Copy link
Contributor Author

Choose a reason for hiding this comment

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

These two functions look extremely similar to the submit pledge button pair (above). Thoughts on condensing, or fine to keep separate?

Copy link
Member

Choose a reason for hiding this comment

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

I'd say keep separate - reasonable for them to diverge a bit here

help_text: I18n.t('patient.status.help.resolved')}
help_text: I18n.t('patient.status.help.resolved')},
completed: { key: I18n.t('patient.status.key.completed'),
help_text: I18n.t('patient.status.help.completed') },
Copy link
Contributor Author

Choose a reason for hiding this comment

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

What do we use help_text for? I wasn't able to find a reference

Copy link
Member

Choose a reason for hiding this comment

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

These show up in the Status tooltip on patient records!

image

Comment on lines +345 to +350

def validate_only_one_practical_support
# TODO - Should not be able to use both practical support configs at the
# same time.
true
end
Copy link
Contributor Author

Choose a reason for hiding this comment

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

After this is done, I'm thinking about opening a separate issue to combine "practical support mode" and "hide practical supports" into a single option — practical supports only, on, off. in the interim, should we have some validation? Weird things will happen if we turn on both options.

How would that best work? We can't compare the actual saved values because on validation, the submitted value has not yet been committed. We could have a conditional check based on the option key?

Copy link
Member

Choose a reason for hiding this comment

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

Let me say a bit more about the thinking here, and let's see if that makes sense with how you're thinking about it. I think there are three possible states:

  • fund is not doing practical support at all (in which case we'd want to hide these options) - unusual but it happens
  • fund is doing pledges AND practical support - most of our clientele
  • fund is doing zero pledges, ONLY practical support - unusual but it happens

So I think you're def right that 'hide practical support on' and 'practical support only on' doesn't make sense, but I think the other cases (hide off, practical only on; hide on, practical only off; hide off, practical only off) are cases I think we'd like to support. Does that match your understanding?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

what do you think about this modal? i don't know if we need any confirmation steps, so maybe we don't even need the modal - just the button?

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 yes to the modal -- an extra UX step to signify 'this is a significant action' is helpful (and helps prevent problems from misclicks)

Comment on lines +1 to 15
<% if Config.practical_support_mode? %>
<% if not patient.marked_complete? %>
<%= link_to mark_complete_button,
mark_complete_path(patient),
class: 'submit-pledge-button', # reuse classes from submit pledge button
aria: { label: t('patient.menu.mark_complete') },
remote: true %>
<% else %>
<%= link_to mark_incomplete_button,
mark_complete_path(patient),
class: 'submit-pledge-button',
aria: { label: t('patient.menu.mark_incomplete') },
remote: true %>
<% end %>
<% else %>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

same question here as with the button controller function - very similar code between in/complete and submit/cancel.

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 reasonable to collapse this one if you like. Up to you!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ergh i hate this but is it ok?

Copy link
Member

Choose a reason for hiding this comment

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

this reads as ok to me, what's the issue?

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.

Add a Practical Support Mode config
2 participants