- 
                Notifications
    You must be signed in to change notification settings 
- Fork 134
[POS Orders] Fetch order when sending receipt if it's not in DB #14817
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
[POS Orders] Fetch order when sending receipt if it's not in DB #14817
Conversation
| 📲 You can test the changes from this Pull Request in WooCommerce-Wear Android by scanning the QR code below to install the corresponding build.
 | 
        
          
                ...c/main/kotlin/com/woocommerce/android/ui/woopos/emailreceipt/WooPosEmailReceiptRepository.kt
              
                Outdated
          
            Show resolved
            Hide resolved
        
      | 📲 You can test the changes from this Pull Request in WooCommerce Android by scanning the QR code below to install the corresponding build. 
 | 
        
          
                ...st/kotlin/com/woocommerce/android/ui/woopos/emailreceipt/WooPosEmailReceiptRepositoryTest.kt
              
                Fixed
          
            Show fixed
            Hide fixed
        
              
          
                ...st/kotlin/com/woocommerce/android/ui/woopos/emailreceipt/WooPosEmailReceiptRepositoryTest.kt
              
                Fixed
          
            Show fixed
            Hide fixed
        
              
          
                ...st/kotlin/com/woocommerce/android/ui/woopos/emailreceipt/WooPosEmailReceiptRepositoryTest.kt
              
                Fixed
          
            Show fixed
            Hide fixed
        
              
          
                ...st/kotlin/com/woocommerce/android/ui/woopos/emailreceipt/WooPosEmailReceiptRepositoryTest.kt
              
                Fixed
          
            Show fixed
            Hide fixed
        
              
          
                ...st/kotlin/com/woocommerce/android/ui/woopos/emailreceipt/WooPosEmailReceiptRepositoryTest.kt
              
                Fixed
          
            Show fixed
            Hide fixed
        
      | orderMapper.toAppModel(it) | ||
| } | ||
|  | ||
| suspend fun fetchOrderById(orderId: Long): Order? { | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can be private?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, done in 4a8de22
| } | ||
|  | ||
| suspend fun fetchOrderById(orderId: Long): Order? { | ||
| val result = withTimeoutOrNull(AppConstants.REQUEST_TIMEOUT) { | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it can be as simple as
    suspend fun fetchOrderById(orderId: Long): Order? {
        val result = orderStore.fetchSingleOrder(
            selectedSite.get(),
            orderId
        )
        return if (!result.isError) {
            getOrderById(orderId)
        } else {
            null
        }
    }
withTimeoutOrNull looks like it's redundant. There is already a request on timeout in the network layer
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good point, done in 0703601
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! Thanks. I left a couple of suggestions, please take a look
        
          
                ...st/kotlin/com/woocommerce/android/ui/woopos/emailreceipt/WooPosEmailReceiptRepositoryTest.kt
              
                Fixed
          
            Show fixed
            Hide fixed
        
      …s://github.com/woocommerce/woocommerce-android into feat/WOOMOB-1154-POS-Order-email-receipt-fetch
| Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@            Coverage Diff            @@
##              trunk   #14817   +/-   ##
=========================================
  Coverage     38.27%   38.27%           
- Complexity    10089    10091    +2     
=========================================
  Files          2132     2132           
  Lines        120771   120779    +8     
  Branches      16568    16570    +2     
=========================================
+ Hits          46224    46232    +8     
+ Misses        69850    69849    -1     
- Partials       4697     4698    +1     ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
 | 
WOOMOB-1154
Description
From this comment
We fetch the order remotely if it's not in the DB
Test Steps
Open POS after a clean install of the app, and ensure that the receipt is sent.
Images/gif
N/A
RELEASE-NOTES.txtif necessary. Use the "[Internal]" label for non-user-facing changes.