Merge remote-tracking branch 'refs/remotes/origin/UG-12Nov' into UG-1…

…2Nov

docs/DeveloperGuide.md

@@ -159,28 +159,87 @@ Classes used by multiple components are in the `seedu.addressbook.commons` packa
 
 This section describes some noteworthy details on how certain features are implemented.
 
-### Add Recruiter and Recruiter-Organization Link Feature
-The `Recruiter` class is a `Contact` that can have a link to zero or one `Organization` that exists within the `AddressBook`.
+### `Recruiter`-`Organization` link
 
-As it extends the `Contact` class, it inherits all the fields present in `Contact` and also accepts an additional `Organization`. The accepted organization represents an immutable link between the recruiter and the given organization.
+#### Overview
+
+There are two types of contacts in Jobby - `Recruiter` and `Organization`.
+
+Each recruiter can only be linked to zero or one organization while an organization can be linked to multiple recruiters. This association can be represented via a **parent-child** relationship where the parent (`Organization`) is linked to multiple children (`Recruiter`).
+
+#### Implementing the parent-child relationship
+
+For the `Contact` class:
+  * In order to incorporate this relationship into the existing model, the `Contact` class was modified to accept another `Contact` as its parent, accessible through `Contact#getParent()`.
+
+<br>
+
+For the `Recruiter` class:
+  * Since the `Contact` class now accepts another `Contact` as its parent, the `Recruiter` can pass in an existing `Organization` to set it as its parent.
 
-To represent this relationship within the existing Model, `Contact` was modified to accept another `Contact` which serves as its parent. The parent contact can be retrieved via `Contact#getParent()`.
+  * The parent `Organization` can be retrieved via `Recruiter#getOrganization()` which returns an Optional that contains the `Organization` or an empty Optional if the `Recruiter` is not linked to any.
 
-As this represents a Contact-Contact relationship, `Recruiter` overrides the getter method and cast the parent contact to an `Organization` so that it better represents the Recruiter-Organization relationship.
+<br>
 
-To store this relationship in a JSON format, the unique `Id` of the linked organization is stored as an oid attribute on the recruiter. Hence, when the application is launched again, the `oid` is used to identify the organization in the `AddressBook`. This means that all organizations must be parsed and added to the `AddressBook` before `Storage` parses the JSON data for the recruiters. Thus, each time `AddressBook` is written to storage, it is sorted, placing all organizations before recruiters. 
+For the `Organization` class:
+  * The organization does not maintain a direct list of recruiters linked to it.
+
+  * Instead, it is retrieved via `Contact#getChildren(Model model)` where each contact in the model is checked to see whether its parent matches the organization.
+
+<br>
 
 Given below is an example usage scenario and how a recruiter can be linked to an existing organization at each step.
 
-Step 1: The user launches the application. Assume that the `AddressBook` contains a single unlinked organization with the `Id` that has a value of "alex_yeoh" and no recruiters.
+**Step 1.** The user launches the application. Assume that the `AddressBook` contains a single unlinked organization that has the id _alex_yeoh_ and no recruiters.
+
+**Step 2.** The user executes `add --rec --name Ryan --oid alex_yeoh`. As the `--rec` flag is used, the `AddCommandParser` returns a `AddRecruiterCommand`. It also parses _alex_yeoh_ as the id of the organization the recruiter will be linked to and passes it into the `AddRecruiterCommand`.
+
+**Step 3.** During its execution, the `AddRecruiterCommand` will attempt to retrieve a `Contact` that has the id _alex_yeoh_ and pass it into the new `Recruiter` that will be added to the `AddressBook`. This step can be summarized with the activity diagram below:
+
+<img src="images/AddRecruiterActivityDiagram.png" width=600 />
+
+**Step 4.** Once done, the UI will add a new `ContactCard` to the bottom of the contacts list, displaying the details of the newly created `Recruiter`. The link will be displayed as a label within the `ContactCard`: _from organization (alex_yeoh)_
+
+#### Editing and deleting the linked contacts
 
-Step 2: The user executes `add --rec --name Ryan --oid alex_yeoh`. The add command parses the `--rec` flag and knows the user wishes to create a recruiter. It also parses "alex_yeoh" as the `Id of the organization the recruiter will be linked to.
+Now that the basic implementation has been discussed, the next concern is about editing and deleting the linked contacts.
 
-The `AddRecruiterCommand` will attempt to retrieve a `Contact` that has the `Id` "alex_yeoh" and pass it into the new `Recruiter` that will be added to the AddressBook
+As each field in the `Contact` is `final`, editing it would require creating a new `editedContact` and replacing the old one via `AddressBook#setContact(target, editedContact)`.
+
+When **editing** the `Organization`:
+  * As each recruiter maintains an immutable link to the object of its parent organization, editing the organization would require replacing every linked recruiter with a new recruiter that has its parent set to the edited organization.
+
+<br>
+
+When **editing** the `Recruiter`:
+  * Since the `Organization` class does not maintain a direct link to its children and dynamically retrieves them, editing its linked recruiter does not require any edits to itself.
+
+  * Changing the organization the recruiter is linked to would require the user to supply a value to the `--oid` flag when executing the `edit` command.
+
+  * If the value matches the id of an organization within the `AddressBook`, the organization retrieved via `AddressBook#getContactById(Id id)` would be used in creating the new edited recruiter.
+
+<br>
+
+The same principle applies when deleting the linked contacts without recursion. Deleting the parent organization requires replacing every recruiter linked to it, setting their parent to null while deleting its linked recruiter requires no additional replacement.
+
+#### Storing the `Recruiter`-`Organization` link
+
+Since only the recruiter stores a direct link to its parent organization, it is sufficient to store this link in the `JsonAdaptedContact` of a recruiter.
+
+As the id field can uniquely identify the organization, an additional oid field is added to the `JsonAdaptedContact` which records the id of the parent organization. 
+
+Since the organization has to be added to the `AddressBook` before any recruiters can be linked to it, the data is sorted which places any organization at the front of the list, followed by the recruiters. This is performed before writing and after reading from the json data file.
+
+#### Design Considerations
 
-Once done, the UI will display the link as a single line: `from organization (alex_yeoh)`
+**Aspect: How `Recruiter` and `Organization` are being linked**
 
-On the other hand, the Organization class can have links to multiple Recruiters. Hence, a single parent Contact can have multiple children contacts.
+  * **Alternative 1 (current choice):** `Recruiter` maintains a direct link to `Organization` while `Organization` dynamically retrieves a list of its linked `Recruiter` contacts.
+    * Pros: Adheres to AB3's immutability of contacts.
+    * Cons: Expensive to always comb through the `AddressBook` to retrieve all linked `Recruiter` contacts.
+  * **Alternative 2:** `Organization` maintains a list of linked `Recruiters` that can be changed via setter methods.
+    * Pros: Computationally less expensive and easier to deal with.
+    * Cons: Since AB3's design was implemented with immutability in mind, making part of `Organization` mutable might cause unwanted bugs or mistakes in other parts of the application. Additionally, overhauling the classes to be mutable would incur huge cost in development time. 
 
 ### Command Autocompletion
 

docs/diagrams/AddRecruiterActivityDiagram.puml

@@ -0,0 +1,27 @@
+@startuml
+skin rose
+skinparam ActivityFontSize 15
+skinparam ArrowFontSize 12
+start
+
+:AddRecruiterCommand is executed;
+
+if () then ([oid value was passed in])
+
+    if () then ([id belongs to an organization])
+        :Create new Recruiter with parent;
+    else ([else])
+        :throw CommandException;
+        stop
+    endif
+
+else ([else])
+    :Create new Recruiter with no parent;
+endif
+
+:Add Recruiter to AddressBook;
+:return CommandResult;
+
+stop
+
+@enduml

src/main/java/seedu/address/storage/JsonAdaptedContact.java

@@ -29,7 +29,7 @@
 /**
  * Jackson-friendly version of {@link Contact}.
  */
-class JsonAdaptedContact {
+class JsonAdaptedContact implements Comparable<JsonAdaptedContact> {
 
     public static final String MISSING_FIELD_MESSAGE_FORMAT = "Contact's %s field is missing!";
 
@@ -103,6 +103,12 @@ public JsonAdaptedContact(Contact source) {
         }
     }
 
+    /**
+     * Returns the id string stored in this {@code JsonAdaptedContact}
+     */
+    public String getId() {
+        return this.id;
+    }
 
     /**
      * Converts this Jackson-friendly adapted contact object into the model's {@code Contact} object.
@@ -196,4 +202,8 @@ public Contact toModelType(ReadOnlyAddressBook reference) throws IllegalValueExc
         }
     }
 
+    @Override
+    public int compareTo(JsonAdaptedContact o) {
+        return Type.fromString(this.type).compareTo(Type.fromString(o.type));
+    }
 }

src/main/java/seedu/address/storage/JsonSerializableAddressBook.java

@@ -1,8 +1,13 @@
 package seedu.address.storage;
 
 import java.util.ArrayList;
+import java.util.Collections;
+import java.util.Comparator;
 import java.util.List;
+import java.util.Map;
+import java.util.function.Function;
 import java.util.stream.Collectors;
+import java.util.stream.IntStream;
 
 import com.fasterxml.jackson.annotation.JsonCreator;
 import com.fasterxml.jackson.annotation.JsonProperty;
@@ -12,7 +17,6 @@
 import seedu.address.model.AddressBook;
 import seedu.address.model.ReadOnlyAddressBook;
 import seedu.address.model.contact.Contact;
-import seedu.address.model.contact.Type;
 
 /**
  * An Immutable AddressBook that is serializable to JSON format.
@@ -22,14 +26,14 @@ class JsonSerializableAddressBook {
 
     public static final String MESSAGE_DUPLICATE_CONTACT = "Contacts list contains duplicate contact(s).";
 
-    private final List<JsonAdaptedContact> persons = new ArrayList<>();
+    private final List<JsonAdaptedContact> contacts = new ArrayList<>();
 
     /**
-     * Constructs a {@code JsonSerializableAddressBook} with the given persons.
+     * Constructs a {@code JsonSerializableAddressBook} with the given contacts.
      */
     @JsonCreator
-    public JsonSerializableAddressBook(@JsonProperty("persons") List<JsonAdaptedContact> persons) {
-        this.persons.addAll(persons);
+    public JsonSerializableAddressBook(@JsonProperty("contacts") List<JsonAdaptedContact> contacts) {
+        this.contacts.addAll(contacts);
     }
 
     /**
@@ -38,18 +42,10 @@ public JsonSerializableAddressBook(@JsonProperty("persons") List<JsonAdaptedCont
      * @param source future changes to this will not affect the created {@code JsonSerializableAddressBook}.
      */
     public JsonSerializableAddressBook(ReadOnlyAddressBook source) {
-        persons.addAll(
-                source.getContactList().stream()
-                        .sorted((c1, c2) -> {
-                            // TODO: Identified tech debt: Enums have natural ordering.
-                            if (c1.getType() == Type.RECRUITER && c2.getType() == Type.ORGANIZATION) {
-                                return 1;
-                            } else if (c1.getType() == Type.ORGANIZATION && c2.getType() == Type.RECRUITER) {
-                                return -1;
-                            }
-                            return 0;
-                        })
-                .map(JsonAdaptedContact::new).collect(Collectors.toList()));
+        contacts.addAll(source.getContactList().stream()
+                .map(JsonAdaptedContact::new)
+                .sorted()
+                .collect(Collectors.toList()));
     }
 
     /**
@@ -58,14 +54,40 @@ public JsonSerializableAddressBook(ReadOnlyAddressBook source) {
      * @throws IllegalValueException if there were any data constraints violated.
      */
     public AddressBook toModelType() throws IllegalValueException {
+        // Record the original order.
+        Map<String, Integer> orderMap;
+        try {
+            orderMap = IntStream.range(0, contacts.size())
+                    .boxed()
+                    .collect(Collectors.toMap(i -> contacts.get(i).getId(), Function.identity()));
+        } catch (IllegalStateException s) {
+            // Having two duplicate keys in the order map will trigger an IllegalStateException.
+            // In this case, duplicate keys mean duplicate ids which implies duplicate contacts.
+            throw new IllegalValueException(MESSAGE_DUPLICATE_CONTACT);
+        }
+
+        Comparator<Contact> originalOrderComparator = Comparator.comparingInt(c -> orderMap.get(c.getId().value));
+
+        // Create all contacts.
+        List<JsonAdaptedContact> sortedJsonContacts = new ArrayList<>(contacts);
+        Collections.sort(sortedJsonContacts);
+        List<Contact> newContacts = new ArrayList<>();
+
         AddressBook addressBook = new AddressBook();
-        for (JsonAdaptedContact jsonAdaptedContact : persons) {
+        for (JsonAdaptedContact jsonAdaptedContact : sortedJsonContacts) {
             Contact contact = jsonAdaptedContact.toModelType(addressBook);
+            // Defensive check.
             if (addressBook.getContactById(contact.getId()) != null) {
                 throw new IllegalValueException(MESSAGE_DUPLICATE_CONTACT);
             }
             addressBook.addContact(contact);
+            newContacts.add(contact);
         }
+
+        // Add them into the book in the original order.
+        newContacts.sort(originalOrderComparator);
+        addressBook.setContacts(newContacts);
+
         return addressBook;
     }
 

src/test/data/JsonAddressBookStorageTest/invalidAndValidPersonAddressBook.json

@@ -1,12 +1,12 @@
 {
-  "persons": [ {
-    "name": "Valid Person",
+  "contacts": [ {
+    "name": "Valid Contact",
     "id": "a8dfa74c-2229-4f4f-99a5-336c25e6783e",
     "phone": "9482424",
     "email": "hans@example.com",
     "address": "4th street"
   }, {
-    "name": "Person With Invalid Phone Field",
+    "name": "Contact With Invalid Phone Field",
     "id" : "ba6e0af8-5092-47c0-baf7-18d6dc535823",
     "phone": "948asdf2424",
     "email": "hans@example.com",

src/test/data/JsonSerializableAddressBookTest/duplicateOrganizationAddressBook.json

@@ -1,5 +1,5 @@
 {
-  "persons": [ {
+  "contacts": [ {
     "name": "Alice Pauline",
     "type": "organization",
     "id": "a8dfa74c-2229-4f4f-99a5-336c25e6783e",

src/test/data/JsonSerializableAddressBookTest/duplicateRecruiterAddressBook.json

@@ -1,5 +1,5 @@
 {
-  "persons": [ {
+  "contacts": [ {
     "name": "Alice Pauline",
     "type": "recruiter",
     "id": "a8dfa74c-2229-4f4f-99a5-336c25e6783e",

src/test/data/JsonSerializableAddressBookTest/invalidPersonAddressBook.json

@@ -1,5 +1,5 @@
 {
-  "persons": [ {
+  "contacts": [ {
     "name": "Hans Muster",
     "id": "a8dfa74c-2229-4f4f-99a5-336c25e6783e",
     "phone": "9482424",

src/test/data/JsonSerializableAddressBookTest/typicalPersonsAddressBook.json

@@ -1,6 +1,6 @@
 {
   "_comment": "AddressBook save file which contains the same Contact values as in TypicalContacts#getTypicalAddressBook()",
-  "persons" : [ {
+  "contacts" : [ {
     "name" : "NUS SoC",
     "type": "organization",
     "id" : "nus-soc_sg",