-
Notifications
You must be signed in to change notification settings - Fork 7k
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
feat(dynamic-branding): Add config options that represent URLs. #15364
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,4 +1,6 @@ | ||
import { IReduxState } from '../app/types'; | ||
import { IStateful } from '../base/app/types'; | ||
import { toState } from '../base/redux/functions'; | ||
import { doGetJSON } from '../base/util/httpUtils'; | ||
import { isInBreakoutRoom } from '../breakout-rooms/functions'; | ||
|
||
|
@@ -9,12 +11,24 @@ import { isInBreakoutRoom } from '../breakout-rooms/functions'; | |
* {@code getState} function, or the redux state itself. | ||
* @returns {boolean} | ||
*/ | ||
export const isSalesforceEnabled = (state: IReduxState) => { | ||
const { salesforceUrl } = state['features/base/config']; | ||
export function isSalesforceEnabled(state: IReduxState) { | ||
const salesforceUrl = getSalesforceUrl(state); | ||
const isBreakoutRoom = isInBreakoutRoom(state); | ||
|
||
return Boolean(salesforceUrl) && !isBreakoutRoom; | ||
}; | ||
} | ||
|
||
/** | ||
* Returns the salesforce integration URL. | ||
* | ||
* @param {Function|Object} stateful - The redux store or {@code getState} function. | ||
* @returns {URL} - The salesforce integration URL. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Retusns a string. |
||
*/ | ||
export function getSalesforceUrl(stateful: IStateful) { | ||
const state = toState(stateful); | ||
|
||
return state['features/dynamic-branding'].salesforceUrl || state['features/base/config'].salesforceUrl; | ||
} | ||
|
||
/** | ||
* Fetches the Salesforce records that were most recently interacted with. | ||
|
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -15,6 +15,7 @@ import { | |||||
import { | ||||||
executeLinkMeetingRequest, | ||||||
getRecentSessionRecords, | ||||||
getSalesforceUrl, | ||||||
getSessionRecordDetails, | ||||||
searchSessionRecords | ||||||
} from './functions'; | ||||||
|
@@ -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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why do we need a default value here? Make |
||||||
const { jwt = '' } = useSelector((state: IReduxState) => state['features/base/jwt']); | ||||||
const showSearchResults = searchTerm && searchTerm.length > 1; | ||||||
const showNoResults = showSearchResults && records.length === 0; | ||||||
|
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?