Skip to content

Conversation

@staskus
Copy link
Contributor

@staskus staskus commented Jun 13, 2025

Description

Added a functionality to play a sound when barcode scanning fails. Using the same sound as on Android:

  • Created a POS sound player to play POS-related sounds
  • Added a sound .mp3 into a project
  • Updated tests

Steps to reproduce

  1. Enable pointOfSaleBarcodeScanningi1
  2. Open POS
  3. Scan a wrong barcode
  4. Confirm that the sound plays

Testing information

Tested on iPad Air M2 18.3 Device

Screenshots

ScreenRecording_06-13-2025.11-24-03_1.MP4

  • I have considered if this change warrants user-facing release notes and have added them to RELEASE-NOTES.txt if necessary.

@staskus staskus added this to the 22.6 milestone Jun 13, 2025
@staskus staskus requested review from Copilot and joshheald June 13, 2025 09:41
@staskus staskus added type: task An internally driven task. feature: POS labels Jun 13, 2025
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR adds a failure sound cue when barcode scans fail in the POS system and updates tests and project configuration accordingly.

  • Introduces PointOfSaleSoundPlayer and PointOfSaleSound for playing .mp3 assets.
  • Injects a soundPlayer into PointOfSaleAggregateModel and triggers playback on scan errors.
  • Adds a mock sound player and a new async test to verify failure sound behavior.

Reviewed Changes

Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
Classes/POS/Models/PointOfSaleAggregateModel.swift Inject soundPlayer and call playSound(.barcodeScanFailure) on barcode scan errors
Classes/POS/Utils/Audio/PointOfSaleSoundPlayer.swift Implement AVAudio-based sound playback and define the PointOfSaleSound type
Tests/POS/Mocks/MockPointOfSaleSoundPlayer.swift Add a mock conforming to PointOfSaleSoundPlayerProtocol
Tests/POS/Mocks/MockPointOfSaleBarcodeScanService.swift Change mock to a class and allow throwing errors for test scenarios
Tests/POS/Models/PointOfSaleAggregateModelTests.swift Inject the mock sound player into existing tests and add a new failure test
WooCommerce.xcodeproj/project.pbxproj Register PointOfSaleSoundPlayer.swift, MockPointOfSaleSoundPlayer.swift, and pos_scan_failure.mp3 in build and resource targets
Comments suppressed due to low confidence (3)

WooCommerce/WooCommerceTests/POS/Models/PointOfSaleAggregateModelTests.swift:1262

  • You’ve added a test for failure playback. Consider also adding a counterpart test to verify that no sound is played when a barcode scan succeeds, ensuring both paths are covered.
struct BarcodeTests {

WooCommerce/Classes/POS/Utils/Audio/PointOfSaleSoundPlayer.swift:2

  • The file uses DDLogError but doesn’t import the logging framework. Please add the appropriate import CocoaLumberjack (or your chosen logger) at the top so DDLogError resolves correctly.
import AVFoundation

WooCommerce/WooCommerceTests/POS/Models/PointOfSaleAggregateModelTests.swift:28

  • [nitpick] Most existing tests don’t verify sound behavior; since soundPlayer has a default parameter, you can omit injecting it in tests that don’t need to observe playback. This will reduce test boilerplate and maintenance.
soundPlayer: MockPointOfSaleSoundPlayer())

DDLogError("Sound file not found: \(sound.name).\(sound.type)")
return
}
do {
Copy link

Copilot AI Jun 13, 2025

Choose a reason for hiding this comment

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

[nitpick] Consider stopping or reusing the existing audioPlayer before creating a new instance to avoid overlapping or resource leakage when sounds are played in rapid succession.

Suggested change
do {
do {
if let currentPlayer = audioPlayer {
currentPlayer.stop()
audioPlayer = nil
}

Copilot uses AI. Check for mistakes.
Copy link
Contributor

Choose a reason for hiding this comment

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

I tried with this suggestion... but it doesn't really seem to make any difference to how it sounds with lots of rapid failures.

The leak comment is worth thinking about, but I don't really see how it would leak.

It probably doesn't hurt to add this, but I'll leave it up to you @staskus

Copy link
Contributor Author

@staskus staskus Jun 16, 2025

Choose a reason for hiding this comment

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

I think a little bit of optimization makes sense here, especially given that if an item is scanned once, it's likely to be scanned multiple times.

Plus, in an extreme case when we scan multiple wrong items in a row fast, it doesn't make sense to stop the previous sound and start another.

@wpmobilebot
Copy link
Collaborator

wpmobilebot commented Jun 13, 2025

App Icon📲 You can test the changes from this Pull Request in WooCommerce iOS Prototype by scanning the QR code below to install the corresponding build.

App NameWooCommerce iOS Prototype
Build Number30465
VersionPR #15746
Bundle IDcom.automattic.alpha.woocommerce
Commit5e43402
Installation URL3bkn3h38vjdig
Automatticians: You can use our internal self-serve MC tool to give yourself access to those builds if needed.

@wpmobilebot wpmobilebot modified the milestones: 22.6, 22.7 Jun 13, 2025
@wpmobilebot
Copy link
Collaborator

Version 22.6 has now entered code-freeze, so the milestone of this PR has been updated to 22.7.

Copy link
Contributor

@joshheald joshheald left a comment

Choose a reason for hiding this comment

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

Works well. You could integrate the copilot suggestion if you want.

I've added a credit to the licenses section for the sound.

Comment on lines +27 to +28
barcodeScanService: MockPointOfSaleBarcodeScanService(),
soundPlayer: MockPointOfSaleSoundPlayer())
Copy link
Contributor

Choose a reason for hiding this comment

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

Keeping iOS16 support is so annoying with this...

DDLogError("Sound file not found: \(sound.name).\(sound.type)")
return
}
do {
Copy link
Contributor

Choose a reason for hiding this comment

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

I tried with this suggestion... but it doesn't really seem to make any difference to how it sounds with lots of rapid failures.

The leak comment is worth thinking about, but I don't really see how it would leak.

It probably doesn't hurt to add this, but I'll leave it up to you @staskus

joshheald and others added 3 commits June 13, 2025 13:50
- Use a player cache to reuse a player between the plays
- Don't restart the sound it the previous one plays
- Use MainActor for safety when accessing player's cache
@staskus staskus enabled auto-merge June 16, 2025 12:12
@staskus staskus merged commit 5a6eac3 into trunk Jun 16, 2025
13 checks passed
@staskus staskus deleted the woomob-579-woo-posbarcodes-play-error-sound-when-a-barcode-scan-fails branch June 16, 2025 12:32
@joshheald
Copy link
Contributor

@staskus I noticed today that we noticably block the main thread when we play the sound, especially the very first time. Perhaps there's a way we can pre-load the sound on another thread to avoid that?

Repro:

  1. Type an unknown barcode in the simulator but don't press the button
  2. Start tapping other items as fast as you can
  3. Tap Scan
  4. Observe that when the error sound plays it stutters in adding items.

It's also noticable with a stop in the shimmer effect if you're not tapping items and just watching the screen.

@staskus
Copy link
Contributor Author

staskus commented Jun 17, 2025

@joshheald thanks for reporting, I'll check

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

feature: POS type: task An internally driven task.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants