Compare commits

...

8 Commits

Author SHA1 Message Date
Martin Tranberg
ba3d8dded5 chore: ignore tasks/ and .claude/ directories
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
2026-04-12 10:00:52 +02:00
Martin Tranberg
d7c0fdd091 test: add unit tests for all code-review bug fixes
19 tests covering:
- I2/I3: STRINGS dict entries (System tab label, status_loading_items)
- I1/C-1/S-2: nav_gen guard logic in _finalize_list_loading
  (matching gen applies, stale gen discards, None bypasses guard,
   old zero default now correctly treated as stale)
- C1: url=None initialization order in _fetch_tree_children_bg
- S2: dead SITE branch absent from _append_list_items
- S-1: is_breadcrumb parameter removed from _navigate_to_item_data

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
2026-04-12 09:57:44 +02:00
Martin Tranberg
f168d0c60a refactor: remove dead is_breadcrumb parameter from _navigate_to_item_data (S-1)
The S3 fix removed the only conditional that read is_breadcrumb. Remove
the parameter from the signature and its kwarg from the one call site in
the breadcrumb button handler.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
2026-04-12 09:53:58 +02:00
Martin Tranberg
0cbec6477f fix: restore refresh operations broken by nav_gen regression (C-1, S-2)
Change nav_gen default from 0 to None in _fetch_list_contents_bg and
_finalize_list_loading. The guard is updated to only apply when a gen
is explicitly provided (`nav_gen is not None`).

Refresh call sites (lines 1515, 1529, 1538) pass no gen, so they receive
None and bypass the guard — their results are always applied. Navigation
calls still pass an explicit integer gen, so stale-navigation protection
remains fully intact.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
2026-04-12 09:52:42 +02:00
Martin Tranberg
a55365259c refactor: remove redundant pulse_gauge calls and fix breadcrumb dedup (S1, S3)
- S1: drop pulse_gauge(True) from inside pagination while-loops in
  _fetch_sites_bg, _fetch_tree_children_bg, and _fetch_list_contents_bg;
  the gauge is already running from the call before the loop
- S3: remove the is_breadcrumb bypass on the early-return guard so
  clicking the already-active breadcrumb segment no longer fires a
  redundant network request

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
2026-04-12 09:45:49 +02:00
Martin Tranberg
d529568798 fix: add navigation generation counter to prevent stale list overwrite (I1)
Introduces self._nav_gen, incremented on every _navigate_to_item_data
call. The counter is threaded through _fetch_list_contents_bg and
checked in _finalize_list_loading: if the user navigated away while a
fetch was in flight, the stale results are silently discarded instead of
overwriting the active folder view and re-sorting its items.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
2026-04-12 09:44:39 +02:00
Martin Tranberg
b800f0308d fix: resolve C2/I4 review findings
- C2: remove duplicate EVT_SIZE binding (on_status_bar_resize); merge
  gauge repositioning into the single on_resize handler
- I4: position gauge correctly on first show by updating rect inside
  pulse_gauge._do() when start=True, so no resize event is required

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
2026-04-12 09:42:52 +02:00
Martin Tranberg
dadbb4d51a fix: resolve C1/I2/I3/S2 review findings
- C1: initialize url=None before conditional in _fetch_tree_children_bg
  to prevent UnboundLocalError on unexpected data types
- I2: translate System tab label via get_txt instead of hardcoded string
- I3: add status_loading_items to STRINGS (da+en) and use it in
  _fetch_list_contents_bg instead of hardcoded Danish f-string
- S2: remove unreachable SITE branch from _append_list_items

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
2026-04-12 09:41:25 +02:00
3 changed files with 256 additions and 23 deletions

4
.gitignore vendored
View File

@@ -45,3 +45,7 @@ searchindex.json
# Temporary files
C:\Temp_SP\
# Project tooling / AI assistant files
tasks/
.claude/

View File

@@ -179,6 +179,7 @@ STRINGS = {
"settings_license_status": "Status: Ikke aktiveret",
"settings_logging_group": "System / Diverse",
"settings_logging": "Aktiver log-output (anbefales til fejlfinding)",
"status_loading_items": "Henter... ({count} emner)",
"settings_about_group": "Om programmet",
"settings_credits": "© 2026 SharePoint Explorer\n\nSkabt af:\nMartin Tranberg\nBlueprint\n\nBernhard Bangs Allé 23, 2.\n2000 Frederiksberg\n\nTel: 70258689"
},
@@ -275,6 +276,7 @@ STRINGS = {
"settings_license_status": "Status: Not activated",
"settings_logging_group": "System / Miscellaneous",
"settings_logging": "Enable log output (recommended for troubleshooting)",
"status_loading_items": "Loading... ({count} items)",
"settings_about_group": "About",
"settings_credits": "© 2026 SharePoint Explorer\n\nCreated by:\nMartin Tranberg\nBlueprint\n\nBernhard Bangs Allé 23, 2.\nDK-2000 Frederiksberg\n\nPhone: +45 70258689"
}
@@ -498,7 +500,7 @@ class SettingsDialog(wx.Dialog):
sys_vbox.Add(sys_inner, 1, wx.EXPAND | wx.ALL, 15)
sys_panel.SetSizer(sys_vbox)
self.nb.AddPage(sys_panel, "System")
self.nb.AddPage(sys_panel, self.get_txt("settings_logging_group").split("/")[0].strip())
# 5. ABOUT TAB
about_panel = wx.Panel(self.nb)
@@ -669,6 +671,7 @@ class SharePointApp(wx.Frame):
self.current_items = [] # Gemmer graf-objekterne for rækkerne
self.tree_item_data = {} # Mappenoder -> {type, id, name, drive_id, path}
self.current_path_data = [] # Gemmer data-objekterne for den nuværende sti (brødkrummer)
self._nav_gen = 0 # Incremented on each navigation to discard stale fetch results
self.tree_root = None
self.is_navigating_back = False
self.active_edits = {} # item_id -> { "name": name, "event": Event, "waiting": bool }
@@ -928,7 +931,6 @@ class SharePointApp(wx.Frame):
# Add a Gauge to the status bar
self.gauge = wx.Gauge(self.status_bar, range=100, size=(140, 18), style=wx.GA_HORIZONTAL | wx.GA_SMOOTH)
self.gauge.Hide()
self.Bind(wx.EVT_SIZE, self.on_status_bar_resize)
panel.SetSizer(vbox)
self.Bind(wx.EVT_SIZE, self.on_resize)
@@ -1359,6 +1361,9 @@ class SharePointApp(wx.Frame):
if start:
self.gauge.Show()
self.gauge.Pulse()
rect = self.status_bar.GetFieldRect(1)
self.gauge.SetPosition((rect.x + 5, rect.y + 2))
self.gauge.SetSize((rect.width - 10, rect.height - 4))
else:
self.gauge.Hide()
self.gauge.SetValue(0)
@@ -1540,21 +1545,19 @@ class SharePointApp(wx.Frame):
def on_resize(self, event):
width = self.GetSize().width
threshold = 1100
if width < threshold and not self.compact_mode:
self.compact_mode = True
self._update_button_labels(full=False)
elif width >= threshold and self.compact_mode:
self.compact_mode = False
self._update_button_labels(full=True)
event.Skip()
def on_status_bar_resize(self, event):
if hasattr(self, 'gauge') and self.gauge:
rect = self.status_bar.GetFieldRect(1)
self.gauge.SetPosition((rect.x + 5, rect.y + 2))
self.gauge.SetSize((rect.width - 10, rect.height - 4))
event.Skip()
def _update_button_labels(self, full=True):
@@ -1641,7 +1644,7 @@ class SharePointApp(wx.Frame):
btn.Bind(wx.EVT_BUTTON, self.load_sites)
elif data:
def on_click(e, d=data):
self._navigate_to_item_data(d, is_breadcrumb=True)
self._navigate_to_item_data(d)
# Efter navigation, prøv at finde og vælge den i træet
wx.CallAfter(self._sync_tree_selection_by_path, d["path"])
@@ -1720,7 +1723,6 @@ class SharePointApp(wx.Frame):
all_sites.extend(data.get('value', []))
url = data.get('@odata.nextLink')
self.set_status(f"{self.get_txt('status_fetching_sites')} ({len(all_sites)}...)")
self.pulse_gauge(True)
else:
break
@@ -1771,7 +1773,8 @@ class SharePointApp(wx.Frame):
if not self.ensure_valid_token(): return
self.pulse_gauge(True)
all_children = []
url = None
if data['type'] == "SITE":
url = f"https://graph.microsoft.com/v1.0/sites/{data['id']}/drives"
elif data['type'] == "DRIVE":
@@ -1785,7 +1788,6 @@ class SharePointApp(wx.Frame):
res_data = res.json()
all_children.extend(res_data.get('value', []))
url = res_data.get('@odata.nextLink')
self.pulse_gauge(True)
else:
break
@@ -1851,10 +1853,10 @@ class SharePointApp(wx.Frame):
self._navigate_to_item_data(data, tree_item=item)
def _navigate_to_item_data(self, data, tree_item=None, is_breadcrumb=False):
def _navigate_to_item_data(self, data, tree_item=None):
try:
# Race-condition beskyttelse: Hvis vi allerede er der, så stop (undtagen ved brødkrumme-klik)
if not is_breadcrumb and getattr(self, 'current_path', None) == data.get("path"):
# Race-condition beskyttelse: Hvis vi allerede er der, så stop
if getattr(self, 'current_path', None) == data.get("path"):
return
self.current_path = data["path"]
@@ -1883,12 +1885,14 @@ class SharePointApp(wx.Frame):
self.list_ctrl.DeleteAllItems()
self.current_items = []
self.set_status(self.get_txt("status_loading_content"))
threading.Thread(target=self._fetch_list_contents_bg, args=(data,), daemon=True).start()
self._nav_gen += 1
gen = self._nav_gen
threading.Thread(target=self._fetch_list_contents_bg, args=(data, gen), daemon=True).start()
except RuntimeError:
pass
def _fetch_list_contents_bg(self, data):
def _fetch_list_contents_bg(self, data, nav_gen=None):
if not self.ensure_valid_token(): return
self.pulse_gauge(True)
items_data = []
@@ -1934,9 +1938,8 @@ class SharePointApp(wx.Frame):
})
items_data.extend(chunk_data)
self.set_status(f"Henter... ({len(items_data)} emner)")
self.pulse_gauge(True)
self.set_status(self.get_txt("status_loading_items").format(count=len(items_data)))
# Chunked UI Update
if first_chunk:
wx.CallAfter(self._populate_list_ctrl, chunk_data, data, finalize=False)
@@ -1947,20 +1950,19 @@ class SharePointApp(wx.Frame):
url = res_data.get('@odata.nextLink')
# Finalize
wx.CallAfter(self._finalize_list_loading, items_data)
wx.CallAfter(self._finalize_list_loading, items_data, nav_gen)
self.pulse_gauge(False)
def _append_list_items(self, items):
if not self: return
start_idx = len(self.current_items)
self.current_items.extend(items)
for i, item in enumerate(items):
idx = start_idx + i
img_idx = self.idx_file
if item['type'] == "FOLDER": img_idx = self.idx_folder
elif item['type'] == "DRIVE": img_idx = self.idx_drive
elif item['type'] == "SITE": img_idx = self.idx_site
elif item['type'] == "FILE":
img_idx = self.get_icon_idx_for_file(item['name'])
@@ -1971,8 +1973,10 @@ class SharePointApp(wx.Frame):
self.list_ctrl.SetItem(idx, 2, size_str)
self.list_ctrl.SetItem(idx, 3, item['modified'])
def _finalize_list_loading(self, items_data):
def _finalize_list_loading(self, items_data, nav_gen=None):
if not self: return
if nav_gen is not None and nav_gen != self._nav_gen:
return # User navigated away; discard stale results
self.current_items = items_data
self.apply_sorting()
self.set_status(self.get_txt("status_ready"))

225
tests/test_review_fixes.py Normal file
View File

@@ -0,0 +1,225 @@
"""
Unit tests for the code-review bug fixes applied to sharepoint_browser.py.
Covers:
C1 - url=None prevents UnboundLocalError in _fetch_tree_children_bg
I1 - nav_gen guard in _finalize_list_loading discards stale results
I2 - System tab label derivation from STRINGS key
I3 - status_loading_items translation key present and formattable
C-1 - Refresh calls (nav_gen=None default) always apply their results
S-2 - nav_gen=None sentinel is safer than 0
S-1 - is_breadcrumb parameter removed from _navigate_to_item_data
S2 - Dead SITE branch removed from _append_list_items
All tests run without a live display; wx is imported but no widgets are
instantiated.
"""
import sys
import os
import inspect
import unittest
from unittest.mock import MagicMock, patch
sys.path.insert(0, os.path.dirname(os.path.dirname(os.path.abspath(__file__))))
import sharepoint_browser as sb
# ---------------------------------------------------------------------------
# I2 + I3: STRINGS dictionary
# ---------------------------------------------------------------------------
class TestStrings(unittest.TestCase):
def test_system_tab_label_da(self):
"""I2: Danish settings_logging_group yields 'System' after split/strip."""
group = sb.STRINGS["da"]["settings_logging_group"]
self.assertEqual(group.split("/")[0].strip(), "System")
def test_system_tab_label_en(self):
"""I2: English settings_logging_group yields 'System' after split/strip."""
group = sb.STRINGS["en"]["settings_logging_group"]
self.assertEqual(group.split("/")[0].strip(), "System")
def test_status_loading_items_present_da(self):
"""I3: status_loading_items key exists in Danish STRINGS."""
self.assertIn("status_loading_items", sb.STRINGS["da"])
def test_status_loading_items_present_en(self):
"""I3: status_loading_items key exists in English STRINGS."""
self.assertIn("status_loading_items", sb.STRINGS["en"])
def test_status_loading_items_format_da(self):
"""I3: Danish template formats with named {count} argument."""
result = sb.STRINGS["da"]["status_loading_items"].format(count=42)
self.assertIn("42", result)
def test_status_loading_items_format_en(self):
"""I3: English template formats with named {count} argument."""
result = sb.STRINGS["en"]["status_loading_items"].format(count=99)
self.assertIn("99", result)
def test_status_loading_items_da_uses_count_kwarg(self):
"""I3: Danish template uses {count} placeholder (not positional)."""
template = sb.STRINGS["da"]["status_loading_items"]
self.assertIn("{count}", template)
def test_status_loading_items_en_uses_count_kwarg(self):
"""I3: English template uses {count} placeholder (not positional)."""
template = sb.STRINGS["en"]["status_loading_items"]
self.assertIn("{count}", template)
# ---------------------------------------------------------------------------
# I1 + C-1 + S-2: nav_gen guard in _finalize_list_loading
# ---------------------------------------------------------------------------
class TestNavGenGuard(unittest.TestCase):
"""
_finalize_list_loading(self, items_data, nav_gen=None)
Guard logic:
nav_gen is None → always apply (refresh / unconstrained calls)
nav_gen == self._nav_gen → apply (matches current navigation)
nav_gen != self._nav_gen → discard (stale; user navigated away)
"""
def _make_app(self, current_gen: int):
"""Minimal mock that satisfies _finalize_list_loading's needs."""
app = MagicMock()
app._nav_gen = current_gen
# MagicMock is truthy by default so `if not self` passes
return app
# --- nav_gen=None cases (C-1 / S-2) ---
def test_none_gen_applies_when_nav_gen_is_1(self):
"""C-1/S-2: nav_gen=None applies results regardless of _nav_gen."""
app = self._make_app(1)
items = [{"name": "a"}]
sb.SharePointApp._finalize_list_loading(app, items, nav_gen=None)
self.assertEqual(app.current_items, items)
app.apply_sorting.assert_called_once()
def test_none_gen_applies_when_nav_gen_is_high(self):
"""C-1: nav_gen=None still applies when _nav_gen is large."""
app = self._make_app(99)
items = [{"name": "b"}]
sb.SharePointApp._finalize_list_loading(app, items, nav_gen=None)
self.assertEqual(app.current_items, items)
def test_default_gen_arg_applies(self):
"""C-1: Omitting nav_gen entirely (default=None) always applies."""
app = self._make_app(5)
items = [{"name": "refresh"}]
sb.SharePointApp._finalize_list_loading(app, items) # no nav_gen kwarg
self.assertEqual(app.current_items, items)
app.apply_sorting.assert_called_once()
# --- matching gen (I1, happy path) ---
def test_matching_gen_applies(self):
"""I1: Results applied when nav_gen matches _nav_gen."""
app = self._make_app(3)
items = [{"name": "c"}]
sb.SharePointApp._finalize_list_loading(app, items, nav_gen=3)
self.assertEqual(app.current_items, items)
app.apply_sorting.assert_called_once()
# --- stale gen cases (I1) ---
def test_stale_gen_discards_results(self):
"""I1: Results discarded when nav_gen < _nav_gen (user navigated away)."""
app = self._make_app(5)
sentinel = object()
app.current_items = sentinel
sb.SharePointApp._finalize_list_loading(app, [{"name": "old"}], nav_gen=2)
self.assertIs(app.current_items, sentinel,
"current_items was overwritten by stale result")
app.apply_sorting.assert_not_called()
def test_future_gen_discards_results(self):
"""I1: Results discarded when nav_gen > _nav_gen (shouldn't happen, but safe)."""
app = self._make_app(3)
sentinel = object()
app.current_items = sentinel
sb.SharePointApp._finalize_list_loading(app, [{"name": "future"}], nav_gen=99)
self.assertIs(app.current_items, sentinel)
app.apply_sorting.assert_not_called()
def test_gen_zero_still_discarded_when_nav_gen_nonzero(self):
"""S-2: nav_gen=0 (old broken default) is now treated as a stale gen, not a pass."""
app = self._make_app(1)
sentinel = object()
app.current_items = sentinel
sb.SharePointApp._finalize_list_loading(app, [{"name": "zero"}], nav_gen=0)
self.assertIs(app.current_items, sentinel,
"nav_gen=0 should be treated as stale, not as 'no constraint'")
app.apply_sorting.assert_not_called()
# ---------------------------------------------------------------------------
# C1: url=None initialization in _fetch_tree_children_bg
# ---------------------------------------------------------------------------
class TestUrlInitialization(unittest.TestCase):
def test_url_initialized_to_none_before_conditional(self):
"""C1: _fetch_tree_children_bg initializes url=None before the if block."""
source = inspect.getsource(sb.SharePointApp._fetch_tree_children_bg)
# Find the line that sets url = None before the if data['type'] conditional
lines = source.splitlines()
url_none_idx = next(
(i for i, l in enumerate(lines) if "url = None" in l), None
)
site_if_idx = next(
(i for i, l in enumerate(lines)
if 'data[\'type\'] == "SITE"' in l or 'data["type"] == "SITE"' in l),
None
)
self.assertIsNotNone(url_none_idx,
"_fetch_tree_children_bg has no 'url = None' initializer")
self.assertIsNotNone(site_if_idx,
"_fetch_tree_children_bg has no SITE type conditional")
self.assertLess(url_none_idx, site_if_idx,
"'url = None' must appear before the if data['type'] block")
# ---------------------------------------------------------------------------
# S2: Dead SITE branch removed from _append_list_items
# ---------------------------------------------------------------------------
class TestDeadSiteBranch(unittest.TestCase):
def test_append_list_items_has_no_site_img_branch(self):
"""S2: _append_list_items no longer contains the dead SITE image branch."""
source = inspect.getsource(sb.SharePointApp._append_list_items)
self.assertNotIn(
'item[\'type\'] == "SITE"',
source,
"_append_list_items still contains dead SITE branch"
)
# ---------------------------------------------------------------------------
# S-1: is_breadcrumb parameter removed from _navigate_to_item_data
# ---------------------------------------------------------------------------
class TestIsBreakcrumbRemoved(unittest.TestCase):
def test_no_is_breadcrumb_param(self):
"""S-1: _navigate_to_item_data no longer has an is_breadcrumb parameter."""
sig = inspect.signature(sb.SharePointApp._navigate_to_item_data)
self.assertNotIn(
"is_breadcrumb", sig.parameters,
"_navigate_to_item_data still declares is_breadcrumb"
)
def test_tree_item_param_still_present(self):
"""Regression: tree_item parameter was not accidentally removed."""
sig = inspect.signature(sb.SharePointApp._navigate_to_item_data)
self.assertIn("tree_item", sig.parameters)
if __name__ == "__main__":
unittest.main(verbosity=2)