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

Change the custom public port ENV variable name to HTTP_PORT #9598

Merged
merged 2 commits into from Aug 12, 2022

Conversation

ahukkanen
Copy link
Contributor

@ahukkanen ahukkanen commented Jul 21, 2022

🎩 What? Why?

As described at #9559, the asset URLs are pointing to the correct host with an incorrect port name. Actually this concerns all URLs generated using the EngineRouter as described at #9558.

This happens when Puma is running separately in a separate port which is forwarded to the public internet using an HTTP server in front which forwards all requests to a specific domain to this port on the local machine.

This causes e.g. Heroku to produce incorrect asset URLs or any other URLs created using EngineRouter.

This fixes the issue by renaming the PORT ENV variable (introduced in #9403) to HTTP_PORT which is not defined at Heroku.

📌 Related Issues

Testing

  • Run the Decidim rails server (i.e. Puma) in a custom port, not 80 or 443
  • Run the Decidim server in a non-development and non-test environment using e.g. RAILS_ENV=production (and configure it accordingly)
  • Add a HTTP server (such as nginx) in front of Decidim and forward the requests coming to port :80 (see configuration example for nginx)
For testing the favicon/asset URLs
For testing the other URLs generated through EngineRouter
  • Using the same configuration as before
  • Start the Rails console using PORT=1234 RAILS_ENV=production bundle exec rails c (make sure spring is restarted if you're using it, run spring stop before starting the console)
  • Run Decidim::ResourceLocatorPresenter.new(Decidim::Proposals::Proposal.first).url
  • See that the URL is generated correctly without the port 1234 and the issue described at Notifications link doesn't work on Push Notifications #9558 no longer exists

Or alternatively, find one of these routes through the UI, these should be all over, e.g. in notifications, emails, exports, etc.

@ahukkanen ahukkanen added this to Pending review from Product in Maintainers via automation Jul 21, 2022
@ahukkanen ahukkanen moved this from Pending review from Product to To Review by Core in Maintainers Jul 21, 2022
@ahukkanen ahukkanen added module: core type: fix PRs that implement a fix for a bug labels Jul 22, 2022
@verarojman verarojman moved this from To Review by Core to In Review / Blocked / Waiting for feedback in Maintainers Aug 4, 2022
Copy link
Contributor

@verarojman verarojman left a comment

Choose a reason for hiding this comment

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

LGTM 👌🏾

@verarojman verarojman merged commit ab994a5 into decidim:develop Aug 12, 2022
Maintainers automation moved this from In Review / Blocked / Waiting for feedback to Done Aug 12, 2022
@ahukkanen ahukkanen deleted the fix/9559 branch August 12, 2022 10:32
entantoencuanto added a commit that referenced this pull request Aug 25, 2022
* develop:
  Redesign: pages (#9457)
  Add "no-reply" notification at the email footers (#9668)
  merge layout center into one-col, allowing 3 variations (#9755)
  Fix form error overlap with character counter in the admin panel (#9683)
  Change the custom public port ENV variable name to HTTP_PORT (#9598)
  Redesign: login & signup (#9455)
  Fix redundant notification on comments with linked proposals (#9676)
eliegaboriau pushed a commit to eliegaboriau/decidim that referenced this pull request Oct 25, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
module: core type: fix PRs that implement a fix for a bug
Projects
Development

Successfully merging this pull request may close these issues.

No favicon/app icon in the Push Notification Notifications link doesn't work on Push Notifications
2 participants