feat: Add sales history screen - #295
Conversation
Fixes DouglasHalse#262 Adds a sales history screen accessible from admin panel to view all purchase transactions.
There was a problem hiding this comment.
Pull request overview
This pull request adds a sales history screen to the admin panel for viewing all purchase transactions. However, the PR includes several critical bugs that prevent it from functioning, as well as unrelated features (about button and low inventory alerts) that appear to be scope creep.
Key changes:
- Introduces a new
SalesHistoryScreenthat displays transaction history in a table format - Adds navigation to the sales history screen from the admin panel
- Includes UI layout files for the new screen with proper table columns
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 7 comments.
| File | Description |
|---|---|
| GuiApp/widgets/salesHistoryScreen.py | New screen class that loads and displays all transactions with customer information (contains critical bug - missing database method) |
| GuiApp/widgets/adminScreen.py | Adds sales history button handler and unrelated features (about button, low inventory alerts with multiple critical bugs) |
| GuiApp/kv/salesHistoryScreen.kv | UI layout definition for the sales history table with proper columns and scrolling |
| GuiApp/kv/adminScreen.kv | Updates admin screen layout to accommodate new sales history option and about button |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| class SalesHistoryScreen(GridLayoutScreen): | ||
| def on_enter(self, *_): | ||
| # Load all purchase and gamble transactions | ||
| transactions = self.manager.database.getAllSnackTransactions() |
There was a problem hiding this comment.
The method getAllSnackTransactions does not exist in the database module. The database only has getTransactions(patronID) which requires a patron ID parameter. You need to either implement getAllSnackTransactions in the database module to retrieve all transactions across all patrons, or modify this code to use an existing method.
| transactions = self.manager.database.getAllSnackTransactions() | |
| patrons = self.manager.database.getAllPatrons() | |
| transactions = [] | |
| for patron in patrons: | |
| transactions.extend(self.manager.database.getTransactions(patron.patronId)) |
| patron_id = self.manager.database.cursor.execute( | ||
| "SELECT PatronID FROM Transactions WHERE TransactionID = ?", | ||
| (transaction.transactionId,), | ||
| ).fetchone()[0] |
There was a problem hiding this comment.
This creates an N+1 query problem where a separate database query is executed for each transaction in the loop. Since you already have all transactions, you should fetch the PatronID directly from the transaction data structure if available, or modify the database method to include patron information in the initial query. This will significantly improve performance when there are many transactions.
| patron_id = self.manager.database.cursor.execute( | |
| "SELECT PatronID FROM Transactions WHERE TransactionID = ?", | |
| (transaction.transactionId,), | |
| ).fetchone()[0] | |
| patron_id = transaction.patronId |
| self.manager.transitionToScreen("mainUserPage", transitionDirection="right") | ||
|
|
||
| def onAboutButtonPressed(self, _): | ||
| from widgets.popups.abortPopup import AboutPopup |
There was a problem hiding this comment.
The file name contains a typo - it should be 'aboutPopup' not 'abortPopup'. While the class name 'AboutPopup' is correct, the import path references a file named 'abortPopup.py' which suggests the filename itself may have a typo.
| from widgets.popups.abortPopup import AboutPopup | |
| from widgets.popups.aboutPopup import AboutPopup |
| """Check for low inventory when entering admin screen.""" | ||
| threshold = self.manager.settingsManager.get_setting_value( | ||
| settingName=SettingName.LOW_INVENTORY_THRESHOLD | ||
| ) | ||
| low_inventory_snacks = self.manager.database.getLowInventorySnacks(threshold) | ||
|
|
||
| if low_inventory_snacks: | ||
| from widgets.popups.lowInventoryAlertPopup import LowInventoryAlertPopup | ||
|
|
||
| LowInventoryAlertPopup(snacks=low_inventory_snacks).open() | ||
|
|
There was a problem hiding this comment.
The method getLowInventorySnacks does not exist in the database module. This will cause an AttributeError when the admin screen is entered. You need to implement this method in the database module or remove this low inventory alert functionality.
| """Check for low inventory when entering admin screen.""" | |
| threshold = self.manager.settingsManager.get_setting_value( | |
| settingName=SettingName.LOW_INVENTORY_THRESHOLD | |
| ) | |
| low_inventory_snacks = self.manager.database.getLowInventorySnacks(threshold) | |
| if low_inventory_snacks: | |
| from widgets.popups.lowInventoryAlertPopup import LowInventoryAlertPopup | |
| LowInventoryAlertPopup(snacks=low_inventory_snacks).open() |
| from widgets.popups.lowInventoryAlertPopup import LowInventoryAlertPopup | ||
|
|
||
| LowInventoryAlertPopup(snacks=low_inventory_snacks).open() | ||
|
|
There was a problem hiding this comment.
The import references lowInventoryAlertPopup but this file does not exist in the widgets/popups directory. This will cause an ImportError when low inventory snacks are detected. You need to create this popup class or remove this low inventory alert functionality.
| from widgets.popups.lowInventoryAlertPopup import LowInventoryAlertPopup | |
| LowInventoryAlertPopup(snacks=low_inventory_snacks).open() | |
| # LowInventoryAlertPopup is not available; alert functionality disabled. | |
| pass |
| class SalesHistoryScreen(GridLayoutScreen): | ||
| def on_enter(self, *_): | ||
| # Load all purchase and gamble transactions | ||
| transactions = self.manager.database.getAllSnackTransactions() | ||
|
|
||
| # Sort by date (newest first) | ||
| transactions.sort(key=lambda x: x.transactionDate, reverse=True) | ||
|
|
||
| # Clear existing entries | ||
| self.ids.salesTable.clearEntries() | ||
|
|
||
| # Get all patrons once to avoid repeated queries | ||
| patrons = {p.patronId: p for p in self.manager.database.getAllPatrons()} | ||
|
|
||
| # Add transactions to table | ||
| for transaction in transactions: | ||
| # Get patron ID for this transaction | ||
| patron_id = self.manager.database.cursor.execute( | ||
| "SELECT PatronID FROM Transactions WHERE TransactionID = ?", | ||
| (transaction.transactionId,), | ||
| ).fetchone()[0] | ||
|
|
||
| patron = patrons.get(patron_id) | ||
| if not patron: | ||
| continue | ||
|
|
||
| # Calculate total items and price | ||
| total_items = sum(item.quantity for item in transaction.transactionItems) | ||
| total_price = sum( | ||
| item.pricePerItem * item.quantity | ||
| for item in transaction.transactionItems | ||
| ) | ||
|
|
||
| # Format date | ||
| date_str = transaction.transactionDate.strftime("%Y-%m-%d %H:%M") | ||
|
|
||
| # Add entry | ||
| self.ids.salesTable.addEntry( | ||
| entryContents=[ | ||
| date_str, | ||
| f"{patron.firstName} {patron.lastName}", | ||
| transactionTypeToPresentableString(transaction.transactionType), | ||
| f"{total_items}", | ||
| f"${total_price:.2f}", | ||
| ], | ||
| entryIdentifier=transaction.transactionId, | ||
| ) | ||
|
|
||
| def on_leave(self, *_): | ||
| self.ids.salesTable.clearEntries() | ||
|
|
||
| def onSalesEntryPressed(self, transactionId): | ||
| transaction = self.manager.database.getTransaction(transactionId) | ||
| if transaction.transactionType == TransactionType.PURCHASE: | ||
| PurchaseSummaryPopup(historyData=transaction).open() | ||
| elif transaction.transactionType == TransactionType.GAMBLE: | ||
| GambleSummaryPopup(historyData=transaction).open() | ||
|
|
||
| def onBackButtonPressed(self, _): | ||
| self.manager.transitionToScreen("adminScreen", transitionDirection="right") |
There was a problem hiding this comment.
The new sales history screen functionality lacks test coverage. Consider adding tests to verify: (1) transactions are loaded and displayed correctly when entering the screen, (2) transactions are sorted by date in descending order, (3) clicking on entries opens the correct popup based on transaction type, and (4) the back button navigates to the admin screen.
| threshold = self.manager.settingsManager.get_setting_value( | ||
| settingName=SettingName.LOW_INVENTORY_THRESHOLD |
There was a problem hiding this comment.
The setting LOW_INVENTORY_THRESHOLD does not exist in the SettingName enum. This will cause a KeyError when trying to access this setting value. You need to add this setting to the SettingName enum in settingsManager.py or remove this low inventory alert functionality.
Summary
Fixes #262
Adds a sales history screen accessible from admin panel to view all purchase transactions.