Merge branch 'master' into 12894-per-recipient-stats-calculated-by-name

build.gradle

@@ -101,7 +101,9 @@ dependencies {
         exclude group: "org.apache.jmeter", module: "bom"
     }
 
+    liquibaseRuntime("org.liquibase:liquibase-core:4.19.0")
     liquibaseRuntime("info.picocli:picocli:4.7.1")
+    liquibaseRuntime("org.postgresql:postgresql:42.7.2")
     liquibaseRuntime(sourceSets.main.output)
 }
 
@@ -136,6 +138,10 @@ sourceSets {
     }
 }
 
+if (!project.hasProperty("runList")) {
+    project.ext.set("runList", "main")
+}
+
 liquibase {
     activities {
         main {
@@ -145,7 +151,23 @@ liquibase {
             username project.properties['liquibaseUsername']
             password project.properties['liquibasePassword']
         }
+        snapshot {
+            url project.properties['liquibaseDbUrl']
+            username project.properties['liquibaseUsername']
+            password project.properties['liquibasePassword']
+            snapshotFormat "json"
+            outputFile "liquibase-snapshot.json"
+        }
+        diffMain {
+            searchPath "${projectDir}"
+            changeLogFile "src/main/resources/db/changelog/db.changelog-new.xml"
+            referenceUrl  project.properties['liquibaseDbUrl']
+            referenceUsername project.properties['liquibaseUsername']
+            referencePassword project.properties['liquibasePassword']
+            url "offline:postgresql?snapshot=liquibase-snapshot.json"
+        }
     }
+    runList = project.ext.runList
 }
 
 tasks.withType(cz.habarta.typescript.generator.gradle.GenerateTask) {

docs/_markbind/layouts/default.md

@@ -27,6 +27,7 @@
   * [Captcha]({{ baseUrl }}/captcha.html)
   * [Documentation]({{ baseUrl }}/documentation.html)
   * [Emails]({{ baseUrl }}/emails.html)
+  * [Unit Testing]({{ baseUrl }}/unit-testing.html)
   * [End-to-End Testing]({{ baseUrl }}/e2e-testing.html)
   * [Performance Testing]({{ baseUrl }}/performance-testing.html)
   * [Accessibility Testing]({{ baseUrl }}/axe-testing.html)

docs/development.md

@@ -291,7 +291,9 @@ There are two big categories of testing in TEAMMATES:
 - **Component tests**: white-box unit and integration tests, i.e. they test the application components with full knowledge of the components' internal workings. This is configured in `src/test/resources/testng-component.xml` (back-end) and `src/web/jest.config.js` (front-end).
 - **E2E (end-to-end) tests**: black-box tests, i.e. they test the application as a whole without knowing any internal working. This is configured in `src/e2e/resources/testng-e2e.xml`. To learn more about E2E tests, refer to this [document](e2e-testing.md).  
 
-#### Running the tests
+<div id="running-tests">
+
+#### Running tests
 
 ##### Frontend tests
 
@@ -335,6 +337,8 @@ You can generate the coverage data with `jacocoReport` task after running tests,
 
 The report can be found in the `build/reports/jacoco/jacocoReport/` directory.
 
+</div>
+
 ## Deploying to a staging server
 
 > `Staging server` is the server instance you set up on Google App Engine for hosting the app for testing purposes.

docs/schema-migration.md

@@ -0,0 +1,34 @@
+<frontmatter>
+  title: "Schema Migration"
+</frontmatter>
+
+# SQL Schema Migration
+
+Teammates uses _[Liquibase]_(https://docs.liquibase.com/start/home.html), a database schema change management solution that enables developers to revise and release database changes to production. The maintainers in charge of releases (Release Leader) will be in charge of generating a _Liquibase_ changelog prior to each release to keep the production databases schema in sync with the code. Therefore this section is just for documentation purposes for contributors.
+
+## Liquibase in Teammates
+_Liquibase_ is made available using the [gradle plugin](https://github.com/liquibase/liquibase-gradle-plugin), providing _liquibase_ functions as tasks. Try `gradle tasks | grep "liquibase"` to see all the tasks available. In teammates, change logs (more in the next section) are written in _XML_.
+
+### Liquibase connection
+Amend the `liquibaseDbUrl`, `liquibaseUsername` and `liquibasePassword` in `gradle.properties` to allow the _Liquibase_ plugin to connect your database.
+
+## Change logs, change sets and change types
+A _change log_ is a file that contains a series of _change sets_ (analagous to a transaction) which applies _change types_ (actions). You can refer to this page on liquibase on the types of [change types](https://docs.liquibase.com/change-types/home.html) that can be used.
+
+## Gradle Activities for Liquibase
+Activities in Gradle are a way of specifying different variables provided by gradle to the Liquibase plugin. The argument `runList` provided by `-pRunList=<activity_name>` e.g `./gradlew liquibaseSnapshot -PrunList=snapshot` is used to specify which activity to be used for the Liquibase command. In this case the `liquibaseSnapshot` command is run using the `snapshot` activity.
+
+Here is a brief description of the activities defined for Liquibase
+1. Main: The default activity used by Liquibase commands and is used for running changelogs against a database. This is used by default if a `runList` is not defined
+2. Snapshot: Used to specify output format and name for snapshots i.e JSON
+3. diffMain: Specify the reference and the target database to generate changelog that contains operations to update reference database to the state of the target database. i.e the reference is the JSON file generated by the snapshot command, this can be replaced with a live database which is used as reference.
+
+## Generating/ Updating liquibase change logs
+1. Ensure `diff-main` activity in `build.gradle` is pointing to the latest release changelog `src/main/resources/db/changelog/db.changelog-<release_number>.xml`
+2. Delete the `postgres-data` folder to clear any old database schemas
+3. Run `git checkout <reference_branch>` and 
+4. Run the server using `./gradlew serverRun` to generate tables found on branch
+5. Generate snapshot of database by running `./gradlew liquibaseSnapshot -PrunList=snapshot`, the snapshot will be output to `liquibase-snapshot.json`
+6. Checkout your branch and repeat steps 2 and 4 to generate the tables found on your branch
+7. Run `./gradlew liquibaseDiffChangeLog -PrunList=diffMain` to generate changeLog to resolve database schema differences
+

docs/unit-testing.md

@@ -0,0 +1,206 @@
+<frontmatter>
+  title: "Unit Testing"
+</frontmatter>
+
+# Unit Testing  
+
+## What is Unit Testing?
+
+Unit testing is a testing methodology where the objective is to test components in isolation.
+
+- It aims to ensure all components of the application work as expected, assuming its dependencies are working.   
+- This is done in TEAMMATES by using mocks to simulate a component's dependencies.  
+
+Frontend Unit tests in TEAMMATES are located in `.spec.ts` files, while Backend Unit tests in TEAMMATES can be found in the package `teammates.test`.
+
+
+## Writing Unit Tests
+
+### General guidelines
+
+#### Include only relevant details in tests
+When writing unit tests, reduce the amount of noise in the code to make it easier for future developers to follow.
+
+The code below has a lot of noise in creation of the `studentModel`:
+
+```javascript
+it('displayInviteButton: should display "Send Invite" button when a student has not joined the course', () => {
+    component.studentModels = [
+      {
+        student: {
+          name: 'tester',
+          teamName: 'Team 1',
+          email: '[email protected]',
+          joinState: JoinState.NOT_JOINED,
+          sectionName: 'Tutorial Group 1',
+          courseId: 'text-exa.demo',
+        },
+        isAllowedToViewStudentInSection: true,
+        isAllowedToModifyStudent: true,
+      },
+    ];
+
+    expect(sendInviteButton).toBeTruthy();
+});
+```
+
+However, what is important is only the student joinState. We should thus reduce the noise by including only the relevant details:
+
+```javascript
+it('displayInviteButton: should display "Send Invite" button when a student has not joined the course', () => {
+    component.studentModels = [
+      studentModelBuilder
+        .joinState(JoinState.NOT_JOINED)
+        .build()
+    ];
+
+    expect(sendInviteButton).toBeTruthy();
+});
+```
+
+Including only the relevant details in tests makes it easier for future developers to read and understand the purpose of the test.
+
+#### Favor readability over uniqueness
+Since tests don't have tests, it should be easy for developers to manually inspect them for correctness, even at the expense of greater code duplication.
+
+Take the following test for example:
+
+```java
+@BeforeMethod
+public void setUp() {
+    users = new User[]{new User("alice"), new User("bob")};
+}
+
+@Test
+public void test_register_canRegisterMultipleUsers() {
+    registerAllUsers();
+    for (User user : users) {
+        assertTrue(forum.hasRegisteredUser(user));
+    }
+}
+
+private void registerAllUsers() {
+    for (User user : users) {
+        forum.register(user);
+    }
+}
+```
+
+While the code reduces duplication, it is not as straightforward for a developer to follow.
+
+A more readable way to write this test would be:
+```java
+@Test
+public void test_register_canRegisterMultipleUsers() {
+    User user1 = new User("alice");
+    User user2 = new User("bob");
+
+    forum.register(user1);
+    forum.register(user2);
+
+    assertTrue(forum.hasRegisteredUser(user1));
+    assertTrue(forum.hasRegisteredUser(user2));
+}
+```
+
+By choosing readability over uniqueness in writing unit tests, there is code duplication, but the test flow is easier for a reader to follow.
+
+
+#### Inline mocks in test code
+
+Inlining mock return values in the unit test itself improves readability:
+
+```javascript
+it('getStudentCourseJoinStatus: should return true if student has joined the course' , () => {
+    jest.spyOn(courseService, 'getJoinCourseStatus')
+        .mockReturnValue(of({ hasJoined: true }));
+
+    expect(student.getJoinCourseStatus).toBeTruthy();
+});
+```
+
+By injecting the values in the test right before they are used, developers are able to more easily trace the code and understand the test.
+
+### Frontend
+
+#### Naming
+Unit tests for a function should follow the format:
+
+`"<function-name>: should ... when/if ..."`
+
+Example:
+
+```javascript
+  it('hasSection: should return false when there are no sections in the course')
+```
+
+#### Creating test data
+To aid with [including only relevant details in tests](#include-only-relevant-details-in-tests), use the builder in `src/web/test-helpers/generic-builder.ts`
+
+Usage:
+```javascript
+const instructorModelBuilder = createBuilder<InstructorListInfoTableRowModel>({
+    email: '[email protected]',
+    name: 'Instructor',
+    hasSubmittedSession: false,
+    isSelected: false,
+});
+
+it('isAllInstructorsSelected: should return false if at least one instructor !isSelected', () => {
+component.instructorListInfoTableRowModels = [
+  instructorModelBuilder.isSelected(true).build(),
+  instructorModelBuilder.isSelected(false).build(),
+  instructorModelBuilder.isSelected(true).build(),
+];
+
+expect(component.isAllInstructorsSelected).toBeFalsy();
+});
+
+```
+
+#### Testing event emission
+In Angular, child components emit events. To test for event emissions, we've provided a utility function in `src/test-helpers/test-event-emitter`
+
+Usage:
+```javascript
+@Output()
+deleteCommentEvent: EventEmitter<number> = new EventEmitter();
+
+triggerDeleteCommentEvent(index: number): void {
+  this.deleteCommentEvent.emit(index);
+}
+
+it('triggerDeleteCommentEvent: should emit the correct index to deleteCommentEvent', () => {
+  let emittedIndex: number | undefined;
+  testEventEmission(component.deleteCommentEvent, (index) => { emittedIndex = index; });
+
+  component.triggerDeleteCommentEvent(5);
+  expect(emittedIndex).toBe(5);
+});
+```
+
+### Backend
+
+#### Naming
+Unit test names should follow the format: `test<functionName>_<scenario>_<outcome>`
+
+Examples:
+```java
+public void testGetComment_commentDoesNotExist_returnsNull()
+public void testCreateComment_commentDoesNotExist_success()
+public void testCreateComment_commentAlreadyExists_throwsEntityAlreadyExistsException()
+```
+
+#### Creating test data
+To aid with [including only relevant details in tests](#include-only-relevant-details-in-tests), use the `getTypicalX` functions in `BaseTestCase`, where X represents an entity.
+
+Example:
+```java
+Account account = getTypicalAccount();
+account.setEmail("[email protected]");
+
+Student student = getTypicalStudent();
+student.setName("New Student Name");
+```
+
+<include src="development.md#running-tests" />

src/e2e/java/teammates/e2e/cases/AdminSearchPageE2ETest.java

@@ -31,7 +31,7 @@ protected void prepareTestData() {
         putDocuments(testData);
         sqlTestData = loadSqlDataBundle("/AdminSearchPageE2ETest_SqlEntities.json");
         removeAndRestoreSqlDataBundle(sqlTestData);
-        doPutDocumentsSql(sqlTestData);
+        putSqlDocuments(sqlTestData);
     }
 
     @Test

src/e2e/java/teammates/e2e/cases/axe/AdminSearchPageAxeTest.java

@@ -25,7 +25,7 @@ protected void prepareTestData() {
         putDocuments(testData);
         sqlTestData = loadSqlDataBundle("/AdminSearchPageE2ETest_SqlEntities.json");
         removeAndRestoreSqlDataBundle(sqlTestData);
-        doPutDocumentsSql(sqlTestData);
+        putSqlDocuments(sqlTestData);
     }
 
     @Test