Re-enabled bulk csv upload #849
Conversation
Summary of ChangesHello @Banibrata-Manna, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request re-enables the bulk CSV upload feature, providing users with a streamlined way to import inventory cycle counts. It introduces a dedicated user interface for uploading CSV files, mapping their columns to predefined application fields, and managing these mappings. The changes also include the necessary backend service integrations and state management updates to process the uploaded data and display the status of recent uploads. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request re-enables the bulk CSV upload feature, which is a great addition. However, the current implementation has several critical issues that must be addressed before merging. There is significant code duplication in a new component, which also seems to be incorrectly implemented for its purpose. I've also found a couple of bugs that will lead to runtime errors, related to an incorrect use of this in a Vue setup script and the possibility of committing undefined to the Vuex store. Additionally, there are several medium-severity issues concerning code quality, maintainability, and user experience that I've highlighted with suggestions for improvement.
| //Todo: Generating unique identifiers as we are currently storing in local storage. Need to remove it as we will be storing data on server. | ||
| function generateUniqueMappingPrefId() { | ||
| const id = Math.floor(Math.random() * 1000); | ||
| return !fieldMappings.value[id] ? id : this.generateUniqueMappingPrefId(); |
There was a problem hiding this comment.
The recursive call this.generateUniqueMappingPrefId() will cause a runtime error because this is undefined within the <script setup> context. You should make a direct recursive call to the function instead.
return !fieldMappings.value[id] ? id : generateUniqueMappingPrefId();
| <template> | ||
| <ion-header> | ||
| <ion-toolbar> | ||
| <ion-buttons slot="start"> | ||
| <ion-button @click="closeModal"> | ||
| <ion-icon :icon="close" /> | ||
| </ion-button> | ||
| </ion-buttons> | ||
| <ion-title>{{ translate("CSV Mapping") }}</ion-title> | ||
| </ion-toolbar> | ||
| </ion-header> | ||
|
|
||
| <ion-item> | ||
| <ion-input :label="translate('Mapping name')" :placeholder="translate('Field mapping name')" v-model="mappingName" /> | ||
| </ion-item> | ||
|
|
||
| <ion-content> | ||
| <div> | ||
| <ion-list> | ||
| <ion-item-divider> | ||
| <ion-label>{{ translate("Required") }} </ion-label> | ||
| </ion-item-divider> | ||
| <ion-item :key="field" v-for="(fieldValues, field) in getFields(fields, true)"> | ||
| <ion-select :label="translate(fieldValues.label)" interface="popover" :placeholder = "translate('Select')" v-model="fieldMapping[field]"> | ||
| <ion-select-option :key="index" v-for="(prop, index) in fileColumns">{{ prop }}</ion-select-option> | ||
| </ion-select> | ||
| </ion-item> | ||
| <ion-item-divider> | ||
| <ion-label>{{ translate("Optional") }} </ion-label> | ||
| </ion-item-divider> | ||
| <ion-item :key="field" v-for="(fieldValues, field) in getFields(fields, false)"> | ||
| <ion-select :label="translate(fieldValues.label)" interface="popover" :placeholder = "translate('Select')" v-model="fieldMapping[field]"> | ||
| <ion-select-option :key="index" v-for="(prop, index) in fileColumns">{{ prop }}</ion-select-option> | ||
| </ion-select> | ||
| </ion-item> | ||
| </ion-list> | ||
| </div> | ||
| <ion-fab vertical="bottom" horizontal="end" slot="fixed"> | ||
| <ion-fab-button @click="saveMapping"> | ||
| <ion-icon :icon="saveOutline" /> | ||
| </ion-fab-button> | ||
| </ion-fab> | ||
| </ion-content> | ||
| </template> | ||
|
|
||
| <script setup> | ||
| import { | ||
| IonButtons, | ||
| IonButton, | ||
| IonContent, | ||
| IonFab, | ||
| IonFabButton, | ||
| IonHeader, | ||
| IonIcon, | ||
| IonInput, | ||
| IonItem, | ||
| IonItemDivider, | ||
| IonLabel, | ||
| IonSelect, | ||
| IonSelectOption, | ||
| IonTitle, | ||
| IonToolbar, | ||
| IonList, | ||
| modalController | ||
| } from '@ionic/vue'; | ||
| import { close, save, saveOutline } from "ionicons/icons"; | ||
| import { computed, defineProps, onMounted, ref } from "vue"; | ||
| import { useStore } from "vuex"; | ||
| import { showToast } from '@/utils'; | ||
| import { translate } from "@hotwax/dxp-components"; | ||
| const store = useStore(); | ||
| const props = defineProps(["content", "seletedFieldMapping", "mappingType"]) | ||
| const fieldMappings = computed(() => store.getters["user/getFieldMappings"]) | ||
| let mappingName = ref(null) | ||
| let fieldMapping = ref ({}) | ||
| let fileColumns = ref([]) | ||
| let identificationTypeId = ref('SKU') | ||
| const fields = process.env["VUE_APP_MAPPING_INVCOUNT"] ? JSON.parse(process.env["VUE_APP_MAPPING_INVCOUNT"]) : {} | ||
| onMounted(() => { | ||
| fieldMapping.value = { ...props.seletedFieldMapping } | ||
| fileColumns.value = Object.keys(props.content[0]); | ||
| }) | ||
| function getFields(fields, required = true) { | ||
| return Object.keys(fields).reduce((result, key) => { | ||
| if (fields[key].required === required) { | ||
| result[key] = fields[key]; | ||
| } | ||
| return result; | ||
| }, {}); | ||
| } | ||
| function closeModal() { | ||
| modalController.dismiss({ dismissed: true }); | ||
| } | ||
| async function saveMapping() { | ||
| if(!mappingName.value || !mappingName.value.trim()) { | ||
| showToast(translate("Enter mapping name")); | ||
| return | ||
| } | ||
| if (!areAllFieldsSelected()) { | ||
| showToast(translate("Map all required fields")); | ||
| return | ||
| } | ||
| const id = generateUniqueMappingPrefId(); | ||
| await store.dispatch("user/createFieldMapping", { id, name: mappingName.value, value: fieldMapping.value, mappingType: props.mappingType }) | ||
| closeModal(); | ||
| } | ||
| function areAllFieldsSelected() { | ||
| const requiredFields = Object.keys(getFields(fields, true)); | ||
| const selectedFields = Object.keys(fieldMapping.value).filter(key => fieldMapping.value[key] !== '') | ||
| return requiredFields.every(field => selectedFields.includes(field)); | ||
| } | ||
| //Todo: Generating unique identifiers as we are currently storing in local storage. Need to remove it as we will be storing data on server. | ||
| function generateUniqueMappingPrefId() { | ||
| const id = Math.floor(Math.random() * 1000); | ||
| return !fieldMappings.value[id] ? id : this.generateUniqueMappingPrefId(); | ||
| } | ||
| </script> | ||
| <style scoped> | ||
| ion-content { | ||
| --padding-bottom: 80px; | ||
| } | ||
| </style> No newline at end of file |
There was a problem hiding this comment.
This component is an exact copy of CreateMappingModal.vue, which introduces significant code duplication. Furthermore, it's being used as a popover for actions on uploaded files in BulkUpload.vue, but its implementation is for creating a field mapping. The props it receives in BulkUpload.vue (systemMessage, fileName) do not match the props it declares (content, seletedFieldMapping, mappingType), which will lead to incorrect behavior. This component needs to be implemented correctly for its intended purpose, or removed if CreateMappingModal.vue can be reused in a more appropriate way.
| async fetchCycleCountImportSystemMessages({commit} ,payload) { | ||
| let systemMessages; | ||
| try { | ||
| const twentyFourHoursEarlier = DateTime.now().minus({ hours: 24 }); | ||
| const resp = await CountService.fetchCycleCountImportSystemMessages({ | ||
| systemMessageTypeId: "ImportInventoryCounts", | ||
| initDate_from: twentyFourHoursEarlier.toMillis(), | ||
| orderByField: 'initDate desc, processedDate desc', | ||
| pageSize: 100 | ||
| }) | ||
| if (!hasError(resp)) { | ||
| systemMessages = resp.data | ||
| } else { | ||
| throw resp.data; | ||
| } | ||
| } catch (err: any) { | ||
| logger.error(err) | ||
| } | ||
| commit(types.COUNT_IMPORT_SYSTEM_MESSAGES_UPDATED, systemMessages) | ||
| }, |
There was a problem hiding this comment.
In the fetchCycleCountImportSystemMessages action, if an API error occurs, systemMessages will remain undefined. Committing undefined to the store will cause a runtime error in BulkUpload.vue when systemMessages.length is accessed. To prevent this, initialize systemMessages to an empty array. Also, the payload parameter is unused and can be removed.
| async fetchCycleCountImportSystemMessages({commit} ,payload) { | |
| let systemMessages; | |
| try { | |
| const twentyFourHoursEarlier = DateTime.now().minus({ hours: 24 }); | |
| const resp = await CountService.fetchCycleCountImportSystemMessages({ | |
| systemMessageTypeId: "ImportInventoryCounts", | |
| initDate_from: twentyFourHoursEarlier.toMillis(), | |
| orderByField: 'initDate desc, processedDate desc', | |
| pageSize: 100 | |
| }) | |
| if (!hasError(resp)) { | |
| systemMessages = resp.data | |
| } else { | |
| throw resp.data; | |
| } | |
| } catch (err: any) { | |
| logger.error(err) | |
| } | |
| commit(types.COUNT_IMPORT_SYSTEM_MESSAGES_UPDATED, systemMessages) | |
| }, | |
| async fetchCycleCountImportSystemMessages({commit}) { | |
| let systemMessages = []; | |
| try { | |
| const twentyFourHoursEarlier = DateTime.now().minus({ hours: 24 }); | |
| const resp = await CountService.fetchCycleCountImportSystemMessages({ | |
| systemMessageTypeId: "ImportInventoryCounts", | |
| initDate_from: twentyFourHoursEarlier.toMillis(), | |
| orderByField: 'initDate desc, processedDate desc', | |
| pageSize: 100 | |
| }) | |
| if (!hasError(resp)) { | |
| systemMessages = resp.data | |
| } else { | |
| throw resp.data; | |
| } | |
| } catch (err: any) { | |
| logger.error(err) | |
| } | |
| commit(types.COUNT_IMPORT_SYSTEM_MESSAGES_UPDATED, systemMessages) | |
| }, |
| const store = useStore(); | ||
| const props = defineProps(["content", "seletedFieldMapping", "mappingType"]) |
There was a problem hiding this comment.
There is a typo in the prop name seletedFieldMapping. It should be selectedFieldMapping. This typo is also present where the prop is used (e.g., line 85).
const props = defineProps(["content", "selectedFieldMapping", "mappingType"])
| let mappingName = ref(null) | ||
| let fieldMapping = ref ({}) | ||
| let fileColumns = ref([]) | ||
| let identificationTypeId = ref('SKU') |
There was a problem hiding this comment.
The variable identificationTypeId is defined but never used within the component. It should be removed to improve code clarity.
| }); | ||
| } | ||
|
|
||
| const bulkUploadInventoryCounts = async (payload: any): Promise <any> => { |
There was a problem hiding this comment.
The payload and return types for bulkUploadInventoryCounts (and fetchCycleCountImportSystemMessages on line 39) are typed as any. Using any undermines the benefits of TypeScript. Please define specific interfaces for payloads and response data to improve type safety and code maintainability.
| import { addOutline, cloudUploadOutline, ellipsisVerticalOutline, trashBinOutline } from "ionicons/icons"; | ||
| import { translate } from '@/i18n'; | ||
| import { computed, ref } from "vue"; | ||
| import { useStore } from 'vuex'; | ||
| import logger from "@/logger"; | ||
| import { hasError, jsonToCsv, parseCsv, showToast } from "@/utils"; | ||
| import CreateMappingModal from "@/components/CreateMappingModal.vue"; | ||
| import { CountService } from "@/services/CountService" | ||
| import CycleCountUploadActionPopover from "@/components/CycleCountUploadActionPopover.vue" | ||
| const store = useStore(); | ||
| const fieldMappings = computed(() => store.getters["user/getFieldMappings"]) | ||
| const systemMessages = computed(() => store.getters["count/getCycleCountImportSystemMessages"]) | ||
| let file = ref(null) | ||
| let uploadedFile = ref({}) | ||
| let fileName = ref(null) | ||
| let content = ref([]) | ||
| let fieldMapping = ref({}) | ||
| let fileColumns = ref([]) | ||
| let selectedMappingId = ref(null) | ||
| const fileUploaded = ref(false); |
There was a problem hiding this comment.
There are unused assets in this component. The icon trashBinOutline is imported on line 110 but never used. Similarly, the ref fileUploaded is defined on line 132, but its value is never read. Please remove them to keep the code clean.
| function getFileProcessingStatus(systemMessage) { | ||
| let processingStatus = "pending" | ||
| if (systemMessage.statusId === 'SmsgConsumed') { | ||
| processingStatus = "processed" | ||
| } else if (systemMessage.statusId === 'SmsgConsuming') { | ||
| processingStatus = "processing" | ||
| } else if (systemMessage.statusId === 'SmsgCancelled') { | ||
| processingStatus = 'cancelled' | ||
| } else if (systemMessage.statusId === 'SmsgError') { | ||
| processingStatus = 'error' | ||
| } | ||
| return processingStatus; | ||
| } |
There was a problem hiding this comment.
The getFileProcessingStatus function can be simplified and made more readable by using a map object instead of a series of if-else if statements.
function getFileProcessingStatus(systemMessage) {
const statusMap = {
'SmsgConsumed': 'processed',
'SmsgConsuming': 'processing',
'SmsgCancelled': 'cancelled',
'SmsgError': 'error'
};
return statusMap[systemMessage.statusId] || 'pending';
}
| } catch { | ||
| content.value = [] | ||
| showToast(translate("Please upload a valid csv to continue")); | ||
| } |
There was a problem hiding this comment.
The catch block in the parse function swallows the error without logging it. This makes debugging difficult if an error occurs during file parsing. It's a good practice to log the caught error.
} catch (err) {
content.value = []
logger.error("Failed to parse CSV file:", err)
showToast(translate("Please upload a valid csv to continue"));
}
| const data = jsonToCsv(uploadedData) | ||
| const formData = new FormData(); | ||
| formData.append("uploadedFile", data, fileName.value); | ||
| formData.append("fileName", fileName.value.replace(".csv", "")); |
There was a problem hiding this comment.
Using fileName.value.replace(".csv", "") to get the filename without the extension is not robust. It will only replace the first occurrence. For a file named my.file.csv.csv, it would result in my.file.csv. A better approach is to remove only the last extension.
formData.append("fileName", fileName.value.substring(0, fileName.value.lastIndexOf('.')) || fileName.value);
1 participant
Re Enabled the Bulk CSV Upload and added it's route in menu.