From 9ea9fcd3ffd371bcfaccbe9d0c63fe1f0e5efaa3 Mon Sep 17 00:00:00 2001
From: Jumpy Squirrel <jsquirrel_github_9a6d@packetloss.de>
Date: Fri, 6 Dec 2024 13:17:36 +0100
Subject: [PATCH 1/6] feat(#239): api spec changes for multiple package
 occurence

---
 api/openapi-spec/openapi.yaml | 94 +++++++++++++++++++++++++----------
 1 file changed, 67 insertions(+), 27 deletions(-)

diff --git a/api/openapi-spec/openapi.yaml b/api/openapi-spec/openapi.yaml
index d4c943e..91a2c2c 100644
--- a/api/openapi-spec/openapi.yaml
+++ b/api/openapi-spec/openapi.yaml
@@ -1704,7 +1704,6 @@ components:
         - email
         - phone
         - birthday
-        - packages
       properties:
         id:
           type: integer
@@ -1818,8 +1817,26 @@ components:
           example: art,anim,music,suit
         packages:
           type: string
-          description: A comma separated list of packages as declared in configuration. Packages are the things that cost money, like being a supersponsor or a day guest for a certain day. They can be configured with respect to who may add / remove them, if they are on by default, and whether they are visible if not selected (admin only, normal user, completely disabled). There is also configuration as to which packages are mutually exclusive, such as sponsor and supersponsor.
-          example: room-none,attendance,sponsor
+          description: |-
+            A comma separated list of packages as declared in configuration.
+            
+            Packages are the things that cost money, like being a supersponsor or a day guest for a certain day. They can be
+            configured with respect to who may add / remove them, if they are on by default, and whether they are visible if
+            not selected (admin only, normal user, completely disabled). There is also configuration as to which packages are
+            mutually exclusive, such as sponsor and supersponsor.
+            
+            If configured with a max_count greater than 1, certain packages may occur multiple times in this list.
+            
+            Any order of packages is accepted in requests, but in responses the list of packages will always be sorted
+            in alphabetical order.
+            
+            Note: If packages_list is also specified in a request, it takes precedence over this field. In responses,
+            both fields will always be present and filled.
+            
+            DEPRECATED - see packages_list
+          example: attendance,room-none,sponsor
+        packages_list:
+          $ref: '#/components/schemas/PackagesList'
         user_comments:
           type: string
           description: Optional comments the attendee wishes to make regarding their registration. Not processed in any way.
@@ -1981,32 +1998,20 @@ components:
             - suit
         packages:
           type: string
-          description: DEPRECATED - use packages_list for new implementations. A comma separated list of packages as declared in configuration. Packages are the things that cost money, like being a supersponsor or a day guest for a certain day. They can be configured with respect to who may add / remove them, if they are on by default, and whether they are visible if not selected (admin only, normal user, completely disabled). There is also configuration as to which packages are mutually exclusive, such as sponsor and supersponsor.
+          description: |-
+            DEPRECATED - use packages_list for new implementations.
+            
+            A comma separated list of packages as declared in configuration, sorted alphabetically by package name.
+            
+            Packages are the things that cost money, like being a supersponsor or a day guest for a certain day. They can be
+            configured with respect to who may add / remove them, if they are on by default, and whether they are visible if
+            not selected (admin only, normal user, completely disabled). There is also configuration as to which packages are
+            mutually exclusive, such as sponsor and supersponsor.
+
+            If configured with a max_count greater than 1, certain packages may occur multiple times in this list.
           example: room-none,attendance,sponsor
         packages_list:
-          type: array
-          items:
-            type: object
-            required:
-              - name
-              - count
-            properties:
-              name:
-                type: string
-                example: attendance
-                description: the code for the package
-              count:
-                type: integer
-                example: 1
-                description: the number of times the package was purchased (at the moment, only the value 1 is supported, but this may change in the future).
-          description: A sorted list of packages as declared in configuration. Packages are the things that cost money, like being a supersponsor or a day guest for a certain day. They can be configured with respect to who may add / remove them, if they are on by default, and whether they are visible if not selected (admin only, normal user, completely disabled). There is also configuration as to which packages are mutually exclusive, such as sponsor and supersponsor.
-          example:
-            - name: attendance
-              count: 1
-            - name: room-none
-              count: 1
-            - name: sponsor
-              count: 1
+          $ref: '#/components/schemas/PackagesList'
         user_comments:
           type: string
           description: Optional comments the attendee wishes to make regarding their registration. Not processed in any way.
@@ -2305,6 +2310,41 @@ components:
           - configuration
           - balances
           - all
+    PackagesList:
+      type: array
+      items:
+        type: object
+        required:
+          - name
+          - count
+        properties:
+          name:
+            type: string
+            example: attendance
+            description: the code for the package
+          count:
+            type: integer
+            example: 1
+            description: the number of times the package was purchased (at the moment, only the value 1 is supported, but this may change in the future).
+      description: |-
+        A sorted list of packages as declared in configuration. 
+        
+        Packages are the things that cost money, like being a supersponsor or a day guest for a certain day. They can be
+        configured with respect to who may add / remove them, if they are on by default, and whether they are visible if
+        not selected (admin only, normal user, completely disabled). There is also configuration as to which packages are mutually exclusive, such as sponsor and supersponsor.
+        
+        It is an error to specify a package multiple times in the list. Requests need not sort the package list by name,
+        but responses sent by this service always have the package list sorted by package name.
+        
+        The count field can be left unspecified when sending this data structure, that means a count of 1, but it will always be present
+        and filled in responses sent by this service.
+      example:
+        - name: attendance
+          count: 1
+        - name: room-none
+          count: 1
+        - name: sponsor
+          count: 1
     AdminInfo:
       type: object
       required:

From 787cf35328beb5a268dc65af8d4cc46a4c37a7b1 Mon Sep 17 00:00:00 2001
From: Jumpy Squirrel <jsquirrel_github_9a6d@packetloss.de>
Date: Fri, 6 Dec 2024 17:58:38 +0100
Subject: [PATCH 2/6] feat(#239): implement mapping, calculation and
 validation, keeping tests on old package field

---
 internal/api/v1/attendee/api.go               |  9 +--
 internal/repository/config/structure.go       |  1 +
 internal/repository/config/validation.go      |  6 ++
 internal/service/attendeesrv/attendeesrv.go   | 56 ++++++++++------
 internal/service/attendeesrv/dues.go          |  8 +--
 .../web/controller/attendeectl/mapping.go     | 67 ++++++++++++++++++-
 .../web/controller/attendeectl/validation.go  | 50 +++++++++++++-
 .../controller/attendeectl/validation_test.go | 24 ++++++-
 test/acceptance/admin_acc_test.go             |  6 +-
 test/acceptance/attendee_acc_test.go          | 56 ++++++++++++----
 test/acceptance/utils_test.go                 |  2 +-
 11 files changed, 234 insertions(+), 51 deletions(-)

diff --git a/internal/api/v1/attendee/api.go b/internal/api/v1/attendee/api.go
index 1f294f3..3d8ae83 100644
--- a/internal/api/v1/attendee/api.go
+++ b/internal/api/v1/attendee/api.go
@@ -30,9 +30,10 @@ type AttendeeDto struct {
 	RegistrationLanguage string `json:"registration_language"` // one out of configurable subset of RFC 5646 locales (default en-US)
 
 	// comma separated lists, allowed choices are convention dependent
-	Flags    string `json:"flags"`    // hc,anon,ev
-	Packages string `json:"packages"` // room-none,attendance,stage,sponsor,sponsor2
-	Options  string `json:"options"`  // art,anim,music,suit
+	Flags        string         `json:"flags"`    // hc,anon,ev
+	Packages     string         `json:"packages"` // room-none,attendance,stage,sponsor,sponsor2
+	PackagesList []PackageState `json:"packages_list"`
+	Options      string         `json:"options"` // art,anim,music,suit
 
 	// comments
 	UserComments string `json:"user_comments"`
@@ -137,5 +138,5 @@ type ChoiceState struct {
 
 type PackageState struct {
 	Name  string `json:"name"`
-	Count int    `json:"count"`
+	Count int    `json:"count"` // defaults to 1 if unset in requests
 }
diff --git a/internal/repository/config/structure.go b/internal/repository/config/structure.go
index bb43615..9a34f9b 100644
--- a/internal/repository/config/structure.go
+++ b/internal/repository/config/structure.go
@@ -133,6 +133,7 @@ type (
 		VisibleFor    []string `yaml:"visible_for"`            // list of permissions which allow seeing the flag/option/package. Admin can always see everything, "self" can always see non-admin_only, but you can add it for admin_only fields. This field also controls who else can see the info based on their permissions admin field. Example: "self,sponsordesk"
 		Group         string   `yaml:"group"`                  // set if attendee has this group during initial registration
 		Mandatory     bool     `yaml:"at-least-one-mandatory"` // one of these MUST be chosen (no constraint if not set on any choices)
+		MaxCount      int      `yaml:"max_count"`              // only supported for packages, 0 means 1 so can be left out of config
 		Constraint    string   `yaml:"constraint"`
 		ConstraintMsg string   `yaml:"constraint_msg"`
 	}
diff --git a/internal/repository/config/validation.go b/internal/repository/config/validation.go
index 1f1cccf..762f316 100644
--- a/internal/repository/config/validation.go
+++ b/internal/repository/config/validation.go
@@ -45,6 +45,12 @@ func setConfigurationDefaults(c *Application) {
 	if c.Security.Cors.AllowOrigin == "" {
 		c.Security.Cors.AllowOrigin = "*"
 	}
+	for name, pkgConf := range c.Choices.Packages {
+		if pkgConf.MaxCount == 0 {
+			pkgConf.MaxCount = 1
+			c.Choices.Packages[name] = pkgConf
+		}
+	}
 	if len(c.SpokenLanguages) == 0 {
 		c.SpokenLanguages = []string{"en-US"}
 	}
diff --git a/internal/service/attendeesrv/attendeesrv.go b/internal/service/attendeesrv/attendeesrv.go
index d2bfa02..a0d1e91 100644
--- a/internal/service/attendeesrv/attendeesrv.go
+++ b/internal/service/attendeesrv/attendeesrv.go
@@ -164,7 +164,7 @@ func (s *AttendeeServiceImplData) CanChangeChoiceToCurrentStatus(ctx context.Con
 		if v.Mandatory {
 			oneIsMandatory = true
 			mandatoryList = append(mandatoryList, k)
-			if newChoices[k] {
+			if newChoices[k] > 0 {
 				satisfiesOneIsMandatory = true
 			}
 		}
@@ -221,11 +221,11 @@ func userAlreadyHasAnotherRegistration(ctx context.Context, identity string, exp
 	return count != expectedCount, nil
 }
 
-func checkNoForbiddenChanges(ctx context.Context, what string, key string, choiceConfig config.ChoiceConfig, originalChoices map[string]bool, newChoices map[string]bool) error {
+func checkNoForbiddenChanges(ctx context.Context, what string, key string, choiceConfig config.ChoiceConfig, originalChoices map[string]int, newChoices map[string]int) error {
 	if originalChoices[key] != newChoices[key] {
 		// tolerate removing a read-only choice that has a constraint that forbids it anyway
 		if choiceConfig.ReadOnly {
-			if originalChoices[key] && !newChoices[key] {
+			if originalChoices[key] > 0 && newChoices[key] == 0 {
 				if canAllowRemovalDueToConstraint(ctx, what, key, choiceConfig, originalChoices, newChoices) {
 					return nil
 				}
@@ -240,14 +240,14 @@ func checkNoForbiddenChanges(ctx context.Context, what string, key string, choic
 	return nil
 }
 
-func canAllowRemovalDueToConstraint(ctx context.Context, what string, key string, choiceConfig config.ChoiceConfig, originalChoices map[string]bool, newChoices map[string]bool) bool {
+func canAllowRemovalDueToConstraint(ctx context.Context, what string, key string, choiceConfig config.ChoiceConfig, originalChoices map[string]int, newChoices map[string]int) bool {
 	if choiceConfig.Constraint != "" {
 		constraints := strings.Split(choiceConfig.Constraint, ",")
 		for _, cn := range constraints {
 			constraintK := cn
 			if strings.HasPrefix(cn, "!") {
 				constraintK = strings.TrimPrefix(cn, "!")
-				if newChoices[constraintK] {
+				if newChoices[constraintK] > 0 {
 					aulogging.Logger.Ctx(ctx).Info().Printf("can allow removal of read only %s %s - it would violate a constraint for %s anyway", what, key, constraintK)
 					return true
 				}
@@ -257,13 +257,13 @@ func canAllowRemovalDueToConstraint(ctx context.Context, what string, key string
 	return false
 }
 
-func checkNoForbiddenChangesAfterPayment(ctx context.Context, what string, key string, choiceConfig config.ChoiceConfig, configuration map[string]config.ChoiceConfig, originalChoices map[string]bool, newChoices map[string]bool, currentStatus status.Status) error {
+func checkNoForbiddenChangesAfterPayment(ctx context.Context, what string, key string, choiceConfig config.ChoiceConfig, configuration map[string]config.ChoiceConfig, originalChoices map[string]int, newChoices map[string]int, currentStatus status.Status) error {
 	if ctxvalues.HasApiToken(ctx) || ctxvalues.IsAuthorizedAsGroup(ctx, config.OidcAdminGroup()) {
 		return nil
 	}
 
 	if currentStatus == status.PartiallyPaid || currentStatus == status.Paid || currentStatus == status.CheckedIn {
-		if originalChoices[key] && !newChoices[key] && choiceConfig.Price > 0 {
+		if originalChoices[key] > 0 && newChoices[key] == 0 && choiceConfig.Price > 0 {
 			oldDues := calcTotalDuesHelper(configuration, originalChoices)
 			newDues := calcTotalDuesHelper(configuration, newChoices)
 
@@ -276,28 +276,28 @@ func checkNoForbiddenChangesAfterPayment(ctx context.Context, what string, key s
 	return nil
 }
 
-func calcTotalDuesHelper(configuration map[string]config.ChoiceConfig, choices map[string]bool) (dues int64) {
-	for k, selected := range choices {
+func calcTotalDuesHelper(configuration map[string]config.ChoiceConfig, choices map[string]int) (dues int64) {
+	for k, count := range choices {
 		choiceConfig, ok := configuration[k]
-		if ok && selected {
-			dues += choiceConfig.Price
+		if ok && count > 0 {
+			dues += choiceConfig.Price * int64(count)
 		}
 	}
 	return dues
 }
 
-func checkNoConstraintViolation(key string, choiceConfig config.ChoiceConfig, newChoices map[string]bool) error {
+func checkNoConstraintViolation(key string, choiceConfig config.ChoiceConfig, newChoices map[string]int) error {
 	if choiceConfig.Constraint != "" {
 		constraints := strings.Split(choiceConfig.Constraint, ",")
 		for _, cn := range constraints {
 			constraintK := cn
 			if strings.HasPrefix(cn, "!") {
 				constraintK = strings.TrimPrefix(cn, "!")
-				if newChoices[key] && newChoices[constraintK] {
+				if newChoices[key] > 0 && newChoices[constraintK] > 0 {
 					return errors.New("cannot pick both " + key + " and " + constraintK + " - constraint violated")
 				}
 			} else {
-				if newChoices[key] && !newChoices[constraintK] {
+				if newChoices[key] > 0 && newChoices[constraintK] == 0 {
 					return errors.New("when picking " + key + ", must also pick " + constraintK + " - constraint violated")
 				}
 			}
@@ -306,31 +306,45 @@ func checkNoConstraintViolation(key string, choiceConfig config.ChoiceConfig, ne
 	return nil
 }
 
-func choiceStrToMap(choiceStr string, configuration map[string]config.ChoiceConfig) map[string]bool {
-	result := make(map[string]bool)
+// choiceStrToMap converts a choice representation in the entity to a map of counts
+//
+// Can be used for packages, flags, options.
+func choiceStrToMap(choiceStr string, configuration map[string]config.ChoiceConfig) map[string]int {
+	result := make(map[string]int)
 	// ensure all available keys present
 	for k, _ := range configuration {
-		result[k] = false
+		result[k] = 0
 	}
 	if choiceStr != "" {
 		choices := strings.Split(choiceStr, ",")
 		for _, pickedKey := range choices {
 			if pickedKey != "" {
-				result[pickedKey] = true
+				currentValue, present := result[pickedKey]
+				if present {
+					result[pickedKey] = currentValue + 1
+				} else {
+					aulogging.Logger.NoCtx().Warn().Printf("encountered non-configured choice key %s - maybe configuration changed after initial reg? This needs fixing! - continuing", pickedKey)
+					result[pickedKey] = 1
+				}
 			}
 		}
 	}
 	return result
 }
 
-func commaSeparatedStrToMap(choiceStr string, allowedValues []string) map[string]bool {
+// commaSeparatedStrToMap converts a comma separated string representation in the entity to a map of booleans
+//
+// Can be used for permissions, languages, etc.
+//
+// IMPORTANT: do not use for choices (packages, flags, options), use choiceStrToMap instead to achieve better validation
+func commaSeparatedStrToMap(commaSeparatedStr string, allowedValues []string) map[string]bool {
 	result := make(map[string]bool)
 	// ensure all available values present
 	for _, k := range allowedValues {
 		result[k] = false
 	}
-	if choiceStr != "" {
-		choices := strings.Split(choiceStr, ",")
+	if commaSeparatedStr != "" {
+		choices := strings.Split(commaSeparatedStr, ",")
 		for _, pickedKey := range choices {
 			if pickedKey != "" {
 				result[pickedKey] = true
diff --git a/internal/service/attendeesrv/dues.go b/internal/service/attendeesrv/dues.go
index eefadb1..2c22ee5 100644
--- a/internal/service/attendeesrv/dues.go
+++ b/internal/service/attendeesrv/dues.go
@@ -158,8 +158,8 @@ func (s *AttendeeServiceImplData) packageDuesByVAT(ctx context.Context, attendee
 	}
 
 	packageConfigs := config.PackagesConfig()
-	for key, selected := range choiceStrToMap(attendee.Packages, packageConfigs) {
-		if selected {
+	for key, count := range choiceStrToMap(attendee.Packages, packageConfigs) {
+		if count > 0 {
 			packageConfig, ok := packageConfigs[key]
 			if !ok {
 				aulogging.Logger.Ctx(ctx).Warn().Printf("attendee id %d has unknown package %s in db - ignoring during dues calculation", attendee.ID, key)
@@ -167,7 +167,7 @@ func (s *AttendeeServiceImplData) packageDuesByVAT(ctx context.Context, attendee
 				vatStr := fmt.Sprintf("%.6f", packageConfig.VatPercent)
 
 				previous, _ := result[vatStr]
-				result[vatStr] = previous + packageConfig.Price
+				result[vatStr] = previous + packageConfig.Price*int64(count)
 			}
 		}
 	}
@@ -180,7 +180,7 @@ func (s *AttendeeServiceImplData) considerGuest(ctx context.Context, adminInfo *
 	if !ok {
 		aulogging.Logger.Ctx(ctx).Warn().Print("admin only flag 'guest' not configured, skipping")
 	}
-	return isGuest
+	return isGuest > 0
 }
 
 func (s *AttendeeServiceImplData) oldDuesByVAT(transactionHistory []paymentservice.Transaction) map[string]int64 {
diff --git a/internal/web/controller/attendeectl/mapping.go b/internal/web/controller/attendeectl/mapping.go
index fcdb324..5fcd3de 100644
--- a/internal/web/controller/attendeectl/mapping.go
+++ b/internal/web/controller/attendeectl/mapping.go
@@ -4,6 +4,7 @@ import (
 	"github.com/eurofurence/reg-attendee-service/internal/api/v1/attendee"
 	"github.com/eurofurence/reg-attendee-service/internal/entity"
 	"github.com/eurofurence/reg-attendee-service/internal/repository/config"
+	"sort"
 	"strings"
 )
 
@@ -36,7 +37,7 @@ func mapDtoToAttendee(dto *attendee.AttendeeDto, a *entity.Attendee) {
 		a.RegistrationLanguage = addWrappingCommas(config.DefaultRegistrationLanguage())
 	}
 	a.Flags = addWrappingCommas(dto.Flags)
-	a.Packages = addWrappingCommas(dto.Packages)
+	a.Packages = packagesFromDto(dto.Packages, dto.PackagesList)
 	a.Options = addWrappingCommas(dto.Options)
 	a.UserComments = dto.UserComments
 }
@@ -63,7 +64,8 @@ func mapAttendeeToDto(a *entity.Attendee, dto *attendee.AttendeeDto) {
 	dto.SpokenLanguages = removeWrappingCommas(a.SpokenLanguages)
 	dto.RegistrationLanguage = removeWrappingCommas(a.RegistrationLanguage)
 	dto.Flags = removeWrappingCommas(a.Flags)
-	dto.Packages = removeWrappingCommas(a.Packages)
+	dto.Packages = packagesFromEntity(a.Packages)
+	dto.PackagesList = packagesListFromEntity(a.Packages)
 	dto.Options = removeWrappingCommas(a.Options)
 	dto.UserComments = a.UserComments
 }
@@ -97,3 +99,64 @@ func sliceContains(slice []string, singleValue string) bool {
 	}
 	return false
 }
+
+func packagesFromDto(commaSeparated string, asList []attendee.PackageState) string {
+	// only use commaSeparated if no list is supplied
+	if len(asList) == 0 {
+		asList = packageListFromCommaSeparated(commaSeparated)
+	}
+
+	var result strings.Builder
+	result.WriteString(",")
+	for _, item := range asList {
+		// item.Count = 0 should be interpreted as 1 (allows omitting Count in requests)
+		for i := 0; i == 0 || i < item.Count; i++ {
+			result.WriteString(item.Name + ",")
+		}
+	}
+	return result.String()
+}
+
+func packagesFromEntity(entityPackages string) string {
+	return removeWrappingCommas(entityPackages)
+}
+
+func packagesListFromEntity(entityPackages string) []attendee.PackageState {
+	unwrapped := removeWrappingCommas(entityPackages)
+	asList := packageListFromCommaSeparated(unwrapped)
+	return asList
+}
+
+// packageListFromCommaSeparated takes a comma separated list, without leading and trailing commas, and converts
+// it into a sorted package_list
+func packageListFromCommaSeparated(commaSeparatedValue string) []attendee.PackageState {
+	result := make([]attendee.PackageState, 0)
+
+	if commaSeparatedValue == "" {
+		return result
+	}
+
+	counts := make(map[string]int)
+	chosenValues := strings.Split(commaSeparatedValue, ",")
+	for _, v := range chosenValues {
+		currentCount, ok := counts[v]
+		if !ok {
+			counts[v] = 1
+		} else {
+			counts[v] = currentCount + 1
+		}
+	}
+
+	for name, count := range counts {
+		result = append(result, attendee.PackageState{
+			Name:  name,
+			Count: count,
+		})
+	}
+
+	sort.Slice(result, func(i, j int) bool {
+		return result[i].Name < result[j].Name
+	})
+
+	return result
+}
diff --git a/internal/web/controller/attendeectl/validation.go b/internal/web/controller/attendeectl/validation.go
index 552629a..b6c5828 100644
--- a/internal/web/controller/attendeectl/validation.go
+++ b/internal/web/controller/attendeectl/validation.go
@@ -2,11 +2,13 @@ package attendeectl
 
 import (
 	"context"
+	"fmt"
 	aulogging "github.com/StephanHCB/go-autumn-logging"
 	"github.com/eurofurence/reg-attendee-service/internal/api/v1/attendee"
 	"github.com/eurofurence/reg-attendee-service/internal/api/v1/status"
 	"github.com/eurofurence/reg-attendee-service/internal/web/util/validation"
 	"net/url"
+	"sort"
 	"strings"
 	"unicode"
 
@@ -94,7 +96,8 @@ func validate(ctx context.Context, a *attendee.AttendeeDto, trustedOriginalState
 		errs.Add("registration_language", "registration_language field must be one of "+strings.Join(config.AllowedRegistrationLanguages(), ",")+" or it can be left blank, which counts as "+config.DefaultRegistrationLanguage())
 	}
 	validation.CheckCombinationOfAllowedValues(&errs, config.AllowedFlagsNoAdmin(), "flags", a.Flags)
-	validation.CheckCombinationOfAllowedValues(&errs, config.AllowedPackages(), "packages", a.Packages)
+	checkPackagesValid(&errs, config.PackagesConfig(), a.Packages)
+	checkPackagesListValid(&errs, config.PackagesConfig(), a.PackagesList)
 	validation.CheckCombinationOfAllowedValues(&errs, config.AllowedOptions(), "options", a.Options)
 	if a.TshirtSize != "" && validation.NotInAllowedValues(config.AllowedTshirtSizes(), a.TshirtSize) {
 		errs.Add("tshirt_size", "optional tshirt_size field must be empty or one of "+strings.Join(config.AllowedTshirtSizes(), ","))
@@ -146,3 +149,48 @@ func validateDueDateChange(ctx context.Context, d *attendee.DueDate, trustedOrig
 	}
 	return errs
 }
+
+func checkPackagesList(errs *url.Values, cfg map[string]config.ChoiceConfig, key string, pkgList []attendee.PackageState) {
+	namesOk := true
+
+	for _, v := range pkgList {
+		c, ok := cfg[v.Name]
+		if !ok {
+			namesOk = false
+		} else {
+			if v.Count > c.MaxCount {
+				errs.Add(key, fmt.Sprintf("package %s occurs too many times, can occur at most %d times", v.Name, c.MaxCount))
+			}
+		}
+	}
+
+	if !namesOk {
+		allowedCommaSeparated := strings.Join(sortedKeys(cfg), ",")
+		if key == "packages" {
+			// different error message for packages field
+			errs.Add(key, fmt.Sprintf("%s field must be a comma separated combination of any of %s", key, allowedCommaSeparated))
+		} else {
+			errs.Add(key, fmt.Sprintf("%s can only contain package names %s", key, allowedCommaSeparated))
+		}
+	}
+}
+
+func checkPackagesListValid(errs *url.Values, cfg map[string]config.ChoiceConfig, pkgList []attendee.PackageState) {
+	checkPackagesList(errs, cfg, "packages_list", pkgList)
+}
+
+func checkPackagesValid(errs *url.Values, cfg map[string]config.ChoiceConfig, commaSeparatedValue string) {
+	asList := packageListFromCommaSeparated(commaSeparatedValue)
+	checkPackagesList(errs, cfg, "packages", asList)
+}
+
+func sortedKeys(choiceMap map[string]config.ChoiceConfig) []string {
+	keys := make([]string, len(choiceMap))
+	i := 0
+	for k := range choiceMap {
+		keys[i] = k
+		i++
+	}
+	sort.Strings(keys)
+	return keys
+}
diff --git a/internal/web/controller/attendeectl/validation_test.go b/internal/web/controller/attendeectl/validation_test.go
index c9cdea1..1f4eb4a 100644
--- a/internal/web/controller/attendeectl/validation_test.go
+++ b/internal/web/controller/attendeectl/validation_test.go
@@ -32,9 +32,27 @@ func tstCreateValidAttendee() attendee.AttendeeDto {
 		SpokenLanguages:      "de,en",
 		RegistrationLanguage: "en-US",
 		Flags:                "anon,ev",
-		Packages:             "room-none,attendance,stage,sponsor2",
-		Options:              "music,suit",
-		TshirtSize:           "XXL",
+		Packages:             "attendance,room-none,sponsor2,stage", // must be sorted for tests to work
+		PackagesList: []attendee.PackageState{
+			{
+				Name:  "attendance",
+				Count: 1,
+			},
+			{
+				Name:  "room-none",
+				Count: 1,
+			},
+			{
+				Name:  "sponsor2",
+				Count: 1,
+			},
+			{
+				Name:  "stage",
+				Count: 1,
+			},
+		},
+		Options:    "music,suit",
+		TshirtSize: "XXL",
 	}
 }
 
diff --git a/test/acceptance/admin_acc_test.go b/test/acceptance/admin_acc_test.go
index d24a69c..706d6bc 100644
--- a/test/acceptance/admin_acc_test.go
+++ b/test/acceptance/admin_acc_test.go
@@ -1099,7 +1099,7 @@ func TestSearch_RegdeskOk(t *testing.T) {
       "flags_list": ["anon","hc","terms-accepted"],
       "options": "music,suit",
       "options_list": ["music","suit"],
-      "packages": "room-none,attendance,stage,sponsor2",
+      "packages": "attendance,room-none,sponsor2,stage",
       "packages_list": [
         {
           "name": "attendance",
@@ -1172,7 +1172,7 @@ func TestSearch_SponsordeskOk(t *testing.T) {
       "flags_list": ["anon","hc","terms-accepted"],
       "options": "music,suit",
       "options_list": ["music","suit"],
-      "packages": "room-none,attendance,stage,sponsor2",
+      "packages": "attendance,room-none,sponsor2,stage",
       "packages_list": [
         {
           "name": "attendance",
@@ -1374,7 +1374,7 @@ func TestSearch_AdminOk(t *testing.T) {
       "flags_list": ["anon","hc","terms-accepted"],
       "options": "music,suit",
       "options_list": ["music","suit"],
-      "packages": "room-none,attendance,stage,sponsor2",
+      "packages": "attendance,room-none,sponsor2,stage",
       "packages_list": [
         {
           "name": "attendance",
diff --git a/test/acceptance/attendee_acc_test.go b/test/acceptance/attendee_acc_test.go
index 3e5a1d7..ac1305d 100644
--- a/test/acceptance/attendee_acc_test.go
+++ b/test/acceptance/attendee_acc_test.go
@@ -367,6 +367,8 @@ func TestCreateNewAttendee_NoLoginRequired_After_Anon(t *testing.T) {
 	attendeeReadAgain := tstReadAttendee(t, response.location)
 	// difference in id is ok, so copy it over to expected
 	attendeeSent.Id = attendeeReadAgain.Id
+	// TODO #239 for now we test for the classic package field - remove and send list instead later
+	attendeeReadAgain.PackagesList = nil
 	require.EqualValues(t, attendeeSent, attendeeReadAgain, "attendee data read did not match sent data")
 }
 
@@ -392,6 +394,8 @@ func TestCreateNewAttendee_NoLoginRequired_After_User(t *testing.T) {
 	tstParseJson(readAgainResponse.body, &attendeeReadAgain)
 	// difference in id is ok, so copy it over to expected
 	attendeeSent.Id = attendeeReadAgain.Id
+	// TODO #239 for now we test for the classic package field - remove and send list instead later
+	attendeeReadAgain.PackagesList = nil
 	require.EqualValues(t, attendeeSent, attendeeReadAgain, "attendee data read did not match sent data")
 }
 
@@ -741,6 +745,8 @@ func TestCreateNewAttendee_AutomaticGroupFlag(t *testing.T) {
 	attendeeSent.Id = attendeeReadAgain.Id
 	// we expect the 'ev' flag added
 	attendeeSent.Flags += ",ev"
+	// TODO #239 for now we test for the classic package field - remove and send list instead later
+	attendeeReadAgain.PackagesList = nil
 	require.EqualValues(t, attendeeSent, attendeeReadAgain, "attendee data read did not match expected data")
 }
 
@@ -1181,22 +1187,28 @@ func tstUpdateExistingAttendee_RemovePackage_NoCostReduce_Allowed(t *testing.T,
 	loc, att := tstRegisterAttendeeAndTransitionToStatus(t, testcase, targetStatus)
 	// update to prepare test case
 	changedAttendee := att
-	changedAttendee.Packages = "room-none,stage,attendance,sponsor2,boat-trip" // adds boat-trip
+	changedAttendee.Packages = "attendance,boat-trip,room-none,sponsor2,stage" // adds boat-trip
+	// TODO #239 for now we test for the classic package field - remove and send list instead later
+	changedAttendee.PackagesList = nil
 	response := tstPerformPut(loc, tstRenderJson(changedAttendee), token)
 	require.Equal(t, http.StatusOK, response.status, "unexpected http response status for update")
 	require.Equal(t, loc, response.location, "location unexpectedly changed during update")
 
 	docs.When("when they send updated attendee info and change packages in a way that does not reduce total dues")
 	changedAttendee2 := att
-	changedAttendee2.Packages = "room-none,stage,attendance,sponsor2,mountain-trip" // removes boat-trip, but adds mountain-trip which is more expensive
+	changedAttendee2.Packages = "attendance,mountain-trip,room-none,sponsor2,stage" // removes boat-trip, but adds mountain-trip which is more expensive
+	// TODO #239 for now we test for the classic package field - remove and send list instead later
+	changedAttendee2.PackagesList = nil
 	response2 := tstPerformPut(loc, tstRenderJson(changedAttendee2), token)
 
 	docs.Then("then the attendee is successfully updated and the changed data can be read again")
 	require.Equal(t, http.StatusOK, response2.status, "unexpected http response status for update")
 	require.Equal(t, loc, response2.location, "location unexpectedly changed during update")
 	attendeeReadAgain := tstReadAttendee(t, loc)
+	// TODO #239 for now we test for the classic package field - remove and check list instead later
+	attendeeReadAgain.PackagesList = nil
 	require.EqualValues(t, changedAttendee2, attendeeReadAgain, "attendee data read did not match updated data")
-	require.EqualValues(t, "room-none,stage,attendance,sponsor2,mountain-trip", attendeeReadAgain.Packages, "attendee data read did not match expected package value")
+	require.EqualValues(t, "attendance,mountain-trip,room-none,sponsor2,stage", attendeeReadAgain.Packages, "attendee data read did not match expected package value")
 }
 
 func tstUpdateExistingAttendee_RemovePackage_Allowed(t *testing.T, testcase string, who string, targetStatus status.Status, token string) {
@@ -1209,15 +1221,19 @@ func tstUpdateExistingAttendee_RemovePackage_Allowed(t *testing.T, testcase stri
 
 	docs.When("when they send updated attendee info and remove a package that has associated cost")
 	changedAttendee := att
-	changedAttendee.Packages = "room-none,attendance,stage" // removes sponsor2
+	changedAttendee.Packages = "attendance,room-none,stage" // removes sponsor2
+	// TODO #239 for now we test for the classic package field - remove and send changed list instead later
+	changedAttendee.PackagesList = nil
 	response := tstPerformPut(loc, tstRenderJson(changedAttendee), token)
 
 	docs.Then("then the attendee is successfully updated and the changed data can be read again")
 	require.Equal(t, http.StatusOK, response.status, "unexpected http response status for update")
 	require.Equal(t, loc, response.location, "location unexpectedly changed during update")
 	attendeeReadAgain := tstReadAttendee(t, loc)
+	// TODO #239 for now we test for the classic package field - remove and check changed list instead later
+	attendeeReadAgain.PackagesList = nil
 	require.EqualValues(t, changedAttendee, attendeeReadAgain, "attendee data read did not match updated data")
-	require.EqualValues(t, "room-none,attendance,stage", attendeeReadAgain.Packages, "attendee data read did not match expected package value")
+	require.EqualValues(t, "attendance,room-none,stage", attendeeReadAgain.Packages, "attendee data read did not match expected package value")
 }
 
 func TestUpdateExistingAttendee_RemovePackageWithCostReduction_AfterPaid_UserForbidden(t *testing.T) {
@@ -1270,15 +1286,21 @@ func tstUpdateExistingAttendee_AddOnePackage(t *testing.T, testcase string, orig
 
 	docs.When("when they send updated attendee info and add a package that has associated cost")
 	changedAttendee := att
-	changedAttendee.Packages = "room-none,attendance,stage,sponsor2,boat-trip" // adds boat-trip
+	changedAttendee.Packages = "room-none,attendance,stage,sponsor2,boat-trip" // adds boat-trip (and tests automatic reordering)
+	// TODO #239 for now we test for the classic package field - remove and send list instead later
+	changedAttendee.PackagesList = nil
 	response := tstPerformPut(loc, tstRenderJson(changedAttendee), token)
 
 	docs.Then("then the attendee is successfully updated and the changed data can be read again")
 	require.Equal(t, http.StatusOK, response.status, "unexpected http response status for update")
 	require.Equal(t, loc, response.location, "location unexpectedly changed during update")
 	attendeeReadAgain := tstReadAttendee(t, loc)
+	// TODO #239 for now we test for the classic package field - remove and check list instead later
+	attendeeReadAgain.PackagesList = nil
+	// test automatic reordering of packages to alphabetical order
+	changedAttendee.Packages = "attendance,boat-trip,room-none,sponsor2,stage"
 	require.EqualValues(t, changedAttendee, attendeeReadAgain, "attendee data read did not match updated data")
-	require.EqualValues(t, "room-none,attendance,stage,sponsor2,boat-trip", attendeeReadAgain.Packages, "attendee data read did not match expected package value")
+	require.EqualValues(t, "attendance,boat-trip,room-none,sponsor2,stage", attendeeReadAgain.Packages, "attendee data read did not match expected package value")
 
 	docs.Then("and the expected mail messages have been sent")
 	tstRequireMailRequests(t, expectedMails)
@@ -1318,20 +1340,26 @@ func tstUpdateExistingAttendee_AddTwoPackages(t *testing.T, testcase string, ori
 
 	docs.When("when they send updated attendee info and add a package that has associated cost")
 	changedAttendee := att
-	changedAttendee.Packages = "room-none,attendance,stage,sponsor2,boat-trip" // adds boat-trip
+	changedAttendee.Packages = "attendance,boat-trip,room-none,sponsor2,stage" // adds boat-trip
+	// TODO #239 for now we test for the classic package field - remove and send list instead later
+	changedAttendee.PackagesList = nil
 	response := tstPerformPut(loc, tstRenderJson(changedAttendee), token)
 
 	docs.When("and then send updated attendee info and add a second package that has associated cost")
 	changedAttendee2 := att
-	changedAttendee2.Packages = "room-none,attendance,stage,sponsor2,boat-trip,mountain-trip" // adds mountain-trip
+	changedAttendee2.Packages = "attendance,boat-trip,mountain-trip,room-none,sponsor2,stage" // adds mountain-trip
+	// TODO #239 for now we test for the classic package field - remove and send list instead later
+	changedAttendee2.PackagesList = nil
 	response2 := tstPerformPut(loc, tstRenderJson(changedAttendee2), token)
 
 	docs.Then("then the attendee is successfully updated both times and the final data can be read again")
 	require.Equal(t, http.StatusOK, response.status, "unexpected http response status for update")
 	require.Equal(t, http.StatusOK, response2.status, "unexpected http response status for update")
 	attendeeReadAgain := tstReadAttendee(t, loc)
+	// TODO #239 for now we test for the classic package field - remove and check list instead later
+	attendeeReadAgain.PackagesList = nil
 	require.EqualValues(t, changedAttendee2, attendeeReadAgain, "attendee data read did not match final updated data")
-	require.EqualValues(t, "room-none,attendance,stage,sponsor2,boat-trip,mountain-trip", attendeeReadAgain.Packages, "attendee data read did not match expected package value")
+	require.EqualValues(t, "attendance,boat-trip,mountain-trip,room-none,sponsor2,stage", attendeeReadAgain.Packages, "attendee data read did not match expected package value")
 
 	docs.Then("and the expected mail messages have been sent with the extended due date kept in place")
 	for i := range expectedMails {
@@ -1376,15 +1404,19 @@ func tstUpdateExistingAttendee_AddOnePackage_Admin_SuppressEmailWorks(t *testing
 
 	docs.When("when an admin sends updated attendee info and adds a package that has associated cost (with suppressMinorUpdateEmail flag set)")
 	changedAttendee := att
-	changedAttendee.Packages = "room-none,attendance,stage,sponsor2,boat-trip" // adds boat-trip
+	changedAttendee.Packages = "attendance,boat-trip,room-none,sponsor2,stage" // adds boat-trip
+	// TODO #239 for now we test for the classic package field - remove and send list instead later
+	changedAttendee.PackagesList = nil
 	response := tstPerformPut(loc+"?suppressMinorUpdateEmail=yes", tstRenderJson(changedAttendee), token)
 
 	docs.Then("then the attendee is successfully updated and the changed data can be read again")
 	require.Equal(t, http.StatusOK, response.status, "unexpected http response status for update")
 	require.Equal(t, loc, response.location, "location unexpectedly changed during update")
 	attendeeReadAgain := tstReadAttendee(t, loc)
+	// TODO #239 for now we test for the classic package field - remove and check list instead later
+	attendeeReadAgain.PackagesList = nil
 	require.EqualValues(t, changedAttendee, attendeeReadAgain, "attendee data read did not match updated data")
-	require.EqualValues(t, "room-none,attendance,stage,sponsor2,boat-trip", attendeeReadAgain.Packages, "attendee data read did not match expected package value")
+	require.EqualValues(t, "attendance,boat-trip,room-none,sponsor2,stage", attendeeReadAgain.Packages, "attendee data read did not match expected package value")
 
 	docs.Then("and no mail messages have been sent because of the suppressMinorUpdateEmail flag")
 	tstRequireMailRequests(t, nil)
diff --git a/test/acceptance/utils_test.go b/test/acceptance/utils_test.go
index d8a867c..b3580a8 100644
--- a/test/acceptance/utils_test.go
+++ b/test/acceptance/utils_test.go
@@ -176,7 +176,7 @@ func tstBuildValidAttendee(testcase string) attendee.AttendeeDto {
 		SpokenLanguages:      "de,en",
 		RegistrationLanguage: "en-US",
 		Flags:                "anon,hc,terms-accepted",
-		Packages:             "room-none,attendance,stage,sponsor2",
+		Packages:             "attendance,room-none,sponsor2,stage",
 		Options:              "music,suit",
 		TshirtSize:           "XXL",
 	}

From 7f9af12f2d1994d149acb20f894630bc0cf426e6 Mon Sep 17 00:00:00 2001
From: Jumpy Squirrel <jsquirrel_github_9a6d@packetloss.de>
Date: Sun, 8 Dec 2024 11:43:18 +0100
Subject: [PATCH 3/6] feat(#239): cover mapping and validation changes

---
 .../controller/attendeectl/mapping_test.go    | 38 +++++++++++++++++++
 .../controller/attendeectl/validation_test.go | 33 +++++++++++++---
 test/testconfig-base.yaml                     |  1 +
 3 files changed, 66 insertions(+), 6 deletions(-)

diff --git a/internal/web/controller/attendeectl/mapping_test.go b/internal/web/controller/attendeectl/mapping_test.go
index ad5642c..b013687 100644
--- a/internal/web/controller/attendeectl/mapping_test.go
+++ b/internal/web/controller/attendeectl/mapping_test.go
@@ -20,3 +20,41 @@ func TestMapping(t *testing.T) {
 	attendeeDtoSource.Id = attendeeDtoResult.Id
 	require.EqualValues(t, attendeeDtoSource, attendeeDtoResult, "unexpected difference after mapping back and forth")
 }
+
+func TestMapping_PackagesClassic(t *testing.T) {
+	docs.Description("mapping an attendee dto back and forth should result in the same data - packages provided as comma separated list")
+	attendeeDtoSource := tstCreateValidAttendee()
+	keepPkgList := attendeeDtoSource.PackagesList
+	attendeeDtoSource.PackagesList = nil
+	attendeeEntity := entity.Attendee{}
+	mapDtoToAttendee(&attendeeDtoSource, &attendeeEntity)
+
+	attendeeDtoResult := attendee.AttendeeDto{}
+	mapAttendeeToDto(&attendeeEntity, &attendeeDtoResult)
+	// id differences are ok because the field is only mapped one way, so overwrite with actual value
+	attendeeDtoSource.Id = attendeeDtoResult.Id
+	// reverse mapping fills the package list, so restore it now
+	attendeeDtoSource.PackagesList = keepPkgList
+	require.EqualValues(t, attendeeDtoSource, attendeeDtoResult, "unexpected difference after mapping back and forth")
+}
+
+func TestMapping_PackagesAsList(t *testing.T) {
+	docs.Description("mapping an attendee dto back and forth should result in the same data - packages provided in packages_list field")
+	attendeeDtoSource := tstCreateValidAttendee()
+	keepPkgClassic := attendeeDtoSource.Packages
+	attendeeDtoSource.Packages = ""
+	attendeeEntity := entity.Attendee{}
+	mapDtoToAttendee(&attendeeDtoSource, &attendeeEntity)
+
+	attendeeDtoResult := attendee.AttendeeDto{}
+	mapAttendeeToDto(&attendeeEntity, &attendeeDtoResult)
+	// id differences are ok because the field is only mapped one way, so overwrite with actual value
+	attendeeDtoSource.Id = attendeeDtoResult.Id
+	// reverse mapping fills both packages_list and packages, so restore it now
+	attendeeDtoSource.Packages = keepPkgClassic
+	require.EqualValues(t, attendeeDtoSource, attendeeDtoResult, "unexpected difference after mapping back and forth")
+}
+
+func TestMapping_DuplicatePackage(t *testing.T) {
+
+}
diff --git a/internal/web/controller/attendeectl/validation_test.go b/internal/web/controller/attendeectl/validation_test.go
index 1f4eb4a..9cbbfdf 100644
--- a/internal/web/controller/attendeectl/validation_test.go
+++ b/internal/web/controller/attendeectl/validation_test.go
@@ -32,12 +32,16 @@ func tstCreateValidAttendee() attendee.AttendeeDto {
 		SpokenLanguages:      "de,en",
 		RegistrationLanguage: "en-US",
 		Flags:                "anon,ev",
-		Packages:             "attendance,room-none,sponsor2,stage", // must be sorted for tests to work
+		Packages:             "attendance,mountain-trip,mountain-trip,room-none,sponsor2,stage", // must be sorted for tests to work
 		PackagesList: []attendee.PackageState{
 			{
 				Name:  "attendance",
 				Count: 1,
 			},
+			{
+				Name:  "mountain-trip",
+				Count: 2,
+			},
 			{
 				Name:  "room-none",
 				Count: 1,
@@ -210,7 +214,17 @@ func TestValidateChoiceFieldsAndId(t *testing.T) {
 	a.Gender = "348trhkuth4uihgkj4h89"
 	a.Options = "music,awoo"
 	a.Flags = "hc,noflag"
-	a.Packages = "helicopterflight,boattour,room-none"
+	a.Packages = "helicopterflight,boattour,room-none,room-none"
+	a.PackagesList = []attendee.PackageState{
+		{
+			Name:  "helicopterflight",
+			Count: 1,
+		},
+		{
+			Name:  "room-none",
+			Count: 2,
+		},
+	}
 	a.TshirtSize = "micro"
 	a.Telegram = "iforgotthe_at_atthebeginning"
 	a.Country = "XX" // not in ISO-3166-1
@@ -218,10 +232,17 @@ func TestValidateChoiceFieldsAndId(t *testing.T) {
 	a.RegistrationLanguage = "not_a_LANG"
 
 	expected := url.Values{
-		"gender":                []string{"optional gender field must be one of male, female, other, notprovided, or it can be left blank, which counts as notprovided"},
-		"options":               []string{"options field must be a comma separated combination of any of anim,art,music,suit"},
-		"flags":                 []string{"flags field must be a comma separated combination of any of anon,ev,hc,terms-accepted"},
-		"packages":              []string{"packages field must be a comma separated combination of any of attendance,boat-trip,day-fri,day-sat,day-thu,mountain-trip,room-none,sponsor,sponsor2,stage"},
+		"gender":  []string{"optional gender field must be one of male, female, other, notprovided, or it can be left blank, which counts as notprovided"},
+		"options": []string{"options field must be a comma separated combination of any of anim,art,music,suit"},
+		"flags":   []string{"flags field must be a comma separated combination of any of anon,ev,hc,terms-accepted"},
+		"packages": []string{
+			"package room-none occurs too many times, can occur at most 1 times",
+			"packages field must be a comma separated combination of any of attendance,boat-trip,day-fri,day-sat,day-thu,mountain-trip,room-none,sponsor,sponsor2,stage",
+		},
+		"packages_list": []string{
+			"package room-none occurs too many times, can occur at most 1 times",
+			"packages_list can only contain package names attendance,boat-trip,day-fri,day-sat,day-thu,mountain-trip,room-none,sponsor,sponsor2,stage",
+		},
 		"registration_language": []string{"registration_language field must be one of en-US or it can be left blank, which counts as en-US"},
 		"spoken_languages":      []string{"spoken_languages field must be a comma separated combination of any of en,de"},
 		"telegram":              []string{"optional telegram field must contain your @username from telegram, or it can be left blank"},
diff --git a/test/testconfig-base.yaml b/test/testconfig-base.yaml
index ed30b87..7abf1d5 100644
--- a/test/testconfig-base.yaml
+++ b/test/testconfig-base.yaml
@@ -127,6 +127,7 @@ choices:
       description: 'Mountain Trip'
       price: 3000
       vat_percent: 19
+      max_count: 2
     day-thu:
       description: 'Day Guest (Thursday)'
       price: 6000

From 8ce32a6066e75a9aa553ee9654706de031345f1f Mon Sep 17 00:00:00 2001
From: Jumpy Squirrel <jsquirrel_github_9a6d@packetloss.de>
Date: Sun, 8 Dec 2024 13:17:18 +0100
Subject: [PATCH 4/6] fix(#239): package list wins when checking constraints,
 and test with package list

---
 internal/service/attendeesrv/attendeesrv.go   | 39 +++++++-
 internal/service/attendeesrv/generate.go      |  2 +-
 internal/service/attendeesrv/interfaces.go    |  2 +-
 .../web/controller/attendeectl/config_test.go |  2 +-
 .../web/controller/attendeectl/mapping.go     | 16 ++--
 .../web/controller/attendeectl/validation.go  |  3 +-
 test/acceptance/attendee_acc_test.go          | 89 +++++++++----------
 test/acceptance/utils_test.go                 | 56 +++++++++++-
 8 files changed, 149 insertions(+), 60 deletions(-)

diff --git a/internal/service/attendeesrv/attendeesrv.go b/internal/service/attendeesrv/attendeesrv.go
index a0d1e91..6981ac2 100644
--- a/internal/service/attendeesrv/attendeesrv.go
+++ b/internal/service/attendeesrv/attendeesrv.go
@@ -5,6 +5,7 @@ import (
 	"errors"
 	"fmt"
 	aulogging "github.com/StephanHCB/go-autumn-logging"
+	"github.com/eurofurence/reg-attendee-service/internal/api/v1/attendee"
 	"github.com/eurofurence/reg-attendee-service/internal/api/v1/status"
 	"github.com/eurofurence/reg-attendee-service/internal/entity"
 	"github.com/eurofurence/reg-attendee-service/internal/repository/config"
@@ -140,12 +141,18 @@ func (s *AttendeeServiceImplData) CanChangeEmailTo(ctx context.Context, original
 }
 
 func (s *AttendeeServiceImplData) CanChangeChoiceTo(ctx context.Context, what string, originalChoiceStr string, newChoiceStr string, configuration map[string]config.ChoiceConfig) error {
-	return s.CanChangeChoiceToCurrentStatus(ctx, what, originalChoiceStr, newChoiceStr, configuration, "irrelevant")
+	originalChoicesMap := choiceStrToMap(originalChoiceStr, configuration)
+	newChoicesMap := choiceStrToMap(newChoiceStr, configuration)
+	return s.canChangeChoiceLowlevel(ctx, what, originalChoicesMap, newChoicesMap, configuration, "irrelevant")
 }
 
-func (s *AttendeeServiceImplData) CanChangeChoiceToCurrentStatus(ctx context.Context, what string, originalChoiceStr string, newChoiceStr string, configuration map[string]config.ChoiceConfig, currentStatus status.Status) error {
-	originalChoices := choiceStrToMap(originalChoiceStr, configuration)
-	newChoices := choiceStrToMap(newChoiceStr, configuration)
+func (s *AttendeeServiceImplData) CanChangeChoiceToCurrentStatus(ctx context.Context, what string, originalChoice []attendee.PackageState, newChoice []attendee.PackageState, configuration map[string]config.ChoiceConfig, currentStatus status.Status) error {
+	originalChoicesMap := choiceListToMap(originalChoice, configuration)
+	newChoicesMap := choiceListToMap(newChoice, configuration)
+	return s.canChangeChoiceLowlevel(ctx, what, originalChoicesMap, newChoicesMap, configuration, currentStatus)
+}
+
+func (s *AttendeeServiceImplData) canChangeChoiceLowlevel(ctx context.Context, what string, originalChoices map[string]int, newChoices map[string]int, configuration map[string]config.ChoiceConfig, currentStatus status.Status) error {
 	oneIsMandatory := false
 	satisfiesOneIsMandatory := false
 	mandatoryList := make([]string, 0)
@@ -332,6 +339,30 @@ func choiceStrToMap(choiceStr string, configuration map[string]config.ChoiceConf
 	return result
 }
 
+// choiceListToMap converts a choice list to a map of counts
+//
+// Can be used for packages, flags, options.
+func choiceListToMap(choiceList []attendee.PackageState, configuration map[string]config.ChoiceConfig) map[string]int {
+	result := make(map[string]int)
+	// ensure all available keys present
+	for k, _ := range configuration {
+		result[k] = 0
+	}
+	for _, entry := range choiceList {
+		currentValue, present := result[entry.Name]
+		if present {
+			if entry.Count == 0 {
+				entry.Count = 1
+			}
+			result[entry.Name] = currentValue + entry.Count
+		} else {
+			aulogging.Logger.NoCtx().Warn().Printf("encountered non-configured choice key '%s' - maybe configuration changed after initial reg? This needs fixing! - continuing", entry.Name)
+			result[entry.Name] = 1
+		}
+	}
+	return result
+}
+
 // commaSeparatedStrToMap converts a comma separated string representation in the entity to a map of booleans
 //
 // Can be used for permissions, languages, etc.
diff --git a/internal/service/attendeesrv/generate.go b/internal/service/attendeesrv/generate.go
index 95d0b5c..7b5c06d 100644
--- a/internal/service/attendeesrv/generate.go
+++ b/internal/service/attendeesrv/generate.go
@@ -168,7 +168,7 @@ func mapAttendeeToDto(a *entity.Attendee, dto *attendee.AttendeeDto) {
 	dto.SpokenLanguages = removeWrappingCommas(a.SpokenLanguages)
 	dto.RegistrationLanguage = removeWrappingCommas(a.RegistrationLanguage)
 	dto.Flags = removeWrappingCommas(a.Flags)
-	dto.Packages = removeWrappingCommas(a.Packages)
+	dto.PackagesList = sortedPackageListFromCommaSeparated(removeWrappingCommas(a.Packages))
 	dto.Options = removeWrappingCommas(a.Options)
 	dto.UserComments = a.UserComments
 }
diff --git a/internal/service/attendeesrv/interfaces.go b/internal/service/attendeesrv/interfaces.go
index 0538953..878783a 100644
--- a/internal/service/attendeesrv/interfaces.go
+++ b/internal/service/attendeesrv/interfaces.go
@@ -28,7 +28,7 @@ type AttendeeService interface {
 	CanChangeEmailTo(ctx context.Context, originalEmail string, newEmail string) error
 
 	CanChangeChoiceTo(ctx context.Context, what string, originalChoiceStr string, newChoiceStr string, configuration map[string]config.ChoiceConfig) error
-	CanChangeChoiceToCurrentStatus(ctx context.Context, what string, originalChoiceStr string, newChoiceStr string, configuration map[string]config.ChoiceConfig, currentStatus status.Status) error
+	CanChangeChoiceToCurrentStatus(ctx context.Context, what string, originalChoice []attendee.PackageState, newChoice []attendee.PackageState, configuration map[string]config.ChoiceConfig, currentStatus status.Status) error
 
 	GetAdminInfo(ctx context.Context, attendeeId uint) (*entity.AdminInfo, error)
 	UpdateAdminInfo(ctx context.Context, attendee *entity.Attendee, adminInfo *entity.AdminInfo, suppressMinorUpdateEmail bool) error
diff --git a/internal/web/controller/attendeectl/config_test.go b/internal/web/controller/attendeectl/config_test.go
index c8900ec..58334ce 100644
--- a/internal/web/controller/attendeectl/config_test.go
+++ b/internal/web/controller/attendeectl/config_test.go
@@ -69,7 +69,7 @@ func (s *MockAttendeeService) CanChangeChoiceTo(ctx context.Context, what string
 	return nil
 }
 
-func (s *MockAttendeeService) CanChangeChoiceToCurrentStatus(ctx context.Context, what string, originalChoiceStr string, newChoiceStr string, configuration map[string]config.ChoiceConfig, currentStatus status.Status) error {
+func (s *MockAttendeeService) CanChangeChoiceToCurrentStatus(ctx context.Context, what string, originalChoice []attendee.PackageState, newChoice []attendee.PackageState, configuration map[string]config.ChoiceConfig, currentStatus status.Status) error {
 	return nil
 }
 
diff --git a/internal/web/controller/attendeectl/mapping.go b/internal/web/controller/attendeectl/mapping.go
index 5fcd3de..c5221d8 100644
--- a/internal/web/controller/attendeectl/mapping.go
+++ b/internal/web/controller/attendeectl/mapping.go
@@ -101,14 +101,11 @@ func sliceContains(slice []string, singleValue string) bool {
 }
 
 func packagesFromDto(commaSeparated string, asList []attendee.PackageState) string {
-	// only use commaSeparated if no list is supplied
-	if len(asList) == 0 {
-		asList = packageListFromCommaSeparated(commaSeparated)
-	}
+	alwaysAsList := packagesListWithPrecedence(commaSeparated, asList)
 
 	var result strings.Builder
 	result.WriteString(",")
-	for _, item := range asList {
+	for _, item := range alwaysAsList {
 		// item.Count = 0 should be interpreted as 1 (allows omitting Count in requests)
 		for i := 0; i == 0 || i < item.Count; i++ {
 			result.WriteString(item.Name + ",")
@@ -127,6 +124,15 @@ func packagesListFromEntity(entityPackages string) []attendee.PackageState {
 	return asList
 }
 
+func packagesListWithPrecedence(commaSeparated string, asList []attendee.PackageState) []attendee.PackageState {
+	// only use commaSeparated if no list is supplied
+	if len(asList) == 0 {
+		return packageListFromCommaSeparated(commaSeparated)
+	} else {
+		return asList
+	}
+}
+
 // packageListFromCommaSeparated takes a comma separated list, without leading and trailing commas, and converts
 // it into a sorted package_list
 func packageListFromCommaSeparated(commaSeparatedValue string) []attendee.PackageState {
diff --git a/internal/web/controller/attendeectl/validation.go b/internal/web/controller/attendeectl/validation.go
index b6c5828..08e9d18 100644
--- a/internal/web/controller/attendeectl/validation.go
+++ b/internal/web/controller/attendeectl/validation.go
@@ -107,7 +107,8 @@ func validate(ctx context.Context, a *attendee.AttendeeDto, trustedOriginalState
 	if err := attendeeService.CanChangeChoiceTo(ctx, "flag", trustedOriginalState.Flags, a.Flags, config.FlagsConfigNoAdmin()); err != nil {
 		errs.Add("flags", err.Error())
 	}
-	if err := attendeeService.CanChangeChoiceToCurrentStatus(ctx, "package", trustedOriginalState.Packages, a.Packages, config.PackagesConfig(), currentStatus); err != nil {
+	newPackagesList := packagesListWithPrecedence(a.Packages, a.PackagesList)
+	if err := attendeeService.CanChangeChoiceToCurrentStatus(ctx, "package", packagesListFromEntity(trustedOriginalState.Packages), newPackagesList, config.PackagesConfig(), currentStatus); err != nil {
 		errs.Add("packages", err.Error())
 	}
 	if err := attendeeService.CanChangeChoiceTo(ctx, "option", trustedOriginalState.Options, a.Options, config.OptionsConfig()); err != nil {
diff --git a/test/acceptance/attendee_acc_test.go b/test/acceptance/attendee_acc_test.go
index ac1305d..549268f 100644
--- a/test/acceptance/attendee_acc_test.go
+++ b/test/acceptance/attendee_acc_test.go
@@ -36,8 +36,31 @@ func TestCreateNewAttendeeInvalid(t *testing.T) {
 	docs.When("when they create a new attendee with invalid data")
 	attendeeSent := tstBuildValidAttendee("nav1-")
 	attendeeSent.Nickname = "$%&^@!$"
-	attendeeSent.Packages = attendeeSent.Packages + ",sponsor" // a constraint violation
-	attendeeSent.Birthday = "2004-11-23"                       // too young
+	tstAddPackages(&attendeeSent, "sponsor") // a constraint violation
+	attendeeSent.Birthday = "2004-11-23"     // too young
+	response := tstPerformPost("/api/rest/v1/attendees", tstRenderJson(attendeeSent), tstNoToken())
+
+	docs.Then("then the attendee is rejected with an appropriate error response")
+	tstRequireErrorResponse(t, response, http.StatusBadRequest, "attendee.data.invalid", url.Values{
+		"birthday": []string{"birthday must be no earlier than 1901-01-01 and no later than 2001-08-14"},
+		"nickname": []string{"nickname field must contain at least one alphanumeric character", "nickname field must not contain more than two non-alphanumeric characters (not counting spaces)"},
+		"packages": []string{"cannot pick both sponsor2 and sponsor - constraint violated"},
+	})
+}
+
+func TestCreateNewAttendeeInvalidPackagesList(t *testing.T) {
+	docs.Given("given the configuration for public standard registration")
+	tstSetup(false, false, true)
+	defer tstShutdown()
+
+	docs.Given("given an unauthenticated user")
+
+	docs.When("when they create a new attendee with invalid data")
+	attendeeSent := tstBuildValidAttendee("nav1-")
+	attendeeSent.Nickname = "$%&^@!$"
+	attendeeSent.Packages = ""                                                                            // should be ignored, so let's produce a different error if used
+	attendeeSent.PackagesList = append(attendeeSent.PackagesList, attendee.PackageState{Name: "sponsor"}) // constraint violation, also tests that Count: 0 means Count: 1
+	attendeeSent.Birthday = "2004-11-23"                                                                  // too young
 	response := tstPerformPost("/api/rest/v1/attendees", tstRenderJson(attendeeSent), tstNoToken())
 
 	docs.Then("then the attendee is rejected with an appropriate error response")
@@ -57,7 +80,7 @@ func TestCreateNewAttendeeInvalid_NoMandatoryPackage(t *testing.T) {
 
 	docs.When("when they create a new attendee, but pick none of the at-least-one-mandatory packages")
 	attendeeSent := tstBuildValidAttendee("nav1b-")
-	attendeeSent.Packages = "room-none,sponsor"
+	tstOverridePackages(&attendeeSent, "room-none,sponsor")
 	response := tstPerformPost("/api/rest/v1/attendees", tstRenderJson(attendeeSent), tstNoToken())
 
 	docs.Then("then the attendee is rejected with an appropriate error response")
@@ -165,7 +188,7 @@ func TestCreateNewAttendeeDefaultReadOnlyPackage(t *testing.T) {
 
 	docs.When("when they send a new attendee and attempt to leave out a read only default package (room-none)")
 	attendeeSent := tstBuildValidAttendee("nav6-")
-	attendeeSent.Packages = "attendance,stage,sponsor"
+	tstOverridePackages(&attendeeSent, "attendance,stage,sponsor")
 	response := tstPerformPost("/api/rest/v1/attendees", tstRenderJson(attendeeSent), staffToken)
 
 	docs.Then("then the attendee is rejected with an error response")
@@ -367,8 +390,6 @@ func TestCreateNewAttendee_NoLoginRequired_After_Anon(t *testing.T) {
 	attendeeReadAgain := tstReadAttendee(t, response.location)
 	// difference in id is ok, so copy it over to expected
 	attendeeSent.Id = attendeeReadAgain.Id
-	// TODO #239 for now we test for the classic package field - remove and send list instead later
-	attendeeReadAgain.PackagesList = nil
 	require.EqualValues(t, attendeeSent, attendeeReadAgain, "attendee data read did not match sent data")
 }
 
@@ -394,8 +415,6 @@ func TestCreateNewAttendee_NoLoginRequired_After_User(t *testing.T) {
 	tstParseJson(readAgainResponse.body, &attendeeReadAgain)
 	// difference in id is ok, so copy it over to expected
 	attendeeSent.Id = attendeeReadAgain.Id
-	// TODO #239 for now we test for the classic package field - remove and send list instead later
-	attendeeReadAgain.PackagesList = nil
 	require.EqualValues(t, attendeeSent, attendeeReadAgain, "attendee data read did not match sent data")
 }
 
@@ -745,8 +764,6 @@ func TestCreateNewAttendee_AutomaticGroupFlag(t *testing.T) {
 	attendeeSent.Id = attendeeReadAgain.Id
 	// we expect the 'ev' flag added
 	attendeeSent.Flags += ",ev"
-	// TODO #239 for now we test for the classic package field - remove and send list instead later
-	attendeeReadAgain.PackagesList = nil
 	require.EqualValues(t, attendeeSent, attendeeReadAgain, "attendee data read did not match expected data")
 }
 
@@ -779,7 +796,7 @@ func TestCreateNewAttendee_ReadonlyDefaultPackageWithConstraintRemovable(t *test
 
 	docs.When("when they create a new attendee and remove a read-only default package with matching constraint (stage)")
 	attendeeSent := tstBuildValidAttendee("na63-")
-	attendeeSent.Packages = "room-none,day-sat,boat-trip"
+	tstOverridePackages(&attendeeSent, "room-none,day-sat,boat-trip")
 	response := tstPerformPost("/api/rest/v1/attendees", tstRenderJson(attendeeSent), token)
 
 	docs.Then("then the attendee is successfully created")
@@ -797,7 +814,7 @@ func TestCreateNewAttendee_ReadonlyDefaultPackageNoConstraintNotRemovable(t *tes
 
 	docs.When("when they create a new attendee and try to remove a read-only default package with no matching constraint (room-none)")
 	attendeeSent := tstBuildValidAttendee("na65-")
-	attendeeSent.Packages = "day-sat"
+	tstOverridePackages(&attendeeSent, "day-sat")
 	response := tstPerformPost("/api/rest/v1/attendees", tstRenderJson(attendeeSent), token)
 
 	docs.Then("then the attempt is rejected as invalid (400) with an appropriate error response")
@@ -861,7 +878,7 @@ func TestUpdateExistingAttendee_Self_NoMandatoryPackages(t *testing.T) {
 
 	docs.When("when they attempt to change the selected packages, and pick none of the at-least-one-mandatory packages")
 	changedAttendee := attendee1
-	changedAttendee.Packages = "room-none,sponsor"
+	tstOverridePackages(&changedAttendee, "room-none,sponsor")
 	updateResponse := tstPerformPut(location1, tstRenderJson(changedAttendee), token)
 
 	docs.Then("then the request fails with the appropriate error")
@@ -921,9 +938,9 @@ func TestUpdateExistingAttendeeDataInvalid(t *testing.T) {
 
 	docs.When("when they try to update the information with invalid data")
 	changedAttendee := attendee1
-	changedAttendee.Nickname = "$%&^@!$"                             // not allowed
-	changedAttendee.Packages = changedAttendee.Packages + ",sponsor" // a constraint violation
-	changedAttendee.Birthday = "2004-11-23"                          // too young
+	changedAttendee.Nickname = "$%&^@!$"        // not allowed
+	tstAddPackages(&changedAttendee, "sponsor") // a constraint violation
+	changedAttendee.Birthday = "2004-11-23"     // too young
 	response := tstPerformPut(location1, tstRenderJson(changedAttendee), token)
 
 	docs.Then("then the update is rejected with an appropriate error response")
@@ -1169,7 +1186,7 @@ func tstUpdateExistingAttendee_RemovePackage_Forbidden(t *testing.T, testcase st
 
 	docs.When("when they send updated attendee info and change their packages in a way that reduces total dues")
 	changedAttendee := att
-	changedAttendee.Packages = "room-none,attendance,stage" // removes sponsor2
+	tstOverridePackages(&changedAttendee, "room-none,attendance,stage") // removes sponsor2
 	response := tstPerformPut(loc, tstRenderJson(changedAttendee), token)
 
 	docs.Then("then the update is rejected with the corresponding error message")
@@ -1187,26 +1204,20 @@ func tstUpdateExistingAttendee_RemovePackage_NoCostReduce_Allowed(t *testing.T,
 	loc, att := tstRegisterAttendeeAndTransitionToStatus(t, testcase, targetStatus)
 	// update to prepare test case
 	changedAttendee := att
-	changedAttendee.Packages = "attendance,boat-trip,room-none,sponsor2,stage" // adds boat-trip
-	// TODO #239 for now we test for the classic package field - remove and send list instead later
-	changedAttendee.PackagesList = nil
+	tstAddPackages(&changedAttendee, "boat-trip")
 	response := tstPerformPut(loc, tstRenderJson(changedAttendee), token)
 	require.Equal(t, http.StatusOK, response.status, "unexpected http response status for update")
 	require.Equal(t, loc, response.location, "location unexpectedly changed during update")
 
 	docs.When("when they send updated attendee info and change packages in a way that does not reduce total dues")
 	changedAttendee2 := att
-	changedAttendee2.Packages = "attendance,mountain-trip,room-none,sponsor2,stage" // removes boat-trip, but adds mountain-trip which is more expensive
-	// TODO #239 for now we test for the classic package field - remove and send list instead later
-	changedAttendee2.PackagesList = nil
+	tstOverridePackages(&changedAttendee2, "attendance,mountain-trip,room-none,sponsor2,stage") // removes boat-trip, but adds mountain-trip which is more expensive
 	response2 := tstPerformPut(loc, tstRenderJson(changedAttendee2), token)
 
 	docs.Then("then the attendee is successfully updated and the changed data can be read again")
 	require.Equal(t, http.StatusOK, response2.status, "unexpected http response status for update")
 	require.Equal(t, loc, response2.location, "location unexpectedly changed during update")
 	attendeeReadAgain := tstReadAttendee(t, loc)
-	// TODO #239 for now we test for the classic package field - remove and check list instead later
-	attendeeReadAgain.PackagesList = nil
 	require.EqualValues(t, changedAttendee2, attendeeReadAgain, "attendee data read did not match updated data")
 	require.EqualValues(t, "attendance,mountain-trip,room-none,sponsor2,stage", attendeeReadAgain.Packages, "attendee data read did not match expected package value")
 }
@@ -1221,17 +1232,13 @@ func tstUpdateExistingAttendee_RemovePackage_Allowed(t *testing.T, testcase stri
 
 	docs.When("when they send updated attendee info and remove a package that has associated cost")
 	changedAttendee := att
-	changedAttendee.Packages = "attendance,room-none,stage" // removes sponsor2
-	// TODO #239 for now we test for the classic package field - remove and send changed list instead later
-	changedAttendee.PackagesList = nil
+	tstOverridePackages(&changedAttendee, "attendance,room-none,stage") // removes sponsor2
 	response := tstPerformPut(loc, tstRenderJson(changedAttendee), token)
 
 	docs.Then("then the attendee is successfully updated and the changed data can be read again")
 	require.Equal(t, http.StatusOK, response.status, "unexpected http response status for update")
 	require.Equal(t, loc, response.location, "location unexpectedly changed during update")
 	attendeeReadAgain := tstReadAttendee(t, loc)
-	// TODO #239 for now we test for the classic package field - remove and check changed list instead later
-	attendeeReadAgain.PackagesList = nil
 	require.EqualValues(t, changedAttendee, attendeeReadAgain, "attendee data read did not match updated data")
 	require.EqualValues(t, "attendance,room-none,stage", attendeeReadAgain.Packages, "attendee data read did not match expected package value")
 }
@@ -1287,7 +1294,7 @@ func tstUpdateExistingAttendee_AddOnePackage(t *testing.T, testcase string, orig
 	docs.When("when they send updated attendee info and add a package that has associated cost")
 	changedAttendee := att
 	changedAttendee.Packages = "room-none,attendance,stage,sponsor2,boat-trip" // adds boat-trip (and tests automatic reordering)
-	// TODO #239 for now we test for the classic package field - remove and send list instead later
+	// let's also test that the package List is populated from sending classic packages, so clear the list in this request
 	changedAttendee.PackagesList = nil
 	response := tstPerformPut(loc, tstRenderJson(changedAttendee), token)
 
@@ -1295,10 +1302,10 @@ func tstUpdateExistingAttendee_AddOnePackage(t *testing.T, testcase string, orig
 	require.Equal(t, http.StatusOK, response.status, "unexpected http response status for update")
 	require.Equal(t, loc, response.location, "location unexpectedly changed during update")
 	attendeeReadAgain := tstReadAttendee(t, loc)
-	// TODO #239 for now we test for the classic package field - remove and check list instead later
-	attendeeReadAgain.PackagesList = nil
 	// test automatic reordering of packages to alphabetical order
 	changedAttendee.Packages = "attendance,boat-trip,room-none,sponsor2,stage"
+	// and test that package list is also sent in responses, also sorted alphabetically by names
+	changedAttendee.PackagesList = tstPackagesListFromPackages(changedAttendee.Packages)
 	require.EqualValues(t, changedAttendee, attendeeReadAgain, "attendee data read did not match updated data")
 	require.EqualValues(t, "attendance,boat-trip,room-none,sponsor2,stage", attendeeReadAgain.Packages, "attendee data read did not match expected package value")
 
@@ -1340,24 +1347,18 @@ func tstUpdateExistingAttendee_AddTwoPackages(t *testing.T, testcase string, ori
 
 	docs.When("when they send updated attendee info and add a package that has associated cost")
 	changedAttendee := att
-	changedAttendee.Packages = "attendance,boat-trip,room-none,sponsor2,stage" // adds boat-trip
-	// TODO #239 for now we test for the classic package field - remove and send list instead later
-	changedAttendee.PackagesList = nil
+	tstAddPackages(&changedAttendee, "boat-trip")
 	response := tstPerformPut(loc, tstRenderJson(changedAttendee), token)
 
 	docs.When("and then send updated attendee info and add a second package that has associated cost")
 	changedAttendee2 := att
-	changedAttendee2.Packages = "attendance,boat-trip,mountain-trip,room-none,sponsor2,stage" // adds mountain-trip
-	// TODO #239 for now we test for the classic package field - remove and send list instead later
-	changedAttendee2.PackagesList = nil
+	tstAddPackages(&changedAttendee2, "boat-trip,mountain-trip")
 	response2 := tstPerformPut(loc, tstRenderJson(changedAttendee2), token)
 
 	docs.Then("then the attendee is successfully updated both times and the final data can be read again")
 	require.Equal(t, http.StatusOK, response.status, "unexpected http response status for update")
 	require.Equal(t, http.StatusOK, response2.status, "unexpected http response status for update")
 	attendeeReadAgain := tstReadAttendee(t, loc)
-	// TODO #239 for now we test for the classic package field - remove and check list instead later
-	attendeeReadAgain.PackagesList = nil
 	require.EqualValues(t, changedAttendee2, attendeeReadAgain, "attendee data read did not match final updated data")
 	require.EqualValues(t, "attendance,boat-trip,mountain-trip,room-none,sponsor2,stage", attendeeReadAgain.Packages, "attendee data read did not match expected package value")
 
@@ -1404,17 +1405,13 @@ func tstUpdateExistingAttendee_AddOnePackage_Admin_SuppressEmailWorks(t *testing
 
 	docs.When("when an admin sends updated attendee info and adds a package that has associated cost (with suppressMinorUpdateEmail flag set)")
 	changedAttendee := att
-	changedAttendee.Packages = "attendance,boat-trip,room-none,sponsor2,stage" // adds boat-trip
-	// TODO #239 for now we test for the classic package field - remove and send list instead later
-	changedAttendee.PackagesList = nil
+	tstAddPackages(&changedAttendee, "boat-trip")
 	response := tstPerformPut(loc+"?suppressMinorUpdateEmail=yes", tstRenderJson(changedAttendee), token)
 
 	docs.Then("then the attendee is successfully updated and the changed data can be read again")
 	require.Equal(t, http.StatusOK, response.status, "unexpected http response status for update")
 	require.Equal(t, loc, response.location, "location unexpectedly changed during update")
 	attendeeReadAgain := tstReadAttendee(t, loc)
-	// TODO #239 for now we test for the classic package field - remove and check list instead later
-	attendeeReadAgain.PackagesList = nil
 	require.EqualValues(t, changedAttendee, attendeeReadAgain, "attendee data read did not match updated data")
 	require.EqualValues(t, "attendance,boat-trip,room-none,sponsor2,stage", attendeeReadAgain.Packages, "attendee data read did not match expected package value")
 
diff --git a/test/acceptance/utils_test.go b/test/acceptance/utils_test.go
index b3580a8..2767f3f 100644
--- a/test/acceptance/utils_test.go
+++ b/test/acceptance/utils_test.go
@@ -14,6 +14,7 @@ import (
 	"io/ioutil"
 	"log"
 	"net/http"
+	"sort"
 	"strings"
 	"testing"
 	"time"
@@ -158,6 +159,7 @@ func tstPerformDelete(relativeUrlWithLeadingSlash string, token string) tstWebRe
 }
 
 func tstBuildValidAttendee(testcase string) attendee.AttendeeDto {
+	packages := "attendance,room-none,sponsor2,stage"
 	return attendee.AttendeeDto{
 		Nickname:             "BlackCheetah",
 		FirstName:            "Hans",
@@ -176,12 +178,64 @@ func tstBuildValidAttendee(testcase string) attendee.AttendeeDto {
 		SpokenLanguages:      "de,en",
 		RegistrationLanguage: "en-US",
 		Flags:                "anon,hc,terms-accepted",
-		Packages:             "attendance,room-none,sponsor2,stage",
+		Packages:             packages, // ignored because PackagesList is set and takes precedence
+		PackagesList:         tstPackagesListFromPackages(packages),
 		Options:              "music,suit",
 		TshirtSize:           "XXL",
 	}
 }
 
+func tstOverridePackages(att *attendee.AttendeeDto, packages string) {
+	att.Packages = packages
+	att.PackagesList = tstPackagesListFromPackages(packages)
+}
+
+func tstAddPackages(att *attendee.AttendeeDto, additionalCommaSeparated string) {
+	att.Packages = att.Packages + "," + additionalCommaSeparated
+	att.PackagesList = tstPackagesListFromPackages(att.Packages)
+	// now ensure packages is still sorted
+	att.Packages = tstPackagesFromPackagesList(att.PackagesList)
+}
+
+func tstPackagesListFromPackages(commaSeparated string) []attendee.PackageState {
+	counts := make(map[string]int)
+	names := strings.Split(commaSeparated, ",")
+	for _, name := range names {
+		if name != "" {
+			oldCount, _ := counts[name]
+			counts[name] = oldCount + 1
+		}
+	}
+
+	result := make([]attendee.PackageState, 0)
+	for name, count := range counts {
+		result = append(result, attendee.PackageState{
+			Name:  name,
+			Count: count,
+		})
+	}
+
+	sort.Slice(result, func(i, j int) bool {
+		return result[i].Name < result[j].Name
+	})
+
+	return result
+}
+
+func tstPackagesFromPackagesList(asList []attendee.PackageState) string {
+	resultList := make([]string, 0)
+	for _, entry := range asList {
+		if entry.Count == 0 {
+			entry.Count = 1
+		}
+		for i := 0; i < entry.Count; i++ {
+			resultList = append(resultList, entry.Name)
+		}
+	}
+	sort.Strings(resultList)
+	return strings.Join(resultList, ",")
+}
+
 func tstBuildValidBanRule(testcase string) bans.BanRule {
 	return bans.BanRule{
 		Reason:          testcase,

From dfeae86d95c4faadb9906a314e36957ef7b75fc3 Mon Sep 17 00:00:00 2001
From: Jumpy Squirrel <jsquirrel_github_9a6d@packetloss.de>
Date: Sun, 8 Dec 2024 13:30:08 +0100
Subject: [PATCH 5/6] feat(#239): cover package list precedence and validation
 with acceptance tests

---
 test/acceptance/attendee_acc_test.go | 77 +++++++++++++++++++++++++---
 1 file changed, 69 insertions(+), 8 deletions(-)

diff --git a/test/acceptance/attendee_acc_test.go b/test/acceptance/attendee_acc_test.go
index 549268f..f6e7f17 100644
--- a/test/acceptance/attendee_acc_test.go
+++ b/test/acceptance/attendee_acc_test.go
@@ -48,6 +48,25 @@ func TestCreateNewAttendeeInvalid(t *testing.T) {
 	})
 }
 
+func TestCreateNewAttendeeInvalidClassicPackages(t *testing.T) {
+	docs.Given("given the configuration for public standard registration")
+	tstSetup(false, false, true)
+	defer tstShutdown()
+
+	docs.Given("given an unauthenticated user")
+
+	docs.When("when they create a new attendee with invalid data using a packages list")
+	attendeeSent := tstBuildValidAttendee("nav1b-")
+	attendeeSent.Packages = attendeeSent.Packages + ",sponsor" // constraint violation
+	attendeeSent.PackagesList = nil                            // make packages field actually count
+	response := tstPerformPost("/api/rest/v1/attendees", tstRenderJson(attendeeSent), tstNoToken())
+
+	docs.Then("then the attendee is rejected with an appropriate error response, and the packages field was used")
+	tstRequireErrorResponse(t, response, http.StatusBadRequest, "attendee.data.invalid", url.Values{
+		"packages": []string{"cannot pick both sponsor2 and sponsor - constraint violated"},
+	})
+}
+
 func TestCreateNewAttendeeInvalidPackagesList(t *testing.T) {
 	docs.Given("given the configuration for public standard registration")
 	tstSetup(false, false, true)
@@ -55,18 +74,14 @@ func TestCreateNewAttendeeInvalidPackagesList(t *testing.T) {
 
 	docs.Given("given an unauthenticated user")
 
-	docs.When("when they create a new attendee with invalid data")
-	attendeeSent := tstBuildValidAttendee("nav1-")
-	attendeeSent.Nickname = "$%&^@!$"
+	docs.When("when they create a new attendee with invalid data using a packages list with omitted count field")
+	attendeeSent := tstBuildValidAttendee("nav1c-")
 	attendeeSent.Packages = ""                                                                            // should be ignored, so let's produce a different error if used
 	attendeeSent.PackagesList = append(attendeeSent.PackagesList, attendee.PackageState{Name: "sponsor"}) // constraint violation, also tests that Count: 0 means Count: 1
-	attendeeSent.Birthday = "2004-11-23"                                                                  // too young
 	response := tstPerformPost("/api/rest/v1/attendees", tstRenderJson(attendeeSent), tstNoToken())
 
-	docs.Then("then the attendee is rejected with an appropriate error response")
+	docs.Then("then the attendee is rejected with an appropriate error response, the packages field was ignored, and count was interpreted correctly")
 	tstRequireErrorResponse(t, response, http.StatusBadRequest, "attendee.data.invalid", url.Values{
-		"birthday": []string{"birthday must be no earlier than 1901-01-01 and no later than 2001-08-14"},
-		"nickname": []string{"nickname field must contain at least one alphanumeric character", "nickname field must not contain more than two non-alphanumeric characters (not counting spaces)"},
 		"packages": []string{"cannot pick both sponsor2 and sponsor - constraint violated"},
 	})
 }
@@ -79,7 +94,7 @@ func TestCreateNewAttendeeInvalid_NoMandatoryPackage(t *testing.T) {
 	docs.Given("given an unauthenticated user")
 
 	docs.When("when they create a new attendee, but pick none of the at-least-one-mandatory packages")
-	attendeeSent := tstBuildValidAttendee("nav1b-")
+	attendeeSent := tstBuildValidAttendee("nav1d-")
 	tstOverridePackages(&attendeeSent, "room-none,sponsor")
 	response := tstPerformPost("/api/rest/v1/attendees", tstRenderJson(attendeeSent), tstNoToken())
 
@@ -951,6 +966,52 @@ func TestUpdateExistingAttendeeDataInvalid(t *testing.T) {
 	})
 }
 
+func TestUpdateExistingAttendeeClassicPackagesInvalid(t *testing.T) {
+	docs.Given("given the configuration for standard registration")
+	tstSetup(true, false, true)
+	defer tstShutdown()
+
+	docs.Given("given an existing attendee who is logged in")
+	location1, attendee1 := tstRegisterAttendee(t, "ua4b-")
+
+	docs.Given("given an admin")
+	token := tstValidAdminToken(t)
+
+	docs.When("when they try to update the information with invalid data")
+	changedAttendee := attendee1
+	changedAttendee.Packages = changedAttendee.Packages + ",sponsor" // constraint violation
+	changedAttendee.PackagesList = nil                               // make packages field actually count
+	response := tstPerformPut(location1, tstRenderJson(changedAttendee), token)
+
+	docs.Then("then the update is rejected with an appropriate error response, and the packages field was used")
+	tstRequireErrorResponse(t, response, http.StatusBadRequest, "attendee.data.invalid", url.Values{
+		"packages": []string{"cannot pick both sponsor2 and sponsor - constraint violated"},
+	})
+}
+
+func TestUpdateExistingAttendeePackagesListInvalid(t *testing.T) {
+	docs.Given("given the configuration for standard registration")
+	tstSetup(true, false, true)
+	defer tstShutdown()
+
+	docs.Given("given an existing attendee who is logged in")
+	location1, attendee1 := tstRegisterAttendee(t, "ua4c-")
+
+	docs.Given("given an admin")
+	token := tstValidAdminToken(t)
+
+	docs.When("when they try to update the information with invalid data")
+	changedAttendee := attendee1
+	changedAttendee.Packages = ""                                                                               // should be ignored, so let's produce a different error if used
+	changedAttendee.PackagesList = append(changedAttendee.PackagesList, attendee.PackageState{Name: "sponsor"}) // constraint violation, also tests that Count: 0 means Count: 1
+	response := tstPerformPut(location1, tstRenderJson(changedAttendee), token)
+
+	docs.Then("then the update is rejected with an appropriate error response, the packages field was ignored, and count was interpreted correctly")
+	tstRequireErrorResponse(t, response, http.StatusBadRequest, "attendee.data.invalid", url.Values{
+		"packages": []string{"cannot pick both sponsor2 and sponsor - constraint violated"},
+	})
+}
+
 func TestUpdateNonExistingAttendee(t *testing.T) {
 	docs.Given("given the configuration for standard registration")
 	tstSetup(true, false, true)

From edce44258598cfa2812471128475b98087db6dc7 Mon Sep 17 00:00:00 2001
From: Jumpy Squirrel <jsquirrel_github_9a6d@packetloss.de>
Date: Sun, 8 Dec 2024 14:29:20 +0100
Subject: [PATCH 6/6] feat(#239): cover package occurring multiple times,
 including price calculations

---
 test/acceptance/attendee_acc_test.go | 108 +++++++++++++++++++++++++++
 1 file changed, 108 insertions(+)

diff --git a/test/acceptance/attendee_acc_test.go b/test/acceptance/attendee_acc_test.go
index f6e7f17..40f457e 100644
--- a/test/acceptance/attendee_acc_test.go
+++ b/test/acceptance/attendee_acc_test.go
@@ -838,6 +838,47 @@ func TestCreateNewAttendee_ReadonlyDefaultPackageNoConstraintNotRemovable(t *tes
 	})
 }
 
+func TestCreateNewAttendee_MultiPackage(t *testing.T) {
+	docs.Given("given the configuration for login-only registration after normal reg is open")
+	tstSetup(true, false, true)
+	defer tstShutdown()
+
+	docs.Given("given a logged in user")
+	token := tstValidUserToken(t, 1)
+
+	docs.When("when they attempt to create a new attendee, validly adding a package multiple times")
+	attendeeSent := tstBuildValidAttendee("na66-")
+	tstAddPackages(&attendeeSent, "mountain-trip,mountain-trip")
+	response := tstPerformPost("/api/rest/v1/attendees", tstRenderJson(attendeeSent), token)
+
+	docs.Then("then the attendee is successfully created and can be read again")
+	require.Equal(t, http.StatusCreated, response.status, "unexpected http response status")
+	require.Regexp(t, "^\\/api\\/rest\\/v1\\/attendees\\/[1-9][0-9]*$", response.location, "invalid location header in response")
+	attendeeReadAgain := tstReadAttendee(t, response.location)
+	attendeeSent.Id = attendeeReadAgain.Id // mask expected diff in id
+	require.EqualValues(t, attendeeSent, attendeeReadAgain, "attendee data read did not match updated data")
+}
+
+func TestCreateNewAttendee_MultiPackageTooMany(t *testing.T) {
+	docs.Given("given the configuration for login-only registration after normal reg is open")
+	tstSetup(true, false, true)
+	defer tstShutdown()
+
+	docs.Given("given a logged in user")
+	token := tstValidUserToken(t, 1)
+
+	docs.When("when they attempt to create a new attendee, adding a package too many times")
+	attendeeSent := tstBuildValidAttendee("na67-")
+	tstAddPackages(&attendeeSent, "mountain-trip,mountain-trip,mountain-trip")
+	response := tstPerformPost("/api/rest/v1/attendees", tstRenderJson(attendeeSent), token)
+
+	docs.Then("then the attempt is rejected as invalid (400) with an appropriate error response")
+	tstRequireErrorResponse(t, response, http.StatusBadRequest, "attendee.data.invalid", url.Values{
+		"packages":      []string{"package mountain-trip occurs too many times, can occur at most 2 times"},
+		"packages_list": []string{"package mountain-trip occurs too many times, can occur at most 2 times"},
+	})
+}
+
 // --- update attendee ---
 
 func TestUpdateExistingAttendee_Self(t *testing.T) {
@@ -1491,6 +1532,73 @@ func TestUpdateExistingAttendee_AddOnePackage_AnyTime_AdminWithSuppressEmail(t *
 	}
 }
 
+func tstUpdateExistingAttendee_AddPackageMultipleTimes(t *testing.T, testcase string, origStatus status.Status, token string, expectedMails []mailservice.MailSendDto) {
+	docs.Given("given the configuration for standard registration")
+	tstSetup(true, false, true)
+	defer tstShutdown()
+
+	docs.Given(fmt.Sprintf("given an existing attendee in status %s", origStatus))
+	loc, att := tstRegisterAttendeeAndTransitionToStatus(t, testcase, origStatus)
+
+	docs.When("when they send updated attendee info and add a package multiple times")
+	changedAttendee := att
+	tstAddPackages(&changedAttendee, "mountain-trip,mountain-trip")
+	response := tstPerformPut(loc, tstRenderJson(changedAttendee), token)
+
+	docs.Then("then the attendee is successfully updated and can be read again")
+	require.Equal(t, http.StatusOK, response.status, "unexpected http response status for update")
+	attendeeReadAgain := tstReadAttendee(t, loc)
+	require.EqualValues(t, changedAttendee, attendeeReadAgain, "attendee data read did not match")
+	require.EqualValues(t, "attendance,mountain-trip,mountain-trip,room-none,sponsor2,stage", attendeeReadAgain.Packages, "attendee data read did not match expected package value")
+
+	docs.Then("and the expected mail messages have been sent")
+	tstRequireMailRequests(t, expectedMails)
+}
+
+func TestUpdateExistingAttendee_AddPackageMultipleTimes_UserAllowed(t *testing.T) {
+	for i, origStatus := range []status.Status{status.New, status.Approved, status.PartiallyPaid, status.Paid, status.CheckedIn, status.Cancelled} {
+		testcase := fmt.Sprintf("ua3d-%d", i+1)
+		token := tstValidStaffToken(t, 1) // user who registered, staff makes no difference
+		targetStatus := origStatus
+		mails := []mailservice.MailSendDto{}
+		if origStatus == status.Approved {
+			mail := tstNewStatusMailWithAmounts(testcase, targetStatus, 315.00, 315.00)
+			mails = append(mails, mail)
+		} else if origStatus == status.PartiallyPaid {
+			mail := tstNewStatusMailWithAmounts(testcase, targetStatus, 160.00, 315.00)
+			mails = append(mails, mail)
+		} else if origStatus == status.Paid {
+			targetStatus = status.PartiallyPaid
+			mail := tstNewStatusMailWithAmounts(testcase, targetStatus, 60.00, 315.00)
+			mails = append(mails, mail)
+		}
+		t.Run(string(origStatus), func(t *testing.T) {
+			tstUpdateExistingAttendee_AddPackageMultipleTimes(t, testcase, origStatus, token, mails)
+		})
+	}
+}
+
+func TestUpdateExistingAttendee_AddPackageTooManyTimes(t *testing.T) {
+	docs.Given("given the configuration for standard registration")
+	tstSetup(true, false, true)
+	defer tstShutdown()
+
+	docs.Given("given an existing attendee in status paid")
+	loc, att := tstRegisterAttendeeAndTransitionToStatus(t, "ua3e", status.Paid)
+	token := tstValidUserToken(t, 1)
+
+	docs.When("when they send updated attendee info and try to add a package too many times")
+	changedAttendee := att
+	tstAddPackages(&changedAttendee, "mountain-trip,mountain-trip,mountain-trip")
+	changedAttendee.Packages = att.Packages // should be ignored, so let's make it produce no error if used
+	response := tstPerformPut(loc, tstRenderJson(changedAttendee), token)
+
+	docs.Then("then the request fails with the expected error")
+	tstRequireErrorResponse(t, response, http.StatusBadRequest, "attendee.data.invalid", url.Values{
+		"packages_list": []string{"package mountain-trip occurs too many times, can occur at most 2 times"},
+	})
+}
+
 // --- get attendee ---
 
 func TestDenyReadExistingAttendeeWhileNotLoggedIn(t *testing.T) {