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

fix(FEC-10161): add kava analytics url from server response #82

Merged
merged 3 commits into from
Sep 2, 2020

Conversation

Yuvalke
Copy link
Contributor

@Yuvalke Yuvalke commented Sep 2, 2020

Description of the Changes

Issue: kava reports to production env instead QA
Solution: add service protocol for default only.

CheckLists

  • changes have been done against master branch, and PR does not conflict
  • new unit / functional tests have been added (whenever applicable)
  • test are passing in local environment
  • Travis tests are passing (or test results are not worse than on master branch :))
  • Docs have been updated

Issue: kava reports to production env instead QA
Solution: add service path to configured serviceUrl and set the protocol.
@Yuvalke Yuvalke requested a review from a team September 2, 2020 11:38
@Yuvalke Yuvalke self-assigned this Sep 2, 2020
src/kava.js Outdated
@@ -70,6 +71,7 @@ class Kava extends BasePlugin {
super(name, player, config);
this._rateHandler = new KavaRateHandler();
this._model = new KavaModel();
this._serviceUrl = Utils.Http.protocol + '//' + this.config.serviceUrl.replace(/(^\w+:|^)\/\//, '') + SERVICE_PATH;
Copy link
Contributor

Choose a reason for hiding this comment

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

Use template strings since its much more readable

Copy link
Contributor

Choose a reason for hiding this comment

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

why force the protocol? we assume that a working path is given and nobody promises that SERVICE_PATH is required.
Config should allow getting a fully functional URL as the service url can be proxied by another service fr example in an onPrem env.
The default protocol can be left // so we get the one the page has

src/kava.js Outdated
@@ -70,6 +71,7 @@ class Kava extends BasePlugin {
super(name, player, config);
this._rateHandler = new KavaRateHandler();
this._model = new KavaModel();
this._serviceUrl = Utils.Http.protocol + '//' + this.config.serviceUrl.replace(/(^\w+:|^)\/\//, '') + SERVICE_PATH;
Copy link
Contributor

Choose a reason for hiding this comment

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

why force the protocol? we assume that a working path is given and nobody promises that SERVICE_PATH is required.
Config should allow getting a fully functional URL as the service url can be proxied by another service fr example in an onPrem env.
The default protocol can be left // so we get the one the page has

@Yuvalke
Copy link
Contributor Author

Yuvalke commented Sep 2, 2020

@OrenMe it's our Kava analytics you can't set there URL for another analytics service.
for example, the value we get from the server doesn't have the SERVICE_PATH, so it won't work without that if I understand it correctly.

@OrenMe
Copy link
Contributor

OrenMe commented Sep 2, 2020

the plugin config should be explicit - it refers to a url, not partial.
I would want us to leave it like that so if issue arrises it will be as easy as setting a new URL.
This means - leave default as is, and pass the correct url in kaltura player - add the service path there.
If someone needs to set another url, let's say testing purposes or setting a local proxy he will be able to set his own url.

@Yuvalke Yuvalke requested a review from dan-ziv September 2, 2020 12:17
@Yuvalke Yuvalke merged commit 2574aa4 into master Sep 2, 2020
@Yuvalke Yuvalke deleted the FEC-10161 branch September 2, 2020 12:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants