feat(POS): introduce Shift History dialog with filters and export

[harrishragavan]

Summary

This PR adds a Shift History dialog to the POS interface that allows users to view past POS shifts in one place.

Previously, reviewing shift data required opening POS Opening Shift and POS Closing Shift documents separately, which was time-consuming.

With this change, shift details can now be viewed directly from the POS interface, similar to Invoice History and Draft Invoices.

Features

• View POS shifts in a single table
• Filters for POS Profile and Date Range
• Quick filters for Last 7 Days and Last 1 Month
• Summary cards showing Total Sales, Total Shifts, and Cash Difference
• CSV export support
• Click a row to open the related shift document

Demo

[engahmed1190]

Thanks will review it

[harrishragavan]

Ok 👍

[engahmed1190]

Deep Code Review — PR #186: Shift History Dialog

CRITICAL Issues

1. No Permission Check in get_shift_history (pos_next/api/shifts.py)

Any logged-in user can query ALL shifts for ALL profiles across the entire site. Compare with get_invoices which checks POS Profile User access.

Fix required:

if not frappe.has_permission("POS Opening Shift", "read"):
    frappe.throw(_("Insufficient permissions"))

---

2. No LIMIT Clause in get_shift_history

The query has no pagination or row limit. A date range of a year on a busy site could return thousands of rows, causing slow responses, high memory, and browser freezes.

Fix: Add LIMIT 500 or implement pagination like get_invoices does.

---

3. Correlated Subqueries Are Expensive

Each row runs 3 correlated subqueries (SUM(amount), SUM(closing_amount), SUM(difference)). For 200 shifts = 600 extra queries.

Fix: Rewrite with JOINs:

LEFT JOIN (
    SELECT parent, SUM(amount) as opening_amount
    FROM `tabPOS Opening Shift Detail` GROUP BY parent
) osd ON osd.parent = os.name
LEFT JOIN (
    SELECT parent, SUM(closing_amount) as closing_amount, SUM(difference) as difference
    FROM `tabPOS Closing Shift Detail` GROUP BY parent
) csd ON csd.parent = cs.name

---

HIGH Issues

4. CSV Injection Vulnerability (ShiftHistoryDialog.vue → exportToCSV)

Raw string concatenation with no escaping:

rows.map(e => e.join(",")).join("\n")

• Commas/quotes/newlines in pos_profile or cashier break CSV
• Values starting with =, +, -, @ allow formula injection in Excel

Fix: Wrap values in quotes and escape internal quotes.

---

5. loadProfiles Uses Raw fetch Instead of createResource/call

const res = await fetch("/api/method/frappe.client.get_list", { ... })

This bypasses frappe-ui's CSRF token handling, error handling, and session management. Also calls frappe.client.get_list directly with no row limit.

Fix: Use createResource or the project's call() wrapper from @/utils/apiWrapper.

---

6. loadShifts Guard Is Wrong

function loadShifts() {
    if (props.posProfile) {  // ← checks prop, not filters.pos_profile
        shiftsResource.reload()
    }
}

When user selects "All Profiles" (pos_profile = ""), the guard blocks the request. The server already handles empty pos_profile.

Fix: Remove the guard or check filters.pos_profile instead.

---

7. Missing docstatus Filter in get_shift_history

Query doesn't filter os.docstatus = 1. Draft and cancelled opening shifts will appear in history.

Fix:

conditions.append("os.docstatus = 1")

---

MEDIUM Issues

8. Debounce Timer Leaks (InvoiceHistoryDialog.vue)

Module-level _debounceTimer is never cleared on unmount. If dialog closes while debounce is pending, it fires loadInvoices() on a stale component.

Fix: Clear timer in onUnmounted.

---

9. Breaking API Change in get_invoices

Signature changed from get_invoices(pos_profile, limit=100) to get_invoices(pos_profile, search=None, limit=20, offset=0, ...). Default limit dropped from 100 → 20. Any other callers (offline sync, other components) will silently get fewer results.

Action: Check all call sites for get_invoices before merging.

---

10. Removed showError on Invoice Load Failure

onError(error) {
    console.error("Error loading invoices:", error)
    // showError() was removed — user gets no feedback unless error UI works
    isLoadingMore.value = false
}

The error state UI was added in the template, but verify invoicesResource.error is reactive with createResource.

---

11. Summary Cards Show Client-Side Totals Only

totalGrandTotal and totalCashDiff compute over shifts.value (loaded data). Without pagination this is all data, but if LIMIT is added (as recommended above), summaries become misleading.

Fix: Return server-side aggregates alongside the row data.

---

LOW Issues

12. Indentation Mismatch in shifts.py

Existing codebase uses tabs. New get_shift_history uses 4-space indentation. Must be consistent — use tabs.

---

13. Workspace JSON Is Just Reformatted

The entire posnext.json diff (255+/255-) is whitespace reformatting (tabs → spaces). Adds noise and breaks git blame. Should be a separate commit or excluded.

---

14. from frappe.utils import flt Inside Loop

for row in data:
    from frappe.utils import flt  # ← imported every iteration

Move to top of file.

---

15. onMounted Loads Profiles Even When Dialog Is Hidden

onMounted calls loadProfiles() unconditionally. Move profile loading into the watcher that fires when show becomes true.

---

16. Missing emit Declaration in InvoiceCart.vue

$emit('show-shift-history') isn't declared in defineEmits. Works at runtime but breaks Vue emit validation and IDE support.

---

17. viewShift Always Opens Closing Shift

const doctype = shift.closing_shift_name ? 'pos-closing-shift' : 'pos-opening-shift'

If a shift has both, it always navigates to closing. User might want the opening shift.

---

Summary

Severity	Count	Key Items
Critical	3	No auth check, no LIMIT, correlated subqueries
High	4	CSV injection, raw fetch, wrong guard, missing docstatus filter
Medium	4	Debounce leak, breaking API change, missing error toast, client-side totals
Low	6	Indentation, JSON noise, import in loop, emit declaration

Recommendation: Do not merge as-is. The get_shift_history endpoint needs permission checks, a LIMIT, docstatus filtering, and query optimization before it's production-safe. The CSV export needs sanitization. The get_invoices signature change should be checked for other callers.

[harrishragavan]

Hi, I have made the requested fixes and pushed the updates to this PR. Kindly review the changes. Thank you!

[harrishragavan]

any update ?

[MostafaKadry]

@harrishragavan , We will go back soon, thank you!

[engahmed1190]

Code Review — REQUEST CHANGES

Summary

Adds a Shift History dialog to the POS interface with date filters, summary cards, pagination, and CSV export. Also refactors InvoiceHistoryDialog (server-side search, skeleton loaders, debounce) and improves get_invoices API.

What's Good

• Server-side search for invoices: Moving from client-side filtering to server-side LIKE search is correct. Parameterized queries (%(search)s) — no SQL injection risk
• Pagination with server-side totals: The get_shift_history API returns {rows, totals} with un-paginated aggregates — correct pattern for summary cards
• CSV export: Well-implemented with RFC 4180 compliance, UTF-8 BOM for Excel, and OWASP formula-injection guards (csvEscape). Good security awareness
• User restriction: get_shift_history always enforces os.user = %(session_user)s server-side — cannot be bypassed by clients
• SQL queries: Pre-aggregated LEFT JOINs instead of N+1 subqueries — good performance
• InvoiceHistoryDialog improvements: Skeleton loaders, debounced search, error state with retry, deduplication on load-more — all solid UX improvements

---

ISSUE 1 (BLOCKING): POS Profile not passed to Shift History API

POSSale.vue passes :pos-profile="shiftStore.profileName" to ShiftHistoryDialog, but the component never declares posProfile in defineProps — the prop is silently dropped:

// ShiftHistoryDialog.vue
const props = defineProps({
    modelValue: Boolean,
    currency: { type: String, default: DEFAULT_CURRENCY },
    // posProfile is MISSING
})

And get_shift_history doesn't filter by POS Profile at all — it shows shifts for ALL profiles for the current user. In a multi-profile environment, a cashier assigned to "Counter 1" would see shifts from "Counter 2", "Counter 3", etc.

Fix needed:

1. Add posProfile to defineProps in ShiftHistoryDialog.vue
2. Pass it to get_shift_history API
3. Add os.pos_profile = %(pos_profile)s condition in the SQL query (or make it optional for managers who want to see all)

---

ISSUE 2 (MEDIUM): get_invoices removes per-invoice item loading — breaking change

The old get_invoices loaded items for each invoice:

# OLD — removed in this PR
for invoice in invoices:
    items = frappe.db.sql("""SELECT ... FROM tabSales Invoice Item WHERE parent = ...""")
    invoice.items = items

The new version removes this entirely. If any other caller of get_invoices depends on invoice.items being populated, this is a breaking change. The InvoiceHistoryDialog doesn't use items, but other callers might.

Suggestion: Check if any other code calls get_invoices and uses the items field. If so, add an optional include_items=False parameter.

---

ISSUE 3 (MEDIUM): Workspace JSON is noise

The posnext.json workspace diff is 510 lines of whitespace reformatting (spaces to tabs). The actual content is identical. This makes the PR harder to review and adds merge conflict risk.

Suggestion: Remove this file from the PR or revert the whitespace changes.

---

ISSUE 4 (LOW): InvoiceHistoryDialog changes resource URL

The InvoiceHistoryDialog now uses pos_next.api.invoices.get_invoices instead of frappe.client.get_list. This is a good change (proper API endpoint), but the old endpoint returned draft invoices too (docstatus was not filtered). The new endpoint hardcodes docstatus = 1 — only submitted invoices are returned. If InvoiceHistoryDialog was previously showing drafts, those are now hidden.

---

ISSUE 5 (LOW): ShiftHistoryDialog shows POS Profile column but doesn't filter by it

The table displays shift.pos_profile per row, which is correct for a multi-profile view. But since the API already enforces user-only, the POS Profile column is only useful when a user works across multiple profiles. Consider adding a POS Profile dropdown filter alongside the date range filters.

---

Security

• get_shift_history: Session user enforced server-side ✅
• get_invoices: POS Profile access check via POS Profile User ✅
• Search params use parameterized queries ✅
• CSV export has formula injection protection ✅

Code Quality

• Frontend code is clean, well-organized with section comments
• Debounce implementation is simple and correct (no lodash dependency)
• onUnmounted cleanup prevents stale timer callbacks ✅
• Pagination logic with visible pages ellipsis is well-done

Verdict

The feature is well-built and the code quality is good. The POS Profile gap in shift history is the main blocking issue — it needs to either filter by the active profile or explicitly be designed as an "all profiles" view with appropriate permissions. Fix that + remove the workspace JSON noise, and this is ready.