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 skip flow when actualurl is an empty string #4328

Conversation

whoisurdaddyninja567
Copy link

image

This Test case is from hbbtv.org, which is the HBBTV official site. This case is expect to report a C01 error to the hbbtv test server, but gets none.

First tips

I found out the reason is that atucalurl is an empty string.
If using the old expresseion: if (actualurl && (actualurl !== url)), it will return false and skip the statements in the curly brace. It needs to change to the new expression: if (actualurl !== null && actualurl !== undefined && (actualurl !== url)).

Second tips

The testcase supposes to get the report with serviceLocation, so I append it.

Please have a look, check & verify this Pull Request. Thank you!

@dsilhavy
Copy link
Collaborator

@whoisurdaddyninja567 Thanks for your PR. Following our contribution guidelines, and since this seems to be your first PR, could you please send me a signed copy of dash.js feedback agreement?

Note: If your company is a DASH-IF member or did sign the CLA before you dont need to sign the CLA.

@dsilhavy dsilhavy added this to the 4.7.4 milestone Dec 12, 2023
@dsilhavy dsilhavy self-requested a review December 12, 2023 06:42
@dsilhavy
Copy link
Collaborator

dsilhavy commented Jan 8, 2024

@whoisurdaddyninja567 Did you have the chance to sign the feedback agreement?

@whoisurdaddyninja567
Copy link
Author

Sorry, I'm not ready to sign the feedback agreement.

@dsilhavy
Copy link
Collaborator

Sorry, I'm not ready to sign the feedback agreement.

If you cant sign the FA any time soon I can also offer to close the PR and apply a fix myself.

@dsilhavy
Copy link
Collaborator

As the FA can not be signed at this point I am closing this PR and adding the required check here in #4368

@dsilhavy dsilhavy closed this Jan 23, 2024
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.

2 participants