[#153972] Add the option to remove the cancer center PriceGroup #2347
Conversation
| price_group.update_attribute(:admin_editable, false) | ||
| end | ||
| # This was relevant for updating records when the migration was run back in 2015. | ||
| # The PriceGroup#cancer_center method no longer exists. Leaving this here for documentation. |
There was a problem hiding this comment.
@jhanggi what do you think about this? I could see an argument for leaving it alone or removing it altogether. Let me know what you think.
There was a problem hiding this comment.
I think this is probably a good call to leave it in. The seeds cover the right settings for this attribute, right?
| price_group.update_attribute(:admin_editable, false) | ||
| end | ||
| # This was relevant for updating records when the migration was run back in 2015. | ||
| # The PriceGroup#cancer_center method no longer exists. Leaving this here for documentation. |
There was a problem hiding this comment.
I think this is probably a good call to leave it in. The seeds cover the right settings for this attribute, right?
app/models/price_group.rb
Outdated
| @@ -17,7 +17,7 @@ class PriceGroup < ApplicationRecord | |||
|
|
|||
| default_scope -> { order(is_internal: :desc, display_order: :asc, name: :asc) } | |||
|
|
|||
| before_destroy { throw :abort if global? } | |||
| before_destroy :throw_abort, if: :global? | |||
There was a problem hiding this comment.
It's mainly just a personal preference., but I kind of prefer the block form for one-liners unless the method name aids understanding intention.
What's your opinion for why you pulled this up into a method?
There was a problem hiding this comment.
Ok, I get it now. You made it a method so you could skip it in the rake task.
| task :cancer_center, [:cancer_center_name] => :environment do |_t, args| | ||
| cc_name = args[:cancer_center_name] || "Cancer Center Rate" | ||
| pg = PriceGroup.find_by(name: cc_name) | ||
| abort("No cancer center price group found with name #{cc_name}") unless pg |
There was a problem hiding this comment.
We could do a find_by! and let that exception take care of this for us. This could be better UI, though.
lib/tasks/cleanup.rake
Outdated
| pg = PriceGroup.find_by(name: cc_name) | ||
| abort("No cancer center price group found with name #{cc_name}") unless pg | ||
|
|
||
| PriceGroup.skip_callback(:destroy, :before, :throw_abort) |
There was a problem hiding this comment.
What about instead of skipping the callback, we set pg.global = false. Then the callback should get skipped as well and I would expect the destroy to go through. I think both feel a little hacky, but we are kind of hacking the system here.
There was a problem hiding this comment.
The global? method checks for an associated facility. So the other option would be to set pg.facility = Facility.first or something like that. Do we actually have a cancer center price group on UMass prod? Another possible option would be adding a boolean to settings like cancer_center_enabled and adding some logic to the can_delete? method:
before_destroy { throw :abort unless can_delete? }
def can_delete?
# use !.any? because it uses SQL count(), unlike none?
(is_not_global? || deletable_cancer_denter?) && !order_details.any?
end
def deletable_cancer_center?
!Settings.cancer_center_enabled && name.include?("Cancer"))
end
If we do that, admins can delete the cancer center price group through the UI.
There was a problem hiding this comment.
There definitely is a cancer center group on UMass production. That's why we're trying to get rid of it: to prevent confusion when they go live for real.
If setting the pg.facility = Facility.first works and allows us to delete it without being to disruptive, I'd vote for that. This is a very special case so I'd rather not change internal functionality if we can avoid it.
I'd prefer what you currently have with the skipping the callback over adding a new setting and allowing this to happen on the UI. I'll leave it up to you whether you want to go with the callback skipping or the facility setting.
Release Notes
UMass doesn't have a cancer center Price Group. The seed file will just skip creating a cancer center PriceGroup if no
cancer_centerkey is found insettings.yml. I added a rake task that makes it easier to destroy an existing global PriceGroup if needed. The rake task will fail if the PriceGroup has any associatedorder_details.