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

Use https for outlink #5950

Merged
merged 4 commits into from
Jul 12, 2023
Merged

Use https for outlink #5950

merged 4 commits into from
Jul 12, 2023

Conversation

dakotabenjamin
Copy link
Member

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:

Unlike a CNAME record, you can create an alias record at the top node of a DNS namespace, also known as the zone apex. For example, if you register the DNS name example.com, the zone apex is example.com. You can't create a CNAME record for example.com, but you can create an alias record for example.com that routes traffic to www.example.com (as long as the record type for www.example.com is not of type CNAME).

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.

Copy link
Contributor

@HelNershingThapa HelNershingThapa left a 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?

@dakotabenjamin
Copy link
Member Author

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?

@HelNershingThapa
Copy link
Contributor

we just need to push a deployment with the new context envvar.

Could you also revise the ORG_PRIVACY_POLICY_URL when you do that, please? The privacy policy links would also need some amendments. I can make and push those changes to update the privacy policy URLs in the frontend to use HTTPS consistently in this PR.

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?

I agree that we'd need to parse out that domain. I like hotosm.org being displayed just like it is now.

@dakotabenjamin
Copy link
Member Author

New change looks like this:
image

Copy link
Contributor

@HelNershingThapa HelNershingThapa left a 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 🤨.

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.
@sonarcloud
Copy link

sonarcloud bot commented Jul 10, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@HelNershingThapa HelNershingThapa merged commit b8e73b4 into develop Jul 12, 2023
8 checks passed
@HelNershingThapa HelNershingThapa deleted the fix/org-url branch July 12, 2023 13:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[BUG] Broken website link of HOT
2 participants