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 logic for storing and applying multiple field mappings(#2hran7u) #84

Merged
merged 32 commits into from
Nov 22, 2022

Conversation

disha1202
Copy link
Contributor

@disha1202 disha1202 commented Nov 1, 2022

Related Issues

Closes #81

Short Description and Why It's Useful

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

screen-recorder-fri-nov-04-2022-12-57-09.webm

Screenshot from 2022-11-04 15-03-15

IMPORTANT NOTICE - Remember to add changelog entry

Contribution and Currently Important Rules Acceptance

src/views/PurchaseOrder.vue Outdated Show resolved Hide resolved
src/views/PurchaseOrder.vue Outdated Show resolved Hide resolved
src/views/PurchaseOrder.vue Outdated Show resolved Hide resolved
src/views/PurchaseOrder.vue Outdated Show resolved Hide resolved
src/views/PurchaseOrder.vue Outdated Show resolved Hide resolved
@@ -19,6 +19,14 @@ const mutations: MutationTree <UserState> = {
},
[types.USER_INSTANCE_URL_UPDATED] (state, payload) {
state.instanceUrl = payload;
},
[types.USER_FIELD_MAPPINGS_UPDATED] (state, payload) {
const mapping = state.fieldMappings.find((mapping: any) => mapping.name === payload.name );
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 should be part of action

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated the code.

fields: {
orderId: "",
productSku: "",
date: "",
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
date: "",
orderDate: "",

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated the variable name

@@ -12,6 +12,7 @@ const userModule: Module<UserState, RootState> = {
current: null,
currentFacility: {},
instanceUrl: '',
fieldMappings: []
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 have it as object with key as mappingPrefId and value as object having mappingPrefId, mappingPrefName, mappingPrefTypeEnumId and mappingPrefValue

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Created it as an object with key as mapping name and value as mapping fields.

dateField: "",
quantityField: "",
facilityField: "",
fields: {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not fieldMapping?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated the variable name

const mapping = JSON.parse(JSON.stringify(event.detail.value));
this.fields = mapping.fieldMapping;
this.mappingName = mapping.name;
}
this.orderItemsList = this.content.map(item => {
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 should be done with review only

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Made the requested change.

mapFields(event) {
if(event && event.detail.value){
const mapping = JSON.parse(JSON.stringify(event.detail.value));
this.fields = mapping.fieldMapping;
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
this.fields = mapping.fieldMapping;
this.fields = mapping.fieldMapping;

name:'PurchaseOrderDetail'
})
},
mapFields(event) {
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 check if mapping fields exist in csv

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Made the requested change.
Also displayed a toast if any of the fields is missing.

@adityasharma7
Copy link
Contributor

@disha1202

Could you please share some screenshots and videos for the change?

@disha1202
Copy link
Contributor Author

@disha1202

Could you please share some screenshots and videos for the change?

Sure Sir, added in the PR description.

orderItemsList: [],
}
},
methods: {
saveMapping() {
if (this.mappingName) {
const mappingPrefId = Math.floor(Math.random() * 1000);
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 check if the id already exists.

orderItemsList: [],
}
},
methods: {
generateUniqueId(id) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Improve method name, this is not generic method to generate id for any use case

if(event && event.detail.value) {
const fieldMapping = JSON.parse(JSON.stringify(event.detail.value));
for(const field in fieldMapping.mappingPrefValue){
if(!Object.keys(this.content[0]).includes(fieldMapping.mappingPrefValue[field])) fieldMapping.mappingPrefValue[field] = "";
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 store Object.keys(this.content[0]) to reuse

mapFields(event) {
if(event && event.detail.value) {
const fieldMapping = JSON.parse(JSON.stringify(event.detail.value));
for(const field in fieldMapping.mappingPrefValue){
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 reduce() here

const missingFields = Object.values(fieldMapping.mappingPrefValue).filter(field => {
if(!Object.keys(this.content[0]).includes(field)) return field;
});
if(missingFields.length) showToast(translate(`Some of the mapping fields are missing in the CSV: \n ${ missingFields.join(", ") }`))
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 could pass missingFields as variable
Refer this code
https://github.com/hotwax/preorder/blob/main/src/views/product-details.vue#L347

@adityasharma7 adityasharma7 merged commit 71c23cd into hotwax:main Nov 22, 2022
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.

Functionality to save multiple field mappings
3 participants