diff --git a/backend/common/utils.py b/backend/common/utils.py index 89575200..46ea85ec 100644 --- a/backend/common/utils.py +++ b/backend/common/utils.py @@ -378,6 +378,7 @@ COUNTRIES = ( GOAL_TYPES = ( ("REVENUE", "Revenue"), ("DEALS_CLOSED", "Deals Closed"), + ("ACTIVITIES", "Activities"), ) PERIOD_TYPES = ( diff --git a/backend/common/views/dashboard_views.py b/backend/common/views/dashboard_views.py index 000573d0..bd873b02 100644 --- a/backend/common/views/dashboard_views.py +++ b/backend/common/views/dashboard_views.py @@ -289,7 +289,7 @@ class ApiHomeView(APIView): if is_admin: goal_filter |= Q(assigned_to__isnull=True, team__isnull=True) - active_goals = ( + active_goals = SalesGoal.attach_progress( SalesGoal.objects.filter( org=org, is_active=True, @@ -297,6 +297,7 @@ class ApiHomeView(APIView): period_end__gte=today, ) .filter(goal_filter) + .select_related("assigned_to", "team") .distinct()[:3] ) context["goal_summary"] = [ diff --git a/backend/opportunity/migrations/0016_salesgoal_type_weights_alter_salesgoal_goal_type.py b/backend/opportunity/migrations/0016_salesgoal_type_weights_alter_salesgoal_goal_type.py new file mode 100644 index 00000000..48785a11 --- /dev/null +++ b/backend/opportunity/migrations/0016_salesgoal_type_weights_alter_salesgoal_goal_type.py @@ -0,0 +1,35 @@ +# Generated by Django 6.0.7 on 2026-08-16 09:42 + +from django.db import migrations, models + + +class Migration(migrations.Migration): + dependencies = [ + ("opportunity", "0015_drop_orphan_deal_pipeline_tables"), + ] + + operations = [ + migrations.AddField( + model_name="salesgoal", + name="type_weights", + field=models.JSONField( + blank=True, + default=dict, + help_text='Optional multiplier per opportunity type, e.g. {"RENEWAL": 0.5, "NEW_BUSINESS": 1.5}. A type left out of the map, and a deal with no type at all, counts at its full value. An empty map is an unweighted goal. Not accepted on an ACTIVITIES goal, which has no deal type to weigh.', + verbose_name="Deal Type Weights", + ), + ), + migrations.AlterField( + model_name="salesgoal", + name="goal_type", + field=models.CharField( + choices=[ + ("REVENUE", "Revenue"), + ("DEALS_CLOSED", "Deals Closed"), + ("ACTIVITIES", "Activities"), + ], + max_length=20, + verbose_name="Goal Type", + ), + ), + ] diff --git a/backend/opportunity/models.py b/backend/opportunity/models.py index b5336271..93c9824a 100644 --- a/backend/opportunity/models.py +++ b/backend/opportunity/models.py @@ -2,7 +2,7 @@ from decimal import Decimal from django.core.exceptions import ValidationError from django.db import models -from django.db.models import Q, Sum +from django.db.models import Count, Q, Sum from django.db.models.functions import Coalesce from django.utils import timezone from django.utils.timesince import timesince @@ -456,6 +456,18 @@ class SalesGoal(BaseModel): blank=True, related_name="sales_goals", ) + type_weights = models.JSONField( + _("Deal Type Weights"), + default=dict, + blank=True, + help_text=( + "Optional multiplier per opportunity type, e.g. " + '{"RENEWAL": 0.5, "NEW_BUSINESS": 1.5}. A type left out of the map, ' + "and a deal with no type at all, counts at its full value. An empty " + "map is an unweighted goal. Not accepted on an ACTIVITIES goal, " + "which has no deal type to weigh." + ), + ) is_active = models.BooleanField(default=True) milestone_50_notified = models.BooleanField(default=False) milestone_90_notified = models.BooleanField(default=False) @@ -479,37 +491,244 @@ class SalesGoal(BaseModel): def __str__(self): return f"{self.name} ({self.get_goal_type_display()})" - def compute_progress(self): - """Compute current progress toward this goal from CLOSED_WON opportunities. + def member_profile_ids(self): + """Profile ids whose work counts toward this goal, or None for the org. - Results are cached on the instance to avoid redundant DB queries when - progress_percent and status are accessed in the same request. + `None` is "everybody in the org", which is what an unassigned goal + means. It is distinct from `[]`, an empty team, which counts nothing. """ - if hasattr(self, "_cached_progress"): - return self._cached_progress + if self.assigned_to_id: + return [self.assigned_to_id] + if self.team_id: + return list( + Profile.objects.filter( + user_teams=self.team_id, is_active=True + ).values_list("id", flat=True) + ) + return None + def weight_for(self, deal_type): + """The multiplier this goal applies to one opportunity type. + + A type missing from the map, and a deal saved without a type at all, + weigh 1: an unlisted type is not a type worth nothing, it is a type + nobody chose to re-weigh. `type_weights` is validated on write, so the + values here are already numbers. + """ + if not self.type_weights or not deal_type: + return Decimal("1") + weight = self.type_weights.get(deal_type) + return Decimal("1") if weight is None else Decimal(str(weight)) + + def tally_to_progress(self, tally): + """Fold a per-deal-type tally into this goal's single progress number. + + `tally` maps an opportunity type (or None) to `(amount, count)`. Both + the single-goal query and the batched `attach_progress` build one of + these, so the weighting arithmetic has exactly one definition rather + than a copy per call site. + """ + unit = 0 if self.goal_type == "REVENUE" else 1 + total = Decimal("0") + for deal_type, amounts in tally.items(): + total += self.weight_for(deal_type) * amounts[unit] + return total + + def _opportunity_queryset(self): + """Won deals in this goal's period, one row per deal. + + `Opportunity.assigned_to` is a ManyToMany, so filtering a team goal with + `assigned_to__in=` joins the through table and returns a deal + once per member who holds it. Summing that double-counted a deal shared + by two teammates and inflated the team's attainment. The membership test + is a subquery on `id` instead, which is set membership and cannot + duplicate a row. + """ opps = Opportunity.objects.filter( org=self.org, stage="CLOSED_WON", closed_on__gte=self.period_start, closed_on__lte=self.period_end, ) - if self.assigned_to: - opps = opps.filter(assigned_to=self.assigned_to) - elif self.team: - team_members = Profile.objects.filter(user_teams=self.team, is_active=True) - opps = opps.filter(assigned_to__in=team_members) + member_ids = self.member_profile_ids() + if member_ids is None: + return opps + return opps.filter( + id__in=Opportunity.objects.filter(assigned_to__in=member_ids).values("id") + ) - if self.goal_type == "REVENUE": - result = opps.aggregate(total=Coalesce(Sum("amount"), Decimal("0")))[ - "total" - ] + def _activity_count(self): + """Activities logged by this goal's owners inside the period.""" + from common.models import Activity + + activities = Activity.objects.filter( + org=self.org, + created_at__date__gte=self.period_start, + created_at__date__lte=self.period_end, + ) + member_ids = self.member_profile_ids() + if member_ids is not None: + activities = activities.filter(user_id__in=member_ids) + return Decimal(str(activities.count())) + + def compute_progress(self): + """Compute current progress toward this goal. + + REVENUE and DEALS_CLOSED read CLOSED_WON opportunities in the period; + ACTIVITIES counts logged activity instead, so `type_weights` does not + apply to it and is refused on write. + + Results are cached on the instance to avoid redundant DB queries when + progress_percent and status are accessed in the same request. For a + collection of goals, call `SalesGoal.attach_progress` first: it fills + this same cache for every goal in a fixed number of queries. + """ + if hasattr(self, "_cached_progress"): + return self._cached_progress + + if self.goal_type == "ACTIVITIES": + result = self._activity_count() else: - result = Decimal(str(opps.count())) + # Grouped by type even when unweighted: it is still one query, and + # it keeps both progress paths reading the same tally shape. + tally = { + row["opportunity_type"]: ( + row["amount_total"], + Decimal(str(row["deal_count"])), + ) + for row in self._opportunity_queryset() + .values("opportunity_type") + .annotate( + amount_total=Coalesce(Sum("amount"), Decimal("0")), + deal_count=Count("id"), + ) + } + result = self.tally_to_progress(tally) self._cached_progress = result return result + @classmethod + def attach_progress(cls, goals): + """Populate every goal's progress cache in a fixed number of queries. + + Serializing a list called `compute_progress()` once per goal, so a page + of N goals cost N aggregate queries (the web list asks for up to 1000). + This reads the won deals and activities that span the whole collection's + period once, then does the per-goal arithmetic in Python through the + same `tally_to_progress` the single-goal path uses. + + Returns the list it was given, so callers can wrap a queryset inline. + """ + goals = list(goals) + pending = [g for g in goals if not hasattr(g, "_cached_progress")] + if not pending: + return goals + + org_ids = {g.org_id for g in pending} + period_start = min(g.period_start for g in pending) + period_end = max(g.period_end for g in pending) + + # One query for every team referenced, so a page of team goals does not + # re-read the same membership per goal. + team_ids = {g.team_id for g in pending if g.team_id and not g.assigned_to_id} + members_by_team = {team_id: [] for team_id in team_ids} + if team_ids: + for team_id, profile_id in Profile.objects.filter( + user_teams__in=team_ids, is_active=True + ).values_list("user_teams__id", "id"): + if team_id in members_by_team: + members_by_team[team_id].append(profile_id) + + deal_goals = [g for g in pending if g.goal_type != "ACTIVITIES"] + deals = [] + if deal_goals: + # One row per (deal, assignee); an unassigned deal still arrives + # once, with a null profile, because an org-wide goal counts it. + deals = list( + Opportunity.objects.filter( + org_id__in=org_ids, + stage="CLOSED_WON", + closed_on__gte=period_start, + closed_on__lte=period_end, + ).values_list( + "id", + "org_id", + "closed_on", + "opportunity_type", + "amount", + "assigned_to__id", + ) + ) + + activity_goals = [g for g in pending if g.goal_type == "ACTIVITIES"] + activities = [] + if activity_goals: + from common.models import Activity + + activities = list( + Activity.objects.filter( + org_id__in=org_ids, + created_at__date__gte=period_start, + created_at__date__lte=period_end, + ).values_list("org_id", "created_at", "user_id") + ) + + for goal in pending: + if goal.assigned_to_id: + member_ids = {goal.assigned_to_id} + elif goal.team_id: + member_ids = set(members_by_team.get(goal.team_id, [])) + else: + member_ids = None + + if goal.goal_type == "ACTIVITIES": + goal._cached_progress = Decimal( + str( + sum( + 1 + for org_id, created_at, user_id in activities + if org_id == goal.org_id + and goal.period_start + <= timezone.localtime(created_at).date() + <= goal.period_end + and (member_ids is None or user_id in member_ids) + ) + ) + ) + continue + + tally = {} + counted = set() + for ( + deal_id, + org_id, + closed_on, + deal_type, + amount, + profile_id, + ) in deals: + if org_id != goal.org_id or deal_id in counted: + continue + if not (goal.period_start <= closed_on <= goal.period_end): + continue + if member_ids is not None and profile_id not in member_ids: + continue + # `counted` is what stops a deal shared by two teammates from + # landing in the tally twice, the same duplicate the single-goal + # subquery avoids in SQL. + counted.add(deal_id) + amount_total, deal_count = tally.get( + deal_type, (Decimal("0"), Decimal("0")) + ) + tally[deal_type] = ( + amount_total + (amount or Decimal("0")), + deal_count + 1, + ) + goal._cached_progress = goal.tally_to_progress(tally) + + return goals + @property def progress_percent(self): """Progress as an integer percentage, capped at 100.""" diff --git a/backend/opportunity/serializer.py b/backend/opportunity/serializer.py index 23069033..494df503 100644 --- a/backend/opportunity/serializer.py +++ b/backend/opportunity/serializer.py @@ -10,6 +10,7 @@ from common.serializer import ( TeamsSerializer, UserSerializer, ) +from common.utils import OPPORTUNITY_TYPES from contacts.serializer import ContactSerializer from invoices.serializer import ProductSerializer from opportunity.models import ( @@ -20,6 +21,11 @@ from opportunity.models import ( ) from opportunity.workflow import AMOUNT_REQUIRED_STAGES, CLOSED_STAGES +# A deal type multiplier above this is a data-entry slip, not a quota policy. +# The ceiling exists so one typo cannot make a goal unreachable or trivially +# met; it is deliberately loose enough for any real weighting. +MAX_DEAL_TYPE_WEIGHT = 100 + # Note: Removed unused serializer properties that were computed but never used by frontend: # - get_team_users, get_team_and_assigned_users, get_assigned_users_not_in_teams @@ -469,6 +475,7 @@ class SalesGoalSerializer(serializers.ModelSerializer): "assigned_to_detail", "team", "team_detail", + "type_weights", "is_active", "milestone_50_notified", "milestone_90_notified", @@ -525,6 +532,7 @@ class SalesGoalCreateSerializer(serializers.ModelSerializer): "period_end", "assigned_to", "team", + "type_weights", "is_active", ) @@ -553,6 +561,47 @@ class SalesGoalCreateSerializer(serializers.ModelSerializer): ) return value + def validate_type_weights(self, value): + """Deal type multipliers: a JSON object of known type to a number >= 0. + + Left unchecked this is a free-form JSON column, so a typo like + ``{"RENEWALS": 0.5}`` would be stored happily and then silently weigh + nothing, and a string value would raise from ``Decimal`` deep inside + progress computation on every later read. Both are refused here, where + the caller still gets a 400 that names the problem. + """ + if value in (None, ""): + return {} + if not isinstance(value, dict): + raise serializers.ValidationError( + "Deal type weights must be an object mapping a deal type to a number." + ) + known = {choice[0] for choice in OPPORTUNITY_TYPES} + cleaned = {} + for deal_type, weight in value.items(): + if deal_type not in known: + raise serializers.ValidationError( + f"'{deal_type}' is not a deal type. Choose from: " + f"{', '.join(sorted(known))}." + ) + # bool is an int in Python, and True would quietly become a weight + # of 1, so it is excluded rather than coerced. + if isinstance(weight, bool) or not isinstance(weight, (int, float)): + raise serializers.ValidationError( + f"The weight for '{deal_type}' must be a number." + ) + if weight < 0: + raise serializers.ValidationError( + f"The weight for '{deal_type}' cannot be negative." + ) + if weight > MAX_DEAL_TYPE_WEIGHT: + raise serializers.ValidationError( + f"The weight for '{deal_type}' cannot be above " + f"{MAX_DEAL_TYPE_WEIGHT}." + ) + cleaned[deal_type] = weight + return cleaned + def validate(self, data): period_start = data.get( "period_start", getattr(self.instance, "period_start", None) @@ -569,4 +618,53 @@ class SalesGoalCreateSerializer(serializers.ModelSerializer): raise serializers.ValidationError( {"target_value": "Target value must be greater than 0."} ) + + # A goal counts one person's work or one team's, never both. The model + # let both columns be set at once and `compute_progress` silently + # preferred `assigned_to`, so a goal could name a team on screen while + # scoring a single rep. + assigned_to = data.get( + "assigned_to", getattr(self.instance, "assigned_to", None) + ) + team = data.get("team", getattr(self.instance, "team", None)) + if assigned_to and team: + raise serializers.ValidationError( + {"team": "A goal belongs to a person or to a team, not to both."} + ) + + goal_type = data.get("goal_type", getattr(self.instance, "goal_type", None)) + type_weights = data.get( + "type_weights", getattr(self.instance, "type_weights", None) + ) + if goal_type == "ACTIVITIES" and type_weights: + raise serializers.ValidationError( + { + "type_weights": ( + "An activities goal counts logged activity, which has no " + "deal type to weigh." + ) + } + ) return data + + def update(self, instance, validated_data): + """Save, clearing milestone flags when the bar itself moved. + + The three `milestone_*_notified` flags are one-shot per goal. That is + right while the goal is fixed and wrong the moment an admin raises the + target or shifts the period: the goal has already "notified" at 100% + against a bar that no longer exists, so it could never announce the new + one. Editing a name, an assignee or the paused flag is not a new bar and + leaves the history alone. + """ + moved = [ + field + for field in ("target_value", "period_start", "period_end") + if field in validated_data + and validated_data[field] != getattr(instance, field) + ] + if moved: + validated_data["milestone_50_notified"] = False + validated_data["milestone_90_notified"] = False + validated_data["milestone_100_notified"] = False + return super().update(instance, validated_data) diff --git a/backend/opportunity/tests/test_sales_goals.py b/backend/opportunity/tests/test_sales_goals.py index 5cc27f0d..0d260857 100644 --- a/backend/opportunity/tests/test_sales_goals.py +++ b/backend/opportunity/tests/test_sales_goals.py @@ -687,3 +687,663 @@ class TestGoalMilestoneTask: # Already had 50% notified, should not re-send assert goal.milestone_50_notified is True assert mock_send.call_count == 0 + + +# ---- Weighted goals, activity goals, and batched progress ---- # + + +def _won_deal(org, user, amount, profiles, closed_on=None, deal_type=None): + """A CLOSED_WON opportunity of `deal_type`, assigned to `profiles`.""" + opp = _create_won_opportunity(org, user, amount, closed_on=closed_on) + if deal_type: + opp.opportunity_type = deal_type + opp.save(update_fields=["opportunity_type"]) + for profile in profiles: + opp.assigned_to.add(profile) + return opp + + +@pytest.mark.django_db +class TestTeamProgressCountsEachDealOnce: + """A team goal joins opportunities to members, and a join can duplicate.""" + + def test_a_deal_shared_by_two_team_members_counts_once( + self, team_goal, org_a, admin_user, admin_profile, user_profile, team_a + ): + team_a.users.add(admin_profile, user_profile) + + _won_deal(org_a, admin_user, 50000, [admin_profile, user_profile]) + + assert team_goal.compute_progress() == Decimal("50000") + + def test_a_deal_shared_by_two_team_members_is_one_closed_deal( + self, org_a, team_a, admin_user, admin_profile, user_profile + ): + team_a.users.add(admin_profile, user_profile) + today = timezone.localdate() + goal = SalesGoal.objects.create( + name="Team Deals", + goal_type="DEALS_CLOSED", + target_value=Decimal("10"), + period_type="MONTHLY", + period_start=today.replace(day=1), + period_end=today + timedelta(days=1), + team=team_a, + org=org_a, + ) + + _won_deal(org_a, admin_user, 50000, [admin_profile, user_profile]) + + assert goal.compute_progress() == Decimal("1") + + +@pytest.mark.django_db +class TestActivityGoals: + def test_progress_counts_the_assignees_activities_in_the_period( + self, org_a, admin_profile + ): + from common.models import Activity + + today = timezone.localdate() + goal = SalesGoal.objects.create( + name="Monthly Activities", + goal_type="ACTIVITIES", + target_value=Decimal("20"), + period_type="MONTHLY", + period_start=today.replace(day=1), + period_end=today + timedelta(days=1), + assigned_to=admin_profile, + org=org_a, + ) + for i in range(3): + Activity.objects.create( + user=admin_profile, + action="CREATE", + entity_type="Lead", + entity_id=goal.id, + entity_name=f"Lead {i}", + org=org_a, + ) + + assert goal.compute_progress() == Decimal("3") + + def test_progress_ignores_another_persons_activity( + self, org_a, admin_profile, user_profile + ): + from common.models import Activity + + today = timezone.localdate() + goal = SalesGoal.objects.create( + name="Monthly Activities", + goal_type="ACTIVITIES", + target_value=Decimal("20"), + period_type="MONTHLY", + period_start=today.replace(day=1), + period_end=today + timedelta(days=1), + assigned_to=admin_profile, + org=org_a, + ) + Activity.objects.create( + user=user_profile, + action="CREATE", + entity_type="Lead", + entity_id=goal.id, + org=org_a, + ) + + assert goal.compute_progress() == Decimal("0") + + def test_progress_ignores_an_activity_outside_the_period( + self, org_a, admin_profile + ): + from common.models import Activity + + today = timezone.localdate() + goal = SalesGoal.objects.create( + name="Monthly Activities", + goal_type="ACTIVITIES", + target_value=Decimal("20"), + period_type="MONTHLY", + period_start=today.replace(day=1), + period_end=today + timedelta(days=1), + assigned_to=admin_profile, + org=org_a, + ) + stale = Activity.objects.create( + user=admin_profile, + action="CREATE", + entity_type="Lead", + entity_id=goal.id, + org=org_a, + ) + Activity.objects.filter(pk=stale.pk).update( + created_at=timezone.now() - timedelta(days=90) + ) + + assert goal.compute_progress() == Decimal("0") + + +@pytest.mark.django_db +class TestWeightedGoals: + def test_a_renewal_at_half_weight_contributes_half_its_amount( + self, org_a, admin_user, admin_profile + ): + today = timezone.localdate() + goal = SalesGoal.objects.create( + name="Weighted Revenue", + goal_type="REVENUE", + target_value=Decimal("100000"), + period_type="MONTHLY", + period_start=today.replace(day=1), + period_end=today + timedelta(days=1), + assigned_to=admin_profile, + org=org_a, + type_weights={"RENEWAL": 0.5, "NEW_BUSINESS": 1.5}, + ) + + _won_deal(org_a, admin_user, 20000, [admin_profile], deal_type="RENEWAL") + _won_deal(org_a, admin_user, 20000, [admin_profile], deal_type="NEW_BUSINESS") + + assert goal.compute_progress() == Decimal("40000") + + def test_a_type_absent_from_the_map_keeps_its_full_weight( + self, org_a, admin_user, admin_profile + ): + today = timezone.localdate() + goal = SalesGoal.objects.create( + name="Weighted Revenue", + goal_type="REVENUE", + target_value=Decimal("100000"), + period_type="MONTHLY", + period_start=today.replace(day=1), + period_end=today + timedelta(days=1), + assigned_to=admin_profile, + org=org_a, + type_weights={"RENEWAL": 0.5}, + ) + + _won_deal(org_a, admin_user, 20000, [admin_profile], deal_type="UPSELL") + + assert goal.compute_progress() == Decimal("20000") + + def test_a_deal_with_no_type_keeps_its_full_weight( + self, org_a, admin_user, admin_profile + ): + today = timezone.localdate() + goal = SalesGoal.objects.create( + name="Weighted Revenue", + goal_type="REVENUE", + target_value=Decimal("100000"), + period_type="MONTHLY", + period_start=today.replace(day=1), + period_end=today + timedelta(days=1), + assigned_to=admin_profile, + org=org_a, + type_weights={"RENEWAL": 0.5}, + ) + + _won_deal(org_a, admin_user, 20000, [admin_profile]) + + assert goal.compute_progress() == Decimal("20000") + + def test_weights_scale_a_deals_closed_goal_too( + self, org_a, admin_user, admin_profile + ): + today = timezone.localdate() + goal = SalesGoal.objects.create( + name="Weighted Deals", + goal_type="DEALS_CLOSED", + target_value=Decimal("10"), + period_type="MONTHLY", + period_start=today.replace(day=1), + period_end=today + timedelta(days=1), + assigned_to=admin_profile, + org=org_a, + type_weights={"RENEWAL": 0.5}, + ) + + _won_deal(org_a, admin_user, 20000, [admin_profile], deal_type="RENEWAL") + _won_deal(org_a, admin_user, 30000, [admin_profile], deal_type="RENEWAL") + + assert goal.compute_progress() == Decimal("1") + + def test_an_empty_weight_map_leaves_progress_unweighted( + self, goal_revenue, org_a, admin_user, admin_profile + ): + _won_deal(org_a, admin_user, 25000, [admin_profile], deal_type="RENEWAL") + + assert goal_revenue.type_weights == {} + assert goal_revenue.compute_progress() == Decimal("25000") + + def test_a_zero_weight_drops_the_type_from_the_total( + self, org_a, admin_user, admin_profile + ): + today = timezone.localdate() + goal = SalesGoal.objects.create( + name="New business only", + goal_type="REVENUE", + target_value=Decimal("100000"), + period_type="MONTHLY", + period_start=today.replace(day=1), + period_end=today + timedelta(days=1), + assigned_to=admin_profile, + org=org_a, + type_weights={"RENEWAL": 0}, + ) + + _won_deal(org_a, admin_user, 20000, [admin_profile], deal_type="RENEWAL") + _won_deal(org_a, admin_user, 5000, [admin_profile], deal_type="NEW_BUSINESS") + + assert goal.compute_progress() == Decimal("5000") + + +# ---- Batched progress ---- # + + +def _past_goal(org, name, target, achieved_by, days_ago, profile=None, team=None): + """A finished goal whose period closed `days_ago`.""" + end = timezone.localdate() - timedelta(days=days_ago) + return SalesGoal.objects.create( + name=name, + goal_type="REVENUE", + target_value=Decimal(str(target)), + period_type="MONTHLY", + period_start=end - timedelta(days=29), + period_end=end, + assigned_to=profile, + team=team, + org=org, + ) + + +@pytest.mark.django_db +class TestAttachProgressIsBatched: + def test_batched_progress_matches_the_single_goal_query( + self, org_a, admin_user, admin_profile, user_profile, team_a + ): + team_a.users.add(admin_profile, user_profile) + today = timezone.localdate() + shared = SalesGoal.objects.create( + name="Team Revenue", + goal_type="REVENUE", + target_value=Decimal("100000"), + period_type="MONTHLY", + period_start=today.replace(day=1), + period_end=today + timedelta(days=1), + team=team_a, + org=org_a, + type_weights={"RENEWAL": 0.5}, + ) + mine = SalesGoal.objects.create( + name="My Deals", + goal_type="DEALS_CLOSED", + target_value=Decimal("5"), + period_type="MONTHLY", + period_start=today.replace(day=1), + period_end=today + timedelta(days=1), + assigned_to=admin_profile, + org=org_a, + ) + _won_deal( + org_a, admin_user, 20000, [admin_profile, user_profile], deal_type="RENEWAL" + ) + _won_deal(org_a, admin_user, 30000, [admin_profile]) + + one_at_a_time = [ + SalesGoal.objects.get(pk=shared.pk).compute_progress(), + SalesGoal.objects.get(pk=mine.pk).compute_progress(), + ] + batched = SalesGoal.attach_progress( + SalesGoal.objects.filter(pk__in=[shared.pk, mine.pk]).order_by("name") + ) + + assert one_at_a_time == [Decimal("40000"), Decimal("2")] + assert [g.compute_progress() for g in batched] == [ + Decimal("2"), + Decimal("40000"), + ] + + def test_batched_activity_progress_matches_the_single_goal_query( + self, org_a, admin_profile + ): + from common.models import Activity + + today = timezone.localdate() + goal = SalesGoal.objects.create( + name="Activities", + goal_type="ACTIVITIES", + target_value=Decimal("10"), + period_type="MONTHLY", + period_start=today.replace(day=1), + period_end=today + timedelta(days=1), + assigned_to=admin_profile, + org=org_a, + ) + for i in range(4): + Activity.objects.create( + user=admin_profile, + action="CREATE", + entity_type="Lead", + entity_id=goal.id, + org=org_a, + ) + + batched = SalesGoal.attach_progress(SalesGoal.objects.filter(pk=goal.pk)) + + assert batched[0].compute_progress() == Decimal("4") + assert SalesGoal.objects.get(pk=goal.pk).compute_progress() == Decimal("4") + + def test_listing_more_goals_does_not_cost_more_queries( + self, admin_client, org_a, admin_profile + ): + from django.db import connection + from django.test.utils import CaptureQueriesContext + + def count_queries(): + with CaptureQueriesContext(connection) as ctx: + response = admin_client.get("/api/opportunities/goals/") + assert response.status_code == 200 + return len(ctx.captured_queries) + + for i in range(3): + _past_goal(org_a, f"Old {i}", 1000, None, 30 + i, profile=admin_profile) + few = count_queries() + + for i in range(9): + _past_goal(org_a, f"More {i}", 1000, None, 60 + i, profile=admin_profile) + many = count_queries() + + assert many == few, ( + f"query count grew from {few} to {many} when the page grew from 3 " + "goals to 12: progress is still being computed one goal at a time" + ) + + +# ---- Write validation ---- # + + +@pytest.mark.django_db +class TestGoalWriteValidation: + def _payload(self, **overrides): + today = timezone.localdate() + payload = { + "name": "Q3 Revenue", + "goal_type": "REVENUE", + "target_value": "50000", + "period_type": "QUARTERLY", + "period_start": str(today), + "period_end": str(today + timedelta(days=90)), + } + payload.update(overrides) + return payload + + def test_rejects_a_goal_assigned_to_both_a_person_and_a_team( + self, admin_client, admin_profile, team_a + ): + response = admin_client.post( + "/api/opportunities/goals/", + self._payload(assigned_to=str(admin_profile.id), team=str(team_a.id)), + content_type="application/json", + ) + + assert response.status_code == 400 + assert "team" in str(response.json()["errors"]) + + def test_accepts_a_weight_map_over_known_deal_types(self, admin_client, org_a): + response = admin_client.post( + "/api/opportunities/goals/", + self._payload(type_weights={"RENEWAL": 0.5, "NEW_BUSINESS": 1.5}), + content_type="application/json", + ) + + assert response.status_code == 201 + assert SalesGoal.objects.get(name="Q3 Revenue").type_weights == { + "RENEWAL": 0.5, + "NEW_BUSINESS": 1.5, + } + + def test_rejects_a_weight_for_an_unknown_deal_type(self, admin_client): + response = admin_client.post( + "/api/opportunities/goals/", + self._payload(type_weights={"NOT_A_TYPE": 2}), + content_type="application/json", + ) + + assert response.status_code == 400 + assert "NOT_A_TYPE" in str(response.json()["errors"]) + + def test_rejects_a_negative_weight(self, admin_client): + response = admin_client.post( + "/api/opportunities/goals/", + self._payload(type_weights={"RENEWAL": -1}), + content_type="application/json", + ) + + assert response.status_code == 400 + + def test_rejects_a_non_numeric_weight(self, admin_client): + response = admin_client.post( + "/api/opportunities/goals/", + self._payload(type_weights={"RENEWAL": "heavy"}), + content_type="application/json", + ) + + assert response.status_code == 400 + + def test_rejects_a_weight_map_that_is_not_a_map(self, admin_client): + response = admin_client.post( + "/api/opportunities/goals/", + self._payload(type_weights=["RENEWAL"]), + content_type="application/json", + ) + + assert response.status_code == 400 + + def test_rejects_deal_type_weights_on_an_activities_goal(self, admin_client): + response = admin_client.post( + "/api/opportunities/goals/", + self._payload(goal_type="ACTIVITIES", type_weights={"RENEWAL": 0.5}), + content_type="application/json", + ) + + assert response.status_code == 400 + + def test_accepts_an_activities_goal(self, admin_client): + response = admin_client.post( + "/api/opportunities/goals/", + self._payload(goal_type="ACTIVITIES", target_value="40"), + content_type="application/json", + ) + + assert response.status_code == 201 + assert SalesGoal.objects.get(name="Q3 Revenue").goal_type == "ACTIVITIES" + + +# ---- Milestone flags follow the target ---- # + + +@pytest.mark.django_db +class TestMilestoneFlagsFollowTheTarget: + @pytest.fixture + def notified_goal(self, goal_revenue): + goal_revenue.milestone_50_notified = True + goal_revenue.milestone_90_notified = True + goal_revenue.milestone_100_notified = True + goal_revenue.save() + return goal_revenue + + def test_raising_the_target_clears_the_flags(self, admin_client, notified_goal): + response = admin_client.put( + f"/api/opportunities/goals/{notified_goal.id}/", + {"target_value": "250000"}, + content_type="application/json", + ) + + assert response.status_code == 200 + notified_goal.refresh_from_db() + assert notified_goal.milestone_50_notified is False + assert notified_goal.milestone_90_notified is False + assert notified_goal.milestone_100_notified is False + + def test_moving_the_period_clears_the_flags(self, admin_client, notified_goal): + response = admin_client.put( + f"/api/opportunities/goals/{notified_goal.id}/", + {"period_end": str(notified_goal.period_end + timedelta(days=30))}, + content_type="application/json", + ) + + assert response.status_code == 200 + notified_goal.refresh_from_db() + assert notified_goal.milestone_100_notified is False + + def test_renaming_the_goal_keeps_the_flags(self, admin_client, notified_goal): + response = admin_client.put( + f"/api/opportunities/goals/{notified_goal.id}/", + {"name": "Renamed"}, + content_type="application/json", + ) + + assert response.status_code == 200 + notified_goal.refresh_from_db() + assert notified_goal.name == "Renamed" + assert notified_goal.milestone_100_notified is True + + def test_resubmitting_the_same_target_keeps_the_flags( + self, admin_client, notified_goal + ): + response = admin_client.put( + f"/api/opportunities/goals/{notified_goal.id}/", + {"target_value": str(notified_goal.target_value)}, + content_type="application/json", + ) + + assert response.status_code == 200 + notified_goal.refresh_from_db() + assert notified_goal.milestone_100_notified is True + + +# ---- Attainment history ---- # + + +@pytest.mark.django_db +class TestGoalHistory: + def test_returns_a_finished_period_with_its_attainment( + self, admin_client, org_a, admin_user, admin_profile + ): + goal = _past_goal(org_a, "Last month", 40000, None, 5, profile=admin_profile) + _won_deal(org_a, admin_user, 30000, [admin_profile], closed_on=goal.period_end) + + response = admin_client.get("/api/opportunities/goals/history/") + + assert response.status_code == 200 + periods = response.json()["history"] + assert len(periods) == 1 + assert periods[0]["target"] == 40000.0 + assert periods[0]["achieved"] == 30000.0 + assert periods[0]["percent"] == 75 + assert periods[0]["goals_count"] == 1 + assert periods[0]["attained_count"] == 0 + assert periods[0]["goals"][0]["name"] == "Last month" + + def test_counts_a_goal_that_met_its_target_as_attained( + self, admin_client, org_a, admin_user, admin_profile + ): + goal = _past_goal(org_a, "Last month", 20000, None, 5, profile=admin_profile) + _won_deal(org_a, admin_user, 25000, [admin_profile], closed_on=goal.period_end) + + response = admin_client.get("/api/opportunities/goals/history/") + + period = response.json()["history"][0] + assert period["attained_count"] == 1 + # Uncapped: see TestHistoryKeepsItsUnitsApart for why a settled period + # reports what it actually did rather than stopping at the target. + assert period["percent"] == 125 + + def test_excludes_a_period_that_has_not_finished( + self, admin_client, goal_revenue, org_a, admin_profile + ): + _past_goal(org_a, "Done", 1000, None, 5, profile=admin_profile) + + response = admin_client.get("/api/opportunities/goals/history/") + + names = [g["name"] for p in response.json()["history"] for g in p["goals"]] + assert names == ["Done"] + + def test_groups_goals_that_share_a_period_and_orders_newest_first( + self, admin_client, org_a, admin_profile + ): + _past_goal(org_a, "Older", 1000, None, 60, profile=admin_profile) + _past_goal(org_a, "Newer A", 1000, None, 5, profile=admin_profile) + _past_goal(org_a, "Newer B", 2000, None, 5, profile=admin_profile) + + response = admin_client.get("/api/opportunities/goals/history/") + + periods = response.json()["history"] + assert len(periods) == 2 + assert periods[0]["goals_count"] == 2 + assert periods[0]["target"] == 3000.0 + assert periods[1]["goals_count"] == 1 + + def test_hides_another_persons_finished_goal_from_a_member( + self, user_client, org_a, admin_profile, user_profile + ): + _past_goal(org_a, "Admin only", 1000, None, 5, profile=admin_profile) + _past_goal(org_a, "Mine", 1000, None, 5, profile=user_profile) + + response = user_client.get("/api/opportunities/goals/history/") + + assert response.status_code == 200 + names = [g["name"] for p in response.json()["history"] for g in p["goals"]] + assert names == ["Mine"] + + def test_does_not_leak_another_orgs_history( + self, org_b_client, org_a, admin_profile + ): + _past_goal(org_a, "Org A only", 1000, None, 5, profile=admin_profile) + + response = org_b_client.get("/api/opportunities/goals/history/") + + assert response.status_code == 200 + assert response.json()["history"] == [] + + +@pytest.mark.django_db +class TestHistoryKeepsItsUnitsApart: + def test_does_not_pool_a_revenue_target_with_a_deals_target( + self, admin_client, org_a, admin_profile + ): + end = timezone.localdate() - timedelta(days=5) + common = { + "period_type": "MONTHLY", + "period_start": end - timedelta(days=29), + "period_end": end, + "assigned_to": admin_profile, + "org": org_a, + } + SalesGoal.objects.create( + name="Revenue", goal_type="REVENUE", target_value=Decimal("50000"), **common + ) + SalesGoal.objects.create( + name="Deals", goal_type="DEALS_CLOSED", target_value=Decimal("9"), **common + ) + + response = admin_client.get("/api/opportunities/goals/history/") + + rows = response.json()["history"] + # One row per unit, never a single row claiming a target of 50009. + assert {r["goal_type"] for r in rows} == {"REVENUE", "DEALS_CLOSED"} + by_type = {r["goal_type"]: r for r in rows} + assert by_type["REVENUE"]["target"] == 50000.0 + assert by_type["DEALS_CLOSED"]["target"] == 9.0 + + def test_reports_attainment_above_target_rather_than_capping_it( + self, admin_client, org_a, admin_user, admin_profile + ): + goal = _past_goal(org_a, "Beat it", 20000, None, 5, profile=admin_profile) + _won_deal(org_a, admin_user, 25000, [admin_profile], closed_on=goal.period_end) + + response = admin_client.get("/api/opportunities/goals/history/") + + # A settled period that came in 25% over is not the same result as one + # that landed exactly on target, and capping made them identical. + assert response.json()["history"][0]["percent"] == 125 diff --git a/backend/opportunity/urls.py b/backend/opportunity/urls.py index 7cd69245..051d20e0 100644 --- a/backend/opportunity/urls.py +++ b/backend/opportunity/urls.py @@ -3,6 +3,7 @@ from django.urls import path from opportunity.views.aging_views import StageAgingConfigView from opportunity.views.goal_views import ( SalesGoalDetailView, + SalesGoalHistoryView, SalesGoalLeaderboardView, SalesGoalListView, ) @@ -31,6 +32,7 @@ urlpatterns = [ path("aging-config/", StageAgingConfigView.as_view()), path("goals/", SalesGoalListView.as_view()), path("goals/leaderboard/", SalesGoalLeaderboardView.as_view()), + path("goals/history/", SalesGoalHistoryView.as_view()), path("goals//", SalesGoalDetailView.as_view()), path("/", OpportunityDetailView.as_view()), path("/move/", OpportunityMoveView.as_view()), diff --git a/backend/opportunity/views/goal_views.py b/backend/opportunity/views/goal_views.py index 87de3493..0042adf2 100644 --- a/backend/opportunity/views/goal_views.py +++ b/backend/opportunity/views/goal_views.py @@ -1,3 +1,5 @@ +from decimal import Decimal + from django.db.models import Q from django.utils import timezone from rest_framework import status @@ -68,6 +70,9 @@ class SalesGoalListView(APIView, LimitOffsetPagination): def get(self, request, *args, **kwargs): queryset = self.get_queryset(request) results = self.paginate_queryset(queryset, request, view=self) + # Without this the serializer's progress fields run one aggregate query + # per goal, and the web list asks for up to 1000 of them in a page. + SalesGoal.attach_progress(results) serializer = SalesGoalSerializer(results, many=True) total_count = self.count @@ -215,6 +220,8 @@ class SalesGoalLeaderboardView(APIView): if not _sees_every_goal(request): goals = goals.filter(_visible_to(request.profile)).distinct() + goals = SalesGoal.attach_progress(goals) + leaderboard = [] for goal in goals: # compute_progress() results are cached on the instance @@ -254,3 +261,125 @@ class SalesGoalLeaderboardView(APIView): entry["rank"] = i return Response({"leaderboard": leaderboard}) + + +class SalesGoalHistoryView(APIView): + """Finished goal periods with what each one actually attained. + + The list endpoint answers "how are we doing now"; nothing answered "how did + we do". Goals whose period has closed stay queryable there, but only one at + a time and with no rollup, so a manager could not see attainment move across + quarters. + + Progress is recomputed from the won deals of each closed period rather than + read from a stored snapshot. Those deals are themselves history: their + `closed_on` is in the past and cannot move into or out of a finished window, + so the recomputed number is the same one a snapshot would have frozen, and + it costs no extra table to keep correct. + + Narrowed by `_visible_to` for a non-admin, the same predicate the list and + the leaderboard use. A member reads their own attainment history, not the + org's. + """ + + permission_classes = (IsAuthenticated, HasOrgContext) + + #: Periods returned when the caller does not ask for a number, and the most + #: it will return at once. The window bounds the deal scan behind + #: `attach_progress`: without it, an org in its fifth year would load every + #: won deal it has ever had to render one page. + DEFAULT_PERIODS = 12 + MAX_PERIODS = 60 + + def _requested_periods(self, request): + raw = request.query_params.get("periods") + if not raw: + return self.DEFAULT_PERIODS + try: + value = int(raw) + except (TypeError, ValueError): + return self.DEFAULT_PERIODS + return max(1, min(value, self.MAX_PERIODS)) + + def get(self, request, *args, **kwargs): + org = request.profile.org + today = timezone.localdate() + limit = self._requested_periods(request) + + finished = SalesGoal.objects.filter(org=org, period_end__lt=today) + if not _sees_every_goal(request): + finished = finished.filter(_visible_to(request.profile)).distinct() + + # A row is a period AND a goal type, never a period alone. Grouping on + # the period by itself pooled a revenue target in currency with a + # deals-closed target in deals and printed the sum as money: a month + # holding a $340,000 quota and an 18-deal quota reported a target of + # $340,018. Totals only mean anything within one unit. + # + # Resolved before the goals are read so that both the goal query and the + # deal scan behind `attach_progress` stay bounded. + windows = list( + finished.values_list( + "period_start", "period_end", "period_type", "goal_type" + ) + .distinct() + .order_by("-period_end", "-period_start", "goal_type")[:limit] + ) + if not windows: + return Response({"history": [], "periods_returned": 0}) + + goals = list( + finished.filter( + period_start__gte=min(w[0] for w in windows), + period_end__lte=max(w[1] for w in windows), + ) + .select_related("assigned_to", "assigned_to__user", "team") + .order_by("name") + ) + SalesGoal.attach_progress(goals) + + by_window = {window: [] for window in windows} + for goal in goals: + key = ( + goal.period_start, + goal.period_end, + goal.period_type, + goal.goal_type, + ) + if key in by_window: + by_window[key].append(goal) + + history = [] + for window in windows: + start, end, period_type, goal_type = window + window_goals = by_window[window] + target = sum((g.target_value for g in window_goals), Decimal("0")) + achieved = sum((g.compute_progress() for g in window_goals), Decimal("0")) + history.append( + { + "period_start": start, + "period_end": end, + "period_type": period_type, + "goal_type": goal_type, + "goals_count": len(window_goals), + # Attainment is per goal, not per pooled total: three reps, + # two of whom missed while the third doubled up, is not the + # same story as "the team made its number". + "attained_count": sum( + 1 + for g in window_goals + if g.target_value and g.compute_progress() >= g.target_value + ), + "target": float(target), + "achieved": float(achieved), + # Uncapped, unlike the live `progress_percent`. A settled + # period that came in 25% over is a different result from one + # that landed exactly on target, and capping made the two + # read identically in the one view where the difference is + # final. + "percent": (int(achieved / target * 100) if target else 0), + "goals": SalesGoalSerializer(window_goals, many=True).data, + } + ) + + return Response({"history": history, "periods_returned": len(history)}) diff --git a/backend/pyproject.toml b/backend/pyproject.toml index 33e04865..a44a6fc8 100644 --- a/backend/pyproject.toml +++ b/backend/pyproject.toml @@ -4,7 +4,7 @@ build-backend = "hatchling.build" [project] name = "django-crm" -version = "1.7.0" +version = "1.8.0" # The one-line summary PyPI shows under the project title and in search results, # so it carries the terms people actually search for ("open source CRM", # "self-hosted CRM", "Django CRM") and names the modules rather than the diff --git a/backend/uv.lock b/backend/uv.lock index 72ccd37f..086f1065 100644 --- a/backend/uv.lock +++ b/backend/uv.lock @@ -583,7 +583,7 @@ wheels = [ [[package]] name = "django-crm" -version = "1.7.0" +version = "1.8.0" source = { editable = "." } dependencies = [ { name = "cairocffi" }, diff --git a/docs/api/endpoint-index.md b/docs/api/endpoint-index.md index ce02a5ae..f6d7686d 100644 --- a/docs/api/endpoint-index.md +++ b/docs/api/endpoint-index.md @@ -171,6 +171,7 @@ Interactive versions of the same schema are served by a running backend at | `/api/opportunities/attachment/{id}/` | DELETE | | `/api/opportunities/comment/{id}/` | DELETE, PATCH, PUT | | `/api/opportunities/goals/` | GET, POST | +| `/api/opportunities/goals/history/` | GET | | `/api/opportunities/goals/leaderboard/` | GET | | `/api/opportunities/goals/{id}/` | DELETE, GET, PUT | | `/api/opportunities/kanban/` | GET | diff --git a/frontend/src/lib/components/dashboard/GoalProgress.svelte b/frontend/src/lib/components/dashboard/GoalProgress.svelte deleted file mode 100644 index 83905083..00000000 --- a/frontend/src/lib/components/dashboard/GoalProgress.svelte +++ /dev/null @@ -1,79 +0,0 @@ - - -
-
-
-
- -
-

- Goal Progress -

-
- - View All - - -
- - {#if goals.length === 0} -
-

No active goals

- - Create a goal - -
- {:else} -
- {#each goals as goal (goal.id)} -
-
- {goal.name} - - {goal.progress_percent ?? 0}% - -
- -
- - {goal.goal_type === 'REVENUE' - ? formatCurrency(goal.progress_value || 0, orgCurrency, true) - : `${goal.progress_value || 0} deals`} - - - {goal.goal_type === 'REVENUE' - ? formatCurrency(goal.target_value || 0, orgCurrency, true) - : `${goal.target_value || 0} deals`} - -
-
- {/each} -
- {/if} -
diff --git a/frontend/src/lib/components/dashboard/index.js b/frontend/src/lib/components/dashboard/index.js index e6188020..e73a5bde 100644 --- a/frontend/src/lib/components/dashboard/index.js +++ b/frontend/src/lib/components/dashboard/index.js @@ -1,7 +1,6 @@ export { default as KPICard } from './KPICard.svelte'; export { default as ActivityFeed } from './ActivityFeed.svelte'; export { default as FocusBar } from './FocusBar.svelte'; -export { default as GoalProgress } from './GoalProgress.svelte'; export { default as MiniPipeline } from './MiniPipeline.svelte'; export { default as PipelineChart } from './PipelineChart.svelte'; export { default as TaskList } from './TaskList.svelte'; diff --git a/frontend/src/lib/server/v2/goals.js b/frontend/src/lib/server/v2/goals.js index 11599f82..ec8f6304 100644 --- a/frontend/src/lib/server/v2/goals.js +++ b/frontend/src/lib/server/v2/goals.js @@ -36,12 +36,44 @@ import { apiRequest } from '$lib/api-helpers.js'; import { getOrgPeopleAndTeams } from './org-people.js'; -/** The two goal kinds the backend accepts (common/utils.py GOAL_TYPES). */ -export const GOAL_TYPES = ['REVENUE', 'DEALS_CLOSED']; +/** The three goal kinds the backend accepts (common/utils.py GOAL_TYPES). */ +export const GOAL_TYPES = ['REVENUE', 'DEALS_CLOSED', 'ACTIVITIES']; /** The four periods the backend accepts (common/utils.py PERIOD_TYPES). */ export const PERIOD_TYPES = ['MONTHLY', 'QUARTERLY', 'YEARLY', 'CUSTOM']; +/** + * The deal types a weight can be set against (common/utils.py + * OPPORTUNITY_TYPES). A type left out of a goal's map counts at full value, so + * the form leaves every box blank by default and an untouched form stores `{}`. + * ACTIVITIES goals have no deal type, and the backend refuses weights on one. + */ +export const DEAL_TYPES = ['NEW_BUSINESS', 'EXISTING_BUSINESS', 'RENEWAL', 'UPSELL', 'CROSS_SELL']; + +/** + * Collect the `weight_` inputs into the map the write serializer takes. + * + * A blank box is not a weight of zero, it is "do not re-weigh this type", so it + * is left out of the map entirely. A box with something non-numeric in it is + * passed along as typed rather than coerced: the backend validator names the + * offending type in its 400, and silently turning "heavy" into NaN or 0 would + * store a quota nobody asked for. + * + * @param {FormData} form + * @returns {Record} + */ +export function weightsFromForm(form) { + /** @type {Record} */ + const weights = {}; + for (const type of DEAL_TYPES) { + const raw = form.get(`weight_${type}`)?.toString().trim(); + if (!raw) continue; + const parsed = Number(raw); + weights[type] = Number.isFinite(parsed) ? parsed : raw; + } + return weights; +} + /** * Plain scalar fields the write serializer accepts. `assigned_to`/`team` are * handled apart, through the single `target` picker, so the form can never send @@ -115,6 +147,9 @@ function toGoal(g) { : null, team: g.team_detail ? { id: g.team_detail.id, name: g.team_detail.name } : null, is_active: g.is_active, + // Always an object, never undefined, so the edit form and the list badge + // can read it without guarding at every use. + type_weights: g.type_weights ?? {}, progress_value: Number(g.progress_value ?? 0), progress_percent: g.progress_percent ?? 0, status: g.status @@ -194,11 +229,18 @@ function computeTotals(goals) { * trips. `limit=1000` fetches the whole (small) set so the totals are computed * over every goal, not one page of them. * - * @param {{ cookies: import('@sveltejs/kit').Cookies }} event + * @param {{ cookies: import('@sveltejs/kit').Cookies, url?: URL }} event */ -export async function listGoals({ cookies }) { +export async function listGoals({ cookies, url }) { + const filters = readFilters(url); + const query = new URLSearchParams({ limit: '1000' }); + if (filters.period_type) query.set('period_type', filters.period_type); + if (filters.q) query.set('search', filters.q); + if (filters.window === 'current') query.set('current', 'true'); + if (filters.window === 'active') query.set('active', 'true'); + const [listResp, boardResp] = await Promise.all([ - apiRequest('/opportunities/goals/?limit=1000', {}, { cookies }), + apiRequest(`/opportunities/goals/?${query}`, {}, { cookies }), apiRequest( `/opportunities/goals/leaderboard/?period_type=${LEADERBOARD_PERIOD}`, {}, @@ -212,11 +254,93 @@ export async function listGoals({ cookies }) { return { goals, leaderboard, + filters, totals: computeTotals(goals), can_edit: viewerRole(cookies) === 'ADMIN' }; } +/** The windows the filter bar offers, mapped to the API's own flags. */ +const WINDOWS = ['all', 'current', 'active']; + +/** + * The filter state from the page URL, narrowed to values the API accepts. + * + * `period_type` and `window` are checked against their allowed sets rather than + * forwarded: they are query-string values, so anything at all can arrive there, + * and passing an unknown one straight to the API would turn a typo into an + * empty list with no explanation. `q` is free text by nature and goes through + * `URLSearchParams`, which encodes it. + * + * @param {URL} [url] + */ +function readFilters(url) { + const params = url?.searchParams; + const periodType = params?.get('period_type') ?? ''; + const window = params?.get('window') ?? ''; + return { + period_type: PERIOD_TYPES.includes(periodType) ? periodType : '', + q: (params?.get('q') ?? '').trim(), + window: WINDOWS.includes(window) ? window : '' + }; +} + +/** + * The handful of goals running right now, for the Today page's progress strip. + * + * Deliberately not `/api/dashboard/`, which carries a `goal_summary` of the + * same three goals: that endpoint also builds tasks, activities and pipeline + * aggregates the Today page never reads, and this one is already narrowed to + * what the caller may see by `_visible_to`. + * + * A failure here yields an empty strip rather than a broken dashboard. The + * goals are context on a page whose actual job is the queue, and taking the + * whole page down because a secondary panel failed is the wrong trade. + * + * @param {{ cookies: import('@sveltejs/kit').Cookies }} event + * @param {number} [limit] + */ +export async function getCurrentGoals({ cookies }, limit = 3) { + try { + const resp = await apiRequest( + `/opportunities/goals/?current=true&active=true&limit=${limit}`, + {}, + { cookies } + ); + return (resp?.goals ?? []).map(toGoal); + } catch { + return []; + } +} + +/** + * Finished goal periods with what each one attained, newest first. + * + * The list page answers "how are we doing"; this answers "how did we do", which + * nothing did before. Numbers come from the API already rolled up per period, + * so nothing is re-aggregated here. + * + * @param {{ cookies: import('@sveltejs/kit').Cookies }} event + */ +export async function getGoalHistory({ cookies }) { + const resp = await apiRequest('/opportunities/goals/history/', {}, { cookies }); + return { + history: (resp?.history ?? []).map((period) => ({ + period_start: period.period_start, + period_end: period.period_end, + period_type: period.period_type, + goal_type: period.goal_type, + goals_count: period.goals_count ?? 0, + attained_count: period.attained_count ?? 0, + target: Number(period.target ?? 0), + achieved: Number(period.achieved ?? 0), + percent: period.percent ?? 0, + goals: (period.goals ?? []).map(toGoal) + })), + can_edit: viewerRole(cookies) === 'ADMIN' + }; +} + /** * What the create form needs. `can_edit` gates the whole route: creating a goal * is admin-only server-side, and a non-admin who reaches `/goals/new` gets @@ -265,6 +389,7 @@ export async function getGoalForEdit({ cookies }, id) { period_start: raw.period_start, period_end: raw.period_end, is_active: raw.is_active, + type_weights: raw.type_weights ?? {}, target } }; @@ -284,6 +409,10 @@ function writeBody(values) { if (field in values) body[field] = values[field]; } if ('is_active' in values) body.is_active = values.is_active; + // An object, so it is set apart from the scalar EDITABLE_FIELDS loop. An + // empty map is a meaningful value (it clears the weighting), so it is sent + // whenever the key is present rather than skipped for being falsy. + if ('type_weights' in values) body.type_weights = values.type_weights; if ('target' in values) { const t = values.target ?? 'org'; diff --git a/frontend/src/lib/server/v2/goals.test.js b/frontend/src/lib/server/v2/goals.test.js index 5ec553fd..703866fa 100644 --- a/frontend/src/lib/server/v2/goals.test.js +++ b/frontend/src/lib/server/v2/goals.test.js @@ -164,3 +164,190 @@ describe('listGoals can_edit', () => { expect(signedOut.can_edit).toBe(false); }); }); + +describe('activity goals and deal type weights', () => { + beforeEach(() => { + apiRequest.mockReset(); + }); + + it('offers ACTIVITIES as a goal type the backend accepts', async () => { + const { GOAL_TYPES } = await import('./goals.js'); + expect(GOAL_TYPES).toContain('ACTIVITIES'); + }); + + it('names the five deal types a weight can be set on', async () => { + const { DEAL_TYPES } = await import('./goals.js'); + expect(DEAL_TYPES).toEqual([ + 'NEW_BUSINESS', + 'EXISTING_BUSINESS', + 'RENEWAL', + 'UPSELL', + 'CROSS_SELL' + ]); + }); + + it('reads a weight map off the form, skipping the types left blank', async () => { + const { weightsFromForm } = await import('./goals.js'); + const form = new FormData(); + form.set('weight_RENEWAL', '0.5'); + form.set('weight_NEW_BUSINESS', ''); + form.set('weight_UPSELL', ' '); + + expect(weightsFromForm(form)).toEqual({ RENEWAL: 0.5 }); + }); + + it('passes a non-numeric weight through so the backend can name it', async () => { + const { weightsFromForm } = await import('./goals.js'); + const form = new FormData(); + form.set('weight_RENEWAL', 'heavy'); + + expect(weightsFromForm(form)).toEqual({ RENEWAL: 'heavy' }); + }); + + it('sends the weight map on create', async () => { + const { createGoal } = await import('./goals.js'); + apiRequest.mockResolvedValue({}); + + await createGoal({ cookies }, { name: 'Q3', type_weights: { RENEWAL: 0.5 } }); + + expect(apiRequest.mock.calls[0][1].body.type_weights).toEqual({ RENEWAL: 0.5 }); + }); + + it('carries the stored weights through to the list', async () => { + respond({ goals: [goal({ type_weights: { RENEWAL: 0.5 } })] }); + + const { goals } = await listGoals({ cookies }); + + expect(goals[0].type_weights).toEqual({ RENEWAL: 0.5 }); + }); + + it('reports an unweighted goal as an empty map rather than undefined', async () => { + respond({ goals: [goal()] }); + + const { goals } = await listGoals({ cookies }); + + expect(goals[0].type_weights).toEqual({}); + }); +}); + +describe('listGoals filters', () => { + beforeEach(() => { + apiRequest.mockReset(); + }); + + it('asks the API for the filters the page was given', async () => { + respond({ goals: [] }); + + await listGoals({ cookies, url: new URL('http://x/goals?period_type=QUARTERLY&q=emea') }); + + const listUrl = apiRequest.mock.calls.map((c) => c[0]).find((u) => !u.includes('leaderboard')); + expect(listUrl).toContain('period_type=QUARTERLY'); + expect(listUrl).toContain('search=emea'); + }); + + it('sends only current goals when the page asks for them', async () => { + respond({ goals: [] }); + + await listGoals({ cookies, url: new URL('http://x/goals?window=current') }); + + const listUrl = apiRequest.mock.calls.map((c) => c[0]).find((u) => !u.includes('leaderboard')); + expect(listUrl).toContain('current=true'); + }); + + it('ignores a period type that is not one the backend accepts', async () => { + respond({ goals: [] }); + + await listGoals({ cookies, url: new URL('http://x/goals?period_type=; DROP TABLE') }); + + const listUrl = apiRequest.mock.calls.map((c) => c[0]).find((u) => !u.includes('leaderboard')); + expect(listUrl).not.toContain('period_type'); + }); + + it('reports the active filters back to the page', async () => { + respond({ goals: [] }); + + const data = await listGoals({ + cookies, + url: new URL('http://x/goals?period_type=YEARLY&q=emea&window=current') + }); + + expect(data.filters).toEqual({ period_type: 'YEARLY', q: 'emea', window: 'current' }); + }); +}); + +describe('getGoalHistory', () => { + beforeEach(() => { + apiRequest.mockReset(); + }); + + it('reads finished periods from the history endpoint', async () => { + apiRequest.mockResolvedValue({ + history: [ + { + period_start: '2026-01-01', + period_end: '2026-01-31', + period_type: 'MONTHLY', + goal_type: 'REVENUE', + goals_count: 2, + attained_count: 1, + target: 300, + achieved: 240, + percent: 80, + goals: [goal({ name: 'Jan' })] + } + ] + }); + + const { getGoalHistory } = await import('./goals.js'); + const { history } = await getGoalHistory({ cookies }); + + expect(apiRequest.mock.calls[0][0]).toContain('/opportunities/goals/history/'); + expect(history[0].percent).toBe(80); + expect(history[0].goal_type).toBe('REVENUE'); + expect(history[0].attained_count).toBe(1); + expect(history[0].goals[0].name).toBe('Jan'); + }); + + it('returns an empty history rather than throwing when there is none', async () => { + apiRequest.mockResolvedValue({}); + + const { getGoalHistory } = await import('./goals.js'); + + expect((await getGoalHistory({ cookies })).history).toEqual([]); + }); +}); + +describe('getCurrentGoals', () => { + beforeEach(() => { + apiRequest.mockReset(); + }); + + it('asks only for goals running today that are not paused', async () => { + apiRequest.mockResolvedValue({ goals: [] }); + const { getCurrentGoals } = await import('./goals.js'); + + await getCurrentGoals({ cookies }); + + const url = apiRequest.mock.calls[0][0]; + expect(url).toContain('current=true'); + expect(url).toContain('active=true'); + }); + + it('maps the rows the same way the goals list does', async () => { + apiRequest.mockResolvedValue({ goals: [goal({ name: 'Q3', progress_percent: 62 })] }); + const { getCurrentGoals } = await import('./goals.js'); + + const goals = await getCurrentGoals({ cookies }); + + expect(goals[0].name).toBe('Q3'); + expect(goals[0].progress_percent).toBe(62); + expect(goals[0].type_weights).toEqual({}); + }); + + it('returns nothing rather than breaking the dashboard when the call fails', async () => { + apiRequest.mockRejectedValue(new Error('boom')); + const { getCurrentGoals } = await import('./goals.js'); + + expect(await getCurrentGoals({ cookies })).toEqual([]); + }); +}); diff --git a/frontend/src/lib/utils/goals.js b/frontend/src/lib/utils/goals.js deleted file mode 100644 index 0f37d921..00000000 --- a/frontend/src/lib/utils/goals.js +++ /dev/null @@ -1,29 +0,0 @@ -/** - * Get Tailwind color classes for a goal status value. - * @param {string} statusValue - */ -export function getStatusColor(statusValue) { - switch (statusValue) { - case 'on_track': - return 'text-emerald-600 dark:text-emerald-400'; - case 'at_risk': - return 'text-amber-600 dark:text-amber-400'; - case 'behind': - return 'text-red-600 dark:text-red-400'; - case 'completed': - return 'text-blue-600 dark:text-blue-400'; - default: - return 'text-[var(--text-secondary)]'; - } -} - -/** - * Get Tailwind progress bar color class based on percent. - * @param {number} percent - */ -export function getProgressColor(percent) { - if (percent >= 100) return '[&>[data-slot=progress-indicator]]:bg-blue-500'; - if (percent >= 75) return '[&>[data-slot=progress-indicator]]:bg-emerald-500'; - if (percent >= 50) return '[&>[data-slot=progress-indicator]]:bg-amber-500'; - return '[&>[data-slot=progress-indicator]]:bg-red-500'; -} diff --git a/frontend/src/lib/v2/components/DealTypeWeights.svelte b/frontend/src/lib/v2/components/DealTypeWeights.svelte new file mode 100644 index 00000000..fea458ec --- /dev/null +++ b/frontend/src/lib/v2/components/DealTypeWeights.svelte @@ -0,0 +1,139 @@ + + +{#if goalType !== 'ACTIVITIES'} +
+ + + + + {#if open} +

+ A multiplier on each closed-won deal of that type. Leave a box empty to count that type in + full. At 0.5 a 20,000 renewal counts as 10,000; at 0 it does not count at all. +

+ {/if} +
+{/if} + + diff --git a/frontend/src/lib/v2/enums.js b/frontend/src/lib/v2/enums.js index 1a8c91ae..973c8c5a 100644 --- a/frontend/src/lib/v2/enums.js +++ b/frontend/src/lib/v2/enums.js @@ -222,7 +222,20 @@ export const SOLUTION_STATUS_TONE = { draft: 'slate', reviewed: 'clay', approved * what they mean rather than repeating the wire value: "behind" alone does not * tell you behind on what. They are pace judgements, not percentages. */ -export const GOAL_TYPE_LABEL = { REVENUE: 'Revenue', DEALS_CLOSED: 'Deals closed' }; +export const GOAL_TYPE_LABEL = { + REVENUE: 'Revenue', + DEALS_CLOSED: 'Deals closed', + ACTIVITIES: 'Activities' +}; + +/** Opportunity types, for the per-goal weighting on a revenue or deals goal. */ +export const DEAL_TYPE_LABEL = { + NEW_BUSINESS: 'New business', + EXISTING_BUSINESS: 'Existing business', + RENEWAL: 'Renewal', + UPSELL: 'Upsell', + CROSS_SELL: 'Cross-sell' +}; export const PERIOD_TYPE_LABEL = { MONTHLY: 'Monthly', diff --git a/frontend/src/routes/(app)/+page.server.js b/frontend/src/routes/(app)/+page.server.js index a65da8cd..556423e6 100644 --- a/frontend/src/routes/(app)/+page.server.js +++ b/frontend/src/routes/(app)/+page.server.js @@ -1,6 +1,17 @@ import { getToday } from '$lib/server/v2/today.js'; +import { getCurrentGoals } from '$lib/server/v2/goals.js'; -/** @type {import('./$types').PageServerLoad} */ +/** + * Today, plus the goals running today. + * + * Two requests in parallel rather than one: `/dashboard/today/` builds the + * queue and `/opportunities/goals/` the strip, and neither waits on the other. + * `getCurrentGoals` swallows its own failure, so a goals outage costs the strip + * and not the page. + * + * @type {import('./$types').PageServerLoad} + */ export async function load(event) { - return await getToday(event); + const [today, goals] = await Promise.all([getToday(event), getCurrentGoals(event)]); + return { ...today, goals }; } diff --git a/frontend/src/routes/(app)/+page.svelte b/frontend/src/routes/(app)/+page.svelte index 893c86f5..54574716 100644 --- a/frontend/src/routes/(app)/+page.svelte +++ b/frontend/src/routes/(app)/+page.svelte @@ -2,7 +2,8 @@ import { resolve } from '$app/paths'; import PageHeader from '$lib/v2/components/PageHeader.svelte'; import Pill from '$lib/v2/components/Pill.svelte'; - import { money } from '$lib/v2/format.js'; + import { money, count } from '$lib/v2/format.js'; + import { Target } from '@lucide/svelte'; /** @type {{ data: any }} */ let { data } = $props(); @@ -14,7 +15,24 @@ moss: 'var(--v2-moss)' }; - let { queue, summary, later } = $derived(data); + let { queue, summary, later, goals } = $derived(data); + + /** Revenue goals are money; deals and activities goals are plain counts. */ + const goalValue = (g, n) => (g.goal_type === 'REVENUE' ? money(n, data.org.currency) : count(n)); + + /** + * Coloured on the server's pace judgement, not on the raw percentage, so the + * strip agrees with the goals page rather than calling a goal green in week + * two of a quarter for being 40% of the way there. + */ + const goalColor = (g) => + g.status === 'completed' + ? 'var(--v2-moss)' + : g.status === 'behind' + ? 'var(--v2-rust)' + : g.status === 'at_risk' + ? 'var(--v2-clay)' + : 'var(--v2-slate)'; const plural = (/** @type {number} */ n, /** @type {string} */ one, /** @type {string} */ many) => `${n} ${n === 1 ? one : many}`; @@ -99,6 +117,40 @@ {/if} + + {#if goals.length} +
+ + Where you stand +
+ + {/if} + {#if later.length}
Later this week
{#each later as row (row.id)} @@ -122,3 +174,45 @@ {/if} + + diff --git a/frontend/src/routes/(app)/goals/+page.svelte b/frontend/src/routes/(app)/goals/+page.svelte index 2ec4c7c8..b1d5da26 100644 --- a/frontend/src/routes/(app)/goals/+page.svelte +++ b/frontend/src/routes/(app)/goals/+page.svelte @@ -28,13 +28,27 @@ GOAL_STATUS_TONE, PERIOD_TYPE_LABEL } from '$lib/v2/enums.js'; - import { Plus, Target, Trophy } from '@lucide/svelte'; + import { Plus, Target, Trophy, History } from '@lucide/svelte'; /** @type {{ data: any }} */ let { data } = $props(); let totals = $derived(data.totals); + /** + * The filter bar submits as a plain GET form, so the state lives in the URL: + * a filtered view is linkable, survives a reload, and re-runs the server load + * that applies it. The backend does the filtering (`period_type`, `search`, + * `current`, `active` on the list endpoint); nothing here narrows an array + * that was already fetched in full. + */ + let filters = $derived(data.filters ?? { period_type: '', q: '', window: '' }); + let filtered = $derived(Boolean(filters.period_type || filters.q || filters.window)); + + /** How many deal types this goal re-weighs, for the badge on its card. */ + const weightedTypes = (g) => + Object.entries(g.type_weights ?? {}).filter(([, w]) => Number(w) !== 1).length; + /** How far through the period we are, 0-100. */ function elapsedPercent(g) { const start = new Date(g.period_start).getTime(); @@ -68,7 +82,7 @@ : 'var(--v2-slate)'; }; - /** Revenue goals are money; DEALS_CLOSED goals are a count of deals. */ + /** Revenue goals are money; deals and activities goals are plain counts. */ const value = (g, n) => (g.goal_type === 'REVENUE' ? money(n, data.org.currency) : count(n)); @@ -79,6 +93,7 @@ {count(totals.active)} active goals {/snippet} {#snippet actions()} + History {#if data.can_edit} New goal {/if} @@ -107,28 +122,69 @@ /> + +
+ + + + + {#if filtered} + Clear + {/if} +
{#if data.goals.length === 0} - - - {#snippet icon()}{/snippet} - {#snippet actions()} - {#if data.can_edit} - New goal - {/if} - {/snippet} - + + {#if filtered} + + {#snippet icon()}{/snippet} + {#snippet actions()} + Clear filter + {/snippet} + + {:else} + + {#snippet icon()}{/snippet} + {#snippet actions()} + {#if data.can_edit} + New goal + {/if} + {/snippet} + + {/if} {:else}
@@ -144,6 +200,15 @@
{GOAL_TYPE_LABEL[g.goal_type]} · {PERIOD_TYPE_LABEL[g.period_type]} · {shortDate(g.period_start)} - {shortDate(g.period_end)} + + {#if weightedTypes(g)} + · weighted ({weightedTypes(g)}) + {/if}
@@ -272,3 +337,33 @@ {/if}
+ + diff --git a/frontend/src/routes/(app)/goals/[id]/edit/+page.server.js b/frontend/src/routes/(app)/goals/[id]/edit/+page.server.js index 0c457592..d52930a3 100644 --- a/frontend/src/routes/(app)/goals/[id]/edit/+page.server.js +++ b/frontend/src/routes/(app)/goals/[id]/edit/+page.server.js @@ -1,5 +1,11 @@ import { error, fail, redirect } from '@sveltejs/kit'; -import { getGoalForEdit, updateGoal, deleteGoal, EDITABLE_FIELDS } from '$lib/server/v2/goals.js'; +import { + getGoalForEdit, + updateGoal, + deleteGoal, + weightsFromForm, + EDITABLE_FIELDS +} from '$lib/server/v2/goals.js'; import { readableError } from '$lib/server/v2/form-errors.js'; /** @@ -36,6 +42,10 @@ export const actions = { // The one switch submits an explicit 'true'/'false', so "paused" is a real // value rather than the "absent → leave alone" convention above. if (form.has('is_active')) values.is_active = form.get('is_active') === 'true'; + // Sent on every save, so clearing a box actually clears that weight. The + // "absent means leave alone" convention above would make a weight + // impossible to remove once set. + values.type_weights = weightsFromForm(form); try { await updateGoal(event, event.params.id, values); diff --git a/frontend/src/routes/(app)/goals/[id]/edit/+page.svelte b/frontend/src/routes/(app)/goals/[id]/edit/+page.svelte index a373936f..378ca77c 100644 --- a/frontend/src/routes/(app)/goals/[id]/edit/+page.svelte +++ b/frontend/src/routes/(app)/goals/[id]/edit/+page.svelte @@ -4,6 +4,7 @@ import { enhance } from '$app/forms'; import PageHeader from '$lib/v2/components/PageHeader.svelte'; import NextAction from '$lib/v2/components/NextAction.svelte'; + import DealTypeWeights from '$lib/v2/components/DealTypeWeights.svelte'; import { GOAL_TYPE_LABEL, PERIOD_TYPE_LABEL } from '$lib/v2/enums.js'; import { money, count } from '$lib/v2/format.js'; import { TriangleAlert } from '@lucide/svelte'; @@ -165,6 +166,8 @@
+ +
diff --git a/frontend/src/routes/(app)/goals/history/+page.server.js b/frontend/src/routes/(app)/goals/history/+page.server.js new file mode 100644 index 00000000..0b0ae878 --- /dev/null +++ b/frontend/src/routes/(app)/goals/history/+page.server.js @@ -0,0 +1,14 @@ +import { getGoalHistory } from '$lib/server/v2/goals.js'; + +/** + * Attainment across finished periods. + * + * Open to any member; the endpoint narrows a non-admin to their own goals and + * their teams' with the same predicate the list and the leaderboard use, so + * there is nothing to gate here. + * + * @type {import('./$types').PageServerLoad} + */ +export async function load(event) { + return await getGoalHistory(event); +} diff --git a/frontend/src/routes/(app)/goals/history/+page.svelte b/frontend/src/routes/(app)/goals/history/+page.svelte new file mode 100644 index 00000000..6b7721a2 --- /dev/null +++ b/frontend/src/routes/(app)/goals/history/+page.svelte @@ -0,0 +1,198 @@ + + + + {#snippet crumb()}Goals ›{/snippet} + {#snippet sub()} + Finished periods, newest first. A closed period is settled, so these are results rather than + pace. + {/snippet} + + +
+
+ {#if data.history.length === 0} + + + {#snippet icon()}{/snippet} + {#snippet actions()} + Back to goals + {/snippet} + + {:else} +
+ {#each data.history as period (period.period_start + period.period_end + period.goal_type)} +
+
+
+
+ {shortDate(period.period_start)} - {shortDate(period.period_end)} +
+
+ {PERIOD_TYPE_LABEL[period.period_type]} · {GOAL_TYPE_LABEL[period.goal_type]} · + {period.attained_count} + of {period.goals_count} + {period.goals_count === 1 ? 'goal' : 'goals'} met +
+
+ {period.percent}% of target +
+ +
+ + +
+
+ {unit(period.goal_type, period.achieved)} + of {unit(period.goal_type, period.target)} +
+ +
    + {#each period.goals as g (g.id)} +
  • + {g.name} + {owner(g)} · {GOAL_TYPE_LABEL[g.goal_type]} + + {value(g, g.progress_value)} / {value(g, g.target_value)} + + {met(g) ? 'Met' : 'Missed'} +
  • + {/each} +
+
+ {/each} +
+ +

+ The twelve most recent periods you can see. Attainment is recomputed from the closed-won + deals of each period, so it matches what the goals page showed while the period was running. +

+ {/if} +
+
+ + diff --git a/frontend/src/routes/(app)/goals/new/+page.server.js b/frontend/src/routes/(app)/goals/new/+page.server.js index f94e70c1..59fc3ac5 100644 --- a/frontend/src/routes/(app)/goals/new/+page.server.js +++ b/frontend/src/routes/(app)/goals/new/+page.server.js @@ -1,5 +1,10 @@ import { fail, redirect } from '@sveltejs/kit'; -import { getGoalFormOptions, createGoal, EDITABLE_FIELDS } from '$lib/server/v2/goals.js'; +import { + getGoalFormOptions, + createGoal, + weightsFromForm, + EDITABLE_FIELDS +} from '$lib/server/v2/goals.js'; import { readableError } from '$lib/server/v2/form-errors.js'; /** @@ -29,6 +34,9 @@ export const actions = { for (const field of [...EDITABLE_FIELDS, 'target']) { if (form.has(field)) values[field] = form.get(field)?.toString().trim() ?? ''; } + // The weight inputs are absent from the DOM on an ACTIVITIES goal, so this + // sends `{}` for one, which is what the backend accepts there. + values.type_weights = weightsFromForm(form); try { await createGoal(event, values); diff --git a/frontend/src/routes/(app)/goals/new/+page.svelte b/frontend/src/routes/(app)/goals/new/+page.svelte index 7dd02088..23ac7304 100644 --- a/frontend/src/routes/(app)/goals/new/+page.svelte +++ b/frontend/src/routes/(app)/goals/new/+page.svelte @@ -4,6 +4,7 @@ import { enhance } from '$app/forms'; import PageHeader from '$lib/v2/components/PageHeader.svelte'; import NextAction from '$lib/v2/components/NextAction.svelte'; + import DealTypeWeights from '$lib/v2/components/DealTypeWeights.svelte'; import { GOAL_TYPE_LABEL, PERIOD_TYPE_LABEL } from '$lib/v2/enums.js'; import { money, count } from '$lib/v2/format.js'; import { TriangleAlert } from '@lucide/svelte'; @@ -144,7 +145,10 @@ {/each} -

Revenue counts closed-won amounts; deals counts closed-won deals.

+

+ Revenue counts closed-won amounts, deals counts closed-won deals, and activities counts + the records this person touched. +

@@ -181,6 +185,8 @@

A label for the window below. The dates are what actually scope it.

+ +
diff --git a/mobile/lib/config/api_config.dart b/mobile/lib/config/api_config.dart index 34e95dfa..4c899787 100644 --- a/mobile/lib/config/api_config.dart +++ b/mobile/lib/config/api_config.dart @@ -233,6 +233,11 @@ class ApiConfig { static String get goalsLeaderboard => '$apiBaseUrl/opportunities/goals/leaderboard/'; + /// Finished goal periods with what each one attained, newest first. Grouped + /// by period and goal type, and narrowed server-side to the goals the caller + /// may see, the same rule the list and the leaderboard use. + static String get goalsHistory => '$apiBaseUrl/opportunities/goals/history/'; + /// Shared files. The list answers two separately paginated envelopes, /// `documents_active` and `documents_inactive`. Uploading is open to any /// member; editing and deleting are the uploader or an admin. diff --git a/mobile/lib/data/models/dashboard_data.dart b/mobile/lib/data/models/dashboard_data.dart index b6881f03..fb723b3f 100644 --- a/mobile/lib/data/models/dashboard_data.dart +++ b/mobile/lib/data/models/dashboard_data.dart @@ -246,6 +246,11 @@ class DashboardData { final List tasks; final List activities; + /// The current goals the signed-in person may see, at most three. Served by + /// `/api/dashboard/` as `goal_summary` and narrowed there to their own goals, + /// their teams', and (for an admin) the org-wide ones. + final List goalSummary; + const DashboardData({ this.accountsCount = 0, this.contactsCount = 0, @@ -257,6 +262,7 @@ class DashboardData { this.hotLeads = const [], this.tasks = const [], this.activities = const [], + this.goalSummary = const [], }); factory DashboardData.fromJson(Map json) { @@ -358,6 +364,20 @@ class DashboardData { 'DashboardData.fromJson: Activities parsed: ${activitiesList.length}', ); + // Parse goal summary + final goalSummaryList = []; + if (json['goal_summary'] != null) { + for (final item in json['goal_summary'] as List) { + try { + goalSummaryList.add( + DashboardGoal.fromJson(item as Map), + ); + } catch (e) { + debugPrint('DashboardData.fromJson: Error parsing goal: $e'); + } + } + } + debugPrint('DashboardData.fromJson: All parsing complete!'); return DashboardData( @@ -371,6 +391,58 @@ class DashboardData { hotLeads: hotLeadsList, tasks: tasksList, activities: activitiesList, + goalSummary: goalSummaryList, + ); + } +} + +/// One current goal on the dashboard, from `/api/dashboard/` `goal_summary`. +/// +/// A slimmer shape than [SalesGoal]: the endpoint sends only what a progress +/// strip needs, and parsing it as a full goal would invent nulls for the seven +/// fields it does not carry. +/// +/// The backend has served this field all along and neither client rendered it, +/// so the numbers behind this were already being computed and thrown away. +class DashboardGoal { + const DashboardGoal({ + required this.id, + required this.name, + required this.goalType, + required this.targetValue, + required this.progressValue, + required this.progressPercent, + required this.status, + }); + + final String id; + final String name; + final String goalType; + final double targetValue; + final double progressValue; + final int progressPercent; + + /// The server's pace judgement (`on_track` / `at_risk` / `behind` / + /// `completed`), which compares attainment against elapsed time. Read, never + /// recomputed: this app cannot see the opportunities another person's goal + /// counts. + final String status; + + factory DashboardGoal.fromJson(Map json) { + final target = json['target_value']; + final progress = json['progress_value']; + return DashboardGoal( + id: json['id']?.toString() ?? '', + name: json['name']?.toString() ?? '', + goalType: json['goal_type']?.toString() ?? 'REVENUE', + targetValue: target is num + ? target.toDouble() + : double.tryParse(target?.toString() ?? '') ?? 0, + progressValue: progress is num + ? progress.toDouble() + : double.tryParse(progress?.toString() ?? '') ?? 0, + progressPercent: (json['progress_percent'] as num?)?.toInt() ?? 0, + status: json['status']?.toString() ?? 'on_track', ); } } diff --git a/mobile/lib/data/models/sales_goal.dart b/mobile/lib/data/models/sales_goal.dart index 42ead211..12adb4a3 100644 --- a/mobile/lib/data/models/sales_goal.dart +++ b/mobile/lib/data/models/sales_goal.dart @@ -11,8 +11,37 @@ /// 403 to everyone else, which is why the screens gate on role. library; -/// The two goal kinds the backend accepts (`common/utils.py` GOAL_TYPES). -const List goalTypes = ['REVENUE', 'DEALS_CLOSED']; +/// The three goal kinds the backend accepts (`common/utils.py` GOAL_TYPES). +const List goalTypes = ['REVENUE', 'DEALS_CLOSED', 'ACTIVITIES']; + +/// The deal types a weight can be set against (`common/utils.py` +/// OPPORTUNITY_TYPES). A type left out of a goal's map counts at full value, so +/// the form leaves every box blank and an untouched form stores `{}`. An +/// ACTIVITIES goal has no deal type and the backend refuses weights on one. +const List dealTypes = [ + 'NEW_BUSINESS', + 'EXISTING_BUSINESS', + 'RENEWAL', + 'UPSELL', + 'CROSS_SELL', +]; + +String dealTypeLabel(String type) { + switch (type) { + case 'NEW_BUSINESS': + return 'New business'; + case 'EXISTING_BUSINESS': + return 'Existing business'; + case 'RENEWAL': + return 'Renewal'; + case 'UPSELL': + return 'Upsell'; + case 'CROSS_SELL': + return 'Cross-sell'; + default: + return type; + } +} /// The four periods the backend accepts (`common/utils.py` PERIOD_TYPES). const List goalPeriodTypes = [ @@ -22,8 +51,16 @@ const List goalPeriodTypes = [ 'CUSTOM', ]; -String goalTypeLabel(String type) => - type == 'DEALS_CLOSED' ? 'Deals closed' : 'Revenue'; +String goalTypeLabel(String type) { + switch (type) { + case 'DEALS_CLOSED': + return 'Deals closed'; + case 'ACTIVITIES': + return 'Activities'; + default: + return 'Revenue'; + } +} String goalPeriodLabel(String period) { switch (period) { @@ -75,6 +112,7 @@ class SalesGoal { this.teamId, this.teamName, this.isActive = true, + this.typeWeights = const {}, this.progressValue = 0, this.progressPercent = 0, this.status = 'on_track', @@ -100,6 +138,12 @@ class SalesGoal { final String? teamName; final bool isActive; + + /// Optional multiplier per opportunity type, e.g. `{'RENEWAL': 0.5}`. A type + /// left out weighs 1, so an empty map is an unweighted goal. Never null, so + /// callers do not guard at every use. + final Map typeWeights; + final double progressValue; final int progressPercent; final String status; @@ -113,10 +157,19 @@ class SalesGoal { return 'Whole organisation'; } - /// `REVENUE` targets are money, `DEALS_CLOSED` targets are a count. Handing - /// the caller the distinction rather than a formatted string keeps the org's - /// currency symbol out of this file. - bool get isMoney => goalType != 'DEALS_CLOSED'; + /// How many deal types this goal actually re-weighs. A weight of exactly 1 + /// is the default rather than an adjustment, so it does not count. + int get weightedTypeCount => typeWeights.values.where((w) => w != 1).length; + + /// `REVENUE` targets are money; `DEALS_CLOSED` and `ACTIVITIES` targets are + /// counts. Handing the caller the distinction rather than a formatted string + /// keeps the org's currency symbol out of this file. + /// + /// Written as an equality rather than `!= 'DEALS_CLOSED'`, which is what it + /// used to say: that phrasing made every goal type added later default to + /// money, and ACTIVITIES duly arrived and rendered a quota of forty logged + /// activities as "$40". + bool get isMoney => goalType == 'REVENUE'; factory SalesGoal.fromJson(Map json) { final assigned = json['assigned_to_detail'] as Map?; @@ -134,6 +187,7 @@ class SalesGoal { teamId: json['team']?.toString(), teamName: team?['name']?.toString(), isActive: json['is_active'] as bool? ?? true, + typeWeights: _toWeights(json['type_weights']), progressValue: _toDouble(json['progress_value']), progressPercent: (json['progress_percent'] as num?)?.toInt() ?? 0, status: json['status']?.toString() ?? 'on_track', @@ -310,3 +364,119 @@ String _personName(Map profile) { final email = details?['email']?.toString() ?? ''; return email.isNotEmpty ? email : 'Unnamed'; } + +/// A period as "1 May - 31 May", from the two date-only strings the API sends. +/// +/// Parsed rather than reformatted through DateTime.toLocal(): these are +/// date-only fields, and a timezone conversion can move a period boundary by a +/// day. Anything that does not parse is handed back as it arrived, so a +/// malformed row shows the raw value instead of a wrong date. +String goalDateRange(String start, String end) { + final from = _shortDate(start); + final to = _shortDate(end); + if (from == null || to == null) return '$start to $end'; + return '$from - $to'; +} + +const List _months = [ + 'Jan', + 'Feb', + 'Mar', + 'Apr', + 'May', + 'Jun', + 'Jul', + 'Aug', + 'Sep', + 'Oct', + 'Nov', + 'Dec', +]; + +String? _shortDate(String isoDate) { + final parts = isoDate.split('-'); + if (parts.length != 3) return null; + final month = int.tryParse(parts[1]); + final day = int.tryParse(parts[2]); + if (month == null || day == null || month < 1 || month > 12) return null; + return '$day ${_months[month - 1]}'; +} + +/// The stored deal type weights, narrowed to the numbers they must be. +/// +/// The column is free-form JSON, and the write serializer is what guarantees +/// its shape, so a row written before that validator existed (or by hand in a +/// shell) can still hold a string or a null. Anything that is not a number is +/// dropped rather than coerced: a weight of "heavy" is not a weight of zero, +/// and quietly inventing one would misreport somebody's quota. +Map _toWeights(dynamic value) { + if (value is! Map) return const {}; + final weights = {}; + value.forEach((key, weight) { + if (weight is num) weights[key.toString()] = weight; + }); + return weights; +} + +/// One finished period from `/api/opportunities/goals/history/`. +/// +/// A row is a period AND a goal type. Grouping on the period alone pooled a +/// revenue target in currency with a deals-closed target in deals and printed +/// the sum as money, so the API groups by both and this carries the type that +/// decides how the figures are formatted. +/// +/// [percent] is uncapped, unlike `SalesGoal.progressPercent`: a settled period +/// that came in 25% over is a different result from one that landed exactly on +/// target, and the difference is final. +class GoalHistoryPeriod { + const GoalHistoryPeriod({ + required this.periodStart, + required this.periodEnd, + required this.periodType, + required this.goalType, + required this.goalsCount, + required this.attainedCount, + required this.target, + required this.achieved, + required this.percent, + required this.goals, + }); + + final String periodStart; + final String periodEnd; + final String periodType; + final String goalType; + + /// Goals in this period, and how many of them reached their own target. + /// Counted per goal rather than off the pooled total: three reps, two of whom + /// missed while the third doubled up, is not "the team made its number". + final int goalsCount; + final int attainedCount; + + final double target; + final double achieved; + final int percent; + final List goals; + + /// Same rule as [SalesGoal.isMoney], applied to the period's own type. + bool get isMoney => goalType == 'REVENUE'; + + factory GoalHistoryPeriod.fromJson(Map json) { + final goals = json['goals'] as List? ?? const []; + return GoalHistoryPeriod( + periodStart: json['period_start']?.toString() ?? '', + periodEnd: json['period_end']?.toString() ?? '', + periodType: json['period_type']?.toString() ?? 'MONTHLY', + goalType: json['goal_type']?.toString() ?? 'REVENUE', + goalsCount: (json['goals_count'] as num?)?.toInt() ?? 0, + attainedCount: (json['attained_count'] as num?)?.toInt() ?? 0, + target: _toDouble(json['target']), + achieved: _toDouble(json['achieved']), + percent: (json['percent'] as num?)?.toInt() ?? 0, + goals: goals + .whereType>() + .map(SalesGoal.fromJson) + .toList(), + ); + } +} diff --git a/mobile/lib/providers/goals_provider.dart b/mobile/lib/providers/goals_provider.dart index 60ec147e..a5dc9f07 100644 --- a/mobile/lib/providers/goals_provider.dart +++ b/mobile/lib/providers/goals_provider.dart @@ -7,17 +7,74 @@ import '../services/api_service.dart'; export '../services/api_service.dart' show ApiResponse; +/// What the goals list is narrowed to. +/// +/// Held here rather than in the screen so a refresh keeps the filter, and so +/// the query string has one definition that a test can pin. +class GoalFilters { + const GoalFilters({this.query = '', this.periodType, this.window = ''}); + + final String query; + final String? periodType; + + /// `''` for every goal, `current` for the ones whose period contains today, + /// `active` for the ones nobody has paused. Two separate API flags, because + /// they mean different things: a paused goal can still be in its period. + final String window; + + bool get isFiltered => + query.trim().isNotEmpty || + (periodType ?? '').isNotEmpty || + window.isNotEmpty; + + GoalFilters copyWith({String? query, String? periodType, String? window}) => + GoalFilters( + query: query ?? this.query, + // Passing null clears the period, so it cannot ride copyWith's usual + // "null means unchanged" convention. The screen sends '' to clear. + periodType: periodType == '' ? null : (periodType ?? this.periodType), + window: window ?? this.window, + ); +} + +/// The list query for a set of filters. +/// +/// `period_type` and `window` are checked against their allowed sets rather +/// than forwarded. They come from pickers today, so nothing can currently send +/// a bad one, and a filter that pasted anything through would turn a later typo +/// into an empty list with no explanation. `search` is free text and goes +/// through [Uri], which encodes it. +/// +/// `limit=1000` fetches the whole (small) set so the totals cover every goal +/// rather than one page of them. +String goalListQuery(GoalFilters filters) { + final params = {'limit': '1000'}; + final search = filters.query.trim(); + if (search.isNotEmpty) params['search'] = search; + final period = filters.periodType ?? ''; + if (goalPeriodTypes.contains(period)) params['period_type'] = period; + if (filters.window == 'current') params['current'] = 'true'; + if (filters.window == 'active') params['active'] = 'true'; + return Uri(queryParameters: params).query; +} + /// The goals screen's data: the list, the board, and the derived totals. class GoalsData { const GoalsData({ this.goals = const [], this.leaderboard = const [], this.totals = const GoalTotals(), + this.filters = const GoalFilters(), }); final List goals; final List leaderboard; final GoalTotals totals; + + /// Echoed back so the screen can tell "no goals here" from "no goals match + /// this filter". Saying the first when the second is true sends somebody off + /// to create a duplicate of a goal they already have. + final GoalFilters filters; } /// Sales goals, plus the attainment board. @@ -33,14 +90,14 @@ class GoalsData { class GoalsNotifier extends AsyncNotifier { final ApiService _apiService = ApiService(); + GoalFilters _filters = const GoalFilters(); + @override Future build() => _fetch(); Future _fetch() async { - // `limit=1000` fetches the whole (small) set so the totals cover every - // goal rather than one page of them. final results = await Future.wait([ - _apiService.get('${ApiConfig.goals}?limit=1000'), + _apiService.get('${ApiConfig.goals}?${goalListQuery(_filters)}'), _apiService.get('${ApiConfig.goalsLeaderboard}?period_type=MONTHLY'), ]); @@ -66,6 +123,7 @@ class GoalsNotifier extends AsyncNotifier { goals: goals, leaderboard: leaderboard, totals: goalTotals(goals, today: goalToday()), + filters: _filters, ); } @@ -74,6 +132,14 @@ class GoalsNotifier extends AsyncNotifier { state = await AsyncValue.guard(_fetch); } + /// Narrow the list. The API does the filtering, so this refetches rather than + /// sifting the rows already on screen: a search that only looked at the + /// fetched page would quietly disagree with the same search on the web. + Future applyFilters(GoalFilters filters) async { + _filters = filters; + await refresh(); + } + /// `POST /api/opportunities/goals/`. Admin-only server-side. Future>> createGoal( Map body, @@ -112,6 +178,44 @@ final goalsProvider = AsyncNotifierProvider( GoalsNotifier.new, ); +/// Finished goal periods with what each one attained, newest first. +/// +/// A separate provider rather than a third leg of `GoalsNotifier._fetch`: the +/// goals screen never shows history, so folding it in would make every open of +/// that screen pay for a request nobody on it reads. +final goalHistoryProvider = FutureProvider>(( + ref, +) async { + final response = await ApiService().get(ApiConfig.goalsHistory); + if (!response.success || response.data == null) { + throw Exception(response.message ?? 'Could not load goal history.'); + } + return listFromEnvelope(response.data!, const [ + 'history', + ]).map(GoalHistoryPeriod.fromJson).toList(); +}); + +/// Turn the form's weight boxes into the map the write serializer takes. +/// +/// A blank box is not a weight of zero, it is "do not re-weigh this type", so +/// it stays out of the map. Anything non-numeric is passed through as typed +/// rather than coerced: the backend validator names the offending type in its +/// 400, and turning "heavy" into 0 would store a quota nobody asked for. +/// +/// Always sent on a write, even when empty, so clearing a box actually clears +/// the weight. PUT is partial, so an omitted key would keep the old map and a +/// weight could never be removed. +Map goalWeightFields(Map entered) { + final weights = {}; + for (final type in dealTypes) { + final raw = entered[type]?.trim() ?? ''; + if (raw.isEmpty) continue; + final parsed = num.tryParse(raw); + weights[type] = parsed ?? raw; + } + return {'type_weights': weights}; +} + /// One goal for the edit form. /// /// Fetched rather than taken from the list so the form opens on what the server diff --git a/mobile/lib/routes/app_router.dart b/mobile/lib/routes/app_router.dart index 7e1290f4..3704d20c 100644 --- a/mobile/lib/routes/app_router.dart +++ b/mobile/lib/routes/app_router.dart @@ -64,6 +64,7 @@ import '../screens/documents/documents_list_screen.dart'; import '../screens/documents/document_form_screen.dart'; import '../screens/goals/goals_screen.dart'; import '../screens/goals/goal_form_screen.dart'; +import '../screens/goals/goal_history_screen.dart'; import '../screens/invoices/invoices_list_screen.dart'; import '../screens/invoices/invoice_detail_screen.dart'; import '../screens/invoices/estimates_list_screen.dart'; @@ -172,6 +173,9 @@ class AppRoutes { /// and for the same reason. static const String goals = '/goals'; static const String goalNew = '/goals/new'; + + /// Finished periods and what each attained. Matches the web `/goals/history`. + static const String goalHistory = '/goals/history'; static const String goalEdit = '/goals/:id/edit'; static String goalEditFor(String id) => '/goals/$id/edit'; @@ -453,6 +457,12 @@ final appRouterProvider = Provider((ref) { parentNavigatorKey: _rootNavigatorKey, builder: (context, state) => const GoalFormScreen(), ), + GoRoute( + path: AppRoutes.goalHistory, + name: 'goalHistory', + parentNavigatorKey: _rootNavigatorKey, + builder: (context, state) => const GoalHistoryScreen(), + ), GoRoute( path: AppRoutes.goalEdit, name: 'goalEdit', diff --git a/mobile/lib/screens/dashboard/dashboard_screen.dart b/mobile/lib/screens/dashboard/dashboard_screen.dart index 8ee9979b..be61bda7 100644 --- a/mobile/lib/screens/dashboard/dashboard_screen.dart +++ b/mobile/lib/screens/dashboard/dashboard_screen.dart @@ -150,6 +150,18 @@ class _DashboardScreenState extends ConsumerState { const SizedBox(height: 16), ], + // Where you stand: the goals running right now. + // + // The backend has sent `goal_summary` on this endpoint all along and + // nothing rendered it, so these numbers were being computed and thrown + // away on every dashboard load. Hidden entirely when there are no + // current goals, which is the common case for a member in an org that + // does not set them. + if (data.goalSummary.isNotEmpty) ...[ + _buildGoalSection(data.goalSummary, currencyFormat), + const SizedBox(height: 16), + ], + // Hot Leads if (data.hotLeads.isNotEmpty) ...[ _buildHotLeadsSection(data.hotLeads), @@ -468,6 +480,45 @@ class _DashboardScreenState extends ConsumerState { } } + /// Progress on the goals running today, at most three. + /// + /// Every figure is the server's. `progress_percent` and `status` are computed + /// over closed-won opportunities in the period, including ones assigned to + /// people this app never fetches, so nothing here recomputes them. The colour + /// follows `status` (pace) rather than the raw percentage, so the strip agrees + /// with the goals screen instead of calling a goal green in week two of a + /// quarter for being 40% of the way there. + Widget _buildGoalSection( + List goals, + NumberFormat currencyFormat, + ) { + return Column( + crossAxisAlignment: CrossAxisAlignment.start, + children: [ + Padding( + padding: const EdgeInsets.symmetric(horizontal: 16), + child: Row( + children: [ + const Expanded( + child: Text( + 'Where you stand', + style: TextStyle(fontSize: 16, fontWeight: FontWeight.w600), + ), + ), + TextButton( + onPressed: () => context.push(AppRoutes.goals), + child: const Text('All goals'), + ), + ], + ), + ), + const SizedBox(height: 4), + for (final goal in goals) + _DashboardGoalRow(goal: goal, currencyFormat: currencyFormat), + ], + ); + } + Widget _buildHotLeadsSection(List leads) { return Column( crossAxisAlignment: CrossAxisAlignment.start, @@ -1096,3 +1147,102 @@ class _NotificationBell extends ConsumerWidget { ); } } + +/// One goal on the dashboard strip. +/// +/// A revenue target is money and a deals or activities target is a count, so +/// the unit follows the goal type rather than defaulting to currency: an +/// activities quota rendered as "$40" is what a `!= 'DEALS_CLOSED'` test +/// produced elsewhere in this app. +class _DashboardGoalRow extends StatelessWidget { + const _DashboardGoalRow({required this.goal, required this.currencyFormat}); + + final DashboardGoal goal; + final NumberFormat currencyFormat; + + Color get _colour { + switch (goal.status) { + case 'completed': + return AppColors.success600; + case 'at_risk': + return AppColors.warning600; + case 'behind': + return AppColors.danger600; + default: + return AppColors.primary600; + } + } + + String _value(double value) { + switch (goal.goalType) { + case 'DEALS_CLOSED': + final deals = value.round(); + return '$deals ${deals == 1 ? 'deal' : 'deals'}'; + case 'ACTIVITIES': + final logged = value.round(); + return '$logged ${logged == 1 ? 'activity' : 'activities'}'; + default: + return currencyFormat.format(value); + } + } + + @override + Widget build(BuildContext context) { + return InkWell( + onTap: () => context.push(AppRoutes.goals), + child: Container( + margin: const EdgeInsets.symmetric(horizontal: 16, vertical: 4), + padding: const EdgeInsets.all(14), + decoration: BoxDecoration( + color: AppColors.surface, + border: Border.all(color: Colors.grey.shade200), + borderRadius: BorderRadius.circular(8), + ), + child: Column( + crossAxisAlignment: CrossAxisAlignment.start, + children: [ + Row( + children: [ + Expanded( + child: Text( + goal.name, + maxLines: 1, + overflow: TextOverflow.ellipsis, + style: const TextStyle( + fontSize: 14, + fontWeight: FontWeight.w600, + ), + ), + ), + const SizedBox(width: 8), + Text( + '${goal.progressPercent}%', + style: TextStyle( + fontSize: 13, + fontWeight: FontWeight.w700, + color: _colour, + ), + ), + ], + ), + const SizedBox(height: 8), + ClipRRect( + borderRadius: BorderRadius.circular(3), + child: LinearProgressIndicator( + value: (goal.progressPercent / 100).clamp(0.0, 1.0), + minHeight: 6, + backgroundColor: Colors.grey.shade200, + valueColor: AlwaysStoppedAnimation(_colour), + ), + ), + const SizedBox(height: 6), + Text( + '${_value(goal.progressValue)} of ${_value(goal.targetValue)}', + style: TextStyle(fontSize: 12, color: Colors.grey.shade600), + ), + ], + ), + ), + ); + } +} diff --git a/mobile/lib/screens/goals/goal_form_screen.dart b/mobile/lib/screens/goals/goal_form_screen.dart index 77a316d1..201cb9a3 100644 --- a/mobile/lib/screens/goals/goal_form_screen.dart +++ b/mobile/lib/screens/goals/goal_form_screen.dart @@ -40,6 +40,15 @@ class _GoalFormScreenState extends ConsumerState { final TextEditingController _name = TextEditingController(); final TextEditingController _target = TextEditingController(); + /// One controller per deal type. A blank box means "count this type in full", + /// so nothing is pre-filled with 1: an untouched form has to store an empty + /// map, or every goal would grow five redundant weights the first time + /// somebody opened its edit screen. + final Map _weights = { + for (final type in dealTypes) type: TextEditingController(), + }; + bool _weightsOpen = false; + String _goalType = 'REVENUE'; String _periodType = 'MONTHLY'; String _periodStart = ''; @@ -47,6 +56,13 @@ class _GoalFormScreenState extends ConsumerState { String _targetOwner = 'org'; bool _isActive = true; + /// How many boxes hold something other than the default weight of 1. + int get _weightedCount => _weights.values + .where( + (c) => c.text.trim().isNotEmpty && num.tryParse(c.text.trim()) != 1, + ) + .length; + bool _saving = false; bool _dirty = false; bool _loaded = false; @@ -72,6 +88,9 @@ class _GoalFormScreenState extends ConsumerState { void dispose() { _name.dispose(); _target.dispose(); + for (final controller in _weights.values) { + controller.dispose(); + } super.dispose(); } @@ -94,6 +113,12 @@ class _GoalFormScreenState extends ConsumerState { _periodEnd = goal.periodEnd; _targetOwner = goalTargetValue(goal); _isActive = goal.isActive; + for (final entry in goal.typeWeights.entries) { + _weights[entry.key]?.text = entry.value.toString(); + } + // Open when the goal already carries weights, so an existing weighting is + // visible rather than hidden behind a control nobody thought to tap. + _weightsOpen = goal.typeWeights.isNotEmpty; // Set last: filling the controllers fires the listeners above, and a form // that opens already dirty prompts on the way out of a page nobody edited. _loaded = true; @@ -147,6 +172,17 @@ class _GoalFormScreenState extends ConsumerState { 'period_end': _periodEnd, 'is_active': _isActive, ...goalTargetFields(_targetOwner), + // Empty for an ACTIVITIES goal, which has no deal type and which the + // backend refuses weights on. Sent either way so clearing a box clears + // the weight: PUT is partial, and an omitted key keeps the old map. + ...goalWeightFields( + _goalType == 'ACTIVITIES' + ? const {} + : { + for (final entry in _weights.entries) + entry.key: entry.value.text, + }, + ), }; final notifier = ref.read(goalsProvider.notifier); @@ -323,14 +359,105 @@ class _GoalFormScreenState extends ConsumerState { decoration: InputDecoration( labelText: 'Target', border: const OutlineInputBorder(), - helperText: _goalType == 'DEALS_CLOSED' - ? 'How many deals must close in the period' - : 'Total revenue that must close in the period', + helperText: switch (_goalType) { + 'DEALS_CLOSED' => 'How many deals must close in the period', + 'ACTIVITIES' => + 'How many activities must be logged in the period', + _ => 'Total revenue that must close in the period', + }, helperMaxLines: 2, ), ), const SizedBox(height: 16), + // Optional per-deal-type multipliers. Hidden entirely for an + // ACTIVITIES goal, which counts logged activity and has no deal + // type to weigh; the backend refuses weights on one with a 400. + if (_goalType != 'ACTIVITIES') ...[ + InkWell( + onTap: () => setState(() => _weightsOpen = !_weightsOpen), + child: Container( + // 44px so the row is a real tap target between two other + // controls, where a few pixels either way is easy to miss. + constraints: const BoxConstraints(minHeight: 44), + alignment: Alignment.centerLeft, + child: Row( + children: [ + const Expanded( + child: Text( + 'Weight by deal type', + style: TextStyle(fontWeight: FontWeight.w600), + ), + ), + Text( + _weightedCount == 0 + ? 'Optional' + : '$_weightedCount adjusted', + style: TextStyle( + fontSize: 12, + color: Colors.grey.shade600, + ), + ), + Icon( + _weightsOpen + ? LucideIcons.chevronUp + : LucideIcons.chevronDown, + size: 18, + color: Colors.grey.shade600, + ), + ], + ), + ), + ), + if (_weightsOpen) ...[ + const SizedBox(height: 8), + for (final type in dealTypes) + Padding( + padding: const EdgeInsets.only(bottom: 8), + child: Row( + children: [ + Expanded( + child: Text( + dealTypeLabel(type), + style: TextStyle( + fontSize: 13, + color: Colors.grey.shade700, + ), + ), + ), + SizedBox( + width: 96, + child: TextField( + controller: _weights[type], + textAlign: TextAlign.right, + keyboardType: const TextInputType.numberWithOptions( + decimal: true, + ), + inputFormatters: [ + FilteringTextInputFormatter.allow( + RegExp(r'[0-9.]'), + ), + ], + decoration: const InputDecoration( + hintText: '1', + isDense: true, + border: OutlineInputBorder(), + ), + ), + ), + ], + ), + ), + Text( + 'A multiplier on each closed-won deal of that type. Leave a ' + 'box empty to count that type in full. At 0.5 a 20,000 ' + 'renewal counts as 10,000; at 0 it does not count at all.', + style: TextStyle(fontSize: 12, color: Colors.grey.shade600), + ), + ], + const SizedBox(height: 16), + ], + DropdownButtonFormField( initialValue: _periodType, decoration: const InputDecoration( diff --git a/mobile/lib/screens/goals/goal_history_screen.dart b/mobile/lib/screens/goals/goal_history_screen.dart new file mode 100644 index 00000000..ca67e39e --- /dev/null +++ b/mobile/lib/screens/goals/goal_history_screen.dart @@ -0,0 +1,306 @@ +import 'package:flutter/material.dart'; +import 'package:flutter_riverpod/flutter_riverpod.dart'; +import 'package:lucide_icons_flutter/lucide_icons.dart'; + +import '../../core/theme/theme.dart'; +import '../../data/models/deal.dart' show Currency; +import '../../data/models/sales_goal.dart'; +import '../../providers/goals_provider.dart'; +import '../../providers/settings_provider.dart'; +import 'goals_screen.dart' show formatGoalValue; + +/// How did we do, one finished period at a time. +/// +/// The goals screen answers "how are we doing"; nothing answered this. A closed +/// period cannot move any more, so the interesting number is not pace but +/// whether the number was made, and by how many people. +/// +/// A card is a period AND a goal type, which is how the API groups them: +/// pooling a revenue target in currency with a deals-closed target in deals +/// produced a single meaningless figure, so each card carries one unit. +/// +/// Reading is open to any member and narrowed server-side to their own goals +/// and their teams', the same rule the list and the board use, so nothing here +/// gates on role. +class GoalHistoryScreen extends ConsumerWidget { + const GoalHistoryScreen({super.key}); + + @override + Widget build(BuildContext context, WidgetRef ref) { + final async = ref.watch(goalHistoryProvider); + final symbol = + ref.watch(orgSettingsProvider).value?.currencySymbol ?? + Currency.usd.symbol; + + return Scaffold( + backgroundColor: AppColors.surfaceDim, + appBar: AppBar( + title: const Text('Goal history'), + backgroundColor: AppColors.surface, + elevation: 0, + scrolledUnderElevation: 1, + ), + body: async.when( + loading: () => const Center(child: CircularProgressIndicator()), + error: (_, _) => + _HistoryError(onRetry: () => ref.invalidate(goalHistoryProvider)), + data: (periods) { + if (periods.isEmpty) return const _HistoryEmpty(); + return RefreshIndicator( + onRefresh: () async => ref.invalidate(goalHistoryProvider), + child: ListView( + padding: const EdgeInsets.only(top: 8, bottom: 32), + children: [ + for (final period in periods) + _PeriodCard(period: period, symbol: symbol), + Padding( + padding: const EdgeInsets.fromLTRB(16, 12, 16, 0), + child: Text( + 'The twelve most recent periods you can see. Attainment is ' + 'recomputed from the closed-won deals of each period, so it ' + 'matches what the goals screen showed while the period was ' + 'running.', + style: TextStyle(fontSize: 12, color: Colors.grey.shade600), + ), + ), + ], + ), + ); + }, + ), + ); + } +} + +class _PeriodCard extends StatelessWidget { + const _PeriodCard({required this.period, required this.symbol}); + + final GoalHistoryPeriod period; + final String symbol; + + Color get _colour { + if (period.percent >= 100) return AppColors.success600; + if (period.percent >= 80) return AppColors.warning600; + return AppColors.danger600; + } + + @override + Widget build(BuildContext context) { + final goalWord = period.goalsCount == 1 ? 'goal' : 'goals'; + + return Container( + margin: const EdgeInsets.symmetric(horizontal: 16, vertical: 4), + padding: const EdgeInsets.all(14), + decoration: BoxDecoration( + color: AppColors.surface, + border: Border.all(color: Colors.grey.shade200), + borderRadius: BorderRadius.circular(8), + ), + child: Column( + crossAxisAlignment: CrossAxisAlignment.start, + children: [ + Row( + crossAxisAlignment: CrossAxisAlignment.start, + children: [ + Expanded( + child: Column( + crossAxisAlignment: CrossAxisAlignment.start, + children: [ + Text( + goalDateRange(period.periodStart, period.periodEnd), + style: const TextStyle( + fontWeight: FontWeight.w600, + fontSize: 14, + ), + ), + const SizedBox(height: 2), + Text( + '${goalPeriodLabel(period.periodType)} · ' + '${goalTypeLabel(period.goalType)} · ' + '${period.attainedCount} of ${period.goalsCount} ' + '$goalWord met', + style: TextStyle( + fontSize: 12, + color: Colors.grey.shade600, + ), + ), + ], + ), + ), + const SizedBox(width: 8), + Container( + padding: const EdgeInsets.symmetric(horizontal: 8, vertical: 4), + decoration: BoxDecoration( + color: _colour.withValues(alpha: 0.12), + borderRadius: BorderRadius.circular(4), + ), + child: Text( + '${period.percent}% of target', + style: TextStyle( + fontSize: 12, + fontWeight: FontWeight.w600, + color: _colour, + ), + ), + ), + ], + ), + const SizedBox(height: 10), + ClipRRect( + borderRadius: BorderRadius.circular(3), + child: LinearProgressIndicator( + // `percent` is uncapped so an over-attaining period reports what + // it did; a bar cannot be more than full, and the figure above + // carries the overshoot. + value: (period.percent / 100).clamp(0.0, 1.0), + minHeight: 6, + backgroundColor: Colors.grey.shade200, + valueColor: AlwaysStoppedAnimation(_colour), + ), + ), + const SizedBox(height: 6), + Text( + '${formatGoalValue(period.achieved, goalType: period.goalType, symbol: symbol)}' + ' of ' + '${formatGoalValue(period.target, goalType: period.goalType, symbol: symbol)}', + style: TextStyle(fontSize: 12, color: Colors.grey.shade600), + ), + if (period.goals.isNotEmpty) ...[ + const SizedBox(height: 12), + Divider(height: 1, color: Colors.grey.shade200), + for (final goal in period.goals) + _GoalOutcome(goal: goal, symbol: symbol), + ], + ], + ), + ); + } +} + +class _GoalOutcome extends StatelessWidget { + const _GoalOutcome({required this.goal, required this.symbol}); + + final SalesGoal goal; + final String symbol; + + @override + Widget build(BuildContext context) { + final met = goal.targetValue > 0 && goal.progressValue >= goal.targetValue; + + return Padding( + padding: const EdgeInsets.only(top: 10), + child: Row( + crossAxisAlignment: CrossAxisAlignment.start, + children: [ + Expanded( + child: Column( + crossAxisAlignment: CrossAxisAlignment.start, + children: [ + Text( + goal.name, + maxLines: 1, + overflow: TextOverflow.ellipsis, + style: const TextStyle( + fontSize: 13, + fontWeight: FontWeight.w500, + ), + ), + const SizedBox(height: 2), + Text( + '${goal.targetLabel} · ' + '${formatGoalValue(goal.progressValue, goalType: goal.goalType, symbol: symbol)}' + ' / ' + '${formatGoalValue(goal.targetValue, goalType: goal.goalType, symbol: symbol)}', + maxLines: 2, + overflow: TextOverflow.ellipsis, + style: TextStyle(fontSize: 12, color: Colors.grey.shade600), + ), + ], + ), + ), + const SizedBox(width: 8), + Container( + padding: const EdgeInsets.symmetric(horizontal: 8, vertical: 3), + decoration: BoxDecoration( + color: met + ? AppColors.success600.withValues(alpha: 0.12) + : Colors.grey.shade100, + borderRadius: BorderRadius.circular(4), + ), + child: Text( + met ? 'Met' : 'Missed', + style: TextStyle( + fontSize: 11, + fontWeight: FontWeight.w600, + color: met ? AppColors.success600 : Colors.grey.shade700, + ), + ), + ), + ], + ), + ); + } +} + +class _HistoryEmpty extends StatelessWidget { + const _HistoryEmpty(); + + @override + Widget build(BuildContext context) { + return Center( + child: Padding( + padding: const EdgeInsets.all(32), + child: Column( + mainAxisSize: MainAxisSize.min, + children: [ + Icon(LucideIcons.history, size: 40, color: Colors.grey.shade400), + const SizedBox(height: 12), + const Text( + 'No finished periods yet', + style: TextStyle(fontSize: 16, fontWeight: FontWeight.w600), + ), + const SizedBox(height: 6), + Text( + // Says which set is empty. The endpoint narrows a member to their + // own goals and their teams', so "nothing has finished yet" would + // be a claim about the org that a member cannot actually see. + 'Once a goal\'s period ends it moves here with what it attained. ' + 'Goals still running are on the goals screen.', + textAlign: TextAlign.center, + style: TextStyle(fontSize: 13, color: Colors.grey.shade600), + ), + ], + ), + ), + ); + } +} + +class _HistoryError extends StatelessWidget { + const _HistoryError({required this.onRetry}); + + final VoidCallback onRetry; + + @override + Widget build(BuildContext context) { + return Center( + child: Padding( + padding: const EdgeInsets.all(32), + child: Column( + mainAxisSize: MainAxisSize.min, + children: [ + Icon( + LucideIcons.triangleAlert, + size: 36, + color: Colors.grey.shade400, + ), + const SizedBox(height: 12), + const Text('Could not load goal history'), + const SizedBox(height: 12), + FilledButton(onPressed: onRetry, child: const Text('Try again')), + ], + ), + ), + ); + } +} diff --git a/mobile/lib/screens/goals/goals_screen.dart b/mobile/lib/screens/goals/goals_screen.dart index 8ff53934..c02d3da4 100644 --- a/mobile/lib/screens/goals/goals_screen.dart +++ b/mobile/lib/screens/goals/goals_screen.dart @@ -24,11 +24,28 @@ import '../../widgets/common/badge.dart'; /// goals and their teams'. Creating, editing and deleting are admin-only, so /// the compose button and the row chevrons appear for an admin alone. Hiding /// them is not what keeps a member out. -class GoalsScreen extends ConsumerWidget { +class GoalsScreen extends ConsumerStatefulWidget { const GoalsScreen({super.key}); @override - Widget build(BuildContext context, WidgetRef ref) { + ConsumerState createState() => _GoalsScreenState(); +} + +class _GoalsScreenState extends ConsumerState { + final TextEditingController _search = TextEditingController(); + + @override + void dispose() { + _search.dispose(); + super.dispose(); + } + + void _apply(GoalFilters filters) { + ref.read(goalsProvider.notifier).applyFilters(filters); + } + + @override + Widget build(BuildContext context) { final async = ref.watch(goalsProvider); final isAdmin = ref.watch(isOrgAdminProvider); // The goals endpoints carry no currency of their own, and a REVENUE target @@ -47,6 +64,11 @@ class GoalsScreen extends ConsumerWidget { elevation: 0, scrolledUnderElevation: 1, actions: [ + IconButton( + icon: const Icon(LucideIcons.history), + tooltip: 'Goal history', + onPressed: () => context.push(AppRoutes.goalHistory), + ), if (isAdmin) IconButton( icon: const Icon(LucideIcons.plus), @@ -61,23 +83,42 @@ class GoalsScreen extends ConsumerWidget { onRetry: () => ref.read(goalsProvider.notifier).refresh(), ), data: (data) { - if (data.goals.isEmpty) return _EmptyState(isAdmin: isAdmin); + // An unfiltered empty list is "there are no goals"; a filtered one is + // "none match", and saying the first when the second is true sends + // somebody off to create a goal they already have. + if (data.goals.isEmpty && !data.filters.isFiltered) { + return _EmptyState(isAdmin: isAdmin); + } return RefreshIndicator( onRefresh: () => ref.read(goalsProvider.notifier).refresh(), child: ListView( padding: const EdgeInsets.only(bottom: 96), children: [ - _Summary(totals: data.totals, symbol: symbol), - const _SectionHeader('GOALS'), - for (final goal in data.goals) - _GoalRow( - goal: goal, - symbol: symbol, - onTap: isAdmin - ? () => context.push(AppRoutes.goalEditFor(goal.id)) - : null, - ), - _Leaderboard(rows: data.leaderboard, symbol: symbol), + _Filters( + controller: _search, + filters: data.filters, + onChanged: _apply, + ), + if (data.goals.isEmpty) + _NoMatchState( + onClear: () { + _search.clear(); + _apply(const GoalFilters()); + }, + ) + else ...[ + _Summary(totals: data.totals, symbol: symbol), + const _SectionHeader('GOALS'), + for (final goal in data.goals) + _GoalRow( + goal: goal, + symbol: symbol, + onTap: isAdmin + ? () => context.push(AppRoutes.goalEditFor(goal.id)) + : null, + ), + _Leaderboard(rows: data.leaderboard, symbol: symbol), + ], ], ), ); @@ -87,18 +128,27 @@ class GoalsScreen extends ConsumerWidget { } } -/// A target or an attainment, priced when the goal is about revenue and counted -/// when it is about deals. `compactCurrency` keeps a six-figure quota on one -/// line at 390px, where the full number wraps. +/// A target or an attainment, in the unit its own goal type is measured in. +/// +/// Takes the goal type rather than a bare `isMoney` flag, which is what it used +/// to take: "not money" was rendered as deals, so the moment a third type +/// arrived a quota of forty logged activities printed as "40 deals". +/// +/// `compactCurrency` keeps a six-figure quota on one line at 390px, where the +/// full number wraps. String formatGoalValue( double value, { - required bool isMoney, + required String goalType, required String symbol, }) { - if (!isMoney) { + if (goalType == 'DEALS_CLOSED') { final deals = value.round(); return '$deals ${deals == 1 ? 'deal' : 'deals'}'; } + if (goalType == 'ACTIVITIES') { + final logged = value.round(); + return '$logged ${logged == 1 ? 'activity' : 'activities'}'; + } return NumberFormat.compactCurrency( symbol: symbol, decimalDigits: 0, @@ -235,12 +285,12 @@ class _GoalRow extends StatelessWidget { Widget build(BuildContext context) { final achieved = formatGoalValue( goal.progressValue, - isMoney: goal.isMoney, + goalType: goal.goalType, symbol: symbol, ); final target = formatGoalValue( goal.targetValue, - isMoney: goal.isMoney, + goalType: goal.goalType, symbol: symbol, ); final colour = goalStatusColour(goal.status); @@ -306,9 +356,18 @@ class _GoalRow extends StatelessWidget { StatusBadge(label: goalStatusLabel(goal.status), color: colour), if (!goal.isActive) StatusBadge(label: 'Retired', color: AppColors.gray500), + // Named on the row because a weighted goal's progress does not + // add up to the deals behind it, and somebody checking the + // arithmetic against the pipeline needs to know that before + // they file a bug. + if (goal.weightedTypeCount > 0) + StatusBadge( + label: 'Weighted (${goal.weightedTypeCount})', + color: AppColors.gray500, + ), Text( '${goalPeriodLabel(goal.periodType)} · ' - '${goal.periodStart} to ${goal.periodEnd}', + '${goalDateRange(goal.periodStart, goal.periodEnd)}', style: AppTypography.caption.copyWith( color: AppColors.textTertiary, ), @@ -385,9 +444,9 @@ class _Leaderboard extends StatelessWidget { overflow: TextOverflow.ellipsis, ), Text( - '${formatGoalValue(row.achieved, isMoney: true, symbol: symbol)}' + '${formatGoalValue(row.achieved, goalType: 'REVENUE', symbol: symbol)}' ' of ' - '${formatGoalValue(row.target, isMoney: true, symbol: symbol)}', + '${formatGoalValue(row.target, goalType: 'REVENUE', symbol: symbol)}', style: AppTypography.caption.copyWith( color: AppColors.textSecondary, ), @@ -495,3 +554,150 @@ class _ErrorState extends StatelessWidget { ); } } + +/// Search and the two window choices, matching the web filter bar. +/// +/// The API does the filtering; nothing here sifts rows already fetched, so a +/// search on the phone and the same search on the web return the same set. +/// +/// Search submits on done rather than on every keystroke: each apply is a round +/// trip, and firing one per character would be a request per letter on a phone +/// connection. +class _Filters extends StatelessWidget { + const _Filters({ + required this.controller, + required this.filters, + required this.onChanged, + }); + + final TextEditingController controller; + final GoalFilters filters; + final ValueChanged onChanged; + + @override + Widget build(BuildContext context) { + return Container( + color: AppColors.surface, + padding: const EdgeInsets.fromLTRB(16, 12, 16, 8), + child: Column( + crossAxisAlignment: CrossAxisAlignment.start, + children: [ + TextField( + controller: controller, + textInputAction: TextInputAction.search, + onSubmitted: (value) => onChanged(filters.copyWith(query: value)), + decoration: InputDecoration( + hintText: 'Search goals by name', + prefixIcon: const Icon(LucideIcons.search, size: 18), + suffixIcon: filters.query.isEmpty + ? null + : IconButton( + icon: const Icon(LucideIcons.x, size: 18), + tooltip: 'Clear search', + onPressed: () { + controller.clear(); + onChanged(filters.copyWith(query: '')); + }, + ), + isDense: true, + border: const OutlineInputBorder(), + ), + ), + const SizedBox(height: 8), + SingleChildScrollView( + scrollDirection: Axis.horizontal, + child: Row( + children: [ + _FilterChip( + label: 'Running today', + selected: filters.window == 'current', + onSelected: (on) => + onChanged(filters.copyWith(window: on ? 'current' : '')), + ), + const SizedBox(width: 8), + _FilterChip( + label: 'Not paused', + selected: filters.window == 'active', + onSelected: (on) => + onChanged(filters.copyWith(window: on ? 'active' : '')), + ), + for (final period in goalPeriodTypes) ...[ + const SizedBox(width: 8), + _FilterChip( + label: goalPeriodLabel(period), + selected: filters.periodType == period, + onSelected: (on) => onChanged( + filters.copyWith(periodType: on ? period : ''), + ), + ), + ], + ], + ), + ), + ], + ), + ); + } +} + +class _FilterChip extends StatelessWidget { + const _FilterChip({ + required this.label, + required this.selected, + required this.onSelected, + }); + + final String label; + final bool selected; + final ValueChanged onSelected; + + @override + Widget build(BuildContext context) { + // Standard density and a padded tap target on purpose. `VisualDensity + // .compact` drew these at 34px, under the roughly 44px a thumb needs, and + // they sit in a horizontal scroller where a mis-tap scrolls the row instead + // of toggling the filter. + return FilterChip( + label: Text(label), + selected: selected, + onSelected: onSelected, + showCheckmark: false, + materialTapTargetSize: MaterialTapTargetSize.padded, + padding: const EdgeInsets.symmetric(horizontal: 12, vertical: 12), + ); + } +} + +/// The list came back empty because of the filter, not because the org has no +/// goals. Offers the way out rather than only naming the problem. +class _NoMatchState extends StatelessWidget { + const _NoMatchState({required this.onClear}); + + final VoidCallback onClear; + + @override + Widget build(BuildContext context) { + return Padding( + padding: const EdgeInsets.fromLTRB(32, 48, 32, 32), + child: Column( + children: [ + Icon(LucideIcons.target, size: 36, color: Colors.grey.shade400), + const SizedBox(height: 12), + const Text( + 'No goals match this filter', + style: TextStyle(fontSize: 16, fontWeight: FontWeight.w600), + ), + const SizedBox(height: 6), + Text( + 'Nothing here matches the search and period you picked. Clearing ' + 'the filter shows everything you can see.', + textAlign: TextAlign.center, + style: TextStyle(fontSize: 13, color: Colors.grey.shade600), + ), + const SizedBox(height: 16), + OutlinedButton(onPressed: onClear, child: const Text('Clear filter')), + ], + ), + ); + } +} diff --git a/mobile/test/screens/phone_viewport_test.dart b/mobile/test/screens/phone_viewport_test.dart index 22234115..652daf77 100644 --- a/mobile/test/screens/phone_viewport_test.dart +++ b/mobile/test/screens/phone_viewport_test.dart @@ -63,6 +63,8 @@ import 'package:bottle_crm/data/models/sales_goal.dart'; import 'package:bottle_crm/providers/documents_provider.dart'; import 'package:bottle_crm/providers/goals_provider.dart'; import 'package:bottle_crm/screens/documents/documents_list_screen.dart'; +import 'package:bottle_crm/screens/goals/goal_form_screen.dart'; +import 'package:bottle_crm/screens/goals/goal_history_screen.dart'; import 'package:bottle_crm/screens/goals/goals_screen.dart'; import 'package:bottle_crm/screens/timesheet/timesheet_screen.dart'; import 'package:flutter/material.dart'; @@ -2811,6 +2813,21 @@ void main() { 'says why the board is empty rather than showing a bare heading', (tester) async { await pump(tester, goalsApp(isAdmin: false, board: false)); + // Scrolled to: the board sits under the filter bar, the summary and + // every goal row, so on a 390px screen a lazy ListView has not built it + // yet when the screen first settles. + await tester.scrollUntilVisible( + find.textContaining('Nothing to rank yet'), + 200, + // Named explicitly: the filter bar's chip row is a second Scrollable + // on this screen, and the default finder matches both. + scrollable: find + .descendant( + of: find.byType(ListView), + matching: find.byType(Scrollable), + ) + .first, + ); expect(find.textContaining('Nothing to rank yet'), findsOneWidget); expect(tester.takeException(), isNull); }, @@ -2846,6 +2863,162 @@ void main() { ); } }); + + testWidgets('names a weighted goal so its arithmetic is not a surprise', ( + tester, + ) async { + // A weighted goal's progress does not add up to the deals behind it, and + // somebody reconciling it against the pipeline needs to know before they + // file a bug. + await pump(tester, goalsApp(isAdmin: true)); + expect(find.textContaining('Weighted (1)'), findsOneWidget); + }); + + testWidgets('offers the filter controls at a phone width', (tester) async { + await pump(tester, goalsApp(isAdmin: true)); + expect(find.text('Running today'), findsOneWidget); + expect(find.text('Search goals by name'), findsOneWidget); + expect(tester.takeException(), isNull); + }); + }); + + /// The form pushed on top of a parent route, because a successful save pops + /// back to the list and a single-route test router has nothing to pop. + Widget routedForm(Widget screen) => MaterialApp.router( + routerConfig: GoRouter( + initialLocation: '/here/edit', + routes: [ + GoRoute( + path: '/here', + builder: (_, _) => const Scaffold(body: Text('goals list')), + routes: [GoRoute(path: 'edit', builder: (_, _) => screen)], + ), + ], + ), + ); + + /// The edit form for a goal that already carries weights. + Widget goalFormApp(_CapturingGoals capture) => ProviderScope( + overrides: [ + goalsProvider.overrideWith(() => capture), + isOrgAdminProvider.overrideWithValue(true), + goalProvider('g1').overrideWith( + (ref) async => SalesGoal.fromJson(const { + 'id': 'g1', + 'name': 'Q3 revenue', + 'goal_type': 'REVENUE', + 'target_value': '250000.00', + 'period_type': 'QUARTERLY', + 'period_start': '2026-07-01', + 'period_end': '2026-09-30', + 'type_weights': {'RENEWAL': 0.5}, + 'is_active': true, + }), + ), + ], + child: routedForm(const GoalFormScreen(goalId: 'g1')), + ); + + group('goal form weights', () { + testWidgets('opens already showing a weighting the goal carries', ( + tester, + ) async { + // Hidden behind a collapsed control, an existing weight is invisible, and + // the next person to edit the goal would not know its progress is scaled. + await pump(tester, goalFormApp(_CapturingGoals())); + expect(find.text('Weight by deal type'), findsOneWidget); + expect(find.widgetWithText(TextField, '0.5'), findsOneWidget); + }); + + testWidgets('keeps the weights when the section is collapsed and saved', ( + tester, + ) async { + // The web half of this had a real bug: there the inputs ARE the state, so + // collapsing removed them from the form and the save cleared the goal's + // weighting. Here the controllers belong to the State and outlive the + // widgets, so collapsing is only a view change. This pins that, because + // the two clients reaching the same outcome by different means is exactly + // the kind of thing a later refactor breaks on one side only. + final capture = _CapturingGoals(); + await pump(tester, goalFormApp(capture)); + + await tester.ensureVisible(find.text('Weight by deal type')); + await tester.tap(find.text('Weight by deal type')); + await tester.pumpAndSettle(); + expect(find.widgetWithText(TextField, '0.5'), findsNothing); + + // Scrolled to: the form is taller than a 390px screen, so the save + // button is off-view when it settles. + await tester.ensureVisible(find.text('Save changes')); + await tester.pumpAndSettle(); + await tester.tap(find.text('Save changes')); + await tester.pumpAndSettle(); + + expect(capture.body?['type_weights'], {'RENEWAL': 0.5}); + }); + + testWidgets('clears a weight when the box is emptied', (tester) async { + // The other half of the same rule: the map is sent on every save, so an + // emptied box has to actually remove the weight rather than be ignored. + final capture = _CapturingGoals(); + await pump(tester, goalFormApp(capture)); + + await tester.ensureVisible(find.widgetWithText(TextField, '0.5')); + await tester.enterText(find.widgetWithText(TextField, '0.5'), ''); + await tester.pumpAndSettle(); + + await tester.ensureVisible(find.text('Save changes')); + await tester.pumpAndSettle(); + await tester.tap(find.text('Save changes')); + await tester.pumpAndSettle(); + + expect(capture.body?['type_weights'], isEmpty); + }); + }); + + /// Goal history: one card per finished period and goal type. + Widget historyApp({List? periods}) => ProviderScope( + overrides: [ + goalHistoryProvider.overrideWith( + (ref) async => periods ?? _historyFixture(), + ), + ], + child: routed(const GoalHistoryScreen()), + ); + + group('goal history', () { + testWidgets('renders finished periods without overflowing', (tester) async { + await pump(tester, historyApp()); + expect(find.textContaining('1 May - 31 May'), findsOneWidget); + expect(tester.takeException(), isNull); + }); + + testWidgets('fits with the system font scaled up', (tester) async { + await pump(tester, historyApp(), textScale: 1.5); + expect(tester.takeException(), isNull); + }); + + testWidgets('keeps each unit in its own card', (tester) async { + // Grouping on the period alone pooled a currency target with a deals + // target and printed the sum as money. One card is one unit. + await pump(tester, historyApp()); + expect(find.textContaining(r'$'), findsWidgets); + expect(find.textContaining('4 deals'), findsWidgets); + }); + + testWidgets('reports attainment above target rather than capping it', ( + tester, + ) async { + await pump(tester, historyApp()); + expect(find.textContaining('134% of target'), findsOneWidget); + }); + + testWidgets('says nothing has finished rather than showing a bare list', ( + tester, + ) async { + await pump(tester, historyApp(periods: const [])); + expect(find.textContaining('No finished periods yet'), findsOneWidget); + }); }); /// Documents. [myEmail] decides who uploaded what, which is the whole of the @@ -3923,6 +4096,68 @@ class _FakeGoalsNoBoard extends GoalsNotifier { Future build() async => _goalsFixture(withBoard: false); } +List _historyFixture() => [ + GoalHistoryPeriod.fromJson(const { + 'period_start': '2026-04-01', + 'period_end': '2026-06-30', + 'period_type': 'QUARTERLY', + 'goal_type': 'REVENUE', + 'goals_count': 1, + 'attained_count': 1, + 'target': 984000, + 'achieved': 1318859, + 'percent': 134, + 'goals': [ + { + 'id': 'h1', + 'name': 'Q2 revenue quota', + 'goal_type': 'REVENUE', + 'target_value': '984000.00', + 'progress_value': 1318859, + 'progress_percent': 100, + }, + ], + }), + GoalHistoryPeriod.fromJson(const { + 'period_start': '2026-05-01', + 'period_end': '2026-05-31', + 'period_type': 'MONTHLY', + 'goal_type': 'DEALS_CLOSED', + 'goals_count': 4, + 'attained_count': 0, + 'target': 56, + 'achieved': 4, + 'percent': 7, + 'goals': [ + { + 'id': 'h2', + 'name': 'May new logo drive', + 'goal_type': 'DEALS_CLOSED', + 'target_value': '18', + 'progress_value': 2, + 'progress_percent': 11, + }, + ], + }), +]; + +/// Records the body the form sends instead of calling the API. +class _CapturingGoals extends GoalsNotifier { + Map? body; + + @override + Future build() async => const GoalsData(); + + @override + Future>> updateGoal( + String id, + Map payload, + ) async { + body = payload; + return ApiResponse(success: true, data: const {}, statusCode: 200); + } +} + class _FakeNoGoals extends GoalsNotifier { @override Future build() async => const GoalsData(); @@ -3934,6 +4169,7 @@ GoalsData _goalsFixture({required bool withBoard}) { 'id': 'g1', 'name': 'Q3 revenue', 'goal_type': 'REVENUE', + 'type_weights': {'RENEWAL': 0.5}, 'target_value': '250000.00', 'period_type': 'QUARTERLY', 'period_start': '2026-07-01', diff --git a/mobile/test/unit/goals_test.dart b/mobile/test/unit/goals_test.dart index acad6464..71bf5697 100644 --- a/mobile/test/unit/goals_test.dart +++ b/mobile/test/unit/goals_test.dart @@ -1,5 +1,6 @@ import 'package:bottle_crm/data/models/sales_goal.dart'; import 'package:bottle_crm/providers/goals_provider.dart'; +import 'package:bottle_crm/screens/goals/goals_screen.dart'; import 'package:flutter_test/flutter_test.dart'; SalesGoal goal({ @@ -335,4 +336,209 @@ void main() { } }); }); + + _activitiesAndWeights(); + _dateRangeTests(); + _filterTests(); +} + +void _activitiesAndWeights() { + group('activity goals', () { + test('offers ACTIVITIES as a goal type the backend accepts', () { + expect(goalTypes, contains('ACTIVITIES')); + }); + + test('labels an activities goal', () { + expect(goalTypeLabel('ACTIVITIES'), 'Activities'); + }); + + test('counts activities rather than formatting them as money', () { + // `isMoney` used to read `goalType != 'DEALS_CLOSED'`, so a third goal + // type would have printed "$40" for a quota of forty logged activities. + expect(goal(goalType: 'ACTIVITIES').isMoney, isFalse); + expect(goal(goalType: 'DEALS_CLOSED').isMoney, isFalse); + expect(goal(goalType: 'REVENUE').isMoney, isTrue); + }); + }); + + group('deal type weights', () { + test('names the five deal types a weight can be set against', () { + expect(dealTypes, [ + 'NEW_BUSINESS', + 'EXISTING_BUSINESS', + 'RENEWAL', + 'UPSELL', + 'CROSS_SELL', + ]); + }); + + test('reads the stored weight map off the payload', () { + final parsed = SalesGoal.fromJson({ + 'id': 'g1', + 'name': 'Weighted', + 'goal_type': 'REVENUE', + 'target_value': '100', + 'type_weights': {'RENEWAL': 0.5}, + }); + + expect(parsed.typeWeights, {'RENEWAL': 0.5}); + }); + + test('reports an unweighted goal as an empty map, never null', () { + final parsed = SalesGoal.fromJson({'id': 'g1', 'name': 'Plain'}); + + expect(parsed.typeWeights, isEmpty); + }); + + test('counts only the types actually re-weighed', () { + expect(goal().weightedTypeCount, 0); + expect( + SalesGoal.fromJson({ + 'id': 'g', + 'name': 'n', + // A weight of exactly 1 is the default, so it is not an adjustment. + 'type_weights': {'RENEWAL': 0.5, 'UPSELL': 1}, + }).weightedTypeCount, + 1, + ); + }); + }); + + group('formatGoalValue', () { + test('counts deals as deals', () { + expect( + formatGoalValue(3, goalType: 'DEALS_CLOSED', symbol: r'$'), + '3 deals', + ); + expect( + formatGoalValue(1, goalType: 'DEALS_CLOSED', symbol: r'$'), + '1 deal', + ); + }); + + test('counts activities as activities, not as deals', () { + // The unit used to hang off `isMoney`, so anything that was not revenue + // was printed as deals: a quota of forty logged activities read "40 + // deals". + expect( + formatGoalValue(40, goalType: 'ACTIVITIES', symbol: r'$'), + '40 activities', + ); + expect( + formatGoalValue(1, goalType: 'ACTIVITIES', symbol: r'$'), + '1 activity', + ); + }); + + test('prices revenue in the org currency', () { + expect( + formatGoalValue(120000, goalType: 'REVENUE', symbol: r'$'), + contains(r'$'), + ); + }); + }); + + group('GoalHistoryPeriod', () { + test('reads a finished period and keeps its goal type', () { + final period = GoalHistoryPeriod.fromJson({ + 'period_start': '2026-01-01', + 'period_end': '2026-01-31', + 'period_type': 'MONTHLY', + 'goal_type': 'REVENUE', + 'goals_count': 2, + 'attained_count': 1, + 'target': 300, + 'achieved': 240, + 'percent': 80, + 'goals': [ + {'id': 'g1', 'name': 'Jan', 'goal_type': 'REVENUE'}, + ], + }); + + expect(period.goalType, 'REVENUE'); + expect(period.percent, 80); + expect(period.attainedCount, 1); + expect(period.goals.single.name, 'Jan'); + expect(period.isMoney, isTrue); + }); + + test('does not format a deals period as money', () { + final period = GoalHistoryPeriod.fromJson({ + 'goal_type': 'DEALS_CLOSED', + 'target': 56, + 'achieved': 4, + 'percent': 7, + }); + + expect(period.isMoney, isFalse); + }); + }); +} + +void _dateRangeTests() { + group('goalDateRange', () { + test('prints a period as two short dates', () { + expect(goalDateRange('2026-05-01', '2026-05-31'), '1 May - 31 May'); + }); + + test('hands back a malformed value rather than inventing a date', () { + expect( + goalDateRange('not-a-date', '2026-05-31'), + 'not-a-date to 2026-05-31', + ); + }); + }); +} + +void _filterTests() { + group('goalListQuery', () { + test('asks for everything when nothing is filtered', () { + final query = goalListQuery(const GoalFilters()); + expect(query, contains('limit=1000')); + expect(query, isNot(contains('search='))); + expect(query, isNot(contains('period_type='))); + }); + + test('sends the search text and the period', () { + final query = goalListQuery( + const GoalFilters(query: 'emea', periodType: 'QUARTERLY'), + ); + expect(query, contains('search=emea')); + expect(query, contains('period_type=QUARTERLY')); + }); + + test('encodes search text rather than pasting it into the URL', () { + final query = goalListQuery(const GoalFilters(query: 'a&b c')); + expect(query, isNot(contains('a&b c'))); + expect(query, contains('search=a%26b+c')); + }); + + test('maps the window onto the flag the API actually takes', () { + expect( + goalListQuery(const GoalFilters(window: 'current')), + contains('current=true'), + ); + expect( + goalListQuery(const GoalFilters(window: 'active')), + contains('active=true'), + ); + }); + + test('drops a period the backend does not accept', () { + // The value reaches this from a picker today, but a filter that silently + // forwarded anything would turn a future typo into an empty list with no + // explanation. + expect( + goalListQuery(const GoalFilters(periodType: 'FORTNIGHTLY')), + isNot(contains('period_type')), + ); + }); + + test('knows when a filter is on, so the empty state can say so', () { + expect(const GoalFilters().isFiltered, isFalse); + expect(const GoalFilters(query: 'x').isFiltered, isTrue); + expect(const GoalFilters(window: 'current').isFiltered, isTrue); + expect(const GoalFilters(periodType: 'MONTHLY').isFiltered, isTrue); + }); + }); }