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
Publish enterprise website with https:// #12410
Conversation
websites: [enterprise.website].compact, | ||
).tap do |e| | ||
# The model strips the protocol and we need to add it: | ||
e.websites = e.websites.map { |url| "https://#{url}" } |
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.
Could this be done above on line 24?
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.
Yes, but I would need to add a new method because of the conditionals. The website may be blank and we can't just add "https://" to it in that case. Or do you mean adding the map
on that line? Yes, it would probably fit.
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.
Done.
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'm not sure if the website could be an empty string, but it might be worth using compact_blank
to cater for that.
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.
Good point. I'll update that.
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.
👍
Single review is sufficient |
What? Why?
Related to:
We strip all
http://
andhttps://
from enterprise URLs and addhttp://
in the frontend. While we should fix the data model, I added a quick fix to the DFC API to publish the website URL withhttps://
so that it can be used in links straight away.What should we test?
Release notes
Changelog Category (reviewers may add a label for the release notes):
The title of the pull request will be included in the release notes.
Dependencies
Documentation updates