From 4317a498f0ae49551389199681e002c8cc9639ff Mon Sep 17 00:00:00 2001
From: Tobias Bieniek <tobias@bieniek.cloud>
Date: Mon, 18 Nov 2024 15:11:45 +0100
Subject: [PATCH] models/team: Replace `NewTeam::new()` fn with `Builder`
 pattern

Having two `i32` arguments in the `new()` fn was a bit dangerous and could easily lead to someone unintentionally swapping the values. This commit changes the `NewTeam` construction to use the builder pattern with named fns instead to avoid the potential confusion.
---
 src/models/team.rs         | 37 +++++++++++--------------------------
 src/tests/mod.rs           | 12 +++++-------
 src/tests/team.rs          | 29 +++++++++++++++++------------
 src/typosquat/test_util.rs | 19 +++++++++----------
 4 files changed, 42 insertions(+), 55 deletions(-)

diff --git a/src/models/team.rs b/src/models/team.rs
index 1f00bac3519..93303b278b5 100644
--- a/src/models/team.rs
+++ b/src/models/team.rs
@@ -1,3 +1,4 @@
+use bon::Builder;
 use diesel_async::AsyncPgConnection;
 use http::StatusCode;
 
@@ -35,7 +36,7 @@ pub struct Team {
     pub org_id: Option<i32>,
 }
 
-#[derive(Insertable, AsChangeset, Debug)]
+#[derive(Insertable, AsChangeset, Debug, Builder)]
 #[diesel(table_name = teams, check_for_backend(diesel::pg::Pg))]
 pub struct NewTeam<'a> {
     pub login: &'a str,
@@ -46,22 +47,6 @@ pub struct NewTeam<'a> {
 }
 
 impl<'a> NewTeam<'a> {
-    pub fn new(
-        login: &'a str,
-        org_id: i32,
-        github_id: i32,
-        name: Option<&'a str>,
-        avatar: Option<&'a str>,
-    ) -> Self {
-        NewTeam {
-            login,
-            github_id,
-            name,
-            avatar,
-            org_id,
-        }
-    }
-
     pub fn create_or_update(&self, conn: &mut impl Conn) -> QueryResult<Team> {
         use diesel::insert_into;
         use diesel::RunQueryDsl;
@@ -174,15 +159,15 @@ impl Team {
 
         let org = Handle::current().block_on(app.github.org_by_name(org_name, &token))?;
 
-        NewTeam::new(
-            &login.to_lowercase(),
-            org_id,
-            team.id,
-            team.name.as_deref(),
-            org.avatar_url.as_deref(),
-        )
-        .create_or_update(conn)
-        .map_err(Into::into)
+        NewTeam::builder()
+            .login(&login.to_lowercase())
+            .org_id(org_id)
+            .github_id(team.id)
+            .maybe_name(team.name.as_deref())
+            .maybe_avatar(org.avatar_url.as_deref())
+            .build()
+            .create_or_update(conn)
+            .map_err(Into::into)
     }
 
     /// Phones home to Github to ask if this User is a member of the given team.
diff --git a/src/tests/mod.rs b/src/tests/mod.rs
index 69afb419d2c..501f6d65937 100644
--- a/src/tests/mod.rs
+++ b/src/tests/mod.rs
@@ -104,13 +104,11 @@ fn new_user(login: &str) -> NewUser<'_> {
 }
 
 fn new_team(login: &str) -> NewTeam<'_> {
-    NewTeam {
-        org_id: next_gh_id(),
-        github_id: next_gh_id(),
-        login,
-        name: None,
-        avatar: None,
-    }
+    NewTeam::builder()
+        .login(login)
+        .org_id(next_gh_id())
+        .github_id(next_gh_id())
+        .build()
 }
 
 fn add_team_to_crate(
diff --git a/src/tests/team.rs b/src/tests/team.rs
index ea8a1cea5da..890bdebde4e 100644
--- a/src/tests/team.rs
+++ b/src/tests/team.rs
@@ -95,15 +95,16 @@ async fn add_renamed_team() {
 
     // create team with same ID and different name compared to http mock
     // used for `async_add_named_owner`.await
-    NewTeam::new(
-        "github:test-org:old-core", // different team name
-        1000,                       // same org ID
-        2001,                       // same team id as `core` team
-        None,
-        None,
-    )
-    .create_or_update(&mut conn)
-    .unwrap();
+    let new_team = NewTeam::builder()
+        // different team name
+        .login("github:test-org:old-core")
+        // same org ID
+        .org_id(1000)
+        // same team id as `core` team
+        .github_id(2001)
+        .build();
+
+    new_team.create_or_update(&mut conn).unwrap();
 
     assert_eq!(
         teams::table.count().get_result::<i64>(&mut conn).unwrap(),
@@ -437,9 +438,13 @@ async fn crates_by_team_id_not_including_deleted_owners() {
     let user = app.db_new_user("user-all-teams").await;
     let user = user.as_model();
 
-    let t = NewTeam::new("github:test-org:core", 1000, 2001, None, None)
-        .create_or_update(&mut conn)
-        .unwrap();
+    let new_team = NewTeam::builder()
+        .login("github:test-org:core")
+        .org_id(1000)
+        .github_id(2001)
+        .build();
+
+    let t = new_team.create_or_update(&mut conn).unwrap();
 
     let krate = CrateBuilder::new("foo", user.id).expect_build(&mut conn);
     add_team_to_crate(&t, &krate, user, &mut conn).unwrap();
diff --git a/src/typosquat/test_util.rs b/src/typosquat/test_util.rs
index 23cf9b21b19..591aeee449e 100644
--- a/src/typosquat/test_util.rs
+++ b/src/typosquat/test_util.rs
@@ -48,16 +48,15 @@ pub mod faker {
     }
 
     pub fn team(conn: &mut PgConnection, org: &str, team: &str) -> anyhow::Result<Owner> {
-        Ok(Owner::Team(
-            NewTeam::new(
-                &format!("github:{org}:{team}"),
-                next_gh_id(),
-                next_gh_id(),
-                Some(team),
-                None,
-            )
-            .create_or_update(conn)?,
-        ))
+        let login = format!("github:{org}:{team}");
+        let team = NewTeam::builder()
+            .login(&login)
+            .org_id(next_gh_id())
+            .github_id(next_gh_id())
+            .name(team)
+            .build();
+
+        Ok(Owner::Team(team.create_or_update(conn)?))
     }
 
     pub fn user(conn: &mut PgConnection, login: &str) -> QueryResult<User> {