Refactored Adapter API#1335
Conversation
Moved Adapter APIs to suitables store actions Removed client usages from adpater APIs
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 significantly refactors the application's API interaction layer by eliminating a monolithic adapter file. The core purpose is to improve modularity and maintainability by relocating API logic directly into the Pinia stores responsible for managing the relevant data. This change streamlines how different parts of the application fetch and update data, making the codebase more organized and easier to understand. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. 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
The pull request refactors API-related utility functions by removing the adapter/index.ts file and migrating its contents into methods within the productStore.ts and userProfileStore.ts Pinia stores. This change centralizes API logic, eliminating the need to explicitly pass authentication tokens and base URLs to these functions. Specifically, authStore.ts and productStore.ts were updated to call these new store methods directly, and StorePermissions.vue now uses userProfileStore methods for security group operations. Review comments highlight a flaw in the fetchFacilities logic where facility IDs from party and group queries are incorrectly handled, leading to potential overwrites and incorrect rejections. Additionally, the getProfile method in userProfileStore.ts needs to use the hasError utility for robust error checking, and the getUserPreference method in the same store requires a try-catch block or a safe parsing utility for JSON.parse to prevent unhandled exceptions.
| async fetchFacilities(partyId: string, facilityGroupId: string, isAdminUser: boolean, payload: any): Promise <any> { | ||
| let facilityIds: Array<string> = []; | ||
| let filters: any = {}; | ||
| let resp = {} as any | ||
|
|
||
| // Fetch the facilities associated with party | ||
| if(partyId) { | ||
| try { | ||
| resp = await this.fetchFacilitiesByParty(partyId) | ||
|
|
||
| facilityIds = resp.map((facility: any) => facility.facilityId); | ||
| if (!facilityIds.length) { | ||
| return Promise.reject({ | ||
| code: 'error', | ||
| message: 'Failed to fetch user facilities', | ||
| serverResponse: resp.data | ||
| }) | ||
| } | ||
| } catch(error) { | ||
| return Promise.reject({ | ||
| code: 'error', | ||
| message: 'Failed to fetch user facilities', | ||
| serverResponse: error | ||
| }) | ||
| } | ||
| } | ||
|
|
||
| // Fetch the facilities associated with group | ||
| if(facilityGroupId) { | ||
| try { | ||
| resp = await this.fetchFacilitiesByGroup(facilityGroupId, filters) | ||
|
|
||
| facilityIds = resp.map((facility: any) => facility.facilityId); | ||
| if (!facilityIds.length) { | ||
| return Promise.reject({ | ||
| code: 'error', | ||
| message: 'Failed to fetch user facilities', | ||
| serverResponse: resp.data | ||
| }) | ||
| } | ||
| } catch(error) { | ||
| return Promise.reject({ | ||
| code: 'error', | ||
| message: 'Failed to fetch user facilities', | ||
| serverResponse: error | ||
| }) | ||
| } | ||
| } | ||
|
|
||
| if(facilityIds.length) { | ||
| filters = { | ||
| facilityId: facilityIds.join(","), | ||
| facilityId_op: "in", | ||
| pageSize: facilityIds.length | ||
| } | ||
| } | ||
|
|
||
| const params = { | ||
| url: "oms/facilities", | ||
| method: "GET", | ||
| params: { | ||
| pageSize: 500, | ||
| ...payload, | ||
| ...filters | ||
| } | ||
| } | ||
|
|
||
| let facilities: Array<any> = [] | ||
|
|
||
| try { | ||
| resp = await api(params); | ||
| facilities = resp.data | ||
| } catch(error) { | ||
| return Promise.reject({ | ||
| code: "error", | ||
| message: "Failed to fetch facilities", | ||
| serverResponse: error | ||
| }) | ||
| } | ||
|
|
||
| return Promise.resolve(facilities) | ||
| }, |
There was a problem hiding this comment.
The logic in fetchFacilities has a flaw. When both partyId and facilityGroupId are provided, the facilityIds from the party are overwritten by those from the group. Additionally, the check for empty results (if (!facilityIds.length)) inside the if(facilityGroupId) block can lead to an incorrect rejection if the party query returned facilities but the group query did not.
I recommend refactoring this function to correctly accumulate facility IDs from both sources, remove duplicates, and then perform a single check for whether any facilities were found before proceeding.
There was a problem hiding this comment.
Removed facilityGroup support since we don't need it in this app, It was common code om oms-api.
| if (!resp) { | ||
| throw 'Error getting user profile' | ||
| } |
There was a problem hiding this comment.
The error check if (!resp) is not sufficient. The hasError utility function should be used here to check for application-level errors returned in the response body with a 200 status code. Without it, the application might proceed with an invalid user profile.
| if (!resp) { | |
| throw 'Error getting user profile' | |
| } | |
| if (!resp || hasError(resp)) { | |
| throw 'Error getting user profile'; | |
| } |
There was a problem hiding this comment.
hasError is used for OFBiz api calls, since the user profile comes from Maarg any error in api call will automaticlly be thrown.
| let resp = {} as any; | ||
| try { | ||
| resp = await api(params); | ||
| return Promise.resolve(resp.data[0]?.preferenceValue ? JSON.parse(resp.data[0]?.preferenceValue).toString() : "") |
There was a problem hiding this comment.
The JSON.parse call is not wrapped in a try-catch block. If resp.data[0]?.preferenceValue is not a valid JSON string, this will cause an unhandled exception. The original adapter/index.ts file had a jsonParse utility function to handle this safely. A similar safe parsing mechanism should be used here.
Example of safe parsing:
let parsedValue;
try {
parsedValue = JSON.parse(value);
} catch (e) {
parsedValue = value;
}There was a problem hiding this comment.
Done, Added a function in utils
|
@Banibrata-Manna Thank you for the ping! I'm now proceeding with a detailed code review of your pull request. I will post my findings shortly. |
…ogin Reset auth and other stores on login failure
2 participants
Related Issues
#1334
Short Description and Why It's Useful
Screenshots of Visual Changes before/after (If There Are Any)
Contribution and Currently Important Rules Acceptance