-
Notifications
You must be signed in to change notification settings - Fork 3
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
Feature/cdctoolbox 462 #107
base: dev
Are you sure you want to change the base?
Conversation
cypress/e2e/dataTest.js
Outdated
statusReason: 'OK', | ||
time: '2025-01-09T15:17:08.545Z', | ||
id: 'a25eed98877d42faaee2a8c0c4ab7d78', | ||
} |
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.
don't you have already a success response defined for previous tests?
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.
Resolved, I've reused a success message that was using on another test.
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.
This object is not used in the code/tests
reducers: { | ||
getServerConfiguration(state, action) { | ||
const option = getConfigurationByKey(state.serverConfigurations, action.payload.selectedOption) | ||
console.log('option', option) |
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.
This console.log was forgotten?
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.
Removed
* License: Apache-2.0 | ||
*/ | ||
|
||
export const isInputFilled = (configurations) => { |
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.
Even not having branches the previous code won't work?
src/services/serverImport/dataFlowTemplates/azureTemplate/azureLiteAccount.js
Outdated
Show resolved
Hide resolved
@@ -0,0 +1,33 @@ | |||
export const scheduleResponse = { |
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.
don't you have a successfull gygia response in previous code?
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.
Resolved, I've reused a previous success gigya response.
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.
This object is not being used, can be removed
*/ | ||
|
||
import { initialState, initialStateWithServerConfigurations } from './dataTest' | ||
import { clearConfigurations, getServerConfiguration, setAccountType } from './serverImportSlice' |
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.
remove setAccountType because it is not used
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.
Removed the setAccountType
src/redux/importAccounts/utils.js
Outdated
} | ||
|
||
const hasMandatoryFieldInSugestion = (structure, parentId, parentNode, value) => { | ||
if ((parentId.length = 3)) { |
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.
This condition is correct? The comparison operator is ==.
src/redux/importAccounts/utils.js
Outdated
parentNode = parentId.slice(0, -1).join('.') | ||
} else if (parentId.length < 3) { | ||
parentNode = parentId.slice(0, -1).join('.') | ||
} |
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.
The if and else do the same, can be changed to
if ((parentId.length <= 3)) {
parentNode = parentId.slice(0, -1).join('.')
}
What does it means parentId.length = 3 or <3?
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.
the parentId.length will be the length of the parentNode id, for example if it's subscriptions.newsletter.commercial.lastUpdatedSubscriptionState, the parentId.length will be 3.
src/redux/importAccounts/utils.js
Outdated
} else if (parentId.length < 3) { | ||
parentNode = parentId.slice(0, -1).join('.') | ||
} | ||
if (parentNode.endsWith('optIn')) { |
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.
Why this special case of ending with optIn?
cypress/e2e/dataTest.js
Outdated
statusReason: 'OK', | ||
time: '2025-01-09T15:17:08.545Z', | ||
id: 'a25eed98877d42faaee2a8c0c4ab7d78', | ||
} |
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.
This object is not used in the code/tests
src/redux/importAccounts/utils.js
Outdated
export const hasMandatoryFieldInSugestion = (structure, parentId, parentNode, value) => { | ||
if (parentId.length <= 3) { | ||
parentNode = parentId.slice(0, -1).join('.') | ||
} else if (parentId.length > 3) { |
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.
} else if (parentId.length > 3) { | |
} else { |
@@ -0,0 +1,33 @@ | |||
export const scheduleResponse = { |
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.
This object is not being used, can be removed
async #replaceAndSetDataflow(dataflow, dataflowBody, dataflowConfig) { | ||
dataflowBody.id = dataflow.id | ||
const replacedBody = this.replaceVariables(dataflowBody, dataflowConfig) | ||
await this.#dataFlow.set(this.#site, this.#dataCenter, replacedBody) |
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.
We need to check the response to see if it was successful
const dataflowBody = accountOption === ServerImport.#ACCOUNT_TYPE_LITE ? importLiteAccountAzure : importFullAccountAzure | ||
const createDataflow = await this.#dataFlow.create(this.#site, this.#dataCenter, dataflowBody) | ||
if (createDataflow.errorCode === 0) { | ||
await this.#searchDataflowOnApiKey(this.#site, createDataflow.id, dataflowBody, dataflowConfig) |
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.
We need to return the response after setting everything on dataflow. The user needs to know if there was en error or not.
|
||
static #ACCOUNT_TYPE_LITE = 'Lite' | ||
|
||
constructor(credentials, site, dataCenter) { |
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.
I would like to not see any code regarding Azure on this class.
Create an interface (abstract class) StorageProvider, with some methods like getFullAccountTemplate and getLiteAccountTemplate and receive it as an argument in the constructor. Then implement this interface in a class AzureStorageProvider where is the specific code for azure.
This ServerImport class should not be necessary to change when we implement other StorageProviders.
No description provided.