-
-
Notifications
You must be signed in to change notification settings - Fork 275
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
Use https for outlink #5950
Use https for outlink #5950
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have also updated the ORG_URL in CircleCI Context for staging and production to be "www.hotosm.org"
This change resolves the issue at hand without requiring an additional protocol change. This change alone is sufficient to fix the problem, right?
Do you think it'd be important to consider the impact on forks that rely on HTTP rather than HTTPS for their sites? They may face difficulties in updating their site links. Is it accurate to say that this change could potentially affect these forks?
Instead of explicitly specifying the protocol in the code, why not retrieve the complete URL from an environment variable? By doing so, the protocol can be dynamically set based on the environment, providing more flexibility. Thoughts?
You are correct that this change is not required to fix the issue, we just need to push a deployment with the new context envvar. However, it is my personal opinion that we should not be supporting HTTP in general. Your suggestion to make the whole url with the protocol a part of the envvar is well taken, and I will push a change to reflect that. We will need to parse out the domain to make it look pretty, though, or call it "HOTOSM Homepage" or something to that effect. What do you think? |
Could you also revise the
I agree that we'd need to parse out that domain. I like |
96907f1
to
0944fcc
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This test case should have failed, but it did not 🤨.
tasking-manager/frontend/src/components/header/tests/index.test.js
Lines 25 to 28 in fc5f390
if (ORG_URL) { | |
expect(screen.getByText(ORG_URL)).toBeInTheDocument(); | |
expect(screen.getByText(ORG_URL).closest('a')).toHaveAttribute('href', `http://${ORG_URL}`); | |
} |
Previously, the test checked if the `ORG_URL` was present in the document and if it had the expected `http://` prefix. In the updated version, the test checks for the presence of a text containing the organization name `ORG_NAME` from the environment followed by "Website". Additionally, the test verifies that the corresponding anchor element has the correct href attribute equal to ORG_URL.
Kudos, SonarCloud Quality Gate passed! 0 Bugs No Coverage information |
Partially fixes #5925
This PR updates the protocol to https. I have also updated the ORG_URL in CircleCI Context for staging and production to be "www.hotosm.org"
The reason we can't easily setup a redirect is from AWS:
If this is not sufficient, we may be able to setup a redirect, but I would prefer not to, as this will likely change soon with the new hotosm website.