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

Added the service to generate the feed file #3

Merged

Conversation

Shinde-nutan
Copy link
Contributor

Added a service to generate the orders feed for Netsuite order sync
TODO: add details here

nutan shinde added 5 commits December 4, 2024 10:36
…e to fetch the eligible order ids from the view and itr for each order and prepare OrderDataMap And added system message type and system message type parameter
…e to fetch the eligible order ids from the view and itr for each order and prepare OrderDataMap And added system message type and system message type parameter
service/co/hotwax/netsuite/CreateOrderService.xml Outdated Show resolved Hide resolved
service/co/hotwax/netsuite/CreateOrderService.xml Outdated Show resolved Hide resolved
<description>The System Message Type ID for generating the Brokered Order Items Feed.</description>
</parameter>
<parameter name="systemMessageRemoteId" required="true">
<description>The System Message Remote Id for generating the Brokered Order Items Feed.</description>
Copy link
Contributor

Choose a reason for hiding this comment

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

brokered feed?

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 description.

service/co/hotwax/netsuite/CreateOrderService.xml Outdated Show resolved Hide resolved
</in-parameters>
<actions>
<set field="nowDate" from="ec.user.nowTimestamp"/>
<log message="Generating Create Orders Feed file of HotWax for Order ${orderId} at time ${nowDate}"/>
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
<log message="Generating Create Orders Feed file of HotWax for Order ${orderId} at time ${nowDate}"/>
<log message="Generating Order Feed file for order ${orderId} at time ${nowDate}"/>

try (ordersItr = netsuiteOrders_find.iterator()) {
</script>

<!-- If no orders in BrokeredOrderItemsSyncQueue, then don't generate the file -->
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
<!-- If no orders in BrokeredOrderItemsSyncQueue, then don't generate the file -->
<!-- If no orders in BrokeredOrderItemsSyncQueue, then don't generate the file -->

Brokered reference again

@Shinde-nutan Shinde-nutan marked this pull request as draft December 11, 2024 14:59
@Shinde-nutan Shinde-nutan marked this pull request as ready for review December 16, 2024 05:36
data/ServiceJobData.xml Outdated Show resolved Hide resolved
@Shinde-nutan Shinde-nutan self-assigned this Dec 17, 2024
Copy link
Contributor

@dt2patel dt2patel left a comment

Choose a reason for hiding this comment

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

Added some comments

<moqui.service.message.SystemMessageTypeParameter systemMessageTypeId="NetSuiteOrderItemsFeed"
parameterName="resourcePath" parameterValue="" systemMessageRemoteId=""/>

<moqui.service.message.SystemMessageTypeParameter systemMessageTypeId="NetSuiteOrderItemsFeed"
Copy link
Contributor

Choose a reason for hiding this comment

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

what are these lines? seems duplicate.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, sir will remove this.

parentTypeId="LocalFeedFile"
sendPath=""
sendServiceName="co.hotwax.ofbiz.SystemMessageServices.send#SystemMessageFileSftp"
receivePath="${contentRoot}/CreateOrderFeed/createOrderFeed-${dateTime}.csv"/>
Copy link
Contributor

Choose a reason for hiding this comment

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

why is this set in the receive path? aren't we sending this feed? shouldn't it be in the send path?

<set field="csvFilePath" from="ec.resource.getLocationReference(csvFilePathRef).getUri().getPath()"/>

<!--TODO: Here currently the view used to get the eligible orders for the create feed does not have the shipment method
need to figure out how we can put the checks for include and exclude shipment method.
Copy link
Contributor

Choose a reason for hiding this comment

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

is this still pending?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No sir, Forgot to update the comment after updating the logic,
will update the comments

</entity-find>

<!-- Initialize total amount for non-refunded gift cards -->
<set field="totalNonRefundedGiftCardAmount" type="Integer"/>
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 generally follow this pattern in OFBiz, for example "getValidInventory"

Suggested change
<set field="totalNonRefundedGiftCardAmount" type="Integer"/>
<set field="totalValidGiftCardAmount" type="Integer"/>

<!-- Logic to sum non-refunded gift card payments -->
<script><![CDATA[
def manualRefSet = new HashSet()
def refundedSet = new HashSet()
Copy link
Contributor

Choose a reason for hiding this comment

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

this seems like an odd approach, very brut force. Can we take an approach where we think of this like a logical SQL and implement it as a dynamic view? That way we can get data in a simple query from the DB rather than computing it ourselves here.

It can be changed in the future to reflect different tax codes as needed.
- For Digital Goods, the tax code is sent as "-Not Taxable-" to NetSuite.(in some cases vary based on the )
The current hardcoded "AVATAX" may not be suitable for all product types.
- We can create the record in integration type mapping based on the product types.
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 code this with the expectation that there will be an integration type mapping available for "Default NetSuite Tax Code"

Please make sample data in test environment on OMS side and then update code here so that it's not hardcoded.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure sir, will make the sample data, and update the logic.

Copy link
Contributor Author

@Shinde-nutan Shinde-nutan Jan 9, 2025

Choose a reason for hiding this comment

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

Created the sample data for this:

 <Enumeration enumCode="NETSUITE_TAX_CODE" enumId="NETSUITE_TAX_CODE" enumName="Netsuite Tax codes" enumTypeId="NETSUITE" discription="tax codes mapping between HotWax and Netsuite"/>
    <IntegrationTypeMapping integrationTypeId="NETSUITE_TAX_CODE" mappingKey="DIGITAL_GOOD" mappingValue="-Not Taxable-" description="Tax code for digital goods" />
    <IntegrationTypeMapping integrationTypeId="NETSUITE_TAX_CODE" mappingKey="GIFT_CARD" mappingValue="-Not Taxable-" description="Tax code for gift cards" />
    <IntegrationTypeMapping integrationTypeId="NETSUITE_TAX_CODE" mappingKey="DEFAULT" mappingValue="AVATAX" description="Fallback tax code for unmapped product types" />```

@dt2patel dt2patel merged commit 56f870a into hotwax:main Jan 9, 2025
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.

3 participants