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

[OSU-114] Setup sanger enabled service #4925

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

joaquinco
Copy link
Collaborator

Notes

  • Add sanger_sequencing_enabled to product
  • Allow admins to switch that flag on for Services
  • When the flag is switched on an ExternalService pointing to Sanger is created if it does not exists

Screenshots

Captura de pantalla 2025-01-15 a la(s) 09 50 05

@joaquinco joaquinco marked this pull request as ready for review January 15, 2025 13:18
@joaquinco joaquinco changed the title Osu 114/setup sanger enabled service [OSU-114] Setup sanger enabled service Jan 15, 2025
Copy link
Collaborator

@LeticiaErrandonea LeticiaErrandonea left a comment

Choose a reason for hiding this comment

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

Left a few comments/questions

if sanger_external_service.blank?
Service.transaction do
external_service = UrlService.find_or_create_by(
location: new_sanger_sequencing_submission_path
Copy link
Collaborator

Choose a reason for hiding this comment

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

While this should still work, I think it's better to use the url, so the users see the complete URL (including domain)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeap this work. The only reason I can think of for having the domain for internal routes is end users being accustomed to see it. I'll change it anyway

Comment on lines 218 to 219
sanger_sequencing_enabled: Sanger has been enabled, make sure the Order Form is inactive
sanger_sequencing_disabled: Sanger has been disabled, make sure the Order Form is active
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: should we add the period at the end of the messages?

@@ -62,4 +62,34 @@
context "when billing mode is Skip Review" do
include_examples "creates a product with billing mode", "service", "Skip Review"
end

context "when sanger enable si checked" do
Copy link
Collaborator

Choose a reason for hiding this comment

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

typo: si -> is

spec/system/admin/editing_a_service_spec.rb Show resolved Hide resolved
Copy link
Collaborator

@LeticiaErrandonea LeticiaErrandonea left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for the changes!

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.

3 participants