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

Implemented: support to sync the mappings with server, added support to update and delete field mapping, and improved the UI for the functionality(#85zrhcq56) #159

Merged
merged 25 commits into from
Feb 1, 2023

Conversation

ymaheshwari1
Copy link
Contributor

@ymaheshwari1 ymaheshwari1 commented Jan 27, 2023

Related Issues

Closes #129

Short Description and Why It's Useful

Screenshots of Visual Changes before/after (If There Are Any)

Saved Mapping:

image

Saved Mapping with configuration component:

image

Purchase Order:

image

Create field mapping model:

image

Mobile View (for configuration component):

image

IMPORTANT NOTICE - Remember to add changelog entry

Contribution and Currently Important Rules Acceptance

Additional Information:

  • For now when deleting a mapping the configuration component does not close, but its values gets cleared.

@ymaheshwari1 ymaheshwari1 changed the title Added: service endpoints for create, delete and update mappings(#85zrhcq56) Implemented: support to sync the mappings with server, added support to update and delete field mapping, and improved the UI for the functionality(#85zrhcq56) Jan 27, 2023
@ymaheshwari1 ymaheshwari1 marked this pull request as ready for review January 27, 2023 13:28
Copy link
Contributor

@disha1202 disha1202 left a comment

Choose a reason for hiding this comment

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

LGTM 👍🏻

areAllFieldsSelected() {
return Object.values(this.fieldMapping).every(field => field !== "");
},
generateUniqueMappingPrefId(): any {
Copy link
Contributor

Choose a reason for hiding this comment

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

If we are using the server APIs this might not be needed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sir, as the id is required in the api when creating preference, so generating the id here.
Will create an issue to remove this code when the api will be updated.

return
}
const id = this.generateUniqueMappingPrefId();
return this.store.dispatch("user/createFieldMapping", { id, name: this.mappingName, value: this.fieldMapping }).then(() => {
Copy link
Contributor

Choose a reason for hiding this comment

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

We could use await

<ion-list>
<ion-item>
<ion-label>{{ $t("Order ID") }}</ion-label>
<ion-select interface="popover" v-if="content.length" :placeholder = "$t('Select')" v-model="fieldMapping.orderId">
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we should not have content check here, we should not allow the user to open create mapping modal if there is no content from the parent page

// TODO add any other tasks if need
commit(types.USER_END_SESSION)
resetConfig();
this.dispatch('order/clearOrderList');
// clearning field mappings and current mapping when user logout
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
// clearning field mappings and current mapping when user logout
// clearing field mappings and current mapping when the user logout

// TODO add any other tasks if need
commit(types.USER_END_SESSION)
resetConfig();
this.dispatch('order/clearOrderList');
// clearning field mappings and current mapping when user logout
commit(types.USER_FIELD_MAPPING_UPDATED, {})
dispatch('updateCurrentMapping', '')
Copy link
Contributor

Choose a reason for hiding this comment

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

We should not pass an empty string, even if we want to reset. Think of better approach.

"inputFields": {
"mappingPrefTypeEnumId": "IMPORT_MAPPING_PREF"
},
"fieldList": ["mappingPrefName", "mappingPrefId", "mappingPrefValue"],
Copy link
Contributor

Choose a reason for hiding this comment

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

Add filter by date condition

state.fieldMappings[payload.mappingPrefId] = payload;
},
[types.USER_FIELD_MAPPING_UPDATED] (state, payload) {
if(payload.id) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This should not be done here, we should set the mapping value in mutation and modifications should be done actions

state.fieldMappings = payload;
}
},
[types.USER_FIELD_MAPPING_DELETED] (state, id) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This might not be needed, we could use USER_FIELD_MAPPING_UPDATED

},
areAllFieldsSelected() {
return Object.values(this.fieldMapping).every(field => field !== "");
},
async addFieldMapping() {
const fieldMappingModal = await modalController.create({
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
const fieldMappingModal = await modalController.create({
const createMappingModal = await modalController.create({

async viewMappingConfiguration(mapping: any, id: string) {
this.currentMapping = {
id,
...mapping
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this might not be needed

…rhcq56)

Removed: deleted mapping mutation and used updated mutation for deletion of mappings
Improved: comment and variable name for mapping modal
Moved: logic from mutation to actions
@adityasharma7 adityasharma7 merged commit d035fa5 into hotwax:main Feb 1, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Integrate field mappings feature with API
3 participants