Skip to content

Commit

Permalink
adjust tests to the new behaviour
Browse files Browse the repository at this point in the history
  • Loading branch information
@TicClick
TicClick committed Mar 3, 2024
1 parent 7a3f29c commit e393aee
Show file tree
Hide file tree
Showing 3 changed files with 193 additions and 142 deletions.
151 changes: 89 additions & 62 deletions src/controller/controller_impl.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -224,32 +224,102 @@ impl Controller {
}

/// Purge a pull request from memory, excluding it from conflict detection.
/// If a request contains original articles and has just been merged, send notifications to pull requests with translations.
/// If a request contains original articles and has just been merged, send notifications to pull requests with translations
/// (https://github.com/TicClick/observatory/issues/12 has the rationale).
///
/// This should be done only when a pull request is closed or merged.
async fn finalize_pull(&self, full_repo_name: &str, closed_pull: PullRequest) {
// https://github.com/TicClick/observatory/issues/12
if closed_pull.merged {
let conflicts = self
.conflicts
.by_original(full_repo_name, closed_pull.number);
let mut pending_updates: HashMap<i32, Vec<conflicts::Conflict>> = HashMap::new();
for c in conflicts
.into_iter()
.filter(|c| matches!(c.kind, ConflictType::IncompleteTranslation))
{
pending_updates.entry(c.trigger).or_default().push(c);
if let Some(pulls_map) = self.memory.pulls(full_repo_name) {
let (pending_updates, conflicts_to_remove) = self
.refresh_conflicts(
full_repo_name,
pulls_map,
&closed_pull,
ConflictType::IncompleteTranslation,
)
.await;
if !pending_updates.is_empty() {
let _ = self
.send_updates(pending_updates, conflicts_to_remove, full_repo_name)
.await;
}
}
let _ = self
.send_updates(pending_updates, HashMap::new(), full_repo_name)
.await;
}

self.memory.remove_pull(full_repo_name, &closed_pull);
self.conflicts
.remove_conflicts_by_pull(full_repo_name, closed_pull.number);
}

/// Compare the new pull with existing ones for conflicts:
/// - Known conflicts (same kind + same file set) are skipped, otherwise memory is updated.
/// - Conflicts that don't occur anymore are removed from cache, with subsequent comment removal.
async fn refresh_conflicts(
&self,
full_repo_name: &str,
pulls_map: HashMap<i32, PullRequest>,
new_pull: &PullRequest,
kind_to_match: ConflictType,
) -> (
HashMap<i32, Vec<conflicts::Conflict>>,
HashMap<i32, Vec<conflicts::Conflict>>,
) {
let mut pulls: Vec<PullRequest> = pulls_map
.into_values()
.filter(|other| other.number != new_pull.number)
.collect();
pulls.sort_by_key(|pr| pr.created_at);

let mut pending_updates: HashMap<i32, Vec<conflicts::Conflict>> = HashMap::new();
let mut conflicts_to_remove: HashMap<i32, Vec<conflicts::Conflict>> = HashMap::new();
for other_pull in pulls {
let conflicts = conflicts::compare_pulls(new_pull, &other_pull);

// Note: after a conflict disappears, any interfering updates to the original pull will flip the roles:
// the pull which triggered the new conflict will be considered an original. This is a scenario rare enough
// (think indecisive people bringing changes in and out), but one that we should consider and have written down.
// Also, it's simpler than maintaining a cache of "inactive" conflicts, at least for now.
// Related test: test_new_comment_is_posted_after_removal_in_different_pull

let removed_conflicts = self.conflicts.remove_missing(
full_repo_name,
other_pull.number,
new_pull.number,
&conflicts,
);

for removed in removed_conflicts {
conflicts_to_remove
.entry(removed.trigger)
.or_default()
.push(removed);
}

// This always triggers notifications for `IncompleteTranslation` conflicts -- this is intended,
// since this function is called when they're merged. `Overlap` conflicts may not require an update if their
// contents are identical.
for conflict in conflicts {
match self.conflicts.upsert(full_repo_name, &conflict.clone()) {
Some(uc) => {
if uc.kind == kind_to_match {
pending_updates.entry(uc.trigger).or_default().push(uc);
}
}
None => {
if conflict.kind == kind_to_match {
pending_updates
.entry(conflict.trigger)
.or_default()
.push(conflict);
}
}
}
}
}
(pending_updates, conflicts_to_remove)
}

async fn update_pull(
&self,
full_repo_name: &str,
Expand All @@ -273,6 +343,8 @@ impl Controller {
///
/// If `trigger_updates` is set, check if the update conflicts with existing pull requests,
/// and make its author aware (or other PRs' owners, in rare cases). For details, see [`helpers::conflicts::Storage`].
///
/// Translators are not notified on changes in original articles -- see finalize_pull() for that.
async fn upsert_pull(
&self,
full_repo_name: &str,
Expand All @@ -287,54 +359,9 @@ impl Controller {
self.memory.insert_pull(full_repo_name, new_pull.clone());

if let Some(pulls_map) = self.memory.pulls(full_repo_name) {
let mut pulls: Vec<PullRequest> = pulls_map
.into_values()
.filter(|other| other.number != new_pull.number)
.collect();
pulls.sort_by_key(|pr| pr.created_at);

// Compare the new pull with existing ones for conflicts:
// - Known conflicts (same kind + same file set) are skipped, otherwise memory is updated.
// - Conflicts that don't occur anymore are removed from cache, with subsequent comment removal.

let mut pending_updates: HashMap<i32, Vec<conflicts::Conflict>> = HashMap::new();
let mut conflicts_to_remove: HashMap<i32, Vec<conflicts::Conflict>> = HashMap::new();
for other_pull in pulls {
let conflicts = conflicts::compare_pulls(&new_pull, &other_pull);

// Note: after a conflict disappears, any interfering updates to the original pull will flip the roles:
// the pull which triggered the new conflict will be considered an original. This is a scenario rare enough
// (think indecisive people bringing changes in and out), but one that we should consider and have written down.
// Also, it's simpler than maintaining a cache of "inactive" conflicts, at least for now.
// Related test: test_new_comment_is_posted_after_removal_in_different_pull

let removed_conflicts = self.conflicts.remove_missing(
full_repo_name,
other_pull.number,
new_pull.number,
&conflicts,
);
for removed in removed_conflicts {
conflicts_to_remove
.entry(removed.trigger)
.or_default()
.push(removed);
}

for conflict in conflicts {
if let Some(updated_conflict) = self.conflicts.upsert(full_repo_name, &conflict)
{
// https://github.com/TicClick/observatory/issues/12: Don't notify translators before the original
// pull has been merged in to avoid creating extra work for them.
if !matches!(updated_conflict.kind, ConflictType::IncompleteTranslation) {
pending_updates
.entry(updated_conflict.trigger)
.or_default()
.push(updated_conflict);
}
}
}
}
let (pending_updates, conflicts_to_remove) = self
.refresh_conflicts(full_repo_name, pulls_map, &new_pull, ConflictType::Overlap)
.await;
if trigger_updates {
self.send_updates(pending_updates, conflicts_to_remove, full_repo_name)
.await?;
Expand Down
Loading

0 comments on commit e393aee

Please sign in to comment.