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 development environment a default mailer port configuration #3792

Merged
merged 2 commits into from Jun 2, 2023

Conversation

juankuquintana
Copy link
Contributor

At the moment, when opening a mailer link in development mode it does not include the port so it has to be manually added. This PR addresses such problem by adding a default port in the configuration.

  • A constant created within the Gemcutter module is possible, but decided to minimize changes.
  • Only changed development and test environment to minimize modifications.
  • Used a default port on test environment to test the URL link does contain the port but can be removed and will work, there just won't be a test for it.
  • This PR does not address starting the app with a custom port like rails s -p 3001 as considering such scenario requires code that could be considered "hacky".

Testing

  1. Go to http://localhost:3001/rails/mailers/
  2. Open a mailer that contains a link to the application
  3. Verify that the link contains the current application port

Closes #3728

@simi simi self-requested a review May 15, 2023 22:59
@codecov
Copy link

codecov bot commented May 15, 2023

Codecov Report

Merging #3792 (6a95186) into master (0240dc2) will decrease coverage by 0.01%.
The diff coverage is n/a.

❗ Current head 6a95186 differs from pull request most recent head 45c2494. Consider uploading reports for the commit 45c2494 to get more accurate results

@@            Coverage Diff             @@
##           master    #3792      +/-   ##
==========================================
- Coverage   98.81%   98.81%   -0.01%     
==========================================
  Files         214      213       -1     
  Lines        5250     5239      -11     
==========================================
- Hits         5188     5177      -11     
  Misses         62       62              

see 5 files with indirect coverage changes

@juankuquintana
Copy link
Contributor Author

I've fixed the Rubocop failing scenarios 👍

@juankuquintana juankuquintana force-pushed the add-port-mailer-links branch 2 times, most recently from 314ce0e to 3fa574c Compare May 21, 2023 11:11
@simi simi requested a review from jenshenny May 31, 2023 20:25
@@ -39,6 +39,7 @@
# ActionMailer::Base.deliveries array.
config.action_mailer.delivery_method = :test
config.action_mailer.default_url_options = { host: Gemcutter::HOST,
port: "31337",
Copy link
Member

Choose a reason for hiding this comment

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

Out of curiosity, does 31337 signify anything?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Out of curiosity, does 31337 signify anything?

No, it's a random port number I configured in the test environment 😅 .

@jenshenny jenshenny merged commit 64595b6 into rubygems:master Jun 2, 2023
11 checks passed
@jenshenny
Copy link
Member

Thanks @juankuquintana!

@juankuquintana juankuquintana deleted the add-port-mailer-links branch June 2, 2023 17:02
@rubygems-org-shipit rubygems-org-shipit bot temporarily deployed to staging June 3, 2023 20:35 Inactive
@rubygems-org-shipit rubygems-org-shipit bot temporarily deployed to production June 23, 2023 11:43 Inactive
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.

Include port number in development mailer links
2 participants