| | @@ -1,3 +1,5 @@ |
| 1 | +use std::time::SystemTime; |
| 2 | + |
| 1 | 3 | use crate::domain::{ |
| 2 | 4 | Actor, DomainError, OrgName, RepoId, RepoName, Repository, Visibility, |
| 3 | 5 | repository::{MembershipRepository, OrgRepository, RepoRepository}, |
| | @@ -60,6 +62,11 @@ pub async fn create_repo( |
| 60 | 62 | spec.name.clone(), |
| 61 | 63 | spec.description.clone(), |
| 62 | 64 | spec.visibility, |
| 65 | + // Read here rather than taken as an argument, unlike `touch`: nothing needs to |
| 66 | + // create a repository *as of* a stated time, and a parameter no caller ever |
| 67 | + // varies is a parameter every caller has to think about. Revisit if a clock port |
| 68 | + // appears. |
| 69 | + SystemTime::now(), |
| 63 | 70 | )?; |
| 64 | 71 | |
| 65 | 72 | if repos |
| | @@ -107,6 +114,13 @@ pub struct RepoView { |
| 107 | 114 | pub name: RepoName, |
| 108 | 115 | pub description: Option<String>, |
| 109 | 116 | pub visibility: Visibility, |
| 117 | + /// When code last landed here. The repository page dates itself from this rather |
| 118 | + /// than asking git, which costs a fork. |
| 119 | + pub updated_at: SystemTime, |
| 120 | + /// Whether this repository leads the owner's profile. Here as well as on |
| 121 | + /// [`RepoSummary`] because the settings page renders the pin control from this view |
| 122 | + /// and would otherwise have to read the row a second time to know the box's state. |
| 123 | + pub pinned: bool, |
| 110 | 124 | /// Whether the viewer may change this repository. Decided here so a page and |
| 111 | 125 | /// `/api` cannot disagree about who sees a management control. |
| 112 | 126 | pub viewer_is_owner: bool, |
| | @@ -146,6 +160,8 @@ pub async fn view_repo( |
| 146 | 160 | name: repo.name, |
| 147 | 161 | description: repo.description, |
| 148 | 162 | visibility: repo.visibility, |
| 163 | + updated_at: repo.updated_at, |
| 164 | + pinned: repo.pinned, |
| 149 | 165 | viewer_is_owner: is_org_owner(&org, actor, memberships).await?, |
| 150 | 166 | })) |
| 151 | 167 | } |
| | @@ -159,9 +175,15 @@ pub struct RepoSummary { |
| 159 | 175 | pub name: RepoName, |
| 160 | 176 | pub description: Option<String>, |
| 161 | 177 | pub visibility: Visibility, |
| 178 | + pub updated_at: SystemTime, |
| 179 | + /// Whether this is the owner's lead repository. Carried in the listing because the |
| 180 | + /// profile picks the lead *out of* the listing rather than fetching it separately — |
| 181 | + /// one query, and no chance of the lead and the list disagreeing. |
| 182 | + pub pinned: bool, |
| 162 | 183 | } |
| 163 | 184 | |
| 164 | | −/// Every repository under a handle that the viewer is allowed to see, ordered by name. |
| 185 | +/// Every repository under a handle that the viewer is allowed to see, most recently |
| 186 | +/// updated first — the order [`RepoRepository::list_by_org`] defines. |
| 165 | 187 | /// |
| 166 | 188 | /// `Ok(None)` means no such handle — distinct from `Ok(Some(vec![]))`, which means the |
| 167 | 189 | /// handle exists and the viewer can see nothing under it. A caller serving `/api` needs |
| | @@ -196,6 +218,8 @@ pub async fn list_repos( |
| 196 | 218 | name: repo.name, |
| 197 | 219 | description: repo.description, |
| 198 | 220 | visibility: repo.visibility, |
| 221 | + updated_at: repo.updated_at, |
| 222 | + pinned: repo.pinned, |
| 199 | 223 | }) |
| 200 | 224 | .collect(), |
| 201 | 225 | )) |
| | @@ -257,6 +281,9 @@ async fn changeable_repo( |
| 257 | 281 | pub struct RepoEdit { |
| 258 | 282 | pub description: Option<String>, |
| 259 | 283 | pub visibility: Visibility, |
| 284 | + /// Whether this repository should lead the owner's profile. Setting it unpins |
| 285 | + /// whichever repository held the spot — see [`update_repo`]. |
| 286 | + pub pinned: bool, |
| 260 | 287 | } |
| 261 | 288 | |
| 262 | 289 | /// Changes a repository's description and visibility. |
| | @@ -287,13 +314,39 @@ pub async fn update_repo( |
| 287 | 314 | // length rule lives in exactly one place. The stored name goes back through |
| 288 | 315 | // validation as a side effect — acceptable because it was validated on the way in |
| 289 | 316 | // and has not changed, and the alternative is a second copy of the rule here. |
| 290 | | − let updated = Repository::new( |
| 317 | + let mut updated = Repository::new( |
| 291 | 318 | existing.id, |
| 292 | | − existing.org_id, |
| 319 | + existing.org_id.clone(), |
| 293 | 320 | existing.name.as_str(), |
| 294 | 321 | edit.description.clone(), |
| 295 | 322 | edit.visibility, |
| 323 | + // The existing timestamp, not the current time. `updated_at` means "code last |
| 324 | + // landed here" and orders a portfolio by what is being worked on; rewording a |
| 325 | + // description is not work on the repository and must not jump it to the top. |
| 326 | + existing.updated_at, |
| 296 | 327 | )?; |
| 328 | + updated.pinned = edit.pinned; |
| 329 | + |
| 330 | + // **At most one pinned repository per owner**, enforced here rather than in storage. |
| 331 | + // The rule is not "refuse a second pin" but "pinning this unpins that", and a |
| 332 | + // constraint — a partial unique index would express the shape exactly — can only |
| 333 | + // refuse. Enforcing it in the use case also keeps the two `RepoRepository` |
| 334 | + // implementations from having to agree on a behaviour neither of them is asked for. |
| 335 | + // |
| 336 | + // The two writes are not one transaction, so a crash between them can leave nothing |
| 337 | + // pinned. That is the harmless direction: no lead section, rather than two. |
| 338 | + if updated.pinned { |
| 339 | + for other in repos.list_by_org(&existing.org_id).await? { |
| 340 | + if other.pinned && other.id != updated.id { |
| 341 | + repos |
| 342 | + .save(&Repository { |
| 343 | + pinned: false, |
| 344 | + ..other |
| 345 | + }) |
| 346 | + .await?; |
| 347 | + } |
| 348 | + } |
| 349 | + } |
| 297 | 350 | |
| 298 | 351 | repos.save(&updated).await?; |
| 299 | 352 | |
| | @@ -359,6 +412,10 @@ mod tests { |
| 359 | 412 | }, |
| 360 | 413 | }; |
| 361 | 414 | |
| 415 | + fn at(seconds: u64) -> SystemTime { |
| 416 | + SystemTime::UNIX_EPOCH + std::time::Duration::from_secs(seconds) |
| 417 | + } |
| 418 | + |
| 362 | 419 | /// A `RepoRepository` whose `save` always fails, for exercising compensation. |
| 363 | 420 | /// |
| 364 | 421 | /// Test-local on purpose: fault injection does not belong in the shared fake, where |
| | @@ -387,6 +444,10 @@ mod tests { |
| 387 | 444 | Err(RepositoryError::backend("save failed on purpose")) |
| 388 | 445 | } |
| 389 | 446 | |
| 447 | + async fn touch(&self, _id: &RepoId, _now: SystemTime) -> RepositoryResult<()> { |
| 448 | + Ok(()) |
| 449 | + } |
| 450 | + |
| 390 | 451 | async fn delete(&self, _id: &RepoId) -> RepositoryResult<()> { |
| 391 | 452 | Ok(()) |
| 392 | 453 | } |
| | @@ -796,6 +857,20 @@ mod tests { |
| 796 | 857 | .expect("lookup should not error") |
| 797 | 858 | } |
| 798 | 859 | |
| 860 | + /// Creates a repository and dates it, so an ordering test states its own times |
| 861 | + /// rather than depending on how fast it runs. |
| 862 | + async fn create_dated(&self, name: &str, seconds: u64) -> Repository { |
| 863 | + let repo = self.create_with(Visibility::Public, name).await; |
| 864 | + self.repos |
| 865 | + .touch(&repo.id, at(seconds)) |
| 866 | + .await |
| 867 | + .expect("touch"); |
| 868 | + Repository { |
| 869 | + updated_at: at(seconds), |
| 870 | + ..repo |
| 871 | + } |
| 872 | + } |
| 873 | + |
| 799 | 874 | async fn create_with(&self, visibility: Visibility, name: &str) -> Repository { |
| 800 | 875 | self.create( |
| 801 | 876 | &self.owner, |
| | @@ -945,12 +1020,21 @@ mod tests { |
| 945 | 1020 | } |
| 946 | 1021 | } |
| 947 | 1022 | |
| 948 | | − /// Two public and one private, created out of alphabetical order. |
| 1023 | + /// Two public and one private, dated so that recency and the alphabet disagree. |
| 1024 | + /// |
| 1025 | + /// `zebra` is the most recent and `alpha` the oldest, so a listing that came back |
| 1026 | + /// alphabetical would be visibly wrong rather than accidentally right. |
| 949 | 1027 | async fn mixed() -> Fixture { |
| 950 | 1028 | let f = fixture().await; |
| 951 | | − f.create_with(Visibility::Public, "zebra").await; |
| 952 | | − f.create_with(Visibility::Private, "secret").await; |
| 953 | | − f.create_with(Visibility::Public, "alpha").await; |
| 1029 | + f.create_dated("zebra", 3_000).await; |
| 1030 | + f.repos |
| 1031 | + .save(&Repository { |
| 1032 | + updated_at: at(2_000), |
| 1033 | + ..f.create_with(Visibility::Private, "secret").await |
| 1034 | + }) |
| 1035 | + .await |
| 1036 | + .expect("date the private repo"); |
| 1037 | + f.create_dated("alpha", 1_000).await; |
| 954 | 1038 | f |
| 955 | 1039 | } |
| 956 | 1040 | |
| | @@ -962,7 +1046,7 @@ mod tests { |
| 962 | 1046 | let listed = f.list(actor).await; |
| 963 | 1047 | assert_eq!( |
| 964 | 1048 | Fixture::names(&listed), |
| 965 | | − vec!["alpha", "zebra"], |
| 1049 | + vec!["zebra", "alpha"], |
| 966 | 1050 | "{actor:?} should see only the public repositories" |
| 967 | 1051 | ); |
| 968 | 1052 | } |
| | @@ -976,23 +1060,51 @@ mod tests { |
| 976 | 1060 | let listed = f.list(actor).await; |
| 977 | 1061 | assert_eq!( |
| 978 | 1062 | Fixture::names(&listed), |
| 979 | | − vec!["alpha", "secret", "zebra"], |
| 1063 | + vec!["zebra", "secret", "alpha"], |
| 980 | 1064 | "{actor:?} should see everything" |
| 981 | 1065 | ); |
| 982 | 1066 | } |
| 983 | 1067 | } |
| 984 | 1068 | |
| 985 | 1069 | #[tokio::test] |
| 986 | | − async fn listings_are_ordered_by_name() { |
| 987 | | − // Created zebra, secret, alpha — the order out is not the order in. |
| 1070 | + async fn listings_are_ordered_by_recency_not_by_name() { |
| 1071 | + // Changed deliberately from alphabetical: the profile is a portfolio, and |
| 1072 | + // alphabetical is a filing rule that puts `dotfiles` above the thing being built. |
| 988 | 1073 | let f = mixed().await; |
| 989 | 1074 | |
| 990 | 1075 | assert_eq!( |
| 991 | 1076 | Fixture::names(&f.list(&f.owner).await), |
| 992 | | − vec!["alpha", "secret", "zebra"] |
| 1077 | + vec!["zebra", "secret", "alpha"] |
| 1078 | + ); |
| 1079 | + } |
| 1080 | + |
| 1081 | + #[tokio::test] |
| 1082 | + async fn repositories_updated_in_the_same_second_are_ordered_by_name() { |
| 1083 | + // Every repository that predates `updated_at` shares one timestamp, so without a |
| 1084 | + // tiebreak a profile would reshuffle itself between page loads. |
| 1085 | + let f = fixture().await; |
| 1086 | + for name in ["zebra", "alpha", "middle"] { |
| 1087 | + f.create_dated(name, 1_000).await; |
| 1088 | + } |
| 1089 | + |
| 1090 | + assert_eq!( |
| 1091 | + Fixture::names(&f.list(&f.owner).await), |
| 1092 | + vec!["alpha", "middle", "zebra"] |
| 993 | 1093 | ); |
| 994 | 1094 | } |
| 995 | 1095 | |
| 1096 | + #[tokio::test] |
| 1097 | + async fn a_new_repository_is_dated_when_it_was_created() { |
| 1098 | + let f = fixture().await; |
| 1099 | + let before = SystemTime::now(); |
| 1100 | + |
| 1101 | + let repo = f.create_with(Visibility::Public, "steid").await; |
| 1102 | + |
| 1103 | + assert!(repo.updated_at >= before); |
| 1104 | + assert!(repo.updated_at <= SystemTime::now()); |
| 1105 | + assert!(!repo.pinned, "nothing leads a profile by being created"); |
| 1106 | + } |
| 1107 | + |
| 996 | 1108 | #[tokio::test] |
| 997 | 1109 | async fn a_viewer_who_may_see_nothing_gets_an_empty_list() { |
| 998 | 1110 | // Not a count, not a hint. Either would leak that private repositories exist |
| | @@ -1044,6 +1156,10 @@ mod tests { |
| 1044 | 1156 | assert_eq!(summary.name.as_str(), "steid"); |
| 1045 | 1157 | assert_eq!(summary.description.as_deref(), Some("A gitforge.")); |
| 1046 | 1158 | assert_eq!(summary.visibility, Visibility::Private); |
| 1159 | + // The profile dates each row and picks its lead out of the listing, so both |
| 1160 | + // travel with the summary rather than costing a second lookup. |
| 1161 | + assert!(!summary.pinned); |
| 1162 | + assert!(summary.updated_at <= SystemTime::now()); |
| 1047 | 1163 | } |
| 1048 | 1164 | |
| 1049 | 1165 | #[tokio::test] |
| | @@ -1061,6 +1177,7 @@ mod tests { |
| 1061 | 1177 | "theirs", |
| 1062 | 1178 | None, |
| 1063 | 1179 | Visibility::Public, |
| 1180 | + SystemTime::now(), |
| 1064 | 1181 | ) |
| 1065 | 1182 | .expect("valid repo"), |
| 1066 | 1183 | ) |
| | @@ -1089,6 +1206,7 @@ mod tests { |
| 1089 | 1206 | &RepoEdit { |
| 1090 | 1207 | description: description.map(str::to_owned), |
| 1091 | 1208 | visibility, |
| 1209 | + pinned: false, |
| 1092 | 1210 | }, |
| 1093 | 1211 | &self.orgs, |
| 1094 | 1212 | &self.memberships, |
| | @@ -1272,6 +1390,7 @@ mod tests { |
| 1272 | 1390 | &RepoEdit { |
| 1273 | 1391 | description: None, |
| 1274 | 1392 | visibility: Visibility::Public, |
| 1393 | + pinned: false, |
| 1275 | 1394 | }, |
| 1276 | 1395 | &f.orgs, |
| 1277 | 1396 | &f.memberships, |
| | @@ -1343,6 +1462,175 @@ mod tests { |
| 1343 | 1462 | ); |
| 1344 | 1463 | } |
| 1345 | 1464 | |
| 1465 | + // --- pinning --------------------------------------------------------------- |
| 1466 | + |
| 1467 | + impl Fixture { |
| 1468 | + /// Sets the pin, leaving everything else as stored. |
| 1469 | + async fn set_pin(&self, actor: &Actor, name: &str, pinned: bool) -> Result<Repository> { |
| 1470 | + let existing = self.stored(name).await.expect("the repository exists"); |
| 1471 | + |
| 1472 | + update_repo( |
| 1473 | + actor, |
| 1474 | + &self.handle, |
| 1475 | + &existing.name, |
| 1476 | + &RepoEdit { |
| 1477 | + description: existing.description.clone(), |
| 1478 | + visibility: existing.visibility, |
| 1479 | + pinned, |
| 1480 | + }, |
| 1481 | + &self.orgs, |
| 1482 | + &self.memberships, |
| 1483 | + &self.repos, |
| 1484 | + ) |
| 1485 | + .await |
| 1486 | + } |
| 1487 | + |
| 1488 | + async fn pinned_names(&self) -> Vec<String> { |
| 1489 | + self.list(&self.owner) |
| 1490 | + .await |
| 1491 | + .into_iter() |
| 1492 | + .filter(|summary| summary.pinned) |
| 1493 | + .map(|summary| summary.name.to_string()) |
| 1494 | + .collect() |
| 1495 | + } |
| 1496 | + } |
| 1497 | + |
| 1498 | + #[tokio::test] |
| 1499 | + async fn the_owner_pins_a_repository() { |
| 1500 | + let f = fixture().await; |
| 1501 | + f.create_with(Visibility::Public, "steid").await; |
| 1502 | + |
| 1503 | + let pinned = f |
| 1504 | + .set_pin(&f.owner, "steid", true) |
| 1505 | + .await |
| 1506 | + .expect("should pin"); |
| 1507 | + |
| 1508 | + assert!(pinned.pinned); |
| 1509 | + assert_eq!(f.pinned_names().await, vec!["steid".to_owned()]); |
| 1510 | + } |
| 1511 | + |
| 1512 | + #[tokio::test] |
| 1513 | + async fn pinning_a_second_repository_unpins_the_first() { |
| 1514 | + // At most one lead per owner. Two would leave the profile with no rule for |
| 1515 | + // choosing between them. |
| 1516 | + let f = fixture().await; |
| 1517 | + f.create_with(Visibility::Public, "steid").await; |
| 1518 | + f.create_with(Visibility::Public, "dotfiles").await; |
| 1519 | + f.set_pin(&f.owner, "steid", true) |
| 1520 | + .await |
| 1521 | + .expect("should pin"); |
| 1522 | + |
| 1523 | + f.set_pin(&f.owner, "dotfiles", true) |
| 1524 | + .await |
| 1525 | + .expect("should pin"); |
| 1526 | + |
| 1527 | + assert_eq!(f.pinned_names().await, vec!["dotfiles".to_owned()]); |
| 1528 | + } |
| 1529 | + |
| 1530 | + #[tokio::test] |
| 1531 | + async fn re_pinning_the_same_repository_leaves_it_pinned() { |
| 1532 | + // The unpin sweep skips the repository being saved; getting that wrong would |
| 1533 | + // make a second save of an unchanged form silently clear the pin. |
| 1534 | + let f = fixture().await; |
| 1535 | + f.create_with(Visibility::Public, "steid").await; |
| 1536 | + f.set_pin(&f.owner, "steid", true) |
| 1537 | + .await |
| 1538 | + .expect("should pin"); |
| 1539 | + |
| 1540 | + f.set_pin(&f.owner, "steid", true) |
| 1541 | + .await |
| 1542 | + .expect("should pin"); |
| 1543 | + |
| 1544 | + assert_eq!(f.pinned_names().await, vec!["steid".to_owned()]); |
| 1545 | + } |
| 1546 | + |
| 1547 | + #[tokio::test] |
| 1548 | + async fn unpinning_leaves_nothing_pinned() { |
| 1549 | + let f = fixture().await; |
| 1550 | + f.create_with(Visibility::Public, "steid").await; |
| 1551 | + f.set_pin(&f.owner, "steid", true) |
| 1552 | + .await |
| 1553 | + .expect("should pin"); |
| 1554 | + |
| 1555 | + f.set_pin(&f.owner, "steid", false) |
| 1556 | + .await |
| 1557 | + .expect("should unpin"); |
| 1558 | + |
| 1559 | + assert!(f.pinned_names().await.is_empty()); |
| 1560 | + } |
| 1561 | + |
| 1562 | + #[tokio::test] |
| 1563 | + async fn pinning_only_reaches_the_owners_own_repositories() { |
| 1564 | + // The sweep is scoped to the org. Another owner's lead is not this owner's to |
| 1565 | + // clear. |
| 1566 | + let f = fixture().await; |
| 1567 | + f.create_with(Visibility::Public, "steid").await; |
| 1568 | + |
| 1569 | + let other = Organization::new(OrgId::generate(), "other-org", None).expect("valid org"); |
| 1570 | + f.orgs.save(&other).await.expect("save org"); |
| 1571 | + let mut theirs = Repository::new( |
| 1572 | + RepoId::generate(), |
| 1573 | + other.id.clone(), |
| 1574 | + "theirs", |
| 1575 | + None, |
| 1576 | + Visibility::Public, |
| 1577 | + SystemTime::now(), |
| 1578 | + ) |
| 1579 | + .expect("valid repo"); |
| 1580 | + theirs.pinned = true; |
| 1581 | + f.repos.save(&theirs).await.expect("save repo"); |
| 1582 | + |
| 1583 | + f.set_pin(&f.owner, "steid", true) |
| 1584 | + .await |
| 1585 | + .expect("should pin"); |
| 1586 | + |
| 1587 | + assert!( |
| 1588 | + f.repos |
| 1589 | + .find_by_id(&theirs.id) |
| 1590 | + .await |
| 1591 | + .expect("lookup") |
| 1592 | + .expect("still there") |
| 1593 | + .pinned, |
| 1594 | + "another owner's lead should be untouched" |
| 1595 | + ); |
| 1596 | + } |
| 1597 | + |
| 1598 | + #[tokio::test] |
| 1599 | + async fn a_member_who_is_not_the_owner_cannot_pin() { |
| 1600 | + // Pinning is a mutation, and every repository mutation is owner-only. |
| 1601 | + let f = fixture().await; |
| 1602 | + f.create_with(Visibility::Public, "steid").await; |
| 1603 | + |
| 1604 | + for actor in [&f.member, &f.stranger, &Actor::Anonymous] { |
| 1605 | + let error = f |
| 1606 | + .set_pin(actor, "steid", true) |
| 1607 | + .await |
| 1608 | + .expect_err("should refuse"); |
| 1609 | + |
| 1610 | + assert!( |
| 1611 | + matches!(error, Error::Domain(DomainError::Forbidden)), |
| 1612 | + "{actor:?} should be forbidden, got {error:?}" |
| 1613 | + ); |
| 1614 | + } |
| 1615 | + |
| 1616 | + assert!(f.pinned_names().await.is_empty()); |
| 1617 | + } |
| 1618 | + |
| 1619 | + #[tokio::test] |
| 1620 | + async fn editing_a_repository_does_not_move_its_place_in_the_listing() { |
| 1621 | + // `updated_at` means "code last landed here". Rewording a description must not |
| 1622 | + // jump a dormant repository to the top of a portfolio. |
| 1623 | + let f = fixture().await; |
| 1624 | + let repo = f.create_dated("steid", 1_000).await; |
| 1625 | + |
| 1626 | + let updated = f |
| 1627 | + .update(&f.owner, "steid", Some("A gitforge."), Visibility::Public) |
| 1628 | + .await |
| 1629 | + .expect("should update"); |
| 1630 | + |
| 1631 | + assert_eq!(updated.updated_at, repo.updated_at); |
| 1632 | + } |
| 1633 | + |
| 1346 | 1634 | // --- delete_repo ----------------------------------------------------------- |
| 1347 | 1635 | |
| 1348 | 1636 | /// Git storage whose `remove` always fails, for the best-effort path. |