-
Notifications
You must be signed in to change notification settings - Fork 13k
fix: Livechat widget connecting to localhost instead of actual site url #36332
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
Conversation
|
Looks like this PR is not ready to merge, because of the following issues:
Please fix the issues and try again If you have any trouble, please check the PR guidelines |
🦋 Changeset detectedLatest commit: c436301 The changes in this PR will be included in the next version bump. This PR includes changesets to release 36 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop #36332 +/- ##
===========================================
- Coverage 64.91% 64.91% -0.01%
===========================================
Files 3156 3157 +1
Lines 104944 104947 +3
Branches 19947 19947
===========================================
- Hits 68129 68126 -3
- Misses 34132 34135 +3
- Partials 2683 2686 +3
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
|
Can we have a test for this? |
|
It would be nice, but actually apart from some units i dont see much value 🤔 and even units, i dont see much value either cause the function is just 2 lines... but if u want, i can try to add some. |
It’s not something I want to insist on, but since this is a fix, we should include a test to ensure it doesn’t break again in the future. |
90432d4
pierre-lehnen-rc
left a comment
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.
Note: the unit tests on this PR simply check if the function that replaces URLs is replacing with the right value, assuming a valid input.
It doesn't check if the actual file gets replaced properly - but if that were to fail, then the existing tests would catch the problem already.
The existing tests didn't catch this bug because all of our CI and development environments run in the localhost URL which is used by default before the settings are loaded. Maybe we should have at least some testing environments run on a custom URL...
Proposed changes (including videos or screenshots)
Issue(s)
https://rocketchat.atlassian.net/browse/CTZ-272
Steps to test or reproduce
Basically, since we changed how settings are initialized on #34199, the file that "obtained" the server url to preppend to the livechat widget chunks was using the
localhosturl (default) instead of using the url the server was changed to.This fix should allow it to be more reactive and change everytime the server url changes by just refreshing the widget on browser.
Further comments
Fixes #35485