feat(dynamic-branding): Add config options that represent URLs. #15364
Conversation
![pullrequest[bot]](https://avatars.githubusercontent.com/in/3434?s=60&v=4){kind=small}
There was a problem hiding this comment.
🔍 This pull request has been sent to HackerOne's PullRequest review team because our automation detected one or more changes with potential security impact or requires further evaluation. Experts are now being assigned to this review based on relevant expertise and will validate or dismiss any security findings accordingly and post their feedback as comments within this pull request.
Check the status or cancel this secure code review here.
There was a problem hiding this comment.
I have a feeling that rather than changing every place to read the URLs from 2 places, we might be better off using updateConfig (we already have the mechanism in-place) to update the current config with other partial parts.
I realize that this would have been better from the start, but hey, here we are.
|
|
||
| return prejoinConfig?.preCallTestICEUrl; | ||
| return preCallTest?.iceUrl || prejoinConfig?.preCallTestICEUrl; |
There was a problem hiding this comment.
Let's use ?? everywhere. It makes sense because it won't fall trough if the chosen value is false for example.
There was a problem hiding this comment.
My worry is that ?? won't catch the '' value and we tend to return '' in the backend service if the value is missing. Of course we can change the backend service and actually there is an effort in the direction to return only what the client have set but even then I don't think it is a bad thing to guarded for ''.
There was a problem hiding this comment.
If that's the case it might be better to filter the empty strings when we parse the backend response so we don't need to do check everywhere no?
| * Returns the salesforce integration URL. | ||
| * | ||
| * @param {Function|Object} stateful - The redux store or {@code getState} function. | ||
| * @returns {URL} - The salesforce integration URL. |
| @@ -40,7 +41,7 @@ export const useSalesforceLinkDialog = () => { | |||
| const [ hasDetailsErrors, setDetailsErrors ] = useState(false); | |||
| const conference = useSelector(getCurrentConference); | |||
| const sessionId = conference?.getMeetingUniqueId(); | |||
| const { salesforceUrl = '' } = useSelector((state: IReduxState) => state['features/base/config']); | |||
| const salesforceUrl = useSelector((state: IReduxState) => getSalesforceUrl(state) ?? ''); | |||
There was a problem hiding this comment.
| const salesforceUrl = useSelector((state: IReduxState) => getSalesforceUrl(state) ?? ''); | |
| const salesforceUrl = useSelector(getSalesforceUrl); |
There was a problem hiding this comment.
Why do we need a default value here? Make getSalesforceUrl always return a string then?
hristoterezov
force-pushed
the
config_options_dynamic_branding
branch
from
8a39cf0 to
f5162e0
Compare
|
Closed in favor of #15373 |
No description provided.