diff --git a/README.md b/README.md index ce830e1..fbd2a91 100644 --- a/README.md +++ b/README.md @@ -61,6 +61,64 @@ In a **Working Time Log**, mark _Break_ (`is_break`) for any physical break. Reg German users may refer to [this article](https://www.kanzlei-chevalier.de/blog/dienstreise-als-arbeitszeit) for more information. +## Notes on Time Logs + +Each **Working Time Log** _Note_ is either **internal** or **external**: + +| Type | Format | Used for | +|------|--------|----------| +| **Internal** | plain text | internal documentation only | +| **External** | starts with `+`, at least 3 characters after `+` | customer-facing invoice text | + +On save, billable rows without a _Task_ or Jira _Key_ must include an external note with at least 3 characters after `+`. + +### Short rules + +**No note required when any of the following applies:** + +- the row is a break +- a _Task_ is set +- a Jira _Key_ is set + +**External note required when:** + +- the row has a _Project_ and _Billable_ is greater than `0%` +- no _Task_ and no Jira _Key_ + +### Decision flow + +```mermaid +flowchart TD + A[Working Time Log row] --> B{Break?} + B -->|yes| OK[No note required] + B -->|no| C{Task or Jira Key set?} + C -->|yes| OK + C -->|no| D{Billable > 0% and Project set?} + D -->|no| OK + D -->|yes| E{External note +... with 3+ chars?} + E -->|yes| OK + E -->|no| FAIL[Reject: add Task, Jira Key, or external note] +``` + +### Examples + +| Scenario | Result | +|----------|--------| +| `100%`, no _Task_/_Key_, note = `Fixed bug` | Rejected - internal note does not count for billing | +| `100%`, no _Task_/_Key_, note = `+Fixed bug` | OK - external note satisfies billable rule | +| `100%`, no _Task_/_Key_, note = `+ab` | Rejected - external note needs 3+ characters after `+` | +| `100%`, _Task_ set, no note | OK - _Task_ satisfies billable rule | + +## Billable Time Logs + +Billable rows (_Project_ set and _Billable_ greater than `0%`) must include at least one billing reference: + +- a _Task_, or +- a Jira issue _Key_, or +- an external _Note_ (starting with `+`) + +Rows with only an internal note are rejected on billable rows. Billable time is rounded to 5-minute increments when **Timesheets** are created on submit. + ## Further Reading Want to add pretty time logs to your invoice? Check out our [print formats](https://github.com/alyf-de/erpnext_druckformate). diff --git a/working_time/jira_utils.py b/working_time/jira_utils.py index 9352ee3..1ebb7b4 100644 --- a/working_time/jira_utils.py +++ b/working_time/jira_utils.py @@ -2,16 +2,18 @@ def get_jira_issue_url(jira_site, key): - return f"https://{jira_site}/browse/{key}" if key else None + return f"https://{jira_site}/browse/{key}" if key and jira_site else None def get_description(jira_site, key, note): - if key: + if key and jira_site: description = f"{JiraClient(jira_site).get_issue_summary(key)} ({key})" - if note: - description += f":\n\n{note}" - return description.strip() - elif note: - return note + elif key: + description = key else: - return "-" + description = note or "-" + + if key and note: + description += f":\n\n{note}" + + return description.strip() diff --git a/working_time/working_time/doctype/working_time/test_working_time.py b/working_time/working_time/doctype/working_time/test_working_time.py index 5b1be10..c451d25 100644 --- a/working_time/working_time/doctype/working_time/test_working_time.py +++ b/working_time/working_time/doctype/working_time/test_working_time.py @@ -6,7 +6,13 @@ import frappe from frappe import _dict -from working_time.working_time.doctype.working_time.working_time import aggregate_time_logs +from working_time.jira_utils import get_description +from working_time.working_time.doctype.working_time.working_time import ( + aggregate_time_logs, + billable_row_missing_invoice_reference, + get_note_content, + parse_note, +) class TestWorkingTime(unittest.TestCase): @@ -84,6 +90,79 @@ def test_aggregate_time_logs(self): self.assertEqual(project_b["internal_notes"], []) self.assertEqual(project_b["customer_notes"], ["Customer Note 1"]) + def test_aggregate_time_logs_without_jira_site(self): + logs = [ + _dict( + project="Project A", + duration=3600, + billable="100%", + note="Internal note", + ), + ] + + result = aggregate_time_logs(logs) + + project_a = result[("Project A", None, None)] + self.assertEqual(project_a["customer_notes"], []) + self.assertEqual(project_a["internal_notes"], ["Internal note"]) + + def test_parse_note(self): + customer_note, internal_note = parse_note("internal only") + self.assertIsNone(customer_note) + self.assertEqual(internal_note, "internal only") + + customer_note, internal_note = parse_note("+customer") + self.assertEqual(customer_note, "customer") + self.assertIsNone(internal_note) + + def test_get_note_content(self): + self.assertEqual(get_note_content("+ invoice note"), "invoice note") + self.assertEqual(get_note_content("internal note"), "internal note") + self.assertEqual(get_note_content("+ab"), "ab") + self.assertEqual(get_note_content(""), "") + + def test_billable_row_missing_invoice_reference(self): + log = _dict( + billable="100%", + project="Project A", + task=None, + key=None, + note=None, + is_break=0, + ) + self.assertTrue(billable_row_missing_invoice_reference(log)) + + log.note = "plain note" + self.assertTrue(billable_row_missing_invoice_reference(log)) + + log.note = "+invoice note" + self.assertFalse(billable_row_missing_invoice_reference(log)) + + log.note = "+ab" + self.assertTrue(billable_row_missing_invoice_reference(log)) + + log.note = None + log.key = "KEY-1" + self.assertFalse(billable_row_missing_invoice_reference(log)) + + log.key = None + log.task = "TASK-1" + self.assertFalse(billable_row_missing_invoice_reference(log)) + + log.task = None + log.billable = "0%" + self.assertFalse(billable_row_missing_invoice_reference(log)) + + log.billable = "50%" + log.is_break = 1 + self.assertFalse(billable_row_missing_invoice_reference(log)) + + def test_get_description_without_jira_site(self): + self.assertEqual(get_description(None, "KEY-1", None), "KEY-1") + self.assertEqual(get_description(None, "KEY-1", "extra"), "KEY-1:\n\nextra") + self.assertEqual(get_description(None, None, "note"), "note") + self.assertEqual(get_description(None, None, None), "-") + def test_paid_break_totals(self): working_time = self.get_working_time( [ @@ -187,3 +266,109 @@ def test_mandatory_breaks_policy_uses_productive_time(self): missing_break.before_validate() self.assertRaises(frappe.ValidationError, missing_break.validate_mandatory_breaks, policy) + + def test_external_note_minimum_length_on_billable_row(self): + working_time = self.get_working_time( + [ + { + "from_time": "09:00:00", + "to_time": "10:00:00", + "is_break": 0, + "project": "Project A", + "billable": "100%", + }, + ] + ) + working_time.before_validate() + self.assertRaises(frappe.ValidationError, working_time.validate) + + working_time.time_logs[0].note = "+" + self.assertRaises(frappe.ValidationError, working_time.validate) + + working_time.time_logs[0].note = "+xy" + self.assertRaises(frappe.ValidationError, working_time.validate) + + working_time.time_logs[0].note = "+abc" + working_time.validate() + + def test_note_not_required_for_breaks(self): + working_time = self.get_working_time( + [ + { + "from_time": "12:00:00", + "to_time": "12:30:00", + "is_break": 1, + }, + ] + ) + working_time.before_validate() + working_time.validate() + + def test_note_optional_with_task(self): + working_time = self.get_working_time( + [ + { + "from_time": "09:00:00", + "to_time": "10:00:00", + "is_break": 0, + "project": "Project A", + "task": "TASK-1", + "billable": "0%", + }, + ] + ) + working_time.before_validate() + working_time.validate() + + def test_note_optional_with_jira_key(self): + working_time = self.get_working_time( + [ + { + "from_time": "09:00:00", + "to_time": "10:00:00", + "is_break": 0, + "project": "Project A", + "key": "KEY-1", + "billable": "0%", + }, + ] + ) + working_time.before_validate() + working_time.validate() + + def test_billable_row_optional_with_task(self): + working_time = self.get_working_time( + [ + { + "from_time": "09:00:00", + "to_time": "10:00:00", + "is_break": 0, + "project": "Project A", + "task": "TASK-1", + "billable": "100%", + }, + ] + ) + working_time.before_validate() + working_time.validate() + + def test_billable_row_requires_task_jira_key_or_external_note(self): + working_time = self.get_working_time( + [ + { + "from_time": "09:00:00", + "to_time": "10:00:00", + "is_break": 0, + "project": "Project A", + "billable": "100%", + }, + ] + ) + working_time.before_validate() + self.assertRaises(frappe.ValidationError, working_time.validate) + + working_time.time_logs[0].note = "internal note" + self.assertRaises(frappe.ValidationError, working_time.validate) + + working_time.time_logs[0].note = "+invoice note" + working_time.validate() diff --git a/working_time/working_time/doctype/working_time/working_time.py b/working_time/working_time/doctype/working_time/working_time.py index f243eb6..418e2e5 100644 --- a/working_time/working_time/doctype/working_time/working_time.py +++ b/working_time/working_time/doctype/working_time/working_time.py @@ -19,6 +19,7 @@ FIVE_MINUTES = 5 * 60 ONE_HOUR = 60 * 60 WHOLE_DAY_HOURS = 8 +MIN_NOTE_LENGTH = 3 class WorkingTime(Document): @@ -55,14 +56,9 @@ def validate(self): if log.duration and log.duration < 0: frappe.throw(_("Please fix negative duration in row {0}").format(log.idx)) - if ( - log.billable != "0%" - and log.project - and not log.key - and (not log.note or not log.note.strip().startswith("+")) - ): + if billable_row_missing_invoice_reference(log): frappe.throw( - _("Please add an issue key or invoice note to the billable row {0}").format(log.idx) + _("Please add a task, Jira key, or external note to billable row {0}").format(log.idx) ) self.validate_working_time_policy() @@ -382,18 +378,41 @@ def get_billable_duration(log): return log.duration * float(log.billable.rstrip("% ")) / 100 +def has_external_note(note: str | None) -> bool: + stripped_note = note.strip() if note else "" + return stripped_note.startswith("+") and note_has_minimum_content(note) + + +def billable_row_missing_invoice_reference(log) -> bool: + if log.billable == "0%" or not log.project or log.is_break: + return False + + return not log.task and not log.key and not has_external_note(log.note) + + +def get_note_content(note: str | None) -> str: + stripped_note = note.strip() if note else "" + if not stripped_note: + return "" + if stripped_note.startswith("+"): + return stripped_note[1:].strip() + return stripped_note + + +def note_has_minimum_content(note: str | None) -> bool: + return len(get_note_content(note)) >= MIN_NOTE_LENGTH + + def parse_note(note: str | None) -> tuple[str | None, str | None]: """Parse a note into customer note and internal note.""" - customer_note = None - internal_note = None - stripped_note = note.strip() if note else None - if stripped_note: - if stripped_note.startswith("+"): - customer_note = stripped_note[1:].strip() - else: - internal_note = stripped_note + content = get_note_content(note) + if not content: + return None, None + + if note and note.strip().startswith("+"): + return content, None - return customer_note, internal_note + return None, content def calculate_hours(log) -> tuple[float, float]: