Add catalog validations

packages/server/src/core/bank_rule/mod.rs

@@ -31,8 +31,8 @@ pub struct BankRuleHandler<'a> {
 }
 
 impl<'a> BankRuleHandler<'a> {
-    // This function sets the type of the course, and adds its credit to sum_credit if the user passed the course.
-    // Returns true if the credit have been added, false otherwise.
+    // set the type of the course, and add its credit to sum_credit if the user passed the course.
+    // returns true if the credit have been added, false otherwise.
     pub fn set_type_and_add_credit(
         course_status: &mut CourseStatus,
         bank_name: String,

packages/server/src/core/bank_rule/specialization_groups.rs

@@ -188,8 +188,7 @@ fn run_exhaustive_search(
         &courses,
         &mut best_match,
     )
-    .or(Some(best_match))
-    .unwrap() // unwraping is safe since the line above always returns Some(_)
+    .unwrap_or(best_match)
 }
 
 impl<'a> BankRuleHandler<'a> {

packages/server/src/core/bank_rule/tests.rs

@@ -584,7 +584,7 @@ async fn test_specialization_group() {
 
     // ---------------------------------------------------------------------------
     // change the state of 044202, which is a mandatory course in "תורת התקשורת", to notComplete,
-    // thus the user doen't complete the specialization groups requirement
+    // thus the user doesn't complete the specialization groups requirement
     degree_status.course_statuses[1].state = Some(CourseState::NotComplete);
     degree_status.course_statuses[1].grade = Some(Grade::Numeric(50));
 

packages/server/src/core/catalog_validations.rs

@@ -0,0 +1,8 @@
+use crate::{error::AppError, resources::catalog::Catalog};
+
+use super::credit_transfer_graph::validate_acyclic_credit_transfer_graph;
+
+pub fn validate_catalog(catalog: &Catalog) -> Result<(), AppError> {
+    validate_acyclic_credit_transfer_graph(catalog)?;
+    Ok(())
+}

packages/server/src/core/credit_transfer_graph.rs

@@ -0,0 +1,66 @@
+use petgraph::algo::toposort;
+use petgraph::Graph;
+
+use crate::error::AppError;
+use crate::resources::catalog::Catalog;
+use crate::resources::course::CourseBank;
+
+use super::messages;
+use super::types::CreditOverflow;
+
+fn build_credit_transfer_graph(
+    course_banks: &[CourseBank],
+    credit_overflow_rules: &[CreditOverflow],
+) -> Result<Graph<String, ()>, AppError> {
+    let mut g = Graph::<String, ()>::new();
+    for course_bank in course_banks {
+        g.add_node(course_bank.name.clone());
+    }
+    for credit_rule in credit_overflow_rules {
+        match (
+            g.node_indices().find(|i| g[*i] == credit_rule.from),
+            g.node_indices().find(|i| g[*i] == credit_rule.to),
+        ) {
+            (Some(from), Some(to)) => g.add_edge(from, to, ()),
+            _ => {
+                return Err(AppError::BadRequest(
+                    messages::build_credit_transfer_graph_failed(),
+                ))
+            }
+        };
+    }
+    Ok(g)
+}
+
+pub fn find_traversal_order(catalog: &Catalog) -> Vec<CourseBank> {
+    let g = match build_credit_transfer_graph(&catalog.course_banks, &catalog.credit_overflows) {
+        Ok(graph) => graph,
+        Err(_) => {
+            log::error!("corrupted catalog in the database - return empty list");
+            return vec![];
+        }
+    };
+    let order = toposort(&g, None).unwrap_or_else(|_| {
+        log::error!(
+            "corrupted catalog in the database - course banks will be set in an arbitrary order"
+        );
+        g.node_indices().into_iter().collect::<Vec<_>>()
+    });
+    let mut ordered_course_banks = Vec::<CourseBank>::new();
+    for node in order {
+        if let Some(bank) = catalog.get_course_bank_by_name(&g[node]) {
+            ordered_course_banks.push(bank.clone());
+        }
+    }
+    ordered_course_banks
+}
+
+pub fn validate_acyclic_credit_transfer_graph(catalog: &Catalog) -> Result<(), AppError> {
+    let g = build_credit_transfer_graph(&catalog.course_banks, &catalog.credit_overflows)?;
+    match toposort(&g, None) {
+        Ok(_) => Ok(()),
+        Err(e) => Err(AppError::BadRequest(
+            messages::cyclic_credit_transfer_graph(&g[e.node_id()]),
+        )),
+    }
+}

packages/server/src/core/degree_status/mod.rs

@@ -12,8 +12,6 @@ use crate::resources::{
 };
 use serde::{Deserialize, Serialize};
 
-use super::toposort;
-
 #[derive(Default, Clone, Debug, Deserialize, Serialize)]
 pub struct DegreeStatus {
     pub course_statuses: Vec<CourseStatus>,
@@ -121,7 +119,7 @@ impl DegreeStatus {
         courses: HashMap<CourseId, Course>,
         malag_courses: Vec<CourseId>,
     ) {
-        let course_banks = toposort::set_order(&catalog.course_banks, &catalog.credit_overflows);
+        let course_banks = catalog.get_bank_traversal_order();
 
         // prepare the data for degree status computation
         self.preprocess(&mut catalog);

packages/server/src/core/degree_status/preprocessing.rs

@@ -9,14 +9,17 @@ use crate::{
 use super::DegreeStatus;
 
 impl DegreeStatus {
-    fn reset(&mut self, catalog: &mut Catalog) {
-        self.course_bank_requirements.clear();
-        self.overflow_msgs.clear();
-        self.total_credit = 0.0;
+    // before running the algorithm we remove all the courses added by the algorithm in the previous run to prevent duplications.
+    // the algorithm adds courses only without semester, unmodified, incomplete and to a bank from type "All"
+    fn remove_courses_added_by_algorithm(&mut self, catalog: &mut Catalog) {
+        // get all courses from banks of type All
+        let bank_names = catalog
+            .course_banks
+            .iter()
+            .filter(|bank| bank.rule == Rule::All)
+            .map(|bank| bank.name.clone())
+            .collect::<Vec<_>>();
 
-        // before running the algorithm we remove all the courses added by the algorithm in the previous run to prevent duplication.
-        // the algorithm adds courses only without semeser, unmodified, incomplete and to a bank from type "all"
-        let bank_names = catalog.get_bank_names_by_rule(Rule::All);
         self.course_statuses.retain(|course_status| {
             if let Some(r#type) = &course_status.r#type {
                 course_status.semester.is_some()
@@ -27,8 +30,10 @@ impl DegreeStatus {
                 true
             }
         });
+    }
 
-        // remove course which were added by the user, and were tagged as irrelevant in a previous run
+    // courses which were tagged as irrelevant in a previous run, and then user added the same courses (same course id) manually, should be removed
+    fn remove_irrelevant_courses_added_by_user(&mut self) {
         let dispensable_irrelevant_courses = self
             .course_statuses
             .iter()
@@ -52,8 +57,9 @@ impl DegreeStatus {
             course_status.state != Some(CourseState::Irrelevant)
                 || !dispensable_irrelevant_courses.contains(&course_status.course.id)
         });
+    }
 
-        // clear the type for unmodified and irrelevant courses
+    fn clear_type_for_unmodified_and_irrelevant_courses(&mut self) {
         for course_status in self.course_statuses.iter_mut() {
             if !course_status.modified {
                 course_status.r#type = None;
@@ -75,13 +81,24 @@ impl DegreeStatus {
         }
     }
 
+    fn reset(&mut self, catalog: &mut Catalog) {
+        self.course_bank_requirements.clear();
+        self.overflow_msgs.clear();
+        self.total_credit = 0.0;
+
+        self.remove_courses_added_by_algorithm(catalog);
+        self.remove_irrelevant_courses_added_by_user();
+        self.clear_type_for_unmodified_and_irrelevant_courses();
+        self.remove_irrelevant_courses_from_catalog(catalog);
+    }
+
     pub fn preprocess(&mut self, catalog: &mut Catalog) {
         self.reset(catalog);
-        self.remove_irrelevant_courses_from_catalog(catalog);
+
         self.course_statuses.sort_by(|c1, c2| {
             c1.extract_semester()
                 .partial_cmp(&c2.extract_semester())
-                .unwrap() // unwrap can't fail because we compare only integers or "half integers" (0.5,1,1.5,2,2.5...)
+                .unwrap() // unwrap cannot fail because we compare only integers or "half integers" (0.5,1,1.5,2,2.5...)
         });
     }
 }

packages/server/src/core/messages.rs

@@ -1,3 +1,5 @@
+use std::fmt::Write;
+
 const ZERO: f32 = 0.0;
 const HALF: f32 = 0.5;
 const SINGLE: f32 = 1.0;
@@ -61,7 +63,7 @@ pub fn completed_chain_msg(chain: &[String]) -> String {
         if course == chain.last().unwrap() {
             msg += course;
         } else {
-            msg += &format!("{}, ", course);
+            let _ = write!(msg, "{}, ", course);
         }
     }
     msg
@@ -79,7 +81,7 @@ pub fn completed_specialization_groups_msg(groups: &[String], needed: usize) ->
         if group == groups.last().unwrap() {
             msg += group;
         } else {
-            msg += &format!("{}, ", group);
+            let _ = write!(msg, "{}, ", group);
         }
     }
     msg
@@ -100,3 +102,18 @@ pub fn credit_leftovers_msg(credit: f32) -> String {
 pub fn cannot_find_course() -> String {
     "שגיאה - קורס לא נמצא".to_string()
 }
+
+/////////////////////////////////////////////////////////////////////////////////
+/// Error messages
+/////////////////////////////////////////////////////////////////////////////////
+
+pub fn cyclic_credit_transfer_graph(bank_in_cycle: &str) -> String {
+    format!(
+        "קיימת תלות מעגלית במעברי הנקודות שנקבעו. התלות המעגלית מתחילה ונגמרת ב{}",
+        bank_in_cycle
+    )
+}
+
+pub fn build_credit_transfer_graph_failed() -> String {
+    "בניית הגרף נכשלה".to_string()
+}

packages/server/src/core/mod.rs

@@ -1,10 +1,11 @@
 pub mod bank_rule;
+pub mod credit_transfer_graph;
 pub mod degree_status;
 pub mod messages;
 pub mod parser;
-pub mod toposort;
 pub mod types;
 
+pub mod catalog_validations;
 #[allow(clippy::float_cmp)]
 #[cfg(test)]
 pub mod tests;