diff --git a/.github/workflows/deploy.yml b/.github/workflows/deploy.yml deleted file mode 100644 index 33267480..00000000 --- a/.github/workflows/deploy.yml +++ /dev/null @@ -1,27 +0,0 @@ -name: Deploy to Wiki -on: - push: - branches: - - develop - workflow_dispatch: - -jobs: - deploy: - runs-on: ubuntu-22.04 - steps: - - name: Retrieve authentication token - id: get-token - # SHA1 hash of the release-v0.0.1 commit. - uses: oppia/get-github-app-token@8c3b19db0cdcd0f7fded7dd71e5e0429bf72df1a - with: - app_id: ${{ secrets.OPPIA_WIKI_SYNCHRONIZER_APP_ID }} - private_key: ${{ secrets.OPPIA_WIKI_SYNCHRONIZER_APP_PRIVATE_KEY }} - - uses: actions/checkout@v3 - with: - token: ${{ steps.get-token.outputs.token }} - fetch-depth: 0 - - name: Add remote - run: git remote add deployment https://github.com/${{github.repository_owner}}/oppia.wiki.git - - name: Deploy - run: | - git push deployment develop:master diff --git a/.github/workflows/linter.yml b/.github/workflows/linter.yml deleted file mode 100644 index 83fa06bf..00000000 --- a/.github/workflows/linter.yml +++ /dev/null @@ -1,30 +0,0 @@ -name: Lint Check - -on: - push: - branches: - - develop - pull_request: - branches: - - develop - -jobs: - markdown_linter: - name: Run markdown linter - runs-on: ubuntu-latest - steps: - - uses: actions/checkout@v3 - - uses: actions/setup-python@v3 - - name: Lint markdown files - run: | - python scripts/linter.py - pylint_linter: - name: Run pylint linter - runs-on: ubuntu-latest - steps: - - uses: actions/checkout@v3 - - uses: actions/setup-python@v3 - - name: Lint python files - run: | - pip install pylint - pylint **/*.py diff --git a/.github/workflows/mypy_check.yml b/.github/workflows/mypy_check.yml deleted file mode 100644 index 8d41c984..00000000 --- a/.github/workflows/mypy_check.yml +++ /dev/null @@ -1,20 +0,0 @@ -name: "mypy check" -on: - push: - branches: - - develop - pull_request: - branches: - - develop -jobs: - - static-type-check: - runs-on: ubuntu-latest - steps: - - uses: actions/checkout@v2 - - uses: actions/setup-python@v3 - with: - python-version: '3.10' - - run: pip install mypy - - name: Run mypy check on all files - run: mypy --strict . diff --git a/.gitignore b/.gitignore deleted file mode 100644 index e43b0f98..00000000 --- a/.gitignore +++ /dev/null @@ -1 +0,0 @@ -.DS_Store diff --git a/Acceptance-Tests.md b/Acceptance-Tests.md deleted file mode 100644 index 2cc7cc33..00000000 --- a/Acceptance-Tests.md +++ /dev/null @@ -1,484 +0,0 @@ -# Introduction - -Acceptance tests are end-to-end tests that test the complete functionality of the application, and this will help users to catch bugs and regressions before they are released ensuring that the code does what it is supposed to do. - -This guide will help you to get started on how to write `e2e acceptance test` for a particular user-type. - - -## Files and Directory Structure - -``` -oppia/core/tests/ - └── puppeteer-acceptance-tests - ├── data - │ └── blog-post-thumbnail.svg - │ └── ... - ├── functions - │ └── is-element-clickable.ts - ├── specs - │ ├── blog-admin - │ │ ├── assign-role-to-users-and-change-tag-properties.spec.ts - │ │ └── ... - │ ├── curriculum-admin - │ │ ├── create-publish-unpublish-and-delete-topic-and-skill.spec.ts - │ │ ├── create-edit-and-delete-classroom.spec.ts - │ │ └── ... - │ ├── topic-manager - │ │ ├── create-and-delete-subtopic-and-story.spec.ts - │ │ ├── browse-topics-on-topics-and-skills-dashboard.spec.ts - │ │ └── ... - │ ├── moderator - │ │ ├── view-recent-commits-and-feedback-messages.spec.ts - │ │ └── ... - │ ├── site-admin - │ │ ├── edit-user-roles.spec.ts - │ │ └── ... - │ ├── contributor-dashboard-admin - │ │ ├── manage-translators-and-reviewers.spec.ts - │ │ └── ... - │ ├── release-coordinator - │ │ ├── run-a-beam-job-and-copy-the-output.spec.ts - │ │ └── ... - │ ├── voiceover-admin - │ │ ├── add-voiceover-artist-to-an-exploration.spec.ts - │ ├── logged-out-user - │ │ ├── click-all-buttons-on-contact-us-page.spec.ts - │ │ ├── click-all-buttons-on-creator-guidelines-page.spec.ts - │ │ └── ... - │ ├── logged-in-user - │ │ ├── create-and-delete-account.spec.ts - │ │ ├── access-dashboards-and-other-pages-from-profile-menu.spec.ts - │ │ └── ... - │ ├── translation-admin - │ │ ├── add-and-remove-translation-rights.spec.ts - │ ├── practice-question-admin - │ │ ├── add-and-remove-contribution-rights.spec.ts - ├── utilities - │ ├── common - │ │ ├── puppeteer-utils.ts - │ │ ├── show-message.ts - │ │ ├── test-constants.ts - │ │ ├── console-report.ts - │ │ ├── user-factory.ts - │ ├── user - │ │ ├── blog-admin.ts - │ │ ├── blog-post-editor.ts - │ │ ├── curriculum-admin.ts - │ │ ├── topic-manager.ts - │ │ ├── moderator.ts - │ │ ├── site-admin.ts - │ │ ├── contributor-dashboard-admin.ts - │ │ ├── release-coordinator-admin.ts - │ │ ├── email-dashboard-admin.ts - │ │ ├── voiceover-admin.ts - │ │ ├── logged-in-user.ts - │ │ ├── logged-out-user.ts - │ │ ├── question-admin.ts - │ │ ├── super-admin.ts - │ │ ├── translation-admin.ts -``` - -The directory structure is as follows: -1) The `specs` directory contains all the top-level test files. Each test file is named as `*.spec.ts` and contains the test for a particular user type. For example, the `blog-admin` directory contains available tests for the `Blog Admin` user. - -2) The `utilities` directory contains all the utility files and helper functions, which you would require to write new acceptance tests. This directory can also be used to append more utility functions as needed by the user. -Files included inside this directory are: - - `common/puppeteer-utils.ts` -> This file contains the base _*BaseUser*_ class which provides the most common and useful methods such as _*openBrowser*_, _*goto*_, _*clickOn*_, _*openExternalPdfLink*_ etc. This class also serves as a foundation for defining other user-oriented subclasses, facilitating various testing scenarios. - - `common/user-factory.ts` -> This file contains methods for creating a certain user. The file has different methods for creating different types of users. - - `common/test-constants.ts` -> This file contains defined constants such as _*URLs, roles, etc.*_ which are used in the tests. - - `common/console-report.ts` -> This file contains methods for listening the console errors during a test. - - `common/show-message.ts` -> This file contains methods for displaying messages during the tests. - -3) The `user` directory holds the utility files for different user types. Each user utility class is built upon the base `BaseUser` class containing the original methods along with the ones related to that user type. For example, `blog-post-editor.ts` contains base functions as well as additional functions just related to the `Blog Post Editor` user. -4) The `data` directory contains all the images and other data files used in the tests. - -## How to run the acceptance tests -From the root directory of oppia, run the following command: -``` -python -m scripts.run_acceptance_tests --suite={{suiteName}} -``` - -For example, to run the `check-blog-editor-unable-to-publish-duplicate-blog-post.spec.ts` test, run the following command: -``` -python -m scripts.run_acceptance_tests --suite="blog-editor/check-blog-editor-unable-to-publish-duplicate-blog-post" -``` - -> **TIP:** To reduce the development cycle for the tests, try using `--skip-build` to skip the build in the local environment as this can reduce the run-time of tests. - -**Note: Typically, these tests take anywhere between 2 to 5-6 minutes (excluding the time taken for setting up the server) for any suite to run, both in headless and non-headless modes, assuming the machine has sufficient resources. The duration depends on the tests, and some tests can run longer due to a more extensive setup (if there is a longer setup, it would be mentioned in the timeout in the test block). However, tests with longer setups can go up to 8-10 minutes (currently, we have some such tests). Usually, the total runtime of tests would be around 3-4 minutes in most cases. In any case, if the run-time appears unreasonably long to you on you machine, feel free to raise an issue on our [issue tracker](https://github.com/oppia/oppia/issues).** - -## How to write new tests for a specific user - -1) Create a new directory for the specific user if it doesn't already exist inside the `specs` directory. For example, the `Topic Manager` user can have a directory named `topic-manager`, and within the user directory, each test file is named as `*.spec.ts`. -> Note: Naming convention for directories/files is kebab case, where each word is separated by a (-). - -2) Within the user directory, create a new file for each test. For example, `create-and-delete-subtopic-and-story.spec.ts` and `browse-topics-on-topics-and-skills-dashboard.spec.ts` for the `Topic Manager` user. These top-level tests contain single user stories checking their test steps and expectations mentioned in the testing spreadsheet. - -3) The functionality of the top-level tests for each user type is defined in the `utilities/user` directory. For example, the blog admin tests are written within the `specs/blog-admin` directory, and the functionality of the tests is defined in the `utilities/user/blog-admin.ts` file. -> Note: A utility file is maintained for each user type. The purpose of maintaining this file is to add methods specific to that user on top of the already provided basic methods. This file maintains a user class which is extended from the base class of `puppeteer-utils.ts`. For example, `blog-admin.ts` has a class `BlogAdmin` which has methods like `createDraftBlogPostWithTitle`, `deleteDraftBlogPostWithTitle`, etc., specific to Blog Admin only. Sometimes, when a user (e.g., Topic Manager) requires methods from another user type (e.g., Curriculum Admin), it's acceptable to use intersection types to combine the necessary methods. - -4) The utility files are imported into the top-level test files, and the methods are called to perform the required actions. For example, in the `assign-role-to-users-and-change-tag-properties.spec.ts` file, the `assignRoleToUser` method is called to assign a role to a user. Additionally, the `expectRoleAssignedSuccessfully` method is called to check if the role was assigned successfully. To facilitate instantiation of classes, each utils file should also include a `UserFactory` function. This function's purpose is to instantiate a new class of the corresponding type. For instance, `export let BlogAdminFactory = (): BlogAdmin => new BlogAdmin();` would create a BlogAdmin instance. - -5) After adding a new user utility file, you should make the following changes to the user factory: - -If the role requires a super admin to assign it, first, add the role to the `Roles` enum in `test-constants.ts`. Then, to add it, reference the `USER_ROLE_MAPPING` inside the `user-factory.ts` file. If the user requires a role from the super admin, add the reference accordingly. - -For example, if we want to add Translation Admin with the help of a super admin then: - - • Define the role in `Roles` enum: - ``` - Roles: { - other roles... , - TRANSLATION_ADMIN: 'translation admin', - } - ``` - • Add the role to `USER_ROLE_MAPPING`: - ``` - const USER_ROLE_MAPPING = { - other roles... , - [ROLES.TRANSLATION_ADMIN]: TranslationAdminFactory, - } as const; - ``` - -For roles that don't require super admin privileges, such as `LoggedInUser`, add the factory to the array inside `createNewUser` under `composeUserWithRoles(BaseUserFactory(), [...])`. This ensures that the new user role is included when creating a new user instance. Please ensure to follow the appropriate conventions and guidelines while adding new user-utilities files to the user-factory to maintain consistency and clarity in the testing process. - -6) For each test, the user is created using the `UserFactory` class. For example, in the `assign-role-to-users-and-change-tag-properties.spec.ts` file, the `createNewUser` method is called to create a new user, with the parameter `[ROLES.BLOG_ADMIN]` assigned as the role of the blog admin. The `createNewUser` method is defined in the `user-factory.ts`file. The `createNewUser` method creates a new user with the provided username, email, and role, and then returns the user object. The user object is used to perform the required actions (that are defined in the `utilities/user/*-utils.ts`). - -7) After successful completion of any test step or any expectation, the `showMessage` method is called to log the progress. For example, in the `blog-admin.ts` file, the `showMessage` method is called to log the progress after publishing a new blog post. The `showMessage` method is defined in the `show-message.ts` file. - -8) If there is any error during the test, then we throw errors in the expectation step or there would be a timeout error if some component does not behave as intended. - -9) The `utilities` directory contains all the utility files and helper functions, which you would require to write new acceptance tests. This directory can also be used to append more utility functions as and when required or needed by the user. - -10) The test must be thoroughly tested before submitting a PR. The test can be run locally by running the following command as mentioned above or you can run the test on the CI server by pushing your code to the remote branch in your fork. The CI server will run the test and will show the result. - -11) After writing the test, do not forget to add it in our configuration file `common.py` and in `acceptance.json` file so that it is included in the workflow. - -> Note: Sometimes tests may pass locally but fail on the CI environment due to differences between the local and CI environments. In such cases, debugging and fixing should be done on the CI environment, as that is where the tests are intended to run. - -### Console errors logging functionality in Acceptance Tests - -Acceptance Tests have the capability to detect console errors during CUJs, potentially resulting in test failures. However, there are scenarios where certain console errors can be deemed acceptable and should not cause the test to fail. In order to ignore errors like these, you can use `ConsoleReporter.setConsoleErrorsToIgnore`, which takes in an array of error regexes to match the errors that can be acceptable. For instance, an error like `Blog Post with the given title exists already. Please use a different title.`, which occurs during the 'blog-editor-tests/try-to-publish-a-duplicate-blog-post-and-get-blocked' test, is ignored using the ConsoleReporter since it is an acceptable error in the context of the test. When passing acceptable errors like these to the ConsoleReporter, you should be specific and not use vague errors like `Failed to load resource...`. - -Below is an example of this usage: -```typescript -ConsoleReporter.setConsoleErrorsToIgnore([ - 'Blog Post with the given title exists already. Please use a different title.' -]); -``` - -To handle errors that need to be ignored and are not specific to any acceptance test, you should include them directly within the `console-reporter.ts` utility. In this file, you would add the error regex to the `CONSOLE_ERRORS_TO_IGNORE` array and explain with a comment why this error should be ignored. - -```typescript -const CONSOLE_ERRORS_TO_IGNORE = [ - // These "localhost:9099" are errors related to communicating with the - // Firebase emulator, which would never occur in production, so we just ignore - // them. - escapeRegExp( - 'http://localhost:9099/www.googleapis.com/identitytoolkit/v3/' + - 'relyingparty/getAccountInfo?key=fake-api-key' - ), - // This error covers the case when the PencilCode site uses an - // invalid SSL certificate (which can happen when it expires). - // In such cases, we ignore the error since it is out of our control. - escapeRegExp( - 'https://pencilcode.net/lib/pencilcodeembed.js - Failed to ' + - 'load resource: net::ERR_CERT_DATE_INVALID' - ), -]; -``` -To handle errors that need to be fixed, you should include them directly within the `console-reporter.ts` utility. In this file, you would add the error regex to the `CONSOLE_ERRORS_TO_FIX ` array and add a TODO comment which points to the existing issue number (this comment should be removed when the bug is resolved). If the error doesn't have any corresponding issue, then file a new issue on our [issue tracker](https://github.com/oppia/oppia/issues). - -For example: -```typescript -const CONSOLE_ERRORS_TO_FIX = [ - // TODO(#19746): Development console error "Uncaught in Promise" on signup. - new RegExp( - 'Uncaught \\(in promise\\).*learner_groups_feature_status_handler' - ), - // TODO(#19733): 404 (Not Found) for resources used in midi-js. - escapeRegExp( - 'http://localhost:8181/dist/oppia-angular/midi/examples/soundfont/acoustic' + - '_grand_piano-ogg.js Failed to load resource: the server responded with a ' + - 'status of 404 (Not Found)' - ) -]; -``` - -### Screenshots testing functionality in Acceptance Tests - -Acceptance Tests have a function called `expectScreenshotToMatch` in `puppeteer-utils.ts` to take screenshots of the UI during the acceptance tests and compare them to the existing screenshots in the codebase, which can help with debugging test failures as it provides more information beside the error message. - -To use this functionality, call the function `expectScreenshotToMatch`, which takes in a string as the name of the screenshot, and the absolute path of the directory of the specs file to locate where the folder of the screenshots will be. For instance, to create a screenshot after calling the function `loggedOutUser.clickTeachButtonInAboutMenuOnNavbar` in `logged-out-user/click-all-buttons-on-navbar.spec.ts`, call the function `expectScreenshotToMatch` with the user type `loggedOutUser` to identify which user's browser should be screenshotted, `teachPage`, a name for the screenshots that describes what the page is, and the variable `__dirname`. - -Below is an example of this usage: -```typescript -it( - 'should open teach page when the "For Parents/Teachers" button is clicked in About Menu on navbar', - async function () { - await loggedOutUser.clickTeachButtonInAboutMenuOnNavbar(); - await loggedOutUser.expectScreenshotToMatch('teachPage', __dirname); - }, - DEFAULT_SPEC_TIMEOUT_MSECS -); -``` - -On the first run, a screenshot named `teachPage-snap.png` will be created and stored in a folder based on which mode (prod mode or dev mode) and device environment (desktop or mobile) the test was run in. There are four different folders: - -- `prod-desktop-screenshots`: production mode in desktop environment, in which the `prod_env` flag is used. -- `prod-mobile-screenshots`: production mode in mobile environment, in which the `prod_env` and `mobile` flags are used -- `dev-desktop-screenshots`: local development mode in desktop environments -- `dev-mobile-screenshots`: local development mode in mobile environment, in which the `mobile` flag is used. - -To introduce a new screenshot to the codebase, the test should be run in all these four modes/environments to generate each screenshot in all four folders. - -On CI, we run all the acceptance tests in production mode, so the screenshots in `prod-desktop-screenshots` and `prod-mobile-screenshots` will be compared to the screenshots that are generated during CI checks. If a screenshot doesn't match on CI, it generates two images in two separate folders as artifacts in the GitHub workflow. For example, if the screenshot `teachPage-snap.png` fails in `logged-out-user/click-all-buttons-on-navbar.spec.ts` during the CI checks in desktop environment, two folders will be created, they will be named as `diff-snapshots-logged-out-user_click-all-buttons-on-navbar_desktop_original` and `new-snapshots-logged-out-user_click-all-buttons-on-navbar_desktop_original`. Inside the folder `diff-snapshots-logged-out-user_click-all-buttons-on-navbar_desktop_original`, a screenshot `teachPage-diff.png` will be generated. This screenshot `teachPage-diff.png` will show the difference between the screenshot from the codebase and the new generated screenshot, making it easier to identify the difference. And inside the folder `new-snapshots-logged-out-user_click-all-buttons-on-navbar_desktop_original`, a screenshot `teachPage-received.png` will be generated. This screenshot `teachPage-received.png` will be the new screenshot to be used to replace the old one if needed. Therefore, if we need to replace the old screenshot, we will download this screenshot from the artifacts. - -On the other hand, if the screenshot fails locally (in desktop environment), the screenshot `teachPage-diff.png` will be generated and stored inside a new folder `diff-snapshots` under `logged-out-user/dev-desktop-screenshots` and the screenshot `teachPage-received.png` will be generated and stored inside a new folder `new-snapshots` under `logged-out-user/dev-desktop-screenshots`. - -### Updating Screenshots for Acceptance Tests -When making changes that affect a user journey tested through the acceptance tests or introduce a new feature, a contributor needs to update screenshots to support their changes depending on where the changes are affecting. For example, if the acceptance test only fails in prod+mobile environment, then we should replace the failed screenshots in `prod-mobile-screeenshots`. - -The screenshots in prod (`prod-desktop-screenshots` and `prod-mobile-screenshots`), should be obtained from the CI (not local) run, so that the environment matches future runs. To do this, follow these steps: -1. Go to the summary of the CI run for `full_stack_tests.yml` and scroll down to find an artifact named as `new-snapshots_{_suite_name_}_{desktop/mobile}_original`. Click on the name or the download symbol on the right hand side to download it. -![alt text](./images/AcceptanceTests/NewSnapshotsArtifactInCI.png) - -2. Extract the contents of the artifact. There should be an image and the name of the image should ends with `received.png`. Rename it by replacing the `received` with `snap`. For example, rename it from `blogPage-received.png` to `blogPage-snap.png`. - -3. Navigate to where you have saved the Oppia repo on your local machine and go to `oppia/core/tests/puppeteer-acceptance-tests/specs` - -4. Navigate to the test spec folder. The spec is mentioned in the artifact folder you downloaded in Step 1. (The spec is `blog-post-writer` in this example) - -5. Check if the failure was for desktop or mobile based on the screenshot size or the name in the artifact: -![alt text](./images/AcceptanceTests/DesktopOrMobile.png) - -6. Go to the `prod-desktop-screenshots` or `prod-mobile-screenshots` folder depending on the failure. - -7. Replace the screenshot having the name {screenshot_name}-snap.png with the image renamed in step 2. (Make sure to rename the pasted screenshot to {screenshot_name}-snap.png) - -8. Check that the correct image got replaced. - -9. Commit and push your changes! Self review your PR to verify that the correct image(s) were used. - -For the screenshots in dev (`dev-desktop-screenshots` and `dev-mobile-screenshots`), follow these steps to update the screenshots: -1. Navigate to where you have saved the Oppia repo on your local machine and go to `oppia/core/tests/puppeteer-acceptance-tests/specs` - -2. Navigate to the test spec folder. For example, the spec would be `blog-post-writer` if the screenshot is under the spec `blog-post-writer/create-and-edit-blog-post`. - -3. Go to the `dev-desktop-screenshots`or `dev-mobile-screenshots` folder depending on the failure. - -4. Navigate to `new-snapshot` and rename the image in the folder by replacing the `received` with `snap`. - -5. Navigate back to the `dev-desktop-screenshots`or `dev-mobile-screenshots` folder depending on the failure. Replace the screenshot having the name {screenshot_name}-snap.png with the image renamed in step 4. (Make sure to rename the pasted screenshot to {screenshot_name}-snap.png) - -6. Check that the correct image got replaced. Run the test locally to check if the test passes. - -7. Commit and push your changes! Self review your PR to verify that the correct image(s) were used. - -## Acceptance Tests for Mobile - -Similar to desktop, we also have acceptance tests for mobile to ensure responsiveness and uninterrupted user journeys on small screen devices. While the tests themselves remain largely the same for both desktop and mobile, there are some differences. For instance, large full menus on desktop may be converted to dropdowns, hamburger menus, or other shortcuts on mobile, requiring additional actions to complete the tests. - -### How to write tests for mobile - -There will be no change in the `specs` file of the tests; however, there may be some changes in the `utilities/user` file, which is optional and dependent on the specific test cases. In most cases, the tests will run correctly for both mobile and desktop. - -However, in scenarios where certain actions are affected by the smaller screen size, additional steps may be required. - -For example: consider a scenario where a menu is collapsed into a hamburger menu due to the small screen size: - -![Shortcut Menu](./images/AcceptanceTests/MobileHamburgerMenu.png) - -Here, if we want to click on the "Home" or any other button, we need to first click on the hamburger menu. Additionally, there may be differences in selectors for the same buttons between desktop and mobile. For instance, the publish button in desktop might be `e2e-test-publish-exploration`, while in mobile it could be `e2e-test-mobile-publish-button`. - -We can handle these differences by including conditional statements in the `utilities/user` file, using the `isViewportAtMobileWidth()` function to execute commands specific to mobile devices. - -For example: - -```typescript -async discardCurrentChanges(): Promise { - // Check if the viewport corresponds to a mobile device. - if (this.isViewportAtMobileWidth()) { - // If on mobile, click on the mobile-specific discard button. - await this.clickOn(mobileDiscardButton); - } else { - // If on desktop, click on the desktop-specific discard button. - await this.clickOn(discardDraftButton); - } - // Confirm the discard action, regardless of the viewport size(common in both). - await this.clickOn(discardConfirmButton); -} -``` - -In this example, the `discardCurrentChanges()` function checks if the viewport width corresponds to a mobile device, and if so, clicks on the mobile-specific discard button. Otherwise, it clicks on the desktop-specific discard button. Finally, it confirms the discard action. This approach allows us to maintain a single set of tests while accommodating differences between desktop and mobile environments. - -### How to run mobile acceptance tests -From the root directory of oppia, run the following command: -``` -python -m scripts.run_acceptance_tests --mobile --suite={{suiteName}} -``` - -For example, to run the `check-blog-editor-unable-to-publish-duplicate-blog-post.spec.ts` test, run the following command: -``` -python -m scripts.run_acceptance_tests --mobile --suite="blog-editor/check-blog-editor-unable-to-publish-duplicate-blog-post" -``` - -## Fixing Flakes in Acceptance Tests - -A **flaky test** is a test that behaves inconsistently—passing sometimes and failing at other times—even when no underlying code has changed. This non-determinism may originate from the test itself, the application code, or interactions with the environment. - -For example, suppose that you write a test that clicks a button to open a modal and then clicks a button inside the modal to close it. Sometimes, the modal will open before the test tries to click the close button, so the test will pass. Other times, the test will try to click before the modal has opened, and the test will fail. We can see this schematically: - -```mermaid -flowchart LR -a("<--A-->") - -A("Click to open modal") ----|"//"| B("Modal opens") -A ---- |"//"| C("Click to close modal") -B ---- P("+") -C ---- P -P --> Q("other operations") - -b("<--B-->") - -starts ---- time -----> ends -``` - -The durations of steps `A` and `B` are non-deterministic because `A` depends on how quickly the browser executes the frontend code to open the modal, and `B` depends on how fast the test code runs. Since these operations are happening on separate processes, the operating system makes no guarantees about which will complete first. In other words, we have a race condition. - -This race condition means that the test can fail randomly even when there's nothing wrong with the code of the Oppia application (excluding tests). These failures are called _flakes_. - -### Why flakes are problematic - -Flakes are annoying because they cause failures on PRs even when the code changes in those PRs are fine. This forces developers to rerun the failing tests, which slows development. - -Further, flakes are especially problematic to certain groups of developers: - -* **New contributors**, who are often brand-new to open source software development, can be discouraged by flakes. When they see a failing E2E test on their PR, they may think that they made a mistake and become frustrated when they can't find anything wrong with their code. - -* **Developers without write access to the repository** cannot rerun tests, so they have to ask another developer to restart their tests for them. Waiting for someone to restart their tests can really slow down their work. - -Finally, flakes mean that developers rerun failing tests more readily. We even introduced code to automatically rerun tests under certain conditions. These reruns make it easier for new flakes to slip through because if a new flake causes a test to fail, we might just rerun the test until it passes. - -### Preventing flakes - -Conceptually, preventing flakes is easy. We can use `waitForElementToBeVisible()` statements to make the tests deterministic despite testing a non-deterministic system. For example, suppose we have a function `waitForElementToBeVisible()` that waits for a modal to appear. Then we could write our test like this: -```mermaid - <---A---> - - +-------+ - | Modal | -+----------+ +---//---+ opens +---------------------------------+ -| Click to | | +-------+ | -| open +---+ +----> -| modal | | +----------------+ +-------------+ | -+----------+ +---//---+ waitForElementToBeVisible() +-//-+ Click to +-----+ - +----------------+ | close modal | - +-------------+ - - <---B---><-------C--------> - - ---------------------- time --------------------------------------------> -``` -Now, we know that the test code won't move past `waitForModalwaitForModal()` until after the modal opens. In other words, we know that `B + C > A`. This assures us that the test won't try to close the modal until after the modal has opened. - -The challenge in writing robust E2E tests is making sure to always include a waitFor statement like `waitForModalwaitForModal()`. It's common for people to write E2E tests and forget to include a waitFor somewhere, but when they run the tests, they pass. Their tests might even pass consistently if their race condition only causes the test to fail very rarely. However, months later, an apparently unrelated change might change the runtimes enough that one of the test starts flaking frequently. - -## Fixing Flakes -Fixing a flaky test generally involves three phases: **Reproduction**, **Diagnosis**, and **Fix**. This section outlines the canonical process contributors should follow. - ---- - -### 1. Reproduction - -The first step is to reliably reproduce the flake. Reproduction may be difficult in a local environment since a flake may only surface intermittently. - -To address this: - -* Use the **Stress Test Acceptance Tests** GitHub workflow. - This workflow runs the specified acceptance test suite multiple times in parallel, significantly increasing the likelihood of encountering the flake. -* Trigger the workflow manually in your fork using the following steps: - 1. Navigate to your fork (github.com/YOUR_USERNAME/oppia). - 2. Sync your fork with the upstream repository (oppia/oppia). - 1. Click on "Sync Fork" button in the top right corner of the fork page. ![Ref: Sync Fork Menu](./images/AcceptanceTests/SyncForkButton.png) - 2. Click on "Update Branch" button. ![Ref: Update Branch Popup](./images/AcceptanceTests/UpdateBranchButton.png) - 3. Navigate to the Actions tab in top menu. Then, click on the workflow "Stress Test Acceptance Tests" from the newly opened left menu. - 4. Run the workflow manually. - 1. Click on "Run Workflow" button. ![Ref: Workflow manual trigger menu](./images/AcceptanceTests/RunWorkflowButton.png) - 2. Use the following inputs: - - * **Branch name**: `develop` - * **Run count**: at least 20 times, if you can't observe the flake, then increase the run count. - * **Suite**: the acceptance test suite where the flake occurs (you can find the suite name in acceptance.json file) - -**Note:** After the flake is reproduced, add the link to the Stress Test in the Issue, so that others can look at the same stress test so CI resources are not wasted. - -Your goal in this phase is to reliably observe the flaky behavior and capture concrete failure examples for analysis. - ---- - -### 2. Diagnosis - -Once you can reproduce the flake, you must investigate its root cause. - -1. **Create a debugging doc** - Use the standard [Debugging Doc template](https://docs.google.com/document/d/1qRbvKjJ0A7NPVK8g6XJNISMx_6BuepoCL7F2eIfrGqM/edit) and follow the guidance provided in the [Debugging Docs wiki](https://github.com/oppia/oppia/wiki/Debugging-Docs). - Populate the initial metadata and provide clear links to failing builds. - -2. **Form and test hypotheses** - - * Use the debugging doc to document potential sources of non-determinism. - * Apply hypothesis testing to narrow down the exact cause—this often includes validating timing assumptions, verifying selectors, examining API responses, and checking console logs. - * Reach out in relevant Google Chat groups for support if you encounter uncertainties or need cross-verification. - -The diagnosis is complete when you have a clear, well-supported hypothesis explaining the flake’s cause. - ---- - -### 3. Fix - -Once the root cause is known: - -1. **Implement the fix** - Apply targeted changes in the test or application code as appropriate. - Document the fix clearly in the debugging doc. - -2. **Verify the fix using Stress Tests** - - * Run the **Stress Test Acceptance Tests** workflow again from your fork (you can't use Oppia repo). You can use same steps as in [Reproduction](#1-reproduction) phase. - * Use the same suite and a run count of 20. - * A valid fix should result in **zero flaky failures** across all runs. - * If a failure occurs: - - * It must be a different error unrelated to the original flake; otherwise, the flake is not yet resolved. - -Only after the fix has been validated should you proceed. - ---- - -### 4. Open a Pull Request - -When the fix is verified: - -1. Open a PR with the final changes. -2. Include: - - * A link to the debugging doc - * Stress test proof (links to workflow runs showing zero flakes) -3. Provide a clear PR description summarizing: - - * What the original flake was - * The identified root cause - * What fix was implemented - * Evidence of stability after the fix - -This ensures reviewers have complete visibility into the debugging and validation process. - -## Reference Links -Blog Admin and Blog Editor Tests - - [Blog Admin top-level tests](https://github.com/oppia/oppia/tree/develop/core/tests/puppeteer-acceptance-tests/spec/blog-admin-tests) - [Blog Editor top-level tests](https://github.com/oppia/oppia/tree/develop/core/tests/puppeteer-acceptance-tests/spec/blog-editor-tests) - [user utility files](https://github.com/oppia/oppia/blob/develop/core/tests/puppeteer-acceptance-tests/user-utilities/blog-post-editor-utils.ts) - [puppeteer utility files - base class](https://github.com/oppia/oppia/blob/develop/core/tests/puppeteer-acceptance-tests/puppeteer-testing-utilities/puppeteer-utils.ts) - [puppeteer utility files - user factory](https://github.com/oppia/oppia/blob/develop/core/tests/puppeteer-acceptance-tests/puppeteer-testing-utilities/user-factory.ts) diff --git a/Actions-and-issues.md b/Actions-and-issues.md deleted file mode 100644 index 298d3fc9..00000000 --- a/Actions-and-issues.md +++ /dev/null @@ -1,51 +0,0 @@ -## Table of contents - -* [Introduction](#introduction) -* [Actions](#actions) -* [Issues](#issues) -* [Using actions and issues](#using-actions-and-issues) - -## Introduction - -As a user plays through an exploration, we want to track their progress to identify any problems. If we do find problems with the exploration, we can raise those to the exploration creator. For example, if a user gets stuck in a cycle of cards or submits many incorrect answers to an interaction, the exploration could probably be improved. - -We store the steps a user takes through an exploration as _actions_, and we create _issues_ to keep track of the problems that these actions indicate. Both actions and issues are defined as [[schemas|Schemas]]. - -## Actions - -There are three actions defined at [`extensions/actions/`](https://github.com/oppia/oppia/tree/develop/extensions/actions): - -* `ExplorationStart`: This action is recorded when a learner starts an exploration. It stores the name of the state (i.e. card) they start at. - -* `AnswerSubmit`: This action is recorded when a learner submits a response to an interaction. It stores the ID of the interaction, the name of the state with the interaction, the answer that the learner submitted, the exploration's feedback for that answer, the next state they transition to, and how long the learner spent in the state. Note that the interaction ID is just its name, for example `ItemSelectionInput`. - -* `ExplorationQuit`: This action is recorded when a learner leaves an exploration. It stores the name of the state at which the learner quit and how long the user spent in the last state they visited. - -These actions are recorded using the functions provided by the [stats reporting service](https://github.com/oppia/oppia/tree/develop/core/templates/pages/exploration-player-page/services/stats-reporting.service.ts). For example, when a learner transitions into a terminal state (a card that ends the exploration), the [conversation skin directive](https://github.com/oppia/oppia/tree/develop/core/templates/pages/exploration-player-page/learner-experience/conversation-skin.directive.ts) calls `StatsReportingService.recordExplorationCompleted()`. - -When a learner's journey through an exploration goes smoothly, we don't store the actions anywhere; however, when we detect a problem in a learner's journey, then we create an issue that stores the associated actions. Note that at no point during this process do we store or process any information about the learner's identity. - -## Issues - -We define three different issues at [`extensions/issues/`](https://github.com/oppia/oppia/tree/develop/extensions/issues): - -* `MultipleIncorrectSubmissions`: This issue is recorded when a user submits 3 or more incorrect answers to an interaction. It stores the name of the state where the user provided incorrect answers and the number of incorrect answers that they submitted. - - The logic determining whether to file this issue lives in the `MultipleIncorrectAnswersTracker` class in the [playthrough service](https://github.com/oppia/oppia/tree/develop/core/templates/services/playthrough.service.ts). - -* `CyclicStateTransitions`: This issue is recorded when a user repeats a cycle of states 3 times in a row. Note that our cycle detection isn't particularly clever. As soon as we detect a cycle, we check whether it's equal to the most recent cycle we detected earlier. If it matches, we increment a counter. Otherwise, we reset the counter to 1. Here are some examples: - - * `A -> B -> A -> B -> A -> B -> A`: An issue would be filed because the cycle `A -> B -> A` happened 3 times. - * `A -> B -> A -> C -> A -> B -> A -> C -> A -> B -> A -> C -> A`: An issue would _not_ be filed even though the cycle `A -> B -> A -> C -> A` repeats 3 times. The reason is that we will only detect the smaller `A -> B -> A` and `A -> C -> A` cycles, which alternate. Since they alternate, we keep resetting our counter to 1 and so never reach 3 consecutive cycles. - - The logic behind this cycle detection lives in the `CyclicStateTransitionsTracker` class in the [playthrough service](https://github.com/oppia/oppia/tree/develop/core/templates/services/playthrough.service.ts). - - This issue stores a list of the state names that make up the cycle. - -* `EarlyQuit`: This issue is recorded whenever a user quits an exploration after fewer than 300 seconds. It records the name of the state where the user quit and the time they spent on the exploration. - - The logic behind this cycle detection lives in the `EarlyQuitTracker` class in the [playthrough service](https://github.com/oppia/oppia/tree/develop/core/templates/services/playthrough.service.ts). - -## Using actions and issues - -Right now, neither issues nor their associated actions are surfaced to exploration creators. They used to be available through the improvements tab of the exploration editor, but that tab has been removed. We are currently working on building a new improvements tab. diff --git a/Adding-new-page.md b/Adding-new-page.md deleted file mode 100644 index c241a2c5..00000000 --- a/Adding-new-page.md +++ /dev/null @@ -1,90 +0,0 @@ - -## When adding a new learner-facing page… - -1. Get an approval from Diana ([diana@oppia.org](mailto:diana@oppia.org)) and Sean ([sean@oppia.org](mailto:sean@seanlip.org)) regarding the Page Title and the Page Meta tag content that will be used by the new page before creating a PR. - -2. In the PR that introduces the new page, make sure that it handles the page title and meta tag changes. If it is a public user-facing page, the sitemap should also be updated. - - - - For static pages, the HTML should contain `` and `<meta>` tags. Example: - ```html - <title itemprop="name">Title of the page. - - - - - - - - - - ``` - - - For dynamic pages, follow this example: - - https://github.com/oppia/oppia/blob/dacde388ab3a8eac535a1a848afaed24b9ffc7b6/core/templates/pages/story-viewer-page/story-viewer-page.component.ts#L158-L161 - - -4. In the PR description, explain the following in detail: - - - - Is the page learner facing and public? - - - How can it be accessed i.e. the user journey to get to the new page? - - - A clear description of the contents of the page. - - - What is the page title and meta tag content that will be used in the new page? - - - Does the sitemap.xml need to be updated? - - -4. Add @dchen97 and @seanlip as reviewers for the PR. - -## Technical part - -### Files - -* _generic-page.import.ts_ — imports necessary for the page initialization -* _generic-page.mainpage.html_ — the main HTML -* _generic-page.module.ts_ — Angular module definition - -### Webpack -When you're adding new HTML page (not directive HTML) that uses TypeScript you also need to add it to `webpack.common.config.ts`: - -1. You need to define the TypeScript entry point for the page into `module.exports.entries`. -2. You need to add `new HtmlWebpackPlugin({…})` into `module.exports.plugins`. - -For example when adding **pages/generic-page/generic-page.mainpage.html** with asocciated TypeScript file **pages/generic-page/generic-page.scripts.ts**, you will need to add `page: commonPrefix + '/pages/generic-page/generic-page.scripts.ts'` to `module.exports.entries` and - -```javascript -new HtmlWebpackPlugin({ - chunks: ['page'], - meta: { // if default meta is used this can be ommited - name: 'name', - description: 'description' - }, - filename: 'generic-page.mainpage.html', - template: commonPrefix + '/pages/generic-page/generic-page.mainpage.html', - minify: htmlMinifyConfig, - inject: false -}) -``` -into `module.exports.plugins`. - -### Lighthouse - -The new page should be added both to the _.lighthouserc.js_ and _.lighthouserc-accessibility.js_. The page URL should be added to `ci.collect.url` and in the _.lighthouserc.js_ -```javascript -{ - 'matchingUrlPattern': 'http://[^/]+/url', - 'assertions': { - 'uses-webp-images': [ - 'error', {'maxLength': 0, 'strategy': 'pessimistic'} - ], - 'uses-passive-event-listeners': ['error', {'minScore': 1}], - 'deprecations': ['error', {'minScore': 1}] - } -} -``` -into `ci.collect.assert.assertMatrix`. \ No newline at end of file diff --git a/Adding-new-translations-for-i18n.md b/Adding-new-translations-for-i18n.md deleted file mode 100644 index 282e6848..00000000 --- a/Adding-new-translations-for-i18n.md +++ /dev/null @@ -1,176 +0,0 @@ -## Quick overview - -In order to ensure that the Oppia website is understandable to learners around the world, we provide internationalization (i18n) support on Oppia for learner-facing pages. This enables learners to view the site in different languages using the language-selector dropdown in the navbar. - -Our i18n support uses the [angular-translate](http://angular-translate.github.io/) library, documentation for which can be found [here](https://angular-translate.github.io/docs/#/guide). All platform string translations are provided through our partner, [translatewiki.net](https://translatewiki.net/wiki/Translating:Oppia). - -This wiki page explains how to: -- [Fill in missing platform translations](#how-to-fill-in-missing-platform-translations) -- [Translate lessons and other dynamic text](#how-to-translate-lessons-and-other-dynamic-text) -- [Create an i18n-compliant PR](#how-to-create-an-i18n-compliant-pr) -- [Fetch new translations from translatewiki](#how-to-fetch-new-translations-from-translatewiki) -- [Add a new translation language](#how-to-add-a-new-translation-language) - -## How to fill in missing platform translations - -All our translations are contributed through translatewiki.net. If you see missing translations in a language you're familiar with, please follow these steps to help fix the gap (do not create a PR): - -1. Visit the [Oppia Translatewiki](https://translatewiki.net/wiki/Translating:Oppia) page. Read the notes on that page regarding plural rules and special markup syntax. -2. Click "Translate this project". -3. Select a language to contribute translations for. (Also, read the [notes](https://github.com/oppia/oppia/wiki/Adding-new-translations-for-i18n#note-1-variable-replacement) below describing the translation formats used for variables and plurals.) - -Changes will then be pushed to Oppia automatically by the Translatewiki admins, and they will show up in future Oppia releases. We typically update new translations to the Oppia.org website on a monthly cadence. - -### Important: Don't rely on machine translation - -The translatewiki admins have requested that translators do not rely on machine auto-translation, especially if they don't know the language and cannot fix its mistakes. - -In cases where fixing a translation is absolutely necessary for technical reasons (e.g. if a translatewiki string has errors and it's breaking the tests), they recommend doing the following: -- Add the string `!!FUZZY!!` to the beginning of the machine-translated string. This will update the translation in Oppia's source tree, and also mark it as "needing update" for the translators on translatewiki. -- Ask a translator for that language to fix the translation. You can find active translators by going to [Special:ActiveLanguages](https://translatewiki.net/wiki/Special:ActiveLanguages) and clicking on a language name. - -### Note 1: Variable replacement - -Within translations, you could add variables that would be later replaced with personalized content. For example, in the text - - You have 3 new notifications. - -the number of notifications is a variable, and thus cannot be included directly in the translation. Angular translate solves this problem by using an interpolation service. Your translated phrase should look like: - - You have <[notification_number]> new notifications. - -In the html page, the value of `notification_number` will be substituted accordingly by angular-translate. For more details, please refer to the [angular-translate documentation](https://angular-translate.github.io/docs/#/guide/06_variable-replacement). IMPORTANT: the default mechanism of indicating an expression in Angular is using the symbols {{ and }}, however in Oppia these symbols have been replaced by <[ and ]>. - -### Note 2: Pluralization - -In the example above, if there is only one notification, then we should change "notifications" for "notification". Furthermore, some languages may have more plural forms than English. To handle this, we use a different interpolation service, called [messageformat](https://github.com/SlexAxton/messageformat.js/). In this case, the translation should look like this: - -``` -{notification_number, plural, =0{You have no notifications.} one{You have one notification.} other{You have # notifications.}} -``` - -In this example, the # symbol will be replaced by the value of the `notification_number` variable. For a more elaborate tutorial, please refer to the [angular-translate guide for pluralization](http://angular-translate.github.io/docs/#/guide/14_pluralization). - - -## How to translate lessons and other dynamic text - -If you'd like to help translate Oppia's lessons, please get in touch via our [volunteer form](https://forms.gle/BK99fdqBShY7BPKC8). Successful applicants will be invited to join one of Oppia's translation teams. - -We really appreciate help with translations to make the lessons accessible for students whose first language isn't English. Thank you for helping out! - - -## How to create an i18n-compliant PR - -When developing learner-facing functionality, you must use `I18N_...` strings as placeholders for text content. This enables such strings to be translated. You can see the translations in the [i18n directory](https://github.com/oppia/oppia/tree/develop/assets/i18n), which map translation keys like `I18N_MODULE_STRING_NAME` to the translated strings. When a page is loaded, angular-translate traverses the page's html code and changes the translation key to the appropriate translated string. - -### Good i18n practices - -Please consider i18n while developing. Not all languages are the same: words have different lengths, pluralization rules differ, sentences have different structures and the direction of writing can be from right to left. As a result, sometimes development practices that are generally good (like code reuse) turn out to be less than ideal for i18n. Here are some important points to take into account: -- Do not include raw strings, always use a translation key. Even if it is just an exclamation mark (!) -- When designing a page, plan for the case where strings are twice the length in other languages. Also, think about how the page would look like if the language is written from right to left. -- Try to include as much text as you can in a single key, so that the translator can provide a more coherent translation. Do not divide a paragraph into multiple keys unless you cannot avoid it. Don't split strings up and concatenate them, since different languages will use a different grammatical order. -- If you need to include html in your string, such as `` tags, try and pass the code as an argument to the translation service. -- Note that other languages may have more plural forms or genres than English. So, for example, if you include a sentence that is always going to be plural in English, add pluralization support regardless in your translation (see [Note 2 on pluralization](#note-2-pluralization] above). - -### Adding a new translation key - -To add a new translation key: - -1. Choose a translation key name. These key names are always written in uppercase, with the following parts: - - - Prefix: the key MUST start with `I18N_` (otherwise some tests will break). - - Module name: such as `SIGN_UP_`. Be consistent with existing names, and keep the keys grouped by the module name. - - String name: a meaningful name representing the function of the string in the page, like `PAGE_TITLE`. - - Note that long translation keys are fine -- the key objective is that the role of the string is well described. - -2. Add the new translation key to both the `assets/i18n/en.json` and `assets/i18n/qqq.json` files. In `qqq.json` you should provide translators with a descriptive context for the string. Please see [this page](https://www.mediawiki.org/wiki/Localisation#Message_documentation) for more information on what goes into these descriptions. - -You do not need to modify the other JSON files. Translatewiki will do that after you merge the PR. - -### Updating the English text for a translation key - -When updating the English text for a translation key, first determine whether the new English text has a significantly different meaning to the previous English text. - -- If the new text conveys a different meaning from the previous text: do not reuse the translation key. Create a new translation key instead, and delete the previous one. -- If the new text is similar to the previous text, and previous translations are probably still valid: just update the English value directly in `assets/i18n/en.json`. Translatewiki will pick up the updates and push new translations in due course. - -### Deleting removed translation keys - -If a translation key is not used any more, you must delete it from `assets/i18n/en.json` **and all other translation JSON files**. This is to preserve the property that the keys in other JSON files are a subset of those in `en.json`. - -### Verifying changes to translation files - -To verify your changes to translation files locally, run the following command in a terminal: - -Python: -``` -python -m scripts.run_backend_tests --test_targets=core.controllers.base_test.I18nDictsTests -``` - -This validates the translation JSON files by verifying that the keys are correctly sorted, that the keys in en.json and qqq.json match, that every other translation JSON file has a subset of the keys in en.json, and so on. - - -### Flash of Untranslated Content - -Sometimes, the page is rendered in the browser before the locale file with the translations is loaded. As a result, the user can see the translation keys briefly before they're replaced with the corresponding translations, which is not a good user experience. This problem is known as the "Flash of Untranslated Content", or FoUC. - -When adding a new string to Oppia's HTML code, please take into account the following tips to prevent FoUC: -- The FoUC behaves differently in the preferred language (English) and all the other languages. Please manually check that there is no FoUC in both cases. -- Add the translation key inside the html tag as the value of the translate attribute. This will prevent the key from being shown briefly -- instead, the location of the string will remain empty in the interim. -- If the string is in a very visible location and there is FoUC in the preferred language, add the key and the translation into the `DEFAULT_TRANSLATIONS` constant defined in the file [i18n.js](https://github.com/oppia/oppia/blob/develop/core/templates/i18n.js). - -### Placeholders and tooltips - -Remember to translate placeholders and tooltips! For tooltips, use the translate filter on the tooltip attribute value. For example: - - tooltip="<['I18N_GALLERY_VIEWS_TOOLTIP' | translate]>" - -For placeholders, use the attribute `ng-attr-placeholder` instead of `placeholder`. As a value for this attribute, apply a translate filter to the key. For example: - - ng-attr-placeholder="<['I18N_FORMS_TYPE_NUMBER' | translate]>" - -### Plurals and gender - -Angular translate supports pluralization and representation of different genders using the [messageformat library](https://github.com/SlexAxton/messageformat.js/). For example, if you need to translate the html code - - Select <[choices]> choices. - Select one choice. - Select no choice. - -you must replace this code with: - - }” translate-interpolation="messageformat"> - -and add the translation into the [en.json file](https://github.com/oppia/oppia/blob/develop/assets/i18n/en.json) with the following format: - - “TRANSLATION_KEY”: “{choicesValue, plural, =0{Select no choice.} one{Select one choice.} other{Select # choices.}}” - -For a more complete tutorial, refer to the [angular translate guide](http://angular-translate.github.io/docs/#/guide/14_pluralization) and the [messageformat documentation](https://github.com/SlexAxton/messageformat.js/). - -### Testing - -In e2e tests, to check that a page has no untranslated keys: call the helper function `ensurePageHasNoTranslationIds`, which is located in [webdriverio_utils/general.js](https://github.com/oppia/oppia/blob/develop/core/tests/webdriverio_utils/general.js). - -Also, Karma tests may generate 404 warnings, as the required locale files aren't available in the Karma test environment. To overcome this, add the following line in the first part of your Karma test: - - beforeEach(module('oppia', GLOBALS.TRANSLATOR_PROVIDER_FOR_TESTS)); - -## How to fetch new translations from translatewiki - -Note that we generally do this on a monthly basis. Here are the steps that we follow: - -1. Checkout the `translatewiki-prs` branch. -2. Merge `develop` into `translatewiki-prs` and resolve all conflicts (usually by accepting the changes from `translatewiki-prs`). -3. Run `python -m scripts.run_backend_tests --test_target=core.controllers.base_test` on that branch. This will validate the I18n files. If any errors arise, they need to be fixed. -4. Create a PR (similar to [this one](https://github.com/oppia/oppia/pull/20706)) that brings the new translations from translatewiki into develop. - -The reason we cannot fully automate this yet is because of step 3. Sometimes, the crowdsourced translations on translatewiki incorrectly handle syntax (e.g. HTML tags in the original strings do not show up in the translated text) and this is an issue that needs to be fixed manually. - -## How to add a new translation language - -When a language has reached 90+% completion on [translatewiki](https://translatewiki.net/w/i.php?title=Special:MessageGroupStats&group=oppia), we can add it to the website. - -To do this: in [feconf.py](https://github.com/oppia/oppia/blob/develop/feconf.py), add a new entry to the variable `SUPPORTED_SITE_LANGUAGES` representing the language code and the language name. - -After this, you should be able to see the new language listed in the Oppia splash page and translate the site using the language dropdown in the footer. diff --git a/Adding-static-assets.md b/Adding-static-assets.md deleted file mode 100644 index a68fb7ce..00000000 --- a/Adding-static-assets.md +++ /dev/null @@ -1,32 +0,0 @@ -Oppia uses cache slugs with static resources to cut down on bandwidth requirements for a user. This requires a developer to use appropriate methods (depending on the static resource required) defined in `/core/templates/domain/utilities/UrlInterpolationServiceSpec.js`. - -**Common steps to use these methods**: -1. Include/import `UrlInterpolationServiceSpec.js` in the corresponding html and controller files. -2. Expose the method to the html using `$scope`, -eg. `$scope.getStaticResourceUrl = (UrlInterpolationService.getStaticResourceUrl);` -3. Call the method using angular tags in the html, -eg. `` - -Depending on the static resource type we have the following methods: - -1. **getStaticResourceUrl(resourcePath)**: - For css, js and extension resources. - Usage: - `` - Example: - `` - -2. **getStaticImageUrl(imagePath)**: - This method should be used to reference image files present in `/assets/images` and `imagePath` passed in the method should be relative to `/assets/images` and start with a forward slash. -Example: -`` - -3. **getGadgetImgUrl(gadgetType)**: -This method given a gadget type, returns the complete url path to that gadget type image. -Example: -`` - -4. **getInteractionThumbnailImageUrl(interactionId)**: -This method given an interaction id, returns the complete url path to the thumbnail image for the interaction. -Example: -`` diff --git a/Analyzing-the-Codebase.md b/Analyzing-the-Codebase.md deleted file mode 100644 index 481aee91..00000000 --- a/Analyzing-the-Codebase.md +++ /dev/null @@ -1,90 +0,0 @@ -## Understanding what a function arg means - -_If you're not sure what any argument in a function means, the following guidelines may help you figure it out._ - -First, check whether the arg name is decipherable. A lot of information about the arg can often be obtained by just reading the name. For example, if there is an arg named 'exploration_dicts', you can reasonably establish that this arg is a list of dictionary representations of the 'Exploration' object. - -If that didn't work, another thing you could try would be to figure out how this arg is initialised when the function is called. For example, assume a function: - -``` -def func(some_arg): - return some_arg.some_field -``` - -This doesn't make a lot of sense at first glance since it doesn't offer any information about the arg. But, if you search the file for where this function has been called: - -``` -some_arg = SomeObject(some_field, some_field2) -func(some_arg) -``` - -this tells us that the arg to this function in this case is an object of type 'SomeObject'. This, and other contextual clues, can help us decipher the meaning of the argument. - -Note that, in some cases, the function might be called from a different file entirely, so it would also be a good idea to 'grep' through the codebase to find out where the function is being called from. You can do this by `grep "thing-to-grep" . -r --exclude-dir=third_party --exclude_dir=build --exclude-dir=backend_prod_files` (replace "thing-to-grep" with the phrase that you want to search for). - -*** - -**Now, let's go through a full example of analyzing a function for the meaning of its args.** - -There is a function defined in scripts/custom_lint_checks.py: - -``` -def check_single_constructor_params(self, class_doc, init_doc, class_node): - if class_doc.has_params() and init_doc.has_params(): - self.add_message( - 'multiple-constructor-doc', - args=(class_node.name,), - node=class_node) -``` - -We want to decipher the meaning of the args 'class_doc', 'init_doc' and 'class_node'. - -### 'class_doc' - -It's unclear what this means from the name, so let's search the same file for function call for `check_single_constructor_params`. We find this block of code: - -``` -class_node = checker_utils.node_frame_class(node) - if class_node is not None: - class_doc = docstrings_checker.docstringify(class_node.doc) - self.check_single_constructor_params( - class_doc, node_doc, class_node) -``` - -within another function `check_functiondef_params(self, node, node_doc)`. - -By looking at the line before the function call, we can understand that the `class_doc` arg has to be the return value of `docstrings_checker.docstringify(class_node.doc)`. - -Traversing to the `docstrings_checker.py` file and searching for the `docstringify` method: - -``` -def docstringify(docstring): - for docstring_type in [GoogleDocstring]: - instance = docstring_type(docstring) - if instance.is_valid(): - return instance - - return _check_docs_utils.Docstring(docstring) -``` - -This tells us that the return value of this function is of type `_check_docs_utils.Docstring(docstring)`. Now, checking the imports at the top of the page shows: - -`from pylint.extensions import _check_docs_utils` - -So, the pylint.extensions._check_docs_utils has a class called `Docstring` defined, and this is the type of `class_doc`. (This can be verified by looking at the pylint source code, since pylint is an open source library). - -### 'init_doc': - -Similarly, following the above reasoning, the `init_doc` can also be reasonably estimated to be an argument of type `Docstring`. Since the function in question is titled `check_functiondef_params`, the `init_doc` logically comes out to be "the Docstring class instance that represents the docstrings of the constructor for a class." - -### 'class_node': - -Searching around the custom_lint_checks.py file for occurrences of node and a possible type relation, we come across: - -``` -func_node = node.frame() - if not isinstance(func_node, astroid.FunctionDef): - return -``` - -inside the function 'visit_raise()'. This tells us that the 'func_node' is of type `astroid.FunctionDef`. Hence, we can infer that `class_node` is of type `astroid.ClassDef`. \ No newline at end of file diff --git a/Angular-Migration.md b/Angular-Migration.md deleted file mode 100644 index 068dc7c9..00000000 --- a/Angular-Migration.md +++ /dev/null @@ -1,1059 +0,0 @@ -## Overview - -Angular is an app-design framework and development platform for creating efficient and sophisticated apps. - -Currently, Oppia is in a hybrid state where we have both Angular and AngularJS. This makes our application slow and bulky. The codebase has duplicate libraries since many of the AngularJS libraries are not compatible with Angular. This project aims to migrate the entire codebase to Angular. The benefits of doing this are: - -* Improved Developer Experience: - - * Developing when the application is a hybrid state opens us to a whole host of complicated errors which are in some cases not solvable. - * Angular is being actively maintained and comes out with a lot of new features that aid development. - -* Improved User Experience: - - * When the codebase is completely migrated, the developers will focus their efforts on making new features for the website rather than fixing nasty errors that pop up because of the hybrid state. - * Decreased page loading times as a result of not bundling AngularJS anymore. - * Better application performance in general. - -The project plan will be iterative in nature. We will migrate the services first and then the controllers and directives. The services will be migrated in dependency order. For example, if A depends on B and B depends on C, we will migrate in the order C, B, and then A. - -### Testing videos - -**Note: Angular Migration Pull Requests must be accompanied with a video showing the before and after effects of their change to ensure that nothing is broken. This ensures faster review and a lower risk of reverted PRs** - -## Angular migration tracker - -The [angular migration tracker](https://docs.google.com/spreadsheets/d/1L9Udn-XT6Lk1qaTBUySTw1AnhvQMR-30Qry4rfd-Ovg/edit?usp=sharing) holds the record of which services are to be migrated. The issue [#8472](https://github.com/oppia/oppia/issues/8472) holds a subset of those services that can be migrated without any major blockers. - -## Implementation details to migrate services - -1. Import the following dependencies: - - ```js - import { downgradeInjectable } from '@angular/upgrade/static'; - import { Injectable } from '@angular/core'; - ``` - -2. If the services uses `$http`, import `HttpClient` as a dependency, also import: - - ```js - import { HttpClient } from '@angular/common/http'; - ``` - -3. Change the AngularJS factory definition to and Angular class definition as follows: - - ```js - angular.module('oppia').factory('ServiceName',['dependency1', function(dependency1) { - ``` - - to - - ```js - import { dependency1 } from ... // to be added at the top of the file - .. - .. - export class ServiceName { - ``` - -4. Add a decorator above the class definition: - - ```js - @Injectable({ - providedIn: 'root' - }) - export class ServiceName { - ... - ``` - -5. Add a constructor for the class and inject the dependencies: - - ```js - constructor( - private service1: Service1, - private service2: Service2) {} - ``` - -6. Change `$http.get` requests in the service as follows: - - (a) Change `$http.get` to `this.http.get`: - - ```js - $http.get(url).then(function(response) { - dataDict = angular.copy(response.data); - ``` - - to - - ```js - this.http.get( - url).toPromise().then( - (response) => { - ``` - - The `dataDict` is not required in Angular services. You can directly use the `response` variable. - - (b) Search in the codebase for where the service is used to obtain results from get requests and change `response.data` to `response`. - - (c) Return the `errorCallback` (the reject function) with `errorResponse.error.error` as follows: - - ```js - (errorResponse) => { - errorCallback(errorResponse.error.error); - } - ``` - - (d) Add `$rootScope.$applyAsync()` in the controller/directive that is resolving the HTTP request similar to how it is added [here](https://github.com/oppia/oppia/pull/8427/files#diff-ecf6cefd0707bcbafeb6a0b4009aa60cR78). To do so, perform a global search in the codebase for the function with the HTTP request. For example, if the service is `SkillBackendApiService` and the function in which the HTTP request is made is `fetchSkill`, then search the codebase for `SkillBackendApiService.fetchSkill` and add `$rootScope.$applyAsync()` as follows: - - ```js - SkillBackendApiService.fetchSkill(...).then((...) => { - //resolve function - ... - $rootScope.$applyAsync() //add here - }, (...) => { - ... - //reject function - } - ); - ``` - - Do this for all functions that have `http` calls. - -7. Change `$http.put` or `$http.post` requests as follows: - - (a) Change `$http.post/put` to `this.http.post/put` - - ```js - $http.post(url).then(function(response) { - ... - ``` - - to - - ```js - this.http.post( - url).toPromise().then( - (response) => {...; - ``` - - (b) Add `$rootScope.$applyAsync()` wherever the function with the HTTP request is used. For example, see the changes [here](https://github.com/oppia/oppia/pull/8427/files#diff-ecf6cefd0707bcbafeb6a0b4009aa60cR78). You can find usages of the function just like you found usages when migrating `$http.get` calls in the previous step. - - -8. If you are migrating a service that is named as `.*-backend-api.service.ts`, then please return a domain object and not a dict in the `successCallback` function. For example take a look at [PR #9505](https://github.com/oppia/oppia/pull/9505/files#diff-05de50229b44c01bdaeac172928b514dR64), where the domain object is created via an object factory. You also need to change the piece of code where this response is used because the response is now a domain object instead of a dict. If there is no specific object factory to alter the response to a domain object, create one similar to how it is done in this [change](https://github.com/oppia/oppia/pull/9570/files#diff-09e3c3999c18dabdf2ddedf6e3e250f8R1). - - Topic domain objects need to contain properties that are being read from the backend. Therefore, the topic domain object does not depend on the service being migrated, but rather the expected return value of the function. For example, in `SkillBackendApiService`, the function `fetchSkill` will clearly return a `Skill` object. Note that `SkillObjectFactory.ts` already exists, so we don't need to create it. But if there is no corresponding Object Factory, you need to create one similar to how `SkillObjectFactory` is created. Next, we take the response from the backend and instead of `successCallback(response)`, we resolve `successCallback(SkillObjectFactory.createFromBackendDict(response))`. This passes the frontend `Skill` object to functions that call `fetchSkill` when the promise gets resolved. - - Since before you migrated the file, the calling functions were expecting a backend dict object, the references need to be changed as well. To do this, do a global search in the code-base for the function, e.g. `SkillBackendApiService.fetchSkill` and refactor the code inside the resolve function to reflect that the parameter is now a `Skill` object and not a backend dict object. - - Please note that interfaces/properties in Object Factories and the `.*-backend-api.service.ts` could be in snake_case. If that is the case, please surround them with single quotes as in `'some_property'`. Except for these two categories, all the properties inside all other files should be camel case, e.g. `someProperty`. - -9. For functions in the service, add type definitions for all the arguments as well as return values. - - **Note:** For complex types or some type that is being used over functions or files we can declare an interface. For example in the file [rating-computation.service.ts](https://github.com/oppia/oppia/blob/develop/core/templates/components/ratings/rating-computation/rating-computation.service.ts) we have an export interface to declare the type `RatingFrequencies`. In the same file, we also have a function named static, which is used by the functions of the class itself. - -10. For functions which are private to the service (used as helper functions), add the private keyword. - -11. At the end of the file, add: - - ```js - angular.module('oppia').factory('ServiceName', downgradeInjectable(ServiceName)); - ``` - -For an example of migrating a service, see [this pull request](https://github.com/oppia/oppia/pull/10693/files). - -## Implementation details to migrate tests - -1. Remove all `beforeEach()` blocks and any other service that is not needed in the test file. - -2. Convert all the function keywords to fat arrow functions like this: - - ```js - describe('abc', function() { ... }); - ``` - - to - - ```js - describe('abc', () => { .. }); - ``` - -3. Import TestBed in your spec file - - ```js - import { TestBed } from '@angular/core/testing'; - - import { ServiceName } from ... - ``` - - If your test is for a service that makes HTTP requests, you also need to import the following: - - ```js - import { HttpClientTestingModule, HttpTestingController } from - '@angular/common/http/testing'; - import { TestBed, fakeAsync, flushMicrotasks } from '@angular/core/testing'; - ``` - -4. Add a beforeEach block that creates an instance of service you want to test: - - ```js - beforeEach(() => { - serviceInstance = TestBed.get(ServiceName); - }); - ``` - - (a) If your spec file needs any pipes (filters in angular), import them and add it to the providers in the TestBed configuration - - ```js - beforeEach(() => { - TestBed.configureTestingModule({ - providers: [CamelCaseToHyphensPipe, ConvertToPlainTextPipe] // Any pipe that is required - }); - instance = TestBed.get(ServiceName); - ``` - - (b) If your spec file tests a service that makes HTTP requests, you need to make an `HttpClientTestingModule` and add an `afterEach` statement to check there are no pending requests after each test. For example: - - ```js - beforeEach(() => { - TestBed.configureTestingModule({ - imports: [HttpClientTestingModule], - }); - httpTestingController = TestBed.get(HttpTestingController); - instance = TestBed.get(ServiceName); - }); - - afterEach(() => { - httpTestingController.verify(); - }); - ``` - -5. For each test, replace the name of the service with the instance name that is created above using TestBed. - -6. If your spec file is for a service that makes HTTP requests then: - - (a) Convert each individual test defined in it block as follows: - - ```js - it('should ...', fakeAsync(() => { - . - . - })); - ``` - - (b) Change the test to create an HTTP request via httpTestingController. Here's an example from `topic-viewer-backend-api.service.spec.ts`: - - ```js - $httpBackend.expect('GET', '/topic_data_handler/0').respond( - sampleDataResults); - TopicViewerBackendApiService.fetchTopicData('0').then( - successHandler, failHandler); - $httpBackend.flush(); - ``` - - to - - ```js - topicViewerBackendApiService.fetchTopicData('0').then( - successHandler, failHandler); - var req = httpTestingController.expectOne( - '/topic_data_handler/0'); - expect(req.request.method).toEqual('GET'); - req.flush(sampleDataResults); - - flushMicrotasks(); - ``` - -## Implementation details to migrate directives - -There are two parts to this migration: the TS file and the HTML file. - -### Migrating the logic part (ts file) - -Here are the steps to migrate the logic part. - -#### 1. Create a basic component in the directive file - -Import `Component` to the file: - -```js -import { Component } from '@angular/core'; -``` - -Then take a look at the directive declaration. For example if the directive is declared like this: - -```js -angular.module('oppia').directive('conceptCard', [ - 'UrlInterpolationService', function(UrlInterpolationService) { - return { - restrict: 'E', - scope: {}, - bindToController: { - getSkillIds: '&skillIds', - index: '=' - }, - templateUrl: UrlInterpolationService.getDirectiveTemplateUrl( - '/components/concept-card/concept-card.directive.html'), - controllerAs: '$ctrl', - controller: [ - '$scope', '$filter', '$rootScope', - 'ConceptCardBackendApiService', 'ConceptCardObjectFactory', -``` - -then add the following at the end of the file: - -```js -@Component({ - selector: 'concept-card', - templateUrl: './concept-card.directive.html', - styleUrls: [] -}) -export class ConceptCardComponent {} -``` - -Some points to note: - -* Please keep in mind the name of the directive declared. In this case, it is 'conceptCard'. -* The directive name converted to kebab-case is the selector (conceptCard -> concept-card). -* The name of the class is in CamelCase (note the first letter is capital) suffixed with "Component". So conceptCard -> ConceptCardComponent. -* The template of the directive (in 99% of cases) exists in the same folder as the directive.ts file. - -#### 2. Import and inject the dependencies - -Suppose the AngularJS code has the following dependencies: - -```js -controller: [ - '$scope', '$rootScope', - 'ConceptCardBackendApiService', 'ConceptCardObjectFactory', -``` - -The first two dependencies are interesting ones. They don't have a direct equivalent in Angular. In most cases, you will find something like `$scope.someVariable = `, `$scope.$onDestroy`, `$scope.$onInit`, and `$rootScope.$applyAsync()`. In other cases, contact @srijanreddy98. - -Next, consider the other two dependencies ('ConceptCardBackendApiService', 'ConceptCardObjectFactory'). These are called injectables as they are "injected". These go into your constructor like this: - -1. Import these two services into the directive. - - ```js - import { ConceptCardBackendApiService } from 'service/some-service.ts'; - import { ConceptCardObjectFactory } from 'services/some-other-service.ts'; - ``` - -2. Create a constructor for the class you made in the previous step and add those injectables there: - - ```js - export class ConceptCardComponent { - - constructor( - private conceptCardBackendApiService: ConceptCardBackendApiService, - private conceptCardObjectFactory: ConceptCardObjectFactory - ) {} - - } - ``` - -Sometimes, you will also see dependencies like $window, $log, $timeout etc. - -* For `$window`: Use `window-ref.service.ts`. In the constructor, with other dependencies, inject `WindowRef`: - - ```js - constructor( - ... - private windowRef: WindowRef - ) {} - ``` - - Then instead of `window`, use `windowRef.nativeWindow`, e.g. `windowRef.nativeWindow.location.href`. Also, instead of `location` for setting URLs, use `location.href`. - -* `For $timeout`: Use `setTimeout` - -* `For $log`: Use `logger.service.ts` - -**Notice the casing very carefully. The service is ConceptCardBackendApiService but its instance is called conceptCardBackendApiService (with a small c)** - -#### 3. Adding OnInit / OnDestroy - -You will notice that in almost every directive you have `ctrl.$onInit` and/or `ctrl.$onDestroy`. The equivalent of this in angular is `ngOnInit` and `ngOnDestroy`. - -First, you need to import OnInit from '@angular/core'; Then implement it in the class by changing the class declaration to `export class ConceptCard implements OnInit {` and adding an `ngOnInit() {}` below the constructor. After this step the component class would look like this: - -```js -import { Component, OnInit } from '@angular/core'; -... -export class ConceptCardComponent implements OnInit { - - constructor( - private conceptCardBackendApiService: ConceptCardBackendApiService, - private conceptCardObjectFactory: ConceptCardObjectFactory - ) {} - - ngOnInit(): void { - ... - } -} -``` - -Do the same for `onDestroy`. - -#### 4. Changing the bindToController - -If there is some value in bindToController, then import `Input` from `@angular/core`; - -There are four "syntaxes" that you could run into when trying to migrate bindToController: - -1. `'@'` -2. `'<'` -3. `'='` -4. `'&'` - -##### The syntax for `'@'` - -```js - layoutType: '@', -``` - -will change to - -```js - @Input() layoutType: string; -``` - -**Note that `'@'` is always a string but `'<'` can be of any type (string, number, object or custom types).** - -##### The syntax for '<' - -```js - layoutAlignType: '<', -``` - -changes to - -```js - @Input() layoutAlignType: string; -``` - -(The type of layoutAlignType being string is an example. please be aware of the type used in your case). - -Take a look at the directive name (in this case it is conceptCard). Now do a global search for e.g. `, -``` - -Note: If the syntax looks like `skillIds: '&'`, then just change it to: - -```js -@Input() skillIds: Array, -``` - -(`Array` is an example. please be aware of the type used in your case). - -Next, change all cases of `ctrl.getSkillIds()` to `this.skillIds`. (Notice the parentheses were also removed.) - -Take a look at the directive name (in this case it is conceptCard). Now do a global search for e.g. ``, `xyz` is a string or a variable? To clarify this ambiguity, we use interpolation for variables. Interpolation has 2 forms: - - * `` (The `[]` indicates that the string is a variable) - * `` - - We use the first method in all cases except when the variable is interspersed with other text. e.g. `` - -3. Do not use interpolation with properties marked with [prop], or events. These automatically assume that a variable is passed. - -##### The syntax for `=` - -In most cases, the `=` is the same as `<` when looked at from an Angular2+ perspective, so just follow the steps given for `<` migration. - -#### 5. Start separating the other functions - -Take a look at this example directive code: - -```js -var ctrl = this; -ctrl.isLastWorkedExample = function() { - return ctrl.numberOfWorkedExamplesShown === - ctrl.currentConceptCard.getWorkedExamples().length; -}; - -ctrl.showMoreWorkedExamples = function() { - ctrl.explanationIsShown = false; - ctrl.numberOfWorkedExamplesShown++; -}; - -ctrl.$onInit = function() { - ctrl.conceptCards = []; - ctrl.currentConceptCard = null; - ctrl.numberOfWorkedExamplesShown = 0; - ctrl.loadingMessage = 'Loading'; - ConceptCardBackendApiService.loadConceptCards( - ctrl.getSkillIds() - ).then(function(conceptCardObjects) { - conceptCardObjects.forEach(function(conceptCardObject) { - ctrl.conceptCards.push(conceptCardObject); - }); - ctrl.loadingMessage = ''; - ctrl.currentConceptCard = ctrl.conceptCards[ctrl.index]; - ctrl.numberOfWorkedExamplesShown = 0; - if (ctrl.currentConceptCard.getWorkedExamples().length > 0) { - ctrl.numberOfWorkedExamplesShown = 1; - } - // TODO(#8521): Remove when this directive is migrated to Angular. - $rootScope.$applyAsync(); - }); -}; -``` - -Look at all lines matching the pattern `ctrl.someVariable = function(...) {...`. - -In the component you made in step one, just create those functions without the `ctrl` and `= function`: - -```js -export class ConceptCardComponent implements OnInit { - -constructor( - private conceptCardBackendApiService: ConceptCardBackendApiService, - private conceptCardObjectFactory: ConceptCardObjectFactory -) {} - - ngOnInit() { - } - - isLastWorkedExample() { - } - - showMoreWorkedExamples() { - } - -} -``` - -#### 6. Create class members (variables) - -Till now we only looked at `ctrl.someVariable = function()`. Now we will look at all the other cases. Take a look at this directive code: - -```js -var ctrl = this; -ctrl.isLastWorkedExample = function() { - return ctrl.numberOfWorkedExamplesShown === - ctrl.currentConceptCard.getWorkedExamples().length; -}; - -ctrl.showMoreWorkedExamples = function() { - ctrl.explanationIsShown = false; - ctrl.numberOfWorkedExamplesShown++; -}; - -ctrl.$onInit = function() { - ctrl.conceptCards = []; - ctrl.currentConceptCard = null; - ctrl.numberOfWorkedExamplesShown = 0; - ctrl.loadingMessage = 'Loading'; - ConceptCardBackendApiService.loadConceptCards( - ctrl.getSkillIds() - ).then(function(conceptCardObjects) { - conceptCardObjects.forEach(function(conceptCardObject) { - ctrl.conceptCards.push(conceptCardObject); - }); - ctrl.loadingMessage = ''; - ctrl.currentConceptCard = ctrl.conceptCards[ctrl.index]; - ctrl.numberOfWorkedExamplesShown = 0; - if (ctrl.currentConceptCard.getWorkedExamples().length > 0) { - ctrl.numberOfWorkedExamplesShown = 1; - } - // TODO(#8521): Remove when this directive is migrated to Angular. - $rootScope.$applyAsync(); - }); -}; -``` - -Looking at all the other `ctrl.` declarations we find `ctrl.numberOfWorkedExamplesShown`, `ctrl.currentConceptCard`, `ctrl.explanationIsShown`, `ctrl.numberOfWorkedExamplesShown++``, `ctrl.conceptCards`, `ctrl.loadingMessage`, etc. - -Now we have to define them as class members. In order to do so just remove `ctrl.` from the front of the variable and add them to the class above the constructor. For example: - -```js -export class ConceptCardComponent implements OnInit { -numberOfWorkedExamplesShown: number = 0; -currentConceptCard: ConceptCard; -explanationIsShown: boolean = false; -conceptCards: Array; -loadingMessage: string = ''; - -constructor( - private conceptCardBackendApiService: ConceptCardBackendApiService, - private conceptCardObjectFactory: ConceptCardObjectFactory -) {} - - ngOnInit() { - } - - isLastWorkedExample() { - } - - showMoreWorkedExamples() { - } - -} -``` - -#### 7. Copy the contents of the functions - -Anything with `ctrl.` becomes `this.`. For example: - -```js -ctrl.isLastWorkedExample = function() { - return ctrl.numberOfWorkedExamplesShown === - ctrl.currentConceptCard.getWorkedExamples().length; -}; -``` - -becomes - -```js -isLastWorkedExample(): boolean { - return this.numberOfWorkedExamplesShown === - this.currentConceptCard.getWorkedExamples().length; -} -``` - -**Note the dependency injections also get the `this.` prefix.** - -In the controller.$OnInit function we have: - -```js -ConceptCardBackendApiService.loadConceptCards( - ctrl.getSkillIds() - ) -``` - -This will become: - -```js -this.conceptCardBackendApiService.loadConceptCards( - this.skillIds - ) -``` - -#### 8. Add downgrade statement - -Import downgradeComponent from '@angular/upgrade/static'. Then add the following downgrade statement to the end of the file: - -```js -angular.module('oppia').directive( - 'conceptCard', downgradeComponent( - {component: ConceptCardComponent})); -``` - -#### 9. Change the name of the file - -Rename the file from `*directive|controller.ts` to `*component.ts`. Import this component into the corresponding module page and add it in the `declarations` and `entryComponents`. You can find the corresponding module page as follows: - -* For directives in the pages folder, they will be in the same sub-folder as `*.module.ts` -* For directives in the components folder, the module page is `shared-component.module.ts` - - -### Migrating an HTML file - -This is the easier part of migration but still should be migrated carefully. Here are the migration patterns: - -#### Changing `<[ ... ]>` to `{{ ... }}` - -The interpolation in angular uses `{{ }}` to interpolate. So change `<[ ]>` to `{{ }}`. For example, `
  • <[credit]>
    • ` becomes `
  • {{ credit }}
    • `. - -#### Removing `$ctrl` - -By default in Angular, all the variables of the class you migrated are available in HTML (unlike AngularJS where variables were prefixed by $ctrl or had to attached to the $scope). Remove all `$ctrl.` from HTML. For example, `
  • <[$ctrl.credits]>
    • ` becomes `
  • {{ credit }}
    • `. - -#### Change `ng-if` to `*ngIf` - -For example, `
  • <[$ctrl.credits]>
    • ` becomes `
  • {{ credit }}
    • `. - -#### Change `ng-repeat` to `*ngFor` - -| AngularJS | Angular2+ | -|-----|------| -|`
    ` | `
    `| -|``| `
    `| -|`<[item.letter]>` | `{{ credit.letter }}` | -|`
      ` | `
        `| -|`
      • <[credit]>
        • ` | `
      • {{ name }}
        • `| -|`
      ` | `
    `| -|`
    `| `
    `| - -### Other tags - -* `ng-cloak`: Remove. - -* `ng-class`: Change to `ngClass`. - -* `ng-show`/`ng-hide`: Follow [GeeksForGeeks](https://www.geeksforgeeks.org/what-is-the-equivalent-of-ngshow-and-nghide-in-angular-2/). - -#### HTML tag attributes - -If you see any HTML attribute which looks like `
    `, then just change it to `
    `. - -If you `ng-src`/`ng-srcset`, change it to `[src]`/`[srcset]`. - -#### HTML events - -All the events in HTML are available in angular. Example `onClick` becomes `(click)`, `ng-click` becomes `(click)`, and `ng-submit` becomes `(ngSubmit)`. - -#### Translations - -You may come across the following: - -```html - -``` - -Convert it like this: - -```html - - -``` - -If there are no translate-values, simply use `"'I18N_VARIABLE_NAME' | translate"` - -Please note the single-quote marks around `I18N_VARIABLE_NAME`. - -#### CSS updates - -There may be some style updates required to make sure that the pages look exactly like before. You can find the changes here: https://github.com/oppia/oppia/pull/9980/files#diff-1d203da36aa74eef4c39b05a27eafbaeR40-R46. Besides this, styles that contain the directive name now need to be enclosed in a `
    ` tag. For example compare [this code from before migration](https://github.com/oppia/oppia/pull/9957/files#diff-25860f544f47c16a020aff8bb0c389fdL1-L3) to the [migrated code](https://github.com/oppia/oppia/pull/9957/files#diff-45cbfaec92adcc709712a85df070f455R1-R4). - -## Testing your Pull Request - -1. Ensure your frontend tests pass - - Python: - ```console - python -m scripts.run_frontend_tests - ``` - - Note: If your migrated service involves HTTP calls and when you run the frontend test your frontend test fail for some other service (One error that might pop is `Error: No pending request to flush !`) then go ahead and migrate the failing tests for the other service too. You might have guessed that in such a case we have migrated a service which is now making HTTP calls in Angular using HttpClient but some other service that is issuing HTTP requests to this service is still testing by making calls via AngularJS HTTP module (using $httpBackend). Go through this [PR #9029](https://github.com/oppia/oppia/pull/9029/files), wherein `question-creation.service` and `question-backend-api.service` are migrated to Angular and we went ahead to change relevant tests in `questions-list.service.spec`. - -2. Ensure there are no typescript errors: - - Python: - ```console - python -m scripts.typescript_checks - ``` - -3. Ensure there are no linting errors: - - Python: - ```console - python -m scripts.linters.run_lint_checks - ``` - -4. Test manually. See where the directive you have migrated is being used. You can do this by seeing where it's corresponding `selector` is being used. Then check whether functionality that you have implemented works as expected (like on the develop branch). Add a screen recording of the places where the directive is used when you open your PR! - -## Implementation details to refactor Object Factories - -### 1. Remove certain imports - -The following imports will no longer be required: - -```js -import { downgradeInjectable } from '@angular/upgrade/static'; -import { Injectable } from '@angular/core'; -``` - -### 2. Change the file overview - -Change the file overview to not include the term Object Factory. Instead, replace it with the word "model". - -For example: - -| Before | After | -|--------|-------| -|`Factory for creating new frontend instances of ParamMetadata`|`Model class for creating new frontend instances of ParamMetadata`| - -### 3. Move functions from ObjectFactory class - -Locate the class in the file whose name is suffixed by ObjectFactory. Move all the functions from that ObjectFactory class (except the constructor) and add them to the other class in the file. Add `static` in front of all the functions you moved. - -Before: - -```js -export class ParamMetadata { - action: string; - paramName: string; - source: string; - sourceInd: string; - constructor( - action: string, paramName: string, source: string, sourceInd: string) { - this.action = action; - this.paramName = paramName; - this.source = source; - this.sourceInd = sourceInd; - } -} - -@Injectable({ - providedIn: 'root' -}) -export class ParamMetadataObjectFactory { - createWithSetAction( - paramName: string, source: string, sourceInd: string): ParamMetadata { - return new ParamMetadata( - ExplorationEditorPageConstants.PARAM_ACTION_SET, paramName, source, - sourceInd); - } - - createWithGetAction( - paramName: string, source: string, sourceInd: string): ParamMetadata { - return new ParamMetadata( - ExplorationEditorPageConstants.PARAM_ACTION_GET, paramName, source, - sourceInd); - } -} -``` - -After: - -```js -export class ParamMetadata { - action: string; - paramName: string; - source: string; - sourceInd: string; - constructor( - action: string, paramName: string, source: string, sourceInd: string) { - this.action = action; - this.paramName = paramName; - this.source = source; - this.sourceInd = sourceInd; - } - - static createWithSetAction( - paramName: string, source: string, sourceInd: string): ParamMetadata { - return new ParamMetadata( - ExplorationEditorPageConstants.PARAM_ACTION_SET, paramName, source, - sourceInd); - } - - static createWithGetAction( - paramName: string, source: string, sourceInd: string): ParamMetadata { - return new ParamMetadata( - ExplorationEditorPageConstants.PARAM_ACTION_GET, paramName, source, - sourceInd); - } -} - -@Injectable({ - providedIn: 'root' -}) -export class ParamMetadataObjectFactory { - -} -``` - -Next, remove the @Injectable and the object factory class. Specifically, remove the code that looks like this: - -```js -@Injectable({ - providedIn: 'root' -}) -export class ParamMetadataObjectFactory { - -} -angular.module('oppia').factory( - 'ParamMetadataObjectFactory', - downgradeInjectable(ParamMetadataObjectFactory)); - -``` - -### 4. Remove the imports and class listings / instances - -Remove any references to the object factory from: - -- angular-service.index.ts -- oppia-angular-root.component.ts -- UgradedServices.ts - -### 5. Rename the file - -The file you are working on will be named either `*-object.factory.ts` or `*ObjectFactory.ts`. You need to remove the object factory part and add .model.ts instead. For example, `PlaythroughObjectFactory.ts` should be renamed to `playthrough-object.model.ts and `skill-summary-object.factory.ts` should be renamed to `skill-summary.model.ts`. - -### 6. Change the import (as you have changed the name of the file) and its usage around the codebase. - -**Make sure to search the codebase for the function name to make sure you find all usages.** - -For example, let one of the functions that you moved before (in step 3) be `createWithGetAction`. - -#### Pattern 1: ParamMetadataObjectFactory.createWithGetAction(...) - -First, make sure `ParamMetadata` has been imported: - -```js -import ParamMetadata from param-metadata.model.ts -``` - -Next, change `ParamMetadataObjectFactory.createWithGetAction()` to `ParamMetadata.createWithGetAction(...)`. Do this for all functions, and then remove any other `ParamMetadataObjectFactory` references left in the file. - -#### Pattern 2. this.paramMetadataObjectFactory.createWithGetAction(...) - -Make sure that ParamMetadata has been imported. Then change `this.paramMetadataObjectFactory.createWithGetAction(...)` to `ParamMetadata.createWithGetAction(...)`. Do this for all functions, and remove `paramMetadataObjectFactory` from the constructor. - -### 7. Changing the spec file - -Each ``*-object.factory.ts` will have its corresponding spec file named `*-object.factory.spec.ts`. You will need to follow the procedure mentioned in step 6 (the previous step), to refactor the spec as well. Note that in spec file `ParamMetadataObjectFactory` could be shortened to `pmof`, so searching by the function name in the spec file will be more accurate. - -**PRs for reference: [#10701](https://github.com/oppia/oppia/pull/10701/), [#10713](https://github.com/oppia/oppia/pull/10713/).** - -## FAQ - -### Common Issues with Migrating Services - -1. Front-end tests fail. This can for various reasons, but the most common one is return types. You will get errors like: ‘a’ is not defined on an object of type ‘X’. Try console logging the object you are receiving actually has the property you’re calling and adjust accordingly. This will mostly happen with HttpResponse objects. - -### Common Issues with Migrating Directives - -1. Error like this: - - ```text - 'some-selector' is not a known element: - 1. If 'some-selector' is an Angular component, then verify that it is part of this module. - 2. 2. If 'some-selector' is a Web Component then add 'CUSTOM_ELEMENTS_SCHEMA' to the '@NgModule.schemas' of this component to suppress this message. - ``` - - This can occur for a couple of reasons: - - * The corresponding external Angular module is not yet integrated into the codebase, e.g. For `ngModel`, you need `FormsModule`. - * It is another un-migrated directive. You need to wrap it in an Angular wrapper and import it into your current module. Do it via the shared component module. Use [#9237](https://github.com/oppia/oppia/pull/9237/files) for reference - * The Angular module has a different selector i.e `md-card` becomes `mat-card` - -### Why do we need @Injectable decorator? - -There are two reasons: - -* Our app is in hybrid state i.e. half Angular and half AngularJS, and we need to downgrade each of our services to AngularJS so that our application runs smoothly. -* To define a class as a service, Angular uses the @Injectable() decorator to provide the metadata that allows Angular to inject it into a component as a dependency. When we provide the service at the root level, Angular creates a single, shared instance of the service and injects it into any class that asks for it. Registering the provider in the @Injectable() metadata also allows Angular to optimize an app by removing the service from the compiled app if it isn't used. - -### Why do we need `$rootScope.$applyAsync` with HTTP requests? - -As you can see in the example here, the directive updates the value when the promise is resolved: - -```js -TopicViewerBackendApiService.fetchTopicData(ctrl.topicName).then( - function(topicDataDict) { - ctrl.topicId = topicDataDict.topic_id; - ctrl.canonicalStoriesList = topicDataDict.canonical_story_dicts; - ctrl.degreesOfMastery = topicDataDict.degrees_of_mastery; - ctrl.skillDescriptions = topicDataDict.skill_descriptions; - ctrl.subtopics = topicDataDict.subtopics; - $rootScope.loadingMessage = ''; - ctrl.topicId = topicDataDict.id; -``` - -Everything was working fine before the migration, but after migration, we noticed that all the values in the above-mentioned function were updated but not propagated to the corresponding HTML file. Searching online yielded [a Stack Overflow post](https://stackoverflow.com/a/21659051) which mentions that: - -> Yes, AngularJS's bindings are "turn-based", they only fire on certain DOM events and on calls to `$applyAsync/$digest`. There are some useful services like `$http` and `$timeout` that do the wrapping for you, but anything outside of that requires calls to either `$applyAsync` or `$digest`._ - -The `$digest` cycle is not running after we've upgraded `$http` to `HttpClient`, so we add `$rootScope.$applyAsync` to explicitly ask Angular to propagate the changes to our HTML. - -### What is TestBed? - -When a service has a dependent service, DI (dependency injector) finds or creates that dependent service. And if that dependent service has its own dependencies, DI finds or creates them as well. To quote from the Angular docs, "As a service tester, you must at least think about the first level of service dependencies but you can let Angular DI do the service creation and deal with constructor argument order when you use the TestBed testing utility to provide and create services." - -### Some Common Migration Queries - -1. What are Promises? - - Promises are exactly what they sound like. In the simplest words, they are a promise to the developer that things will work, what to do when they work, and also when they don’t work. - - They can have three states: - - * Resolved: The caller of the promise has executed as expected. - * Rejected: The caller of the promise didn’t execute as expected - * Pending: The caller is yet to be executed - - What exactly are the `resolve` and `reject` that promises accept? They are simply function calls. For example, `successCallback(abcd)` will give parameter `abcd` to the resolve function when the promise caller is called. - - How is it structured? A promise is called using a `.then()` statement after the function. Note that you cannot put this after any function, only one that returns a promise. Then functions follow. If there is only one function, it is the resolve function. If there are two functions, they are called resolve and reject, respectively. The reject function is used mostly for error handling and unexpected behaviour - - For more reading check out the [MDN Guide](https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Promise)! - - -2. What is `rootScope`? - - Angular has scopes. `$scope` is a local scope, which is bound to exactly one controller. It can local properties and functions of the controller. `$rootScope` on the other hand, is the global scope and can be accessed from everywhere. - - What is `$rootScope.$applyAsync()` and why do we use it? `$rootScope.$applyAsync` is used to update the global properties and variables so that the new state can be used by the function where it is called. As to why it is not updated automatically, the reason is that the Angular DOM basically runs in cycles, and apply causes the changes to be saved. Mostly this is done automatically, but in some cases, we have to do it explicitly. Remember that this function will go in the resolve of the promise! This is because we want the variables to get to their new state in case of expected behaviour. - - For more reference, see [the AngularJS docs on scopes](https://docs.angularjs.org/guide/scope). - - -3. How do I assign types in the Angular file? - - Follow a trail. Some are really simple, but they all follow the same pattern. Keep following the variable through different references to see what type to assign. You can even use other references of that variable. For example, if you wanted to assign a type to a function that returned `WindowRef.nativeWindow`, you would go to the `window-ref.service` file and see that `nativeWindow` returned the `_window` object which had a type `Window`. - - Other times it might be obvious from the name. For example, `SkillList` is obviously an array of Skills. However, double-check these! - - The final way is to use a console log statement. This is good for complicated data types. Run the local development server and log the value whose type you want to know. You’ll see something of type Object with certain properties. Search for these properties in the codebase to find out what type it is! - -4. What are fakeAsync() and flushMicrotasks() and why do we use them? - - For this we need to understand why being synchronous is a problem for tests. Compilers don’t compile code line by line. Instead, they push processes into a queue as they come and the resulting processes are pushed once the first processes end. What this means is that a process whose parent was called earlier may be executed after another whose parent was called later due to how much time the parent processes took. To make an asynchronous function synchronous we use fakeAsync combined with flushMicrotasks. FakeAsync creates a fake asynchronous zone wherein you can control process flow. When flushMicrotasks is called, it flushes the task queues, i.e it waits for the processes to leave the queue before proceeding further. Then, the tests are consistent! - -5. What are MockServices/FakeServices that are in the codebase? - - MockServices are basically just used to imitate real services and provide functionality for tests via inorganically-made function copies of the service. These are faster but don’t test services, so be wary of using them. The reason we use them is that we want to want to test the current service, not the other service, so we just use a small shell to provide the functionality we want. - -6. How can I have constants shared across Angular and AngularJS code? - - The Angular 2+ constants file is named _*.constants.ts_ whereas the AngularJS equivalents of those constants must be in a separate file named _*.constants.ajs.ts_. The constants must be first declared in the Angular constants file and then be declared in the corresponding AngularJS constants file by importing the constants class from the Angular constants file and using that class's properties to declare the AngularJS equivalents. Then import the AngularJS constants class in the module and add it to the `providers` list of the `NgModule`. - - For example, if there is a constant named `SKILL_EDITOR_CONSTANT` that needs to be used in skill editor, then add that constant to the `SkillEditorConstants` class of the file _skill-editor-page.constants.ts_ like this: - - ```js - export class SkillEditorPageConstants { - ... - public static SKILL_EDITOR_CONSTANT = 'constant_value'; - ... - } - ``` - - Now, add the constant to the AngularJS file as well: - - ```js - import { SkillEditorPageConstants } from - 'pages/skill-editor-page/skill-editor-page.constants.ts'; - - ... - oppia.constant('SKILL_EDITOR_CONSTANT', SkillEditorPageConstants.SKILL_EDITOR_CONSTANT); - ... - ``` - - And now you can use the constant in both your AngularJS as well as Angular parts of the code! - -## Contact - -For any queries related to angular migration, please don't hesitate to reach out to **Srijan Reddy (@srijanreddy98)**. diff --git a/Apache-Beam-Jobs.md b/Apache-Beam-Jobs.md deleted file mode 100644 index 7e925841..00000000 --- a/Apache-Beam-Jobs.md +++ /dev/null @@ -1,829 +0,0 @@ -## Table of contents - -* [Introduction](#introduction) -* [Apache Beam Job Architecture](#apache-beam-job-architecture) - * [`Pipeline`s](#pipelines) - * [`PValue`s](#pvalues) - * [`PTransform`s](#ptransforms) - * [`ParDo` and `DoFn`](#pardo-and-dofn) - * [`Map` and `FlatMap`](#map-and-flatmap) - * [`Filter`](#filter) - * [`GroupByKey`](#groupbykey) - * [Example of using `GroupByKey`,`Filter`, and `FlatMap`](#example-of-using-groupbykeyfilter-and-flatmap) - * [`Runner`s](#runners) -* [Writing Apache Beam Jobs](#writing-apache-beam-jobs) - * [1. Subclass the `base_jobs.JobBase` class and override the `run()` method](#1-subclass-the-base_jobsjobbase-class-and-override-the-run-method) - * [2. Override the `run()` method to operate on `self.pipeline`](#2-override-the-run-method-to-operate-on-selfpipeline) - * [3. Have the `run()` method return a `PCollection` of `JobRunResult`s](#3-have-the-run-method-return-a-pcollection-of-jobrunresults) - * [4. Add the job module to `core/jobs/registry.py`](#4-add-the-job-module-to-corejobsregistrypy) -* [Testing Apache Beam Jobs](#testing-apache-beam-jobs) - * [1. Inherit from `JobTestBase` and override the class constant `JOB_CLASS`](#1-inherit-from-jobtestbase-and-override-the-class-constant-job_class) - * [2. Run assertions using a `assert_job_output_is_*` method](#2-run-assertions-using-a-assert_job_output_is_-method) -* [Running Apache Beam Jobs](#running-apache-beam-jobs) - * [Local Development Server](#local-development-server) - * [Production Server](#production-server) -* [Beam guidelines](#beam-guidelines) - * [Do not use NDB put/get/delete directly](#do-not-use-ndb-putgetdelete-directly) - * [Use `get_package_file_contents` for accessing files](#use-get_package_file_contents-for-accessing-files) -* [Common Beam errors](#common-beam-errors) - * [`'_UnwindowedValues' object is not subscriptable` error](#_unwindowedvalues-object-is-not-subscriptable-error) - * [`_namedptransform is not iterable` error](#_namedptransform-is-not-iterable-error) -* [Guidelines for writing beam jobs](#guidelines-for-writing-beam-jobs) -* [Case studies](#case-studies) - * [Case Study: `SchemaMigrationJob`](#case-study-schemamigrationjob) - -## Introduction - -[Apache Beam](https://beam.apache.org/) is used by Oppia to perform large-scale datastore operations. We use Apache Beam jobs mostly as batch operation jobs, examples can be: - - * Count the number of models in the datastore. - * Update a property across all models. - * Delete all models that are no longer needed. - * Delete all models that belong to one user. - * Create stats models from other models. - -Jobs can be triggered manually or automatically. Manually this can be done through the release coordinator page. Automatically jobs can be run through code, this is used together with CRON to schedule jobs to be run at a specific date and time. - -If you're already familiar with Apache Beam or are eager to start writing a new job, jump to the [case studies](#case-studies). Otherwise, you can read the whole page. If you still have questions after reading, take a look at the [Apache Beam Programming Guide][1] for more details. - -## Apache Beam Job Architecture - -Conceptually, an Apache Beam job is just a bunch of steps, each of which transforms some input data into some output data. For example, if you wanted to count how many interactions are in all of Oppia's explorations, you could break that task down into a series of transformations: - -```mermaid -flowchart LR - A(Explorations) -->|Count interactions| B(Counts) -->|Sum| C(Total) -``` - -For more complicated tasks, Apache Beam supports tasks whose transformations form a [directed acyclic graph](https://en.wikipedia.org/wiki/Directed_acyclic_graph), or "DAG." These are just graphs with no cycles. For example, if you wanted to find the ratio of interactions to cards, you could use this DAG: -```mermaid -flowchart TD -E(Explorations) -->|Count interactions| C(Count) -->|Sum| NI(Num Interactions) -E -->|Count cards| C2(Count) -->|Sum| NC(Num Cards) -NC --> |Divide| IC(Interactions / Cards) -NI --> |Divide| IC(Interactions / Cards) -``` -Note that the first example we saw, while linear, is still a DAG! - -In Apache Beam, all jobs are represented as these DAGs. The nodes are represented as [`PValue`](#pvalues) objects, and the edges are represented as [`PTransform`](#ptransforms) objects. [`Pipeline`](#pipelines) objects manage the DAGs, and [`Runner`](#runners) objects actually execute the jobs. - -Next, we'll look at each of these components in more detail. - -### `Pipeline`s - -`Pipeline`s manage the "DAG" of `PValue`s and the `PTransform`s that compute them. - -For example, here's a schematic representation of a `Pipeline` that counts the number of occurrences of every word in an input file and writes those counts to an output file: - -```mermaid -flowchart TD -IF(Input File) -->|"io.ReadFromText(fname)"| L(Lines) -->|"FlatMap(str.split)"| W(Words) -->|"combiners.Count.PerElement()"| WC(word counts) --> |"MapTuple(lambda word, count: '%s: %d' % (word, count))"|w("word: #"s) -w -->|"io.WriteToText (ofname)"| OF(Output File) -``` -Here's the code for this job: - -```python -class WordCountJob(base_jobs.JobBase): - def run(self, fname, ofname): - return ( - self.pipeline - | 'Generate Lines' >> beam.io.ReadFromText(fname) - | 'Generate Words' >> beam.FlatMap(str.split) - | 'Generate (word, count)s' >> beam.combiners.Count.PerElement() - | 'Generate "word: #"s' >> ( - beam.MapTuple(lambda word, count: '%s: %d' % (word, count))) - | 'Write to Output File' >> beam.io.WriteToText(ofname) - ) -``` - -You might be wondering what's going on with the `|` and `>>` operators. In Python, objects can change how operators apply to them. Apache Beam has changed what the `|` and `>>` operators do, so `|` doesn't perform an OR operation anymore. Instead, `|` is a synonym for calling a `PCollection`'s `.apply()` method with a `PTransform` to create a new `PCollection`. `>>` lets you name a `PTransform` step, which helps document your job. Note that at the very beginning, we also use `|` between the pipeline object and a `PTransform` to start building the job. - -### `PValue`s - -`PCollection`s are the primary input and output `PValue`s used by `PTransform`s. They are a kind of `PValue` that represent a dataset of (virtually) any size, including unbounded and continuous datasets. - -`PBegin` and `PEnd` are "terminal" `PValue`s that signal that the value cannot be produced by an operation (`PBegin`) or that no operation can act on the value (`PEnd`). For example, a `Pipeline` object is a `PBegin`, and the output of a write operation is a `PEnd`. - -### `PTransform`s - -Recall that `PTransform`s represent the "edges" of the DAG and convert `PValue`s into other `PValue`s. - -#### `ParDo` and `DoFn` - -`ParDo` is the most flexible `PTransform`. It accepts `DoFn`s, which are simple functions, as arguments and applies them to all elements of the input `PCollection` in parallel. It also accepts functions and lambda functions as arguments. It is analogous to the following code: - -```python -do_fn = DoFn() -for value in pcoll: - do_fn(value) -``` -Notice that the return value from the `DoFn` is not used. However, it's possible for the `DoFn` to hold onto state in more advanced implementations. - -#### `Map` and `FlatMap` - -`beam.Map` is an operation that transforms each item in a `PCollection` into a new value using a plain-old function. It is analogous to the following code (where `fn` is the transformation function): - -```python -new_pcoll = [] -for value in pcoll: - new_pcoll.append(fn(value)) -return new_pcoll -``` - -`beam.FlatMap` is a similar transformation, but it _flattens_ the output `PCollection` into a single output `PCollection`. It is analogous to the following code (where `fn` is the transformation function): - -```python -new_pcoll = [] -for value in pcoll: - for sub_value in fn(value): - new_pcoll.append(sub_value) -return new_pcoll -``` - -#### `Filter` - -`beam.Filter` returns a new `PCollection` with all the elements of an input `PCollection`, so long as calling a specified filtering function on the element returned True. It is analogous to the following code (for filtering function `fn`): - -```python -new_pcoll = [] -for value in pcoll: - if fn(value): - new_pcoll.append(value) -return new_pcoll -``` - -#### `GroupByKey` - -`beam.GroupByKey` is useful when you need to perform an operation on elements that share a common property. It takes an input `PCollection` of `(key, value)` elements and returns a mapping from each `key` to all the `values` that were associated with that `key`. It is analogous to the following code: - -```python -groups = collections.defaultdict(lambda: collections.defaultdict(list)) -for i, pcoll in enumerate(pcolls_to_group): - # NOTE: Each PCollection must have (key, value) pairs as elements. - for key, value in pcoll: - # Items from each PCollection are grouped under the same key and - # bucketed into their corresponding index. - groups[key][i].append(value) -return groups -``` - -#### Example of using `GroupByKey`,`Filter`, and `FlatMap` - -For example, in our validation jobs we compute two `PCollection`s: - -```python -# Tuples of (ModelKey, True) for each model in the datastore that exists. -existing_models_pcoll = ... -# Tuples of (ModelKey, str) for each error message that should be reported when -# the corresponding model instance does not exist. -errors_if_missing_pcoll = ... -``` - -To generate a report, we use `GroupByKey` to pair the messages to the existing models. - -After this step, we can filter out the pairs where a model existed and report the errors that are left over. - -```python -error_pcoll = ( - ( - # A PCollection of Tuple[ModelKey, bool]. A ModelKey identifies an - # individual model in the datastore. - existing_models_pcoll, - # A PCollection of Tuple[ModelKey, str]. Each item corresponds to an - # error that should be reported when the corresponding instance does not - # exist. - errors_if_missing_pcoll, - ) - # Returns a PCollection of Tuple[ModelKey, Tuple[List[bool], List[str]]]. - | beam.GroupByKey() - # Discards ModelKey from the PCollection. - | beam.Values() - # Only keep groupings that indicate that the model is missing. - | beam.Filter(lambda (exist_bools, _): not any(exist_bools)) - # Discard the bools and flatten the results into a PCollection of strings. - | beam.FlatMap(lambda (_, errors): errors) -) -``` - -### `Runner`s - -`Runner`s provide the `run()` method used to visit every node (`PValue`) in the pipeline's DAG by executing the edges (`PTransform`s) to compute their values. At Oppia, we use `DataflowRunner` to have our `Pipeline`s run on the [Google Cloud Dataflow service](https://cloud.google.com/dataflow). - -## Writing Apache Beam Jobs - -For this section, we'll walk through the steps of implementing a job by writing one: `CountExplorationStatesJob`. - -It's helpful to begin by sketching a diagram of what you want the job to do. We recommend using pen and paper or a whiteboard, but in this wiki page we'll use ASCII art to keep the document self-contained. - -Here's a diagram for the `CountExplorationStatesJob`: -```mermaid -flowchart LR - A(Explorations) -->|Count states| B(Counts) -->|Sum| C(Total) -``` -> **TIP**: As illustrated, you don't need to know what the names of the `PTransform`s (edges) used in a diagram are. It's easy to look up the appropriate `PTransform` after drawing the diagram. - -Now that we have our bearings, let's get started on implementing the job. - -### 1. Subclass the `base_jobs.JobBase` class and override the `run()` method - -Make sure your job class name is clear and concise, because the name is presented to release coordinators: - -![Screenshot of the Release Coordinator page with a list of job names visible](https://user-images.githubusercontent.com/5094060/135734501-eb9c0370-e98d-4271-b41a-0bf11c25503c.png) - -Job names should follow the convention: `Job`. - -* For example: - - ```python - class WeeklyDashboardStatsComputationJob(base_jobs.JobBase): - """BAD: Name does not begin with a verb.""" - - def run(self): - ... - - - class ComputeStatsJob(base_jobs.JobBase): - """BAD: Unclear what kind of stats are being computed.""" - - def run(self): - ... - - - class CountExplorationStatesJob(base_jobs.JobBase): - """GOOD: Name starts with a verb and "exploration states" is unambiguous.""" - - def run(self): - ... - ``` - -Module names should follow the convention: `__jobs.py`. - -* For example: - * `blog_validation_jobs.py` - * `dashboard_stats_computation_jobs.py` - * `exploration_indexing_jobs.py` - * `exploration_stats_regeneration_jobs.py` - * `model_validation_jobs.py` - - However, you should always prefer placing jobs in preexisting modules if an appropriate one already exists. - -For this example, we will write our job in the module: `core/jobs/batch_jobs/exploration_inspection_jobs.py`. - -### 2. Override the `run()` method to operate on `self.pipeline` - -As illustrated in the Architecture section, jobs are organized by `Pipeline`s, `PTransform`s, and `PCollection`s. Jobs that inherit from `JobBase` are constructed with a `Pipeline` object already accessible via `self.pipeline`. When we write our jobs, we will build them off of `self.pipeline`. - -`Pipeline`s are special `PValue`s that represent the entry-point of a job. `PTransform`s that operate on `Pipeline` are generally "producers"; that is to say, operations that produce initial `PCollection`s to work off of. - -We can represent this in our DAG by adding a special `Pipeline` node. - -```mermaid -flowchart TD -P(Pipeline) -->|"GetModels()"| E(Explorations) -->|"Count states"| C(Counts) -->|"Sum"| T(Total) -``` -> [!NOTE] -> Since pipelines are a part of every job, it's fine to leave it out of a DAG to save on complexity. - -Now, let's see how this would translate into code, starting with the Explorations. - -```python -from core.jobs import base_jobs -from core.jobs.io import ndb_io -from core.platform import models - -(exp_models,) = models.Registry.import_models([models.NAMES.exploration]) - - -class CountExplorationStatesJob(base_jobs.JobBase): - - def run(self): - exp_model_pcoll = ( - self.pipeline - | 'Get all ExplorationModels' >> ndb_io.GetModels( - exp_models.ExplorationModel.get_all()) - ) -``` - -Observe that: -1. We're using `ndb_io.GetModels` rather than `get_multi` -2. We're passing a `Query` to `ndb_io.GetModels` - -We use `ndb_io.GetModels()` because we want to work on `PCollection`s of models, not a list of models. In fact, all operations that can be taken on models (`get`, `put`, `delete`) have analogous `PTransform` interfaces defined in `ndb_io`. They are: - -| NDB function | `PTransform` analogue | -| ---------------------- | -------------------------------------------- | -| `models = get_multi()` | `model_pcoll = ndb_io.GetModels(Query(...))` | -| `put_multi(models)` | `model_pcoll \| ndb_io.PutMulti()` | -| `delete_multi(keys)` | `key_pcoll \| ndb_io.DeleteMulti()` | - -Note that `get_multi` has the biggest change in interface, in that it takes a `Query` argument. You can get a query for any model by using the class method `get_all`. -> **IMPORTANT:** Never use `datastore_services.query_everything()`!! Due to a limitation in Apache Beam, this operation is incredibly slow and inefficient! **You are almost certainly doing something wrong if you need this function.** Ask @brianrodri/@vojtechjelinek for help if you believe you need to use it regardless. - -Why should we use these `PTransform` over the simpler `get`/`put`/`delete` functions? **Performance**. The `get`/`put`/`delete` function calls are all *synchronous*, so your job's performance will suffer greatly by waiting for the operations to complete. - -`PTransform`s, on the other hand, are specially crafted to take advantage of the Apache Beam framework and guarantee better performance. In general, **you should always prefer `ndb_io` over any of the `get`/`put`/`delete` functions!** If you think you have a valid need for avoiding `ndb_io`, then speak with @brianrodri/@vojtechjelinek first. - -Let's get back to implementing the job. - -```python -def run(self): - exp_model_pcoll = ( - self.pipeline - | 'Get all ExplorationModels' >> ndb_io.GetModels( - exp_models.ExplorationModel.get_all()) - ) - - state_count_pcoll = ( - exp_model_pcoll - | 'Count states' >> beam.Map(self.get_number_of_states) - ) -``` - -Note that we chose to use `beam.Map` here instead of `beam.ParDo`. This is mostly a stylistic choice, as `beam.Map` is just a specialized version of `ParDo`, in that `Map` simply takes each input element and "maps" it to a single output element. In our case, each `ExplorationModel` will map to a single `int`, the number of states. - -Here is the implementation of `get_number_of_states`. This function transforms the model into a domain object, and then counts the number of states in the corresponding `dict`. - -```python -def get_number_of_states(self, model: ExplorationModel) -> int: - exploration = exp_fetchers.get_exploration_from_model(model) - return len(exploration.states) -``` - -Finally, we need to sum all the counts together. We'll use `beam.CombineGlobally` to accomplish this, which uses an input function to combine values of a `PCollection`. It returns a `PCollection` with a single element: the result of the combination. - -```python -def run(self): - exp_model_pcoll = ( - self.pipeline - | 'Get all ExplorationModels' >> ndb_io.GetModels( - exp_models.ExplorationModel.get_all()) - ) - - state_count_pcoll = ( - exp_model_pcoll - | 'Count states' >> beam.Map(self.get_number_of_states) - ) - - state_count_sum_pcoll = ( - state_count_pcoll - | 'Sum values' >> beam.CombineGlobally(sum) - ) -``` - -> **IMPORTANT**: We take special care to pass very simple objects (like ints and models) in between `PTransform`s. This is intentional, complex objects cannot be serialized without special care (TL;DR: objects must be [picklable](https://docs.python.org/3/library/pickle.html#what-can-be-pickled-and-unpickled)). When passing objects between `PTransform`s in your jobs, use simple data structures and simple types as much as possible. - -With this, our objective is complete. However, there's still more code to write! - -### 3. Have the `run()` method return a `PCollection` of `JobRunResult`s - -* In English, this means that **the job _must_ report _something_ about what occurred during its execution.** For example, this can be the errors it discovered or the number of successful operations it was able to perform. **Empty results are forbidden!** - - * If you don't think your job has any results worth reporting, then just print a "success" metric with the number of models it processed. - -* `JobRunResult` has two fields: `stdout` and `stderr`. They are analogous to a program's output, and should be used in a similar capacity for jobs -- put problems encountered by the job in `stderr` and informational outputs in `stdout`. - -* `JobRunResult` outputs should answer the following questions: - - * Did the job run without any problems? How and why do I know? - * How much work did the job manage to do? - * If the job encountered a problem, what caused it? - -Our job is trying to report the total number of states across all explorations, so we need to create a `JobRunResult` that holds that information. For this, we can use the `as_stdout` helper method: - -```python -def run(self): - exp_model_pcoll = ( - self.pipeline - | 'Get all ExplorationModels' >> ndb_io.GetModels( - exp_models.ExplorationModel.get_all()) - ) - - state_count_pcoll = ( - exp_model_pcoll - | 'Count states' >> beam.Map(self.get_number_of_states) - ) - - state_count_sum_pcoll = ( - state_count_pcoll - | 'Sum values' >> beam.CombineGlobally(sum) - ) - - return ( - state_count_sum_pcoll - | 'Map as stdout' >> beam.Map(job_run_result.JobRunResult.as_stdout) - ) -``` - -The method maps every element in a `PCollection` to a `JobRunResult` with their stringified-values as its `stdout`. - -### 4. Add the job module to `core/jobs/registry.py` - -To have your job registered and acknowledged by the front-end, make sure to import the module in the corresponding section of `core/jobs/registry.py`: -https://github.com/oppia/oppia/blob/973f777a6c5a8c3442846bda839e63856dfddf72/core/jobs/registry.py#L33-L50 - ---- - -With this, our job is finally completed! - -Here is the cleaned-up implementation of our job: - -```python -from core.domain import exp_fetchers -from core.jobs import base_jobs -from core.jobs.io import ndb_io -from core.platform import models - -import apache_beam as beam - -(exp_models,) = models.Registry.import_models([models.NAMES.exploration]) - - -class CountExplorationStatesJob(base_jobs.JobBase): - - def run(self) -> beam.PCollection[job_run_result.JobRunResult]: - return ( - self.pipeline - | 'Get all ExplorationModels' >> ndb_io.GetModels( - exp_models.ExplorationModel.get_all()) - | 'Count states' >> beam.Map(self.get_number_of_states) - | 'Sum values' >> beam.CombineGlobally(sum) - | 'Map as stdout' >> beam.Map(job_run_result.JobRunResult.as_stdout) - ) - - def get_number_of_states(self, model: exp_models.ExplorationModel) -> int: - exploration = exp_fetchers.get_exploration_from_model(model) - return len(exploration.states) -``` - -## Testing Apache Beam Jobs - -First and foremost, you should follow our guidelines for [writing backend tests](https://github.com/oppia/oppia/wiki/Backend-tests#write-backend-tests). This includes naming your test cases (`test_{{action}}_with_{{with_condition}}_{{has_expected_outcome}}`) and our general test case structure ("Setup", "Baseline verification", "Action", "Endline verification"). - -There are two base classes dedicated to testing our Apache Beam jobs: `PipelinedTestBase` and `JobTestBase`. - -`PipelinedTestBase` (and its subclass, `JobTestBase`) exposes two special assertion methods: `assert_pcoll_equal` and `assert_pcoll_empty`. - -The class operates by first, in `setUp()`, entering the context of a `Pipeline` object (accessible via `self.pipeline`). Upon exiting the context, the `Pipeline` will execute any operations attached to it. Running the `assert_pcoll_*` methods will add a "verification" `PTransform` to the input `PCollection`, and then close the context (thus running it immediately). For this reason, **only _one_ `assert_pcoll_*` method may be called in a test case!** If you want to run multiple assertions on a `PCollection`, then create a separate test case for that purpose. - -> [!NOTE] -> The verification `PTransform` will also run type checks on all inputs/outputs generated by the `PTransform`s under test! - -Here's an example: -```python -def test_validate_model_id_with_invalid_model_id_reports_an_error(self): - # Setup. - invalid_id_model = base_models.BaseModel( - id='123@?!*', - created_on=self.YEAR_AGO, - last_updated=self.NOW) - - # Action. - output = ( - self.pipeline - | beam.Create([invalid_id_model]) - | beam.ParDo(base_validation.ValidateBaseModelId()) - ) - - # Endline verification. - self.assert_pcoll_equal(output, [ - base_validation_errors.ModelIdRegexError( - invalid_id_model, - base_validation.BASE_MODEL_ID_PATTERN), - ]) -``` - -For testing _jobs_, you should follow the following steps (we'll use `CountExplorationStatesJob` as an example): - -### 1. Inherit from `JobTestBase` and override the class constant `JOB_CLASS` - -The current convention is to name your test cases `Tests`, but you can create better names if you want to break tests up. For our example, we'll keep things simple. - -```python -class CountExplorationStatesJobTests(test_jobs.JobTestBase): - - JOB_CLASS = CountExplorationStatesJob -``` - -### 2. Run assertions using a `assert_job_output_is_*` method - -When testing a job, we should aim to cover behavior and common edge cases. For this job, we'll have 3 main tests: -1. When there are no Explorations in the datastore. -2. When there is exactly 1 Exploration in the datastore. -3. When there are many Explorations in the datastore. - -```python -def test_empty_datastore(self): - # Don't add any explorations to the datastore. - self.assert_job_output_is_empty() - -def test_single_exploration(self): - self.save_new_linear_exp_with_state_names_and_interactions( - 'e1', 'o1', ['A', 'B', 'C'], ['TextInput']) - - self.assert_job_output_is([ - job_run_result.JobRunResult(stdout='3'), - ]) - -def test_many_explorations(self): - self.save_new_linear_exp_with_state_names_and_interactions( - 'e1', 'o1', ['A', 'B', 'C'], ['TextInput']) - self.save_new_linear_exp_with_state_names_and_interactions( - 'e2', 'o1', ['D', 'E', 'F', 'G', 'H'], ['TextInput']) - self.save_new_linear_exp_with_state_names_and_interactions( - 'e3', 'o1', ['I', 'J'], ['TextInput']) - self.save_new_linear_exp_with_state_names_and_interactions( - 'e4', 'o1', ['K', 'L', 'M', 'N'], ['TextInput']) - - self.assert_job_output_is([ - job_run_result.JobRunResult(stdout='14'), - ]) -``` - -Note that `self.assert_job_output_is(...)` and `self.assert_job_output_is_empty()` do as advertised -- they run the job to completion and verify the result. - -> **IMPORTANT:** Only one `assert_job_output_is` assertion can be performed in a test body. Multiple calls will result in an exception instructing you to split the test apart. - -Just because a job passes in unit tests does not guarantee it will pass in production. This is because workers, which execute the pipeline code, are run in a special environment where the code base is configured differently. While Oppia's jobs team works to resolve the differences, be careful about using complex and/or confusing objects. The simpler your job, the greater chance it'll work in production! - -## Running Apache Beam Jobs - -### Local Development Server - -These instructions assume you are running a local development server. If you are a release coordinator running these jobs on the production or testing servers, you should already have been granted the "Release Coordinator" role, so you can skip steps 1-3. - -1. Sign in as an administrator ([instructions][3]). -2. Navigate to **Admin Page > Roles Tab**. -3. Add the "Release Coordinator" role to the username you are signed in with. -4. Navigate to http://localhost:8181/release-coordinator, then to the **Beam Jobs tab**. -5. Search for your job and then click the **Play button**. -6. Click "Start new job". - -![Screen recording showing how to run jobs](https://user-images.githubusercontent.com/5094060/128743997-70cca5f9-0b76-4294-806e-f65f5df5be95.gif) - -### Production Server - -Before a job can be run and deployed in production, it must first be tested on the Oppia backup server. - -If your job is not essential for the release and has not been fully tested by the release cut, then it is not going into the release. "Fully tested" means: -- The job should run without failures on the Oppia backup server. -- The job produces the expected output. -- The job has the expected outcome (this must be verified by e.g. user-facing changes, or a validation job, or an output check, etc.). -- The job should be explicitly approved by the server jobs admin (currently @seanlip and @vojtechjelinek). - -Also, in case your job changes data in the datastore, there has to be a validation job accompanying it to verify that the data that you are changing is valid in the server. **The validation job will have to be "Fully tested" before testing on the migration job can start.** - -In case there is invalid data observed, either your migration job should fix it programmatically, or the corresponding data has to be manually fixed before the migration job can be run. This is valid for both testing in the backup server and running in production. - -For a full overview of the process to get your job tested on the Oppia backup server, refer to the corresponding [wiki page](https://github.com/oppia/oppia/wiki/Testing-jobs-and-other-features-on-production). - -#### Instruction for job testers - -There are two ways to perform the testing of the Beam jobs, both of these need to be done by a person that has deploy access to the backup server. - -##### Deploy to backup server - -This way provides full testing of the job. - -1. Deploy branch containing the job to the backup server -2. Run the job through the [release coordinator page](https://oppiaserver-backup-migration.appspot.com/release-coordinator) -3. If the job fails you can check the details of the error on the Google Cloud Console in the Dataflow section - - -##### Run the job on backup server through the local dev server - -The downside of this approach is that you cannot get the job output, you can only verify that it works. - -In order to run jobs through the local dev server you need to have JSON key that will provide access to the backup server. The key can be generated according to step 4 and step 5 of the [Quickstart for Python](https://cloud.google.com/dataflow/docs/quickstarts/quickstart-python). - -1. Have the JSON key ready -2. Add `GOOGLE_APPLICATION_CREDENTIALS: ""` to `env_variables` in _app_dev.yaml_ -3. In _core/domain/beam_job_services.py_ change value of `run_synchronously` to `False` -4. In _core/feconf.py_ - 1. Change the value of `OPPIA_PROJECT_ID` to `'oppiaserver-backup-migration'` - 1. Change the value of `DATAFLOW_TEMP_LOCATION` to `'gs://oppiaserver-backup-migration-beam-jobs-temp/'` - 1. Change the value of `DATAFLOW_STAGING_LOCATION` to `'gs://oppiaserver-backup-migration-beam-jobs-staging/'` -5. Start the dev server and run the job through the release coordinator page -6. If the job fails you can check the details of the error on the Google Cloud Console in the Dataflow section - -## Beam guidelines - -### Do not use NDB put/get/delete directly - -Even though it is possible to use NDB functions directly, they should not be used because they are slow and we have Beam compliant alternatives from them. All these alternatives are located in _core/jobs/io/ndb_io.py_. - -- Instead of using `get`, `get_multi`, `get_by_id`, etc. you should use `GetModels`, and you should pass a query to it, `GetModels` will execute the query and return a `PCollection` of the models that were returned by the query. -- Instead of using `put`, `put_multi`, etc. you should use `PutModels`, and you just pipe a `PCollection` of models to it and they will be put into the datastore. -- Instead of using `delete`, `delete_multi`, etc. you should use `DeleteModels`, and you just pipe a `PCollection` of models to it and they will be deleted from the datastore. - -All of the aforementioned classes are already used in the codebase so you can look for examples. - -### Use `get_package_file_contents` for accessing files - -If you need to access a file in a Beam job, please use `get_package_file_contents` (from _core/constants.py_) instead of `open` or `open_file` (from _core/utils.py_). Also, make sure that the file is included in the _assets_ folder or is listed in _MANIFEST.in_ explicitly. - -#### Example - -When we have a function that is used in a Beam pipeline, like: - -```python -@staticmethod -def function_used_in_beam_pipeline(): - file = utils.open_file('assets/images/about/cc.svg', 'rb') - return file.read() -``` - -it needs to be replaced with something like: - -```python -@staticmethod -def function_used_in_beam_pipeline(): - return constants.get_package_file_contents( - 'assets', 'images/about/cc.svg', binary_mode=True - ) -``` - -## Common Beam errors - -### `'_UnwindowedValues' object is not subscriptable` error - -This error usually happens when you attempt to access an element in what you expect is a list, but Beam actually didn't convert it to a list. The solution usually is to transform the element to list explicitly using `list()`. **Some comments on the internet might suggest a usage of `SessionWindow` or similar stuff, but since all our jobs are batch jobs that process some final list of elements this solution won't work.** - -#### Example - -```python -new_user_stats_models = ( - { - 'suggestion': suggestions_grouped_by_target, - 'opportunity': exp_opportunities - } - | 'Merge models' >> beam.CoGroupByKey() - | 'Get rid of key' >> beam.Values() # pylint: disable=no-value-for-parameter - | 'Generate stats' >> beam.ParDo( - lambda x: self._generate_stats( - x['suggestion'][0] if len(x['suggestion']) else [], - x['opportunity'][0][0] if len(x['opportunity']) else None - )) -) -``` -The code above throws this error `'_UnwindowedValues' object is not subscriptable [while running 'Generate stats']`, we know that the issue is in the last part of the code ('Generate stats' part). After some debugging, we discover that the code needs to be changed to. -```python -| 'Generate stats' >> beam.ParDo( - lambda x: self._generate_stats( - x['suggestion'][0] if len(x['suggestion']) else [], - list(x['opportunity'][0])[0] if len(x['opportunity']) else None - )) -``` - -### `_namedptransform is not iterable` error - -This error sometimes happens when you forget to add a label for some operation (the strings of code before `>>`). The solution is to add a label for all operations. - -#### Example - -```python -some_values = ( - some_models - | beam.Values() -) -``` -The code above might return `'_namedptransform is not iterable` in the job output. We can fix this by adding an appropriate label. -```python -some_values = ( - some_models - | 'Get values' >> beam.Values() -) -``` -## Guidelines for writing beam jobs -This section provides some general guidelines for writing Beam jobs, which would be particularly helpful for new contributors. - -### Planning a job -* If your Beam job includes updating the storage models, make sure to write an audit job. An audit job is similar to your Beam job but doesn't make any changes to the datastore. During testing, the audit job is run before the actual Beam job to prevent any unwanted changes to the datastore in case of an error. Some examples which illustrate this paradigm are listed below: - * [Topic migration job](https://github.com/oppia/oppia/blob/fc2e383032a0f9308fdde03d7efd10971752bacf/core/jobs/batch_jobs/topic_migration_jobs.py#L54) - * [Story migration job](https://github.com/oppia/oppia/blob/fc2e383032a0f9308fdde03d7efd10971752bacf/core/jobs/batch_jobs/story_migration_jobs.py#L60) -* There should also be a job to verify the changes done by your job. -* Consider the example of topic migration job. The [AuditTopicMigrateJob](https://github.com/oppia/oppia/blob/fc2e383032a0f9308fdde03d7efd10971752bacf/core/jobs/batch_jobs/topic_migration_jobs.py#L355) is the audit job which performs all the steps in the main job ([MigrateTopicJob](https://github.com/oppia/oppia/blob/fc2e383032a0f9308fdde03d7efd10971752bacf/core/jobs/batch_jobs/topic_migration_jobs.py#L244)), except it doesn't write those changes to the datastore. -* It is often helpful to include debugging information with your job from the outset, since modifying a failed job and rerunning it can take time. Feel free to include additional logs or counts that will help you debug any issues that arise during execution. To help you identify these easily, you can prefix the relevant lines of output with an identifier in square brackets (e.g. `[NUMBER OF MODELS PROCESSED]`). - -### Executing jobs -* Refer to code from similar jobs to avoid mistakes. The "Troubleshooting" section in the wiki lists common errors encountered while executing jobs. -* Make sure to write tests for all jobs. These tests should check the behaviour of the job for different cases of inputs. The common cases for all jobs are listed below: - * Empty input. - * Input which triggers the job to perform its intended action. - * Incorrect input which causes the job to fail. -* The tests should ensure that the job runs appropriately for all such cases. - -### PR guidelines -* A PR for a Beam job should always have "Proof of work" in the description which should include the successful local run of the Beam job on the release coordinator page and a screenshot of the storage model which is being changed. The storage models can be checked [locally](https://github.com/oppia/oppia/wiki/Debugging-datastore-locally) via dsadmin. - -### Job testing workflow -* A testing request for the job must be submitted as soon as the PR is created using this [form](https://docs.google.com/forms/d/e/1FAIpQLSfvYWscAn18ok06An1oQ54h1VmBHfCX8uuuV01kIvY9WX0-Ug/viewform). Make sure to be clear in your instructions (in the doc) about what exactly needs to be done by the tester. Also, the doc link should be included in the PR description. -* The sample job testing [template](https://docs.google.com/document/d/1Xg0MSIpYUEax8dqH39CKhgqt30f-kr8wxO2F4aSxwz4/edit) should be used as a starting point for the testing doc. Make sure to make a copy before editing the doc. -* In case of multiple jobs to be tested, they should be written in the order of testing. Generally, an audit job is run before the migration job to verify that the job runs as intended. -* The instructions should be concise to prevent any confusion for the tester. Any pre and post checks to be done should also be specified clearly. -* After the doc is complete, request a review from the main code reviewer of the related PR. The reviewer should fill in the approval section at the top of the doc. -* Once approvals have been received, the assigned server admin will then test the PR according to the instructions in the doc and update the PR author with the results. In case of errors, the server admin would provide the relevant logs for debugging. - -## Case Studies - -The case studies are sorted in order of increasing complexity. Study the one that best suits your needs. - -If none of them help you implement your job, you may request a new one by adding a comment to [#13190](https://github.com/oppia/oppia/issues/13190) with answers to the following questions: - -* Why do I want a new case study? -* Why are the current case studies insufficient? -* What answers would the "perfect" case study provide? - -Then we'll start write a new Case Study to help you, and future contributors, as soon as we can (@brianrodri will always notify you of how long it'll take). - -### Case Study: `SchemaMigrationJob` - -**Difficulty:** Medium - -**Key Concepts:** - -* Getting and Putting NDB models -* Partitioning one `PCollection` into many `PCollection`s. -* Returning variable outputs from a `DoFn` - ---- - -Let's start by listing the specification of a schema migration job: - -* We can assume: - - * The schema version of a model is in the closed range `[1, N]`, where `N` is the latest version. - * All migration functions are implemented in terms of taking `n` to `n + 1`. - -- Our job should conform to the following requirements: - - * Models should only be put into storage after successfully migrating to v`N`. - * Models that were already at v`N` should be reported separately. - ```mermaid - flowchart TD - IM(Input Models) -->|"Partition(lambda model: model.schema_version)"| MV("Model @v1") - IM -->|"Partition(lambda model: model.schema_version)"| M(Model ...) - MV -->|"ParDo(MigrateToNextVersion())"| M - MV ---- M - M --> MvN("Model @vN") - MvN -->|"ndb_io.PutModels()"| D(Datastore) - ``` -There's a lot of complexity here, so we'll need many `PTransform`s to write our job. We'll focus on the most interesting one: the loop to migrate models to the next version. - -```python -class MigrateToNextVersion(beam.DoFn): - - def process(self, input_model): - if input_model.schema_version < ExplorationModel.LATEST_SCHEMA_VERSION: - model = job_utils.clone_model(input_model) - exp_services.migrate_to_next_version(model) - yield model - - -class MigrateToLatestVersion(beam.PTransform): - """Diagram: - - .--------------. Partition(lambda model: model.schema_version) - | Input Models | ---------------------------------------------. - '--------------' | - .-----------. | - .----------------------- | Model @v1 | <--| - | '-----------' | - | | - | ParDo(MigrateToNextVersion()) | - >-----------------------------. | - | | | - | v | - | .-----------. | - '----------------------- | Model ... | <--' - '-----------' - | - v - .-----------. - | Model @vN | - '-----------' - """ - - def expand(self, exp_model_pcoll): - models_by_schema_version = ( - exp_model_pcoll - | beam.Partition( - lambda model, _: model.schema_version - 1, - ExplorationModel.LATEST_SCHEMA_VERSION) - ) - - do_fn = MigrateToNextVersion() - results = [models_by_schema_version[0] | beam.Map(do_fn)] - - for models_at_ith_version in models_by_schema_version[1:-1]: - models_to_migrate = ( - (results[-1].updated_models, models_at_ith_version) - | beam.Flatten() - ) - results.append(models_to_migrate | beam.FlatMap(do_fn)) -``` - -Note that this implementation won't work as-is since we focused on the step where we upgrade the models. To get this fully working, we'd need to write a `Pipeline` that handles loading in the models and writing the upgraded models back to the datastore. - -[1]: https://beam.apache.org/documentation/programming-guide/ -[2]: https://github.com/oppia/oppia/blob/4d2f639869e57fbeaada414d923cae83eb0e082e/jobs/job_utils.py#L37-L63 -[3]: https://github.com/oppia/oppia/wiki/How-to-access-Oppia-webpages#log-in-as-a-super-administrator diff --git a/Backend-Type-Annotations.md b/Backend-Type-Annotations.md deleted file mode 100644 index e0bc4ced..00000000 --- a/Backend-Type-Annotations.md +++ /dev/null @@ -1,115 +0,0 @@ -## Why do we use Type Annotations? - -Type Annotations are a new feature added in [PEP 484](https://www.python.org/dev/peps/pep-0484/) that allow for adding type hints to variables. They give information about types of variables to someone who is reading the code. This brings a sense of statically-typed control to the dynamically typed Python. Though Python ignores these type hints during code execution, third-party libraries can be used to statically type-check the codebase. - -To type-check the backend part of our codebase, we use mypy. **All backend files** should include type annotations. - -## Running MyPy check script - -Mypy checks script (`scripts/run_mypy_checks.py`) is the script used to run our mypy type checks. - -It has two modes of running: - -1. Python: -``` -python -m scripts.run_mypy_checks -``` - - This runs the type checks on all the type annotated files in the codebase. - -2. Python: -``` -python -m scripts.run_mypy_checks --files path/file1.py path/file2.py -``` - - This runs the type checks on the files specified, i.e., file1 and file2. - -**Note:** -- [Helpful trick for faster runs]`python -m scripts.run_mypy_checks --skip_install` can be used to skip installation of third party libraries. Please use the `--skip_install` flag **only** when you already have all the third party libraries installed. - - -## Adding type annotations - -To add type annotations to a file, figure out the types of the variables and mention them according to the syntax. - -### Steps to add type annotations to a file: - -> [!IMPORTANT] -> Test file should **always** be type annotated along with the main code file (if the latter one is going to be fully annotated). - -1. Run mypy type checks on the main code file you are trying to annotate. This will give the errors. (Either use the `--files` version or use the normal version but ensure that the file is removed from mypy denylist) -2. Start solving these errors one by one. -3. Let’s say a function is not type annotated, you should first look at the function arguments and the return value. Try to get information of the types from the function docstring, test file, function code and function usage. Let’s say in the example given below, where we have a function to take two integers and convert them to string and return the concatenated string, you can figure out from the function code that the return type will be a string. The type of the arguments can be figured out by taking a look at the docstring, tests and usage of the functions. - - - The original example code: - ```python - def concat(x, y): - return str(x) + str(y) - ``` - - After adding type annotation: - ```python - def concat(x: int, y: int) -> str: - return str(x) + str(y) - ``` - - **Avoid using `Any` type**. Always try to reason out why it is needed, try to arrive at a stricter type. In case you go ahead with `Any` type, make sure you have a proper reason and add an explanatory comment for it. [Example 1](https://github.com/oppia/oppia/blob/b0c6ffb917663fb6482022d0f607377f7e1ee3d0/constants.py#L31-L33), [Example 2](https://github.com/oppia/oppia/blob/develop/core/controllers/access_validators.py#L40-L42). - -4. You may get errors when **Mypy is not able to infer the type of a variable**, then you must specify the type of the variable as demonstrated below. - - - The original code example: - ```python - d = { - ‘a’: 1, - ‘b’: 2, - ‘c’: 3 - } - ``` - - After adding type annotation: - ```python - d: Dict[str, int] = { - ‘a’: 1, - ‘b’: 2, - ‘c’: 3 - } - ``` - -5. **To understand what different error codes mean** take a look at different [Error Codes](https://mypy.readthedocs.io/en/latest/error_code_list.html) in MyPy docs. - - - First try to find the reason behind that error. If that error can be resolved by some improvements in the codebase, then make the necessary changes. - - If there are no options left to resolve that error, then only go for ignoring the error. **For any kind ignore** other than `[no-untyped-call]`, make sure you have a proper reason and add an explanatory comment for it - [Example 1](https://github.com/oppia/oppia/blob/b0c6ffb917663fb6482022d0f607377f7e1ee3d0/core/platform/cache/redis_cache_services.py#L61), [Example 2](https://github.com/oppia/oppia/blob/b0c6ffb917663fb6482022d0f607377f7e1ee3d0/core/controllers/oppia_root.py#L31-L33). - - Some ignored errors can be fixed in future, so make a TODO issue for them with clear explanation like [this](https://github.com/oppia/oppia/issues/13059) and write a TODO in the file with the issue number of the issue created. - - -6. When the main code file has no errors, start type annotating its corresponding test file. Note that in the test file, there may be **cases where we deliberately provide wrong (or wrongly typed) arguments to test** that the function fail on them. Such errors must be silenced using `# type: ignore[]` where is the code of the error to be silenced with an explanatory comment. - - Example of such error code is `[arg-type]`. All cases of [arg-type] ignores should be explained and a TODO for the [issue](https://github.com/oppia/oppia/issues/13528) should be added above such ignores so that this test can be removed if it is unnecessary like [here](https://github.com/oppia/oppia/blob/f7a5746a80730753b32b555306f20c55d4023822/core/storage/email/gae_models_test.py#L164-L166). - -7. When you are done with **fully** adding type annotations to the files, make sure to **remove them from the mypy denylist** as told before. Also run the mypy type checks again on the entire codebase to ensure there are no more type errors. - -For more information on adding types, refer to [Mypy Cheat Sheet(Python 3)](https://mypy.readthedocs.io/en/stable/cheat_sheet_py3.html). - -## Special Cases -1. Description: Code using `inspect.getargspec` method is throwing an error `ValueError: Function has keyword-only parameters or annotations, use getfullargspec() API which can support them` after adding type annotation. - - Explanation: `getargspec` has been [deprecated](https://docs.python.org/3/library/inspect.html#inspect.getargspec) and does not support parsing annotations - - Solution: Use the updated version of the method - `getfullagrspec` - this supports parsing of type annotations. [Example](https://github.com/oppia/oppia/blob/b0c6ffb917663fb6482022d0f607377f7e1ee3d0/schema_utils_test.py#L280). - -## Other Important points -1. Use `str` instead of Text wherever applicable. (Text was used in the Python2 version of codebase. We also have a lint check now to prevent usage of Text in type annotations) -2. For external libraries we obtain the type information from the type stubs defined in the [typeshed](https://github.com/python/typeshed) package (which come bundled with mypy for it's current version `0.812` that we use). - - In case of **missing stubs** (when typeshed doesn't support a library yet), mypy will throw errors and ask you to use type `Any` or type ignores to silence those errors, but this can lead to loose and inconsistent typing for imports from those packages, so we avoid that practice. - - Instead, to overcome that, we follow the practice of **defining the stubs ourselves** only for the part of the library we are using, and place those stubs inside the `stubs/` folder. You can look at the existing stubs as an example to understand how this works. -3. Types (like Dict, Any, Union etc) from the typing module can be imported in the same line. Do not use `isort:ignore`. If the import exceeds line length limit, use parenthesis to span across multiple lines. See the following cases to understand. -```python -# Wrong usage -from typing Any -from typing import Dict - -# Correct usage (1) -from typing import Any, Dict -# Correct Usage (2) -from typing import ( - Any, Callable, Dict, Iterable, Iterator, List, Optional, Sequence, - Type, TypeVar, Tuple, Union) - -``` - -## Troubleshooting -1. If you are seeing type errors for unchanged files, especially which are not part of the Oppia codebase, a possible reason could be that you have the virtual environment directory inside the Oppia root folder. Moving the environment folder out of the Oppia root directory resolves this error. diff --git a/Backend-tests.md b/Backend-tests.md deleted file mode 100644 index 1941af9f..00000000 --- a/Backend-tests.md +++ /dev/null @@ -1,621 +0,0 @@ -## Table of Contents - -* [Introduction](#introduction) -* [Testing philosophy](#testing-philosophy) - * [Unit tests](#unit-tests) - * [Writing comprehensive tests](#writing-comprehensive-tests) - * [Mocking](#mocking) - * [Integration tests](#integration-tests) - * [Unit versus integration tests at Oppia](#unit-versus-integration-tests-at-oppia) - * [When to mock](#when-to-mock) -* [Run backend tests](#run-backend-tests) - * [Identifying whether the tests passed](#identifying-whether-the-tests-passed) - * [Coverage reports](#coverage-reports) - * [Warning: full coverage does not mean your tests are comprehensive](#warning-full-coverage-does-not-mean-your-tests-are-comprehensive) -* [Write backend tests](#write-backend-tests) - * [Backend test structure](#backend-test-structure) - * [Guidelines for writing good tests](#guidelines-for-writing-good-tests) - * [Sharding backend tests](#sharding-backend-tests) - * [How sharding works](#how-sharding-works) - * [Common errors](#common-errors) - * [Adding new tests to shards](#adding-new-tests-to-shards) - * [Common testing scenarios](#common-testing-scenarios) - * [Examples](#examples) - * [Example: Writing unit tests for domain classes](#example-writing-unit-tests-for-domain-classes) - * [Example: Writing integration tests for handlers (controllers)](#example-writing-integration-tests-for-handlers-controllers) - -## Introduction - -All code in Oppia's backend must be thoroughly tested because tests help catch bugs, help new contributors understand our backend code, and ensure that our code doesn't get broken by other developers in the future. - -This guide covers Oppia’s backend tests. We also have separate pages for [[frontend tests|Frontend-tests]] and [[end-to-end tests|End-to-End-Tests]]. - -## Testing philosophy - -Let's begin by explaining some testing philosophy. This will help you design your tests; we'll talk about how to actually write them later. There are two main kinds of tests that we use in the backend: unit tests and integration tests. - -### Unit tests - -Unit tests check that a small unit of code, usually a function or a class, works correctly. By testing only a small piece of code at a time, we can write very thorough tests. For example, it would be a lot harder to write comprehensive test cases for all of Oppia than it would be to write comprehensive test cases for a small utility function that checks whether a username is valid. - -#### Writing comprehensive tests - -Let's consider what test cases we might write for such a utility. Suppose we only want to allow usernames that are between 1 and 7 characters (inclusive), and that include only English letters and Arabic numerals. These test cases would not be comprehensive: - -* `'abc123'`: Valid -* `'aBc'`: Valid -* `'1'`: Valid -* `'abc_123'`: Invalid -* `'abc123de'`: Invalid - -**Exercise:** Take a moment to think about what test cases are missing. - -Here's an example implementation of our utility function that is incorrect but passes these test cases: - -```python -ALLOWED_USERNAME_CHARACTERS = [ - 'a', 'b', 'c', 'd', 'e', 'f', 'g', 'h', 'i', 'j', 'k', 'l', 'm', - 'n', 'o', 'p', 'q', 'r', 's', 't', 'u', 'v', 'w', 'x', 'y', 'z', - 'A', 'B', 'C', 'D', 'E', 'F', 'G', 'H', 'I', 'J', 'K', 'L', 'M', - 'N', 'O', 'P', 'Q', 'R', 'S', 'T', 'U', 'V', 'W', 'X', 'Y', 'Z', - '0', '1', '2', '3', '4', '5', '6', '7', '8', '9', -] -MAX_USERNAME_LENGTH= 7 - -def check_is_username_valid(username): - if not len(username) <= USERNAME_LENGTH_RANGE[1]: - return False - for character in username: - if character not in ALLOWED_USERNAME_CHARACTERS: - return False - return True -``` - -Why is this code incorrect? Consider the case where `username = ''`. The function will return True even though this username is fewer than 1 character long. Our tests would be more thorough if we added a test with an empty string: - -* `''`: Invalid - -**Tests should cover the full range of inputs that the code being tested might be given. Remember to test error cases where the function is used incorrectly.** - -#### Mocking - -Most functions are quite a bit more complicated than our `check_is_username_valid()` example above. Often, they make calls to other functions that perform complicated operations we don't want to test. For example suppose we have a function `download(url)` that downloads the HTML code at `url` and returns it as a string. Now consider the following function that we want to test: - -```python -def get_current_time_utc(): - response = download('https://worldtimeapi.org/api/timezone/Etc/UTC.txt') - for line in response.split('\n'): - if line.startswith('datetime: '): - return line.lstrip('datetime: ') - return None -``` - -Note that https://worldtimeapi.org/api/timezone/Etc/UTC.txt returns text like this: - -```text -abbreviation: UTC -client_ip: -datetime: 2021-08-26T00:38:19.941464+00:00 -... -week_number: 34 -``` - -When testing `get_current_time_utc()`, we don't want to actually query worldtimeapi.org because we don't want our tests to fail just because we lose our network connection or the worldtimeapi.org servers go down. Instead, we want to replace `download()` with a fake function that we control. This function is called a _mock_ or _stub_, and the process of introducing such a function is called _mocking_ or _stubbing_. - -To return to our earlier example, we might replace `download()` with this mock function: - -```python -def mock_download(url): - return '\n'.join([ - 'abbreviation: UTC', - 'client_ip: 127.0.0.1', - 'datetime: 2021-08-26T00:38:19.941464+00:00', - ... - 'week_number: 34', - ]) -``` - -That way our tests still check that we can handle responses from worldtimeapi.org, but we don't have to worry about our test results depending on a network query to an external server. - -### Integration tests - -Unit tests have a major failing: even when each small unit of your code is correct, your program as a whole can be incorrect if you don't put those units together correctly. For example, consider the two functions below: - -```python -def search_for_substring(string_to_search, substring): - return substring in string_to_search - -def get_search_args_for_illegal_usernames(username): - return 'admin', username - - -if __name__ == '__main__': - print(search_for_substring(*get_search_args_for_illegal_usernames('i_am_an_admin'))) -``` - -If you run this code, you'll see `False` printed even though the username contains the substring `admin`. The problem is that `get_search_args_for_illegal_usernames()` returns the substring first and the string to search (the username) second, but `search_for_substring()` expects the arguments to be in the opposite order. In this case the code units are fine. The bug arises when we integrate those units together. - -We use integration tests to catch bugs like these. These tests are actually written just like our unit tests. The only difference is that we mock less in integration tests. - -### Unit versus integration tests at Oppia - -At Oppia, we don't distinguish clearly between unit and integration tests. Many of our backend unit tests end up being a lot like integration tests just because we don't bother to mock everything. However, the distinction is important because we do sometimes write integration tests on purpose, so you'll see some test code described that way. - -### When to mock - -We just saw how the difference between unit and integration tests largely comes down to how much mocking we do, but that raises a question: "When should you mock?" There is no hard rule, but we generally encourage developers to mock only when doing so makes the tests easier to write. Don't bother mocking every function call your unit of code makes. - -In fact, too many mocks can be a problem because when someone changes the code you've mocked, they have to remember to change your mock too. This introduces opportunities for errors that cause the mock code to diverge from the code being mocked, which can let bugs slip past the tests undetected. - -## Run backend tests - -You can run backend tests like this: - -```console -python -m scripts.run_backend_tests -``` - -Alternatively, you can run just a single test module or multiple test modules like this: - -```console -python -m scripts.run_backend_tests --test_targets=core.controllers.editor_test -``` - -The argument to `--test_targets` can be as specific as you like. For example: - -* Run a class of tests: `--test_targets=core.controllers.editor_test.BaseEditorControllerTests` -* Run a single test: `--test_targets=core.controllers.editor_test.BaseEditorControllerTests.test_editor_page` -* Run multiple tests: `--test_targets=core.controllers.editor_test,core.controllers.domain_test` - -If you also want to see the output of `print` statements and error logs in the terminal, use `--verbose` like this: - -Python: -```console -python -m scripts.run_backend_tests --test_targets=core.controllers.editor_test --verbose -``` - -For more information about `--test_targets` and other flags, run: - -```console -python -m scripts.run_backend_tests --help -``` - -Note that while the tests are running, you may see the word `ERROR` show up in the test logs. This does not necessarily mean that an error has occurred; it happens because some tests actually expect an error to be raised. - -If you want to speed up subsequent runs of the tests, use `--skip_install` like this: - -```console -python -m scripts.run_backend_tests --skip_install -``` - -You can also combine this flag with the ones mentioned above. For example, you can use it with the `--test-targets` flag for even faster runs like this: -```console -python -m scripts.run_backend_tests --test_targets=core.controllers.editor_test --skip_install -``` - -Note that this skips reinstalling the required libraries, so remember to run the tests without this flag every once in a while to keep them up to date. - -### Identifying whether the tests passed - -The tests pass if, at the end of the test output, you see the message: - -```text -All tests pass. -``` - -Every line representing a test class will also start with `SUCCESS`. - -However, one or more tests failed if you get something like this instead: - -```text -Ran 326 tests in 47 test classes. -(1 ERRORS, 0 FAILURES) -``` - -You can find more information about the exact errors by scrolling up and looking through the error log for tests marked `FAILED` (indicating that an assertion in the test failed) and `ERROR` (indicating that an exception was raised by the test). - -### Coverage reports - -#### Overall coverage - -We use a simple tool called *code coverage* to check that all of Oppia’s backend code is fully covered by at least one test. Coverage reports specify which lines of each file have not been executed in any test, and they report what percentage of each file (branches and lines) is covered by the tests. Currently, Oppia has achieved **100% backend line coverage**. We require that all changes maintain this full coverage. - -When writing a test for a function or class, you can generate a coverage report to verify that all the lines of the function/class have been included in the tests. To do this, simply add the `--generate_coverage_report` flag to the `run_backend_tests` command: - -Python: -```console -python -m scripts.run_backend_tests --generate_coverage_report -``` - -If there are **any** backend test errors, no coverage report will be produced. Please fix those errors and then re-run the above command. If the tests all pass, a coverage report will be printed that lists each backend file, along with the lines not covered by tests. Here is an example of a coverage report: - -```text -Name Stmts Miss Branch BrPart Cover Missing ----------------------------------------------------------------------------------------------------------------------------- -core/android_validation_constants.py 23 0 0 0 100% -core/constants.py 32 1 2 1 94% 3, 5->4 -core/controllers/access_validators.py 80 0 10 0 100% -``` - -Notice that `constants.py` has only 94% code coverage because line 3 and the branch `5->4` were not covered. This means that none of the tests executed line 3. It also means that a branch in the code causes execution to jump from line 5 to line 4, and none of the tests executed this branch. Here's an example of such a branch: - -```text -1 for elem in lst: -2 if elem == 0: -3 continue -4 pass -``` - -If we find an element in `lst` that is `0`, then we will hit the `continue` statement, and execution will jump from line 3 to line 1. This would be denoted in the coverage report as a branch `3->1`. We currently require that all files have full line coverage, and files not in the `scripts/backend_tests_incomplete_coverage.txt` exclusion list are also required to have full branch coverage. This means that you might see an overall coverage of less than 100% on your PR--that's okay so long as all the missing coverage is coming from branches in files that are in the exclusion list. - -#### Warning: full coverage does not mean your tests are comprehensive - -To see how tests that produce 100% coverage can still not be comprehensive, consider the following pseudocode: - -```text -1 # Compute the absolute value of a number -2 function absoluteValue(number) { -3 if number <= 0 { -4 return -number -5 } else { -6 return number -7 } -8 } -``` - -Now consider the following sets of test cases: - -* `absoluteValue(1)` and `absoluteValue(5)`: These test cases are not comprehensive because they only test positive numbers. Code coverage is also not 100% because line 4 is never executed. -* `absoluteValue(0)` and `absoluteValue(1)`: These test cases are not comprehensive because they do not test negative numbers, and it's important for an absolute value function to correctly handle negative inputs. However, the code coverage is 100% because both blocks of the `if` statement are executed. -* `absoluteValue(-1)`, `absoluteValue(0)`, and `absoluteValue(1)`: These test cases are comprehensive, and code coverage is 100%. Note that even though line 1 doesn't execute, coverage is 100% because line 1 is not executable. - -This example illustrates something very important about code coverage: **Code coverage less than 100% implies that the tests are not comprehensive, but code coverage of 100% does NOT imply that tests are comprehensive.** Therefore, while code coverage is a useful tool, you should primarily think about whether your tests cover all the possible behaviors of the code being tested. In other words, you should have a behavior-first perspective. Don't just think about which lines are covered. - -#### Associated test files - -We require that every backend file have an associated test file. For example, if you create a file `new_file.py`, you will also need to create a test file `new_file_test.py`. We have a CI check (defined by `.github/workflows/backend_associated_test_file_check.yml`) that will fail if any test files are missing. - -#### Per-file coverage - -Above, we discussed overall coverage, which measures how much of our code runs when all the tests run. Per-file coverage is similar, but it measures only how much of a file runs when that file's associated test file runs. We are working to achieve full per-file coverage, but since that work is incomplete, we currently allow files in the exclusion list `scripts/backend_tests_incomplete_coverage.txt` to have incomplete per-file line and branch coverage (though they must still have 100% overall line coverage). - -If your changes result in incomplete per-file coverage of a file not in the exclusion list, you'll see an error like this: - -```text - INCOMPLETE COVERAGE (95.0%): scripts.run_lighthouse_tests_test -Name Stmts Miss Branch BrPart Cover Missing ------------------------------------------------------------------------------ -scripts/run_lighthouse_tests.py 118 5 34 3 95% 107->116, 111-113, 114->107, 117-118 ------------------------------------------------------------------------------ -TOTAL 118 5 34 3 95% -``` - -If you want to check that you've fixed the per-file coverage issue without running all the tests, you can use `--test_targets` to run only the test file associated with the file whose coverage you want to check. Then in the overall coverage report, check the line for your file to see if it's 100%. Note that most other files in the codebase will show as being incompletely covered since you didn't run all the tests, and that's okay. - -## Write backend tests - -### Backend test structure - -We write our backend tests with Python's [unittest framework](https://docs.python.org/3/library/unittest.html). You should familiarize yourself with that framework by reading through the ["basic example" in its documentation](https://docs.python.org/3/library/unittest.html#basic-example). You should also take a look at what [assertion functions](https://docs.python.org/3/library/unittest.html#unittest.TestCase) are available. - -Backend test files live alongside the backend code files they test. For example, alongside `core/controllers/base.py` you'll find `core/controllers/base_test.py`. That `_test.py` suffix is important. It's how we identify which files have tests to run. Note that each file is entirely code or entirely tests. No file mixes code and tests. - -Inside test files, tests are organized into classes whose names end in `Tests`. Test cases are methods of these classes, and the method names begin with `test_`. Note that only methods beginning with `test_` and inside classes that inherit from `unittest.TestCase` are executed as tests. Here is an example of a test from `base_test.py`: - -```python -class HelperFunctionTests(test_utils.GenericTestBase): - - def test_load_template(self): - oppia_root_path = os.path.join( - 'core', 'templates', 'pages', 'oppia-root') - with self.swap(feconf, 'FRONTEND_TEMPLATES_DIR', oppia_root_path): - self.assertIn( - '"Loading | Oppia"', - base.load_template('oppia-root.mainpage.html')) -``` - -Notice that the test class inherits from `test_utils.GenericTestBase`, which provides a few functions you may want to use: - -* `GenericTestBase` inherits from `unittest.TestCase`, so all the normal unittest functions are available. In particular, we use the unittest assertion functions. - -* The base class provides two swap functions for mocking: - - * `swap(object, attribute_to_mock, mock)`: `attribute_to_mock` is the name of the thing you want to mock (it can be a variable, a function, or even a class). `object` is the object that has `attribute_to_mock` as an attribute, and `mock` is the mock value you want to substitute in for `attribute_to_mock`. For example suppose you want to mock the constant `DEV_MODE` in the module `constants` with `True`. You would call `swap(constants, 'DEV_MODE', True)`. - - * `swap_with_checks(object, attribute_to_mock, mock, expected_args=None, expected_kwargs=None, called=True)`: The first three arguments are the same as `swap()`. However, this function also lets you assert that your mock function is called in particular ways. - - `expected_args` takes a list of tuples, where each tuple contains a group of expected positional arguments. The test will assert that the function is called once with each group of expected arguments, in the order in which you specify the groups. Note that the order of the arguments within each group must match the order in which the arguments are passed to the function. - - `expected_kwargs` accepts a list of dictionaries. Each dictionary specifies keyword arguments as key-value pairs, and the test will assert that your function is called once with each group of keyword arguments you specify, in the same order as the dictionaries appear in the list. - - If `expected_args` is `None`, then no assertions are made about the positional arguments that the mock function receives. The same is true of `expected_kwargs`. - - Lastly, `called` specifies whether we expect the mock function to have been called. The test will fail if this assertion is not met. - - Consider this example: - - ```python - self.swap_with_checks( - subprocess, 'Popen', mock_popen, - expected_args=[(['python'],), (['python3'],)], - expected_kwargs=[{'shell': True}, {'shell': False}]) - ``` - - This code will assert that `mock_popen` receives the following calls in order: - - ```python - mock_popen(['python'], shell=True) - mock_popen(['python3'], shell=False) - ``` - - These swap functions each return a context where the mocking has been performed. You'll see this called a `swap` in the code. You can use the swap like this: - - ```python - my_func_swap = self.swap(module, 'my_func', mock_my_func) - - with my_func_swap: - func_being_tested() - ``` - - If you have a lot of swaps, the `with` statements can get pretty burdensome. You can use `GenericTestBase.exit_stack.enter_context` instead like this: - - ```python - my_func_swap = self.swap(module, 'my_func', mock_my_func) - - self.exit_stack.enter_context(my_func_swap) - - func_being_tested() - ``` - - There are also other swap functions you can use. They are defined and documented in [`core/tests/test_utils.py`](https://github.com/oppia/oppia/blob/develop/core/tests/test_utils.py). - -### Guidelines for writing good tests - -1. **Each test method should test only a single behaviour.** This helps both with naming the test and with ensuring that the test doesn't fail for unrelated code changes. - - * When naming a test, start by writing a full sentence that clearly describes its behaviour. Try not to abbreviate if possible, but, if you need to do so in order to fit within the 80-character limit, make sure that the resulting test name is still meaningful and easy to understand. - - * We recommend that test names follow the format: - - ```text - test_{{action}}_with_{{with_condition}}_{{has_expected_outcome}} - ``` - - where `{{action}}`, `{{with_condition}}`, and `{{has_expected_outcome}}` are replaced with appropriate descriptions. Put the outcome at the end, so that it's easy to compare consecutive tests that have slightly different conditions and divergent outcomes. Here are some examples that follow this format: - - * `test_get_by_auth_id_with_invalid_auth_method_name_is_none` - * `test_get_by_auth_id_for_unregistered_auth_id_is_empty_list` - - These names are good because it's easy to see the differences between the tests: one tests an invalid auth method, and the other tests an unregistered auth. Correspondingly, these conditions lead to different outcomes (`name_is_none' vs. 'id_is_empty_list'). - -2. Tests should use the following general structure: - - * **Setup** - this is where you prepare any inputs/environment needed for the test. - * **Baseline verification** - check the values without performing any action. This step is only needed if your action is state-changing (i.e. if the same assert statement would lead to one result at the baseline and a different result at the endline). Check the same values here as you check at the endline. - * **Action** - perform the action or function call that leads to the expected change. - * **Endline verification** - check that the values from the baseline verification have changed accordingly. - -3. **Test the interface, not the implementation.** That is, treat the function as a black box and test its behavior. Don't test that the function uses a particular implementation. This will help you design a better API from an external user’s perspective. For example, consider the following function: - - ```python - def absolute_value(number): - return abs(number) - ``` - - Let's forget for the moment that this function is pointless and just focus on thinking about how to test it. We should test the function by providing some values, some positive and some negative, and checking that their absolute values are returned. We should _not_ test it by mocking `abs()` and making sure it was called correctly, as that would be testing the implementation instead of the interface. Remember that we want to test that the function behaves correctly, and we can demonstrate this most clearly by providing numbers to our function and checking that the function returns absolute values. - - You can check whether you're following this principle by imagining what would happen if you changed the function. For example, say you implemented `absolute_value()` differently: - - ```python - def absolute_value(number): - if number < 0: - return -number - return number - ``` - - After changing the implementation, your tests should all still pass. In our example, our tests that provide various numbers would still pass. Our test that mocked `abs()` would not. - -4. Keep tests **simple**. Don't include any logic in the test. Write the test as a series of straightforward, descriptive and meaningful commands (also known in programming circles as "DAMP"). It's fine if there's some repetition, as long as the tests are easy to read. - -5. **Don't use what you don't need.** By default, prefer to inherit from `unittest.TestCase` when writing tests. Only use `test_utils.GenericTestBase` when you need to interact with models or the app itself, or when you need the mocking capabilities that `GenericTestBase` provides. This helps keep our tests lean and fast. - - As a general rule, if the only thing your test needs to do is run a function and assert something about its return value, then `unittest.TestCase` is good enough. See https://github.com/oppia/oppia/pull/10869/files for an example of where both kinds of tests were necessary. - -6. When building your suite of tests, try to include a range of possible behaviours, such as: - - * "Happy path" cases where the tested code was used correctly. - * Failure cases where the tested code was used improperly. - * Ambiguous cases that are on the edge between happy path cases and failure cases. - * Boundary/edge cases that stress the function's capabilities. Think of these as the test cases you'd use if you were trying to break the code. For example, checking how an absolute value function handles `math.nan` or a string. - - Also, try writing contrasting tests. For example, if you are checking that an exception is raised under a certain criterion, also add a test to ensure that the exception is not raised when the criterion is not satisfied. - -7. For **test outputs**, follow these guidelines: - - * Test each output as exactly and completely as possible. For example, it's better to compare equality for an entire dict rather than just checking that a particular value has changed. - * Use `assertTrue(value)` or `assertFalse(value)` instead of `assertEqual(value, True)` or `assertEqual(value, False)`. - * Use `assertIsNone(value)` instead of `assertEqual(value, None)`. - -8. If you create a new test module (a `*_test.py` file), you will need to add it to a shard in [`oppia/scripts/backend_test_shards.json`](https://github.com/oppia/oppia/blob/develop/scripts/backend_test_shards.json). See the [section on shards below](#sharding-backend-tests). - -### Common testing scenarios - -1. If a function tests **more than one behaviour**, split the test into multiple parts. For example, if you have a single test that looks like this: - - * Test: - - * Setup - * Action 1 - * Assertion 1 - * Action 2 - * Assertion 2 - - then split it into two tests as follows: - - * Test 1: - - * Setup - * Action 1 - * Assertion 1 - - * Test 2: - - * Setup + Action 1 - * Action 2 - * Assertion 2 - -2. For assertions that **check errors** (e.g. self.assertRaises or self.assertRaisesRegexp), keep the part of the code enclosed in self.assertRaises as small as possible so that you can be sure that the error is actually being caused by that part of the code (and not, say, by the setup code). - -3. **Guidelines for testing private methods/functions**: Tests should only be written to verify the behaviour of **public** methods/functions. Here are some suggestions for what to do in specific cases involving private functions (if this doesn't help for your particular case and you're not sure what to do, please talk to **@BenHenning**): - - * If you’re trying to access hidden information, consider getting that information from one level below instead (e.g. the datastore). - * If you want to test code within a private method/function, test it by instead calling a public function that makes use of that function, or move it to a utility (if it's general-purpose) where it becomes public. Avoid testing private APIs since that may lead to brittle tests in unexpected situations (such as when the implementation of the API changes but the behaviour remains the same). - -4. **For unit tests:** - - * You should test each method of a class individually, with one or more test cases for each method. - * Define a `setUp()` method in the test if functionality or variables are going to be reused between tests. - -5. **For integration tests:** - - Begin by defining the section of code you want to test and considering how it's supposed to behave. Then design test cases to check that it behaves correctly. You should write one test for each test case you design. - -### Sharding backend tests - -#### How sharding works - -We shard the backend tests into many smaller jobs that run in parallel on GitHub Actions. In your PRs, you'll see something like this: - -![Display of backend test jobs on a pull request](https://user-images.githubusercontent.com/19878639/109242853-ddd78700-77a9-11eb-9cae-da5cece9ab26.png) - -The jobs named like `Run backend tests (ubuntu-18.04, i)` are running the sharded jobs, each with its own number `i`. The last job, `Check combined backend test coverage`, checks that the code coverage is 100%. All these jobs are defined in [`.github/workflows/backend_tests.yml`](https://github.com/oppia/oppia/blob/develop/.github/workflows/backend_tests.yml). - -The shards are defined in [`scripts/backend_test_shards.json`](https://github.com/oppia/oppia/blob/develop/scripts/backend_test_shards.json). Here's what the top of that file looks like: - -```json -{ - "1": [ - "core.controllers.base_test", - "core.controllers.collection_editor_test", -``` - -Each shard is identified by a name, in this case `1`. You could run this shard with `python -m scripts.run_backend_tests --test_shard 1`. The shards are then defined by a list of test modules. For example, the module name for `core/controllers/base_test.py` is `core.controllers.base_test`. Notice that there is no `.py` at the end! - -#### Common errors - -Whenever you run a shard of backend tests, the `run_backend_tests.py` script checks to make sure the test modules on the filesystem and the modules in the shards file are exactly the same. If a shard has a module that isn't in the filesystem, you'll get a `Modules ... in shards not found` error. This often happens if the module name is incorrect. On the other hand, if there is a module on the filesystem that's not in the shards, you'll get a `Modules ... not in shards` error. This often happens because you forgot to add a new test to the shards. - -#### Adding new tests to shards - -The point of sharding the backend tests is to speed up test runs on PRs. When the backend tests run in parallel, we spread the tests out across the multiple machines available to us on GitHub Actions. This means it's important for the tests to remain evenly distributed across the shards. To help with that, please **add any new tests to the shard with the shortest runtime.** Further, **make sure that all shards run in under 30 minutes.** If all the shards are taking close to 30 minutes, create a new shard in the JSON file. You can find a shard's runtime from the test runs on your PR: - -![Display of backend test jobs on a pull request](https://user-images.githubusercontent.com/19878639/109242853-ddd78700-77a9-11eb-9cae-da5cece9ab26.png) - -### Examples - -#### Example: Writing unit tests for domain classes - -Suppose we have the following domain class: - -```python -class ExplorationTheme(object): - """Domain object representing a theme for an exploration.""" - - def __init__(self, exp_id, theme_str): - """Constructs an ExplorationTheme domain object. - - Args: - exp_id: str. ID of the exploration. - theme_str: str. Theme of the exploration. - """ - self.exp_id = exp_id - self.theme_str = theme_str - - def to_dict(self): - return { - 'exp_id': self.exp_id, - 'theme_str': self.theme_str - } - - def from_dict(cls, exp_theme_dict): - return cls( - exp_theme_dict['exp_id'], exp_theme_dict['theme_str']) -``` - -We can write unit tests for the class like this: - -```python -class ExplorationThemeDomainUnitTests(unittest.TestCase): - """Tests for exploration theme domain class.""" - - def setUp(self): - # Please remember to explicitly call the setUp method with the super class. - super(ExplorationThemeDomainUnitTests, self).setUp() - - self.exp_theme = exp_domain.ExplorationTheme('exp_id1', 'theme1') - - def test_to_dict(self): - self.assertEqual( - self.exp_theme.to_dict(), - { - 'exp_id': 'exp_id1', - 'theme_str': 'theme1' - } - ) - - def test_from_dict(self): - exp_theme_dict = { - 'exp_id': 'exp_id1', - 'theme_str': 'theme1' - } - new_exp_theme = exp_domain.ExplorationTheme.from_dict(exp_theme_dict) - self.assertEqual(new_exp_theme.exp_id, 'exp_id1') - self.assertEqual(new_exp_theme.theme_str, 'theme1') -``` - -#### Example: Writing integration tests for handlers (controllers) - -Suppose we have the following controller: - -```python -class UpdateExplorationVersionHandler(base.BaseHandler): - """Handler that updates an exploration version.""" - - @acl_decorators.can_edit_exploration - def post(self, exp_id): - exp_version = self.payload.get('exp_version') - exploration_instance = exp_models.Exploration.get(exp_id) - exploration_instance.exp_version = exp_version - exploration_instance.save() -``` - -We can write test cases like this: - -```python -class UpdateExplorationVersionHandlerTest(test_utils.GenericTestBase): - """Test for handler that updates the version of an exploration. - - The URL for this handler is: '/explorehandler/update_exp_version/' - """ - - def setUp(self): - super(UpdateExplorationVersionHandlerTest, self).setUp() - - self.exp_id = '15' - - self.login(self.VIEWER_EMAIL) - self.signup(self.VIEWER_EMAIL, self.VIEWER_USERNAME) - exp_services.load_demo(self.exp_id) - - def test_version_gets_updated_correctly(self): - exploration = exp_services.get_exploration_by_id(self.exp_id) - # The exploration is loaded at version 1. - self.assertEqual(exploration.version, 1) - - self.post_json( - '/explorehandler/update_exp_version/%s' % (self.exp_id), - {'exp_version': 123}) - - exploration = exp_services.get_exploration_by_id(self.exp_id) - self.assertEqual(exploration.version, 123) -``` diff --git a/Build-process.md b/Build-process.md deleted file mode 100644 index 4ca8372a..00000000 --- a/Build-process.md +++ /dev/null @@ -1,130 +0,0 @@ -## Table of contents - -* [Introduction](#introduction) -* [Build script](#build-script) -* [Build modes](#build-modes) - * [Build mode constants](#build-mode-constants) - * [Dev mode](#dev-mode) - * [Minify-only mode](#minify-only-mode) - * [Prod mode](#prod-mode) - * [Plain prod mode](#plain-prod-mode) - * [Maintenance mode](#maintenance-mode) - * [Deploy mode](#deploy-mode) -* [Considering build modes when debugging](#considering-build-modes-when-debugging) -* [Source maps](#source-maps) - -## Introduction - -Before we can run the Oppia server, we have to build it. This build process transforms the files defining our application (e.g. code, images, configuration files, etc.) from the developer-friendly format we store in GitHub into a form that can be executed to run the server. If you are running the local development server, this build process makes minimal changes, but when deploying to production, we do much more optimization. - -This page documents how the build process works in each of its modes. Most of the time, you won't have to worry about the build process since it is handled automatically by our scripts like `start.py`. However, you might need to consider the build process if you have trouble reproducing a bug; the bug might only happen in certain build modes. We'll discuss how to think about build modes when debugging below. - -## Build script - -The build process is performed by [`scripts/build.py`](https://github.com/oppia/oppia/blob/develop/scripts/build.py). If you ever want to know exactly how a particular part of the build process works, you should check out the code there. - -## Build modes - -There are two primary build modes: dev mode and prod mode. When running in prod mode, you can also enable maintenance mode or deploy mode, or you can choose to only minify third-party libraries. This means the build mode hierarchy looks like this: - -* Dev mode -* Minify-only mode -* Prod mode - * Maintenance mode - * Deploy mode - -Note that the maintenance and deploy modes can be enabled simultaneously, so you should really think of them as flags. For example, suppose you have decided to enable prod mode. You can further choose to enable maintenance mode, enable dev mode, enable both maintenance mode and dev mode, or leave both maintenance mode and dev mode disabled. - -Dev mode compiles the fastest, but it produces an application that's least like what we run in production. On the other hand, prod mode takes longer to compile but produces an application that is the closest to what we run in production. Minify-only mode is in the middle. - -We use prod mode for most everything besides local development. For example, we use prod mode when deploying to production and testing servers, and we run most tests in prod mode. A notable exception is the lighthouse accessibility tests, which run in dev mode because they don't depend on the minifications and optimizations applied in prod mode, and dev mode compilation is faster. Note that we also sometimes use prod mode locally when debugging to run a version of the app that's closer to the deployed version. - -### Build mode constants - -The build mode affects the application during runtime through the following constants in `assets/constants.ts`: - -* `DEV_MODE`: Set to `true` if and only if we are in dev mode. Otherwise, this is `false`. -* `EMULATOR_MODE`: Set to `true` if and only if we are not in deploy mode. Note that being in deploy mode implies being in prod mode. Otherwise, this is `false`. -* `ENABLE_MAINTENANCE_MODE`: Set to `true` if and only if we are in maintenance mode. Note that being in maintenance mode implies being in prod mode. Otherwise, this is `false`. - -### Dev mode - -We use dev mode for local development. For example, when you run `python -m scripts.start`, you build the app in dev mode. This mode is designed to build as quickly as possible, so it doesn't do much optimization. - -Here's what happens during a dev mode build: - -1. Some third-party libraries are generated. If you recently ran the server, then there might not be much to change here. Otherwise, the third-party libraries from `dependencies.json` are downloaded. (Note that the libraries in `dependencies.json` are just some of our frontend libraries; we have other libraries that get installed to `node_modules`.) The downloaded CSS and JavaScript code files are combined into `third_party/generated/third_party.css` and `third_party/generated/third_party.js`, and the downloaded fonts are placed in `third_party/webfonts/`. -2. The build script sets the constants in `assets/constants.ts` to enable dev mode. Specifically, `DEV_MODE` is `true`, `EMULATOR_MODE` is `true`, and the other constants are `false`. -3. [Webpack](https://webpack.js.org/) bundles our frontend code files and runs with a [dev configuration](https://github.com/oppia/oppia/blob/develop/webpack.dev.config.ts) designed for minimal compilation time. Note that the dev mode configuration file inherits from the [root webpack config file](https://github.com/oppia/oppia/blob/develop/webpack.common.config.ts). In dev mode, webpack runs in "watch" mode so that when files are changed, webpack re-builds the app automatically. - - Note that **this step is not handled by `build.py`** for two reasons: - - * When we run the frontend tests, webpack has to be handled by Karma, not `build.py`. - * When we run in dev mode, we need to keep webpack running so that it can watch for changes to the code. However, we don't want to keep `build.py` running since `build.py` is supposed to build the application and then exit. Therefore, we leave webpack compilation to be handled by whatever script you actually executed, for example `start.py`. - -4. The `app_dev.yaml` configuration file for the app already exists, so it doesn't need to be generated. - -### Minify-only mode - -In this mode, the third-party libraries are installed and minified, but the build script doesn't do anything else. This is only used by the frontend tests, where the `karma.conf.ts` configuration file triggers webpack as described above for [dev mode](#dev-mode). - -### Prod mode - -In prod mode, we more aggressively optimize the app for performance, but the cost of this optimization is that the build process takes longer. Prod mode is also a little more complicated because it has two additional options: maintenance mode, and deploy mode. - -#### Plain prod mode - -First, let's consider what happens in plain prod mode where those two options are both disabled: - -1. The third-party libraries are generated like in [dev mode](#dev-mode), but they are also minified so they take up less space. -2. Hashes are generated for asset files like images. These will be used later (see step 5 below). -3. [Webpack](https://webpack.js.org) runs with watch mode disabled and a [prod configuration](https://github.com/oppia/oppia/blob/develop/webpack.prod.config.ts) optimized for application performance. -4. `app.yaml` is generated from `app_dev.yaml`. The two files are identical in plain prod mode, except for a comment in `app.yaml` noting that it is auto-generated. -5. The third-party libraries, assets, files compiled by webpack, and some other files are all copied to a build folder `build/`. This mimics the build folder we generate and upload to production servers when deploying the app for real. During this process, asset files are renamed to include the hashes we generated earlier. This ensures that if the file content changes, the name changes, so any caches of the old file are invalidated. - -This is the mode our tests run in (except the lighthouse accessibility tests), and it's what you'll want to use if you want to locally run a version of the app that's as close to the production version as possible. You can use this mode like this: - -Python: -```console -python -m scripts.start --prod_env -``` - -#### Maintenance mode - -As far as the build process is concerned, maintenance mode works just like [plain prod mode](#plain-prod-mode) except that the `ENABLE_MAINTENANCE_MODE` constant gets set to `true`. Then when the app runs, only admins can log in. We use this when we are upgrading the production server and need to let jobs run while ensuring users don't change any data. - -This mode is used by our deployment scripts. You can also enable it locally like this: - -Python: -```console -python -m scripts.start --prod_env --maintenance_mode -``` - -#### Deploy mode - -Deploy mode is also very similar to [plain prod mode](#plain-prod-mode), except that the `EMULATOR_MODE` constant is set to `false`. Further, we make some changes to the `app.yaml` file: - -* Remove the `version: default` line -* Remove the environment variables specified in the `ENV_VARS_TO_REMOVE_FROM_DEPLOYED_APP_YAML` constant in `build.py`. We remove these since they are already present in the production environment. - -This mode is used by our deployment scripts. - -## Considering build modes when debugging - -Most bugs appear in all build modes, so we recommend first trying to reproduce bugs locally in dev mode. Dev mode is optimized for fast compilation, so it's usually faster to debug in [dev mode](#dev-mode). If you can't reproduce the bug in dev mode, then you can try using [plain prod mode](#plain-prod-mode). Plain prod mode takes longer to compile, but since it more closely mimics how the application works in production, you may need it for bugs that don't appear in dev mode. - -You should only need to use [maintenance mode](#maintenance-mode) if you are debugging a problem with the mode or testing it. You can't use [deploy mode](#deploy-mode) locally, so bugs specific to deploy mode can only be debugged on a testing server. Finally, we only use the [minify-only mode](#minify-only-mode) mode for the frontend tests, so you shouldn't ever need to run a server this way for debugging. You should just [[debug the frontend tests|Debug-frontend-tests]] directly. - -## Source maps - -Even when running in dev mode, the code your browser runs does not look much like the code on your file system. Files have been combined, and in prod mode, they've been minified. To make debugging easier, you can generate source maps that map from the code your browser is running back to the code on your file system. - -Generating source maps is expensive, so by default we don't make them. However, you can enable source map generation in both dev and prod modes, usually by passing a `--source_maps`, for example: - -Python: -```console -python -m scripts.start --source_maps -python -m scripts.run_e2e_tests --prod_env --source_maps -``` - -With these flags, the `webpack.*.sourcemap.config.ts` configuration files are used to enable source mapping. For information about using source maps, see our [[guide to debugging frontend code|Debug-frontend-code]] diff --git a/Calculating-statistics.md b/Calculating-statistics.md deleted file mode 100644 index a4ec611e..00000000 --- a/Calculating-statistics.md +++ /dev/null @@ -1,60 +0,0 @@ - - -This page should walk you through how to go from a statistic you want to calculate from Oppia data through creating the models, jobs and getting them up and running. For each step of the process, we've included a reference to an example Pull Request which will provide a good idea of what that step includes. - -This document walks through how to create each of these levels. - -The recommended way of going through this process is to: - * Plan the overall approach: start at the presentation layer and work your way down the layers to the event log (steps 1 - 4 in the diagram below). This will not involve writing code. - * Write code to record the data you need for your calculations (steps 5-7). - * Use the data in the UI (step 8). - -Each of these three sections will be a separate commit in a branch off of develop. After all the steps are completed and reviewed, this branch can be merged into develop. - -## 1-4. Figuring out what you need - -1. Start by thinking about what you want to display. This could mean drawing charts and/or listing out data you want to display. -2. List out processed data fields. Figure out which data you need to display and find one field for each. For example, a bulleted list would have one field in the model for each bullet point. A histogram could be drawn by having a list of data point values, but if you want to have standard deviation or mean, you would want to have separate fields for those as well. -3. Map out how you would calculate each field in your model. For some fields this may be straightforward, like how counting how many students go to the page for a number of students that started the exploration, whereas others might be more complex and this stage may take a while, like how confused are students. -4. Figure out what interactions you would need to keep track of to do those calculations. For mapping out completions of an exploration, this might be “I need to know when students complete the exploration.” Keep in mind when you would record these events. So if you wanted to know an average time spent in a particular state it would become “I need to know how much time was spent in the state when the student leaves the exploration”. - -So, you have a model of what you want to create and you know which model it should be in. - -## 5. Creating the events - -1. (Example: [#3841](https://github.com/oppia/oppia/pull/3841)) Changes to core.storage.statistics.gae_models.py: - - 1. Define a class of the form EventLogEntryModel. Each instance of this model will account for one count of the statistic you want to record. Make sure this model contains all the information you would need to identify the context of an instance of this model (exploration_id, schema_version etc...). Instances of this event model will be used to recompute the statistics aggregated model if there are any data discrepancies. - - 1. Now, add the statistic count as a field of the ‘ExplorationStatsModel’ in the same file. The field should be named _v2. Also keep in mind to update the corresponding getter methods and also, the save and retrieve methods for this model. - -2. (Example: [#3857](https://github.com/oppia/oppia/pull/3857)) Changes to ‘core.domain.stats_domain.py’: - - 1. Add the statistics field respectively in the ExplorationStats class and similarly modify corresponding helper methods. - 2. If it is a state level statistic, modify the StateStats class instead of the ExplorationStats class. - -## 6. Create Backend Handlers for incoming statistics - -1. (Example: [#3916](https://github.com/oppia/oppia/pull/3916/files#diff-7dae2fea02c39c3f79ba7f502b2ec181)) Now, open the file ‘core.controllers.reader.py’. - - 1. First, add validation for this new field in the class ‘StatsEventHandler’. This event handler will be updating the aggregated statistics model. - 2. Then, create a new EventHandler for the new statistics, of the form ‘EventHandler’. This will be the event handler that records an instance of each count for the statistic. - -2. (Example: [#3916](https://github.com/oppia/oppia/pull/3916/files#diff-5bc02cefb3ea9e27f1a6776eabd1935d)) Now, open the file ‘main.py’. Create a route through which the event models will be recorded. - -## 7. Record events from the User - -1. (Example: [#3916](https://github.com/oppia/oppia/pull/3916/files#diff-d12d8529029e2a0c1b2430573bace920)) Open the ‘core.templates.dev.head.pages.exploration-player-page.services.stats-reporting-service.ts’. Add a method to this file for handling any record of the event. The function name can be of the form ‘record’. This function performs two things: - - 1. Make a $http.post() call to the URL we created in step 6 (part 2) with the respective args to record an instance of the event log entry model. - 2. Increment count for this statistic field in the aggregatedStats dictionary which holds the values for the ExplorationStatsModel. Periodic calls will automatically be sent to the StatsEventHandler automatically to update the ExplorationStatsModel. - -2. (Example: [#3916](https://github.com/oppia/oppia/pull/3916/files#diff-4d9e2d3cc0a2b28fdbdb3585b080610e)) Now, we need to figure out where in the player view recording will be required. For state related stats, we would probably capture our new statistic on entering a state or leaving a state. For answer related stats, you’d probably capture your statistic inside the submitAnswer function in the player view. - -## 8. Using it in the UI - -Now, your newly added statistic will be available in the ExplorationStatsModel. You should find that the ExplorationStatsModel has already been retrieved in the statistics tab. Visualizing your newly added statistic will be simply using the corresponding field from the stats model in the view. - - - -Well, that’s the gist of it. Have fun recording your stats, and don’t forget to write tests throughout the way. \ No newline at end of file diff --git a/Coding-for-speed-in-GAE.md b/Coding-for-speed-in-GAE.md deleted file mode 100644 index 0eb78e7f..00000000 --- a/Coding-for-speed-in-GAE.md +++ /dev/null @@ -1,11 +0,0 @@ -When writing backend functions, it is good to keep in mind the effect that the written function has on an end user who is using a feature that calls that particular function, particularly the speed. - -If a lot of database requests are required in the backend, it can lead to slowdown for the end user. Hence, it is best to keep the total number of database requests to a minimum on any new function that is written in the backend, as much as possible. - -Try to use the `get_multi` function if the keys of all the models to be fetched are known beforehand or `get_all` to fetch every database entry for a class, though even this one, use only if it's absolutely needed. To filter models based on other conditions, some other GAE functions can be used such as `query()` with filters. - -You can refer to [this](https://github.com/oppia/oppia/blob/e7bd68feca31cd2309ba71c229094f3028ef296b/core/storage/question/gae_models.py#L202) function in the codebase for an example on how to use filters to directly fetch the required models from the datastore. - -The official documentation for the `query` function can be found [here](https://cloud.google.com/datastore/docs/concepts/queries#datastore-datastore-basic-query-python). - - diff --git a/Coding-style-guide.md b/Coding-style-guide.md index 3220ab10..153f1a03 100644 --- a/Coding-style-guide.md +++ b/Coding-style-guide.md @@ -3,13 +3,15 @@ Please follow the following style rules when writing code, in order to minimize If you use [Sublime Text](http://www.sublimetext.com/), consider installing the SublimeLinter, [SublimeLinter-jscs](https://github.com/SublimeLinter/SublimeLinter-jscs) and [SublimeLinter-pylint](https://github.com/SublimeLinter/SublimeLinter-pylint) plugins, following the instructions on their respective pages. ## General + - Ensure that your code looks consistent with the code surrounding it. - Strings should use single quotes (`'`) throughout Python and JavaScript. - Prefer having comments on their own line (above the code that's being commented on), as opposed to next to a line. The exception is when you need to disable a pylint warning for a specific line. - The last character in each file should be a newline. (If you're using Sublime, you can enforce this locally by adding `"ensure_newline_at_eof_on_save": true` to your user preferences file.) -- Avoid introducing `TODO (#XYZ): ...` comments in the files and instead try to do things correctly the first time. If you are going to add a TODO comment in any file then there needs to be (at minimum) a full comment and justification explaining what has been tried and what the issue is. The TODo should also reference an issue created on GitHub for thracking the problem. +- Avoid introducing `TODO (#XYZ): ...` comments in the files and instead try to do things correctly the first time. If you are going to add a TODO comment in any file then there needs to be (at minimum) a full comment and justification explaining what has been tried and what the issue is. The TODO should also reference an issue created on GitHub for tracking the problem. ## Design tips + - Avoid referencing elements of a list by a hardcoded index number, e.g. `item[0]`, `item[1]`. This is because the reader typically has no idea what is significant about the element index in question. If the values in the list are of different types, consider using a domain object instead to model the item being passed around. - Avoid passing raw "dictionaries" (Python dicts or JS objects) between functions, because it's possible to add new fields to them midway through their lifecycle, which can get confusing for readers of the code. Use domain objects instead, since they have a fixed set of fields. - Similarly, if you are passing in two lists of variables and you require both lists to be the same length (because the elements need to correspond with each other), consider using one list of composite domain objects instead. @@ -17,6 +19,7 @@ If you use [Sublime Text](http://www.sublimetext.com/), consider installing the - Functions that start with "get", or which have GET semantics, should, under no circumstances, update or delete anything. They should be safe to call and have no side effects. ## Python + - Consider using a frozenset or tuple to a list, if the data structure is not meant to be subsequently modified. This applies especially to constants. - If you need to raise an Exception, just do `raise Exception` -- no need to define custom exceptions. We tend to use exceptions fairly sparingly, though. - Otherwise, please follow the [Google Python style guide](https://github.com/google/styleguide/blob/gh-pages/pyguide.md). In particular: @@ -41,6 +44,7 @@ If you use [Sublime Text](http://www.sublimetext.com/), consider installing the the last DUPLICATE_EMAIL_INTERVAL_MINS. """ ``` + Docstrings should also contain `Args`, `Returns` and `Raises` whenever applicable in a method. For example: ``` @@ -58,25 +62,30 @@ If you use [Sublime Text](http://www.sublimetext.com/), consider installing the TypeOfException: Short description. """ ``` + - Never use backslashes to end a line. It's hard to tell whether they're escaping newlines, spaces, or something else. Use parentheses instead to break the line up, e.g.: ``` my_variable = ( my_very_long_module_name.my_really_long_function_name()) ``` + - Be careful [not to use mutable objects](https://google.github.io/styleguide/pyguide.html?showone=Default_Argument_Values#Default_Argument_Values) as default values in the function or method definition. i.e., don't do things like `def foo(a, b=[]):`. - Imports should be in three groups: standard libraries, files within the Oppia codebase, and third-party files. Each group should be separated by a single newline. Within each group, imports should be organized alphabetically. If you have additional questions, feel free to reference the [Google Python Style Guide](https://google.github.io/styleguide/pyguide.html#313-imports-formatting). ### Handling bytes and string in Python 3 + - Python 3 differs a lot in handling strings and bytes from Python 2 (you can read more about this in [this article](https://betterprogramming.pub/strings-unicode-and-bytes-in-python-3-everything-you-always-wanted-to-know-27dc02ff2686) or in this [Pragmatic Unicode talk](https://nedbatchelder.com/text/unipain.html)). Basically, strings (`str`) in Python 3 are Unicode by default and “bytes” (`bytes`) are lists of integers from 0 to 255 (lists of 8 bits). There is no implicit conversion between `str` and `bytes` in Python 3, so any conversion needs to be done explicitly using `encode` (`str` → `bytes`) and `decode` (`bytes` → `str`) functions. -Throughout Oppia, we typically use strings. However, you may come across bytes in places where there is an interaction with some outside library or API — for example, when standard input or output is read or written, or when data is read from or written to files. Some standard Python libraries also only accept bytes. +Throughout Oppia, we typically use strings. However, you may come across bytes in places where there is an interaction with some outside library or API — for example, when standard input or output is read or written, or when data is read from or written to files. Some standard Python libraries also only accept bytes. #### Rules for handling strings and bytes + ##### Bytes outside, strings inside The general rule you should follow is to keep all text in Oppia as strings, where possible. If a conversion to bytes is necessary, that conversion should happen as close to the “edges” of the app as possible. So, for example: + - When you receive bytes from some library, immediately convert them to string using decode. - If you need to use a function that needs bytes, use encode to convert the string to bytes immediately before you call the function. @@ -86,49 +95,54 @@ In the Oppia codebase all data (that we can decide about) should be encoded/deco If, in some case, an external source returns or receives data with a different encoding, it is fine to use that encoding only for that source. However, please first be sure to investigate whether that source can be configured to use utf-8 instead. - ### Apache Beam logic - - For pipe operations that span multiple lines, always have the pipe operator (`|`) begin on the new line. - e.g., prefer: +- For pipe operations that span multiple lines, always have the pipe operator (`|`) begin on the new line. - ```python - pcoll = ( - input_pcoll - | "Op1" >> Operation1() - | "Op2" >> Operation2() - | "Op3" >> Operation3() - | "Op4" >> Operation4() - ) - ``` + e.g., prefer: - over: + ```python + pcoll = ( + input_pcoll + | "Op1" >> Operation1() + | "Op2" >> Operation2() + | "Op3" >> Operation3() + | "Op4" >> Operation4() + ) + ``` - ```python - pcoll = ( - input_pcoll | "Op1" >> Operation1() | "Op2" >> - Operation2() | "Op3" >> Operation3() | - "Op4" >> Operation4() - ) - ``` + over: - Note: when all pipe operations can fit in a single line, there's no need to break them up: - ```python - pcoll = input_pcoll | "Sort" >> Sort() - pcoll = ( - input_pcoll | "Sort" >> Sort() | "Unique" >> Unique()) - ```` + ```python + pcoll = ( + input_pcoll | "Op1" >> Operation1() | "Op2" >> + Operation2() | "Op3" >> Operation3() | + "Op4" >> Operation4() + ) + ``` + + Note: when all pipe operations can fit in a single line, there's no need to break them up: + + ```python + pcoll = input_pcoll | "Sort" >> Sort() + pcoll = ( + input_pcoll | "Sort" >> Sort() | "Unique" >> Unique()) + ``` ## Black + Oppia uses [Black](https://black.readthedocs.io/en/stable/) as the standard Python code formatter. -Black enforces a consistent, opinionated style automatically and is run as a pre-commit hook. +Black enforces a consistent, opinionated style automatically and is run as a pre-commit hook. - **Automatic formatting:** Black runs every time you make a commit to ensure consistent code style. - **Manual formatting:** You can format a specific file manually using `black {{filepath}}`. For example, to format android.py, you would run: + ```bash black /home/dev/opensource/oppia/core/controllers/android.py - + + ``` + ## Prettier We use [prettier](https://prettier.io/) to format frontend code. It is configured based on [gts](https://github.com/google/gts). It is run as a pre-commit hook, i.e. it is executed every time you make a commit. @@ -148,77 +162,92 @@ Also, if you're using VSCode, here is a `.vscode/settings.json` that you can use ``` ## JavaScript + _General note: We use the ES2017 standard for our JavaScript/TypeScript code. (See [tsconfig.json](https://github.com/oppia/oppia/blob/57333f23af7b67914dc039671f4bc4e029fbb6e7/tsconfig.json#L4).)_ - We use extra parentheses if a statement breaks across multiple lines, similar to Python. In particular, when code in '(...)' or '[...]' spans more than one line, make a line break after the opening parentheses or bracket. - The indentation is always 2 spaces. - Try to start only function names with verbs to help distinguish them from variables. Conversely, do not start variable names with verbs. - For example: + For example: + - For a boolean variable to check if a card is displayed: + - Correct: `cardIsDisplayed` + - Wrong: `isCardDisplayed` - - For a boolean variable to check if a card is displayed: - - Correct: `cardIsDisplayed` - - Wrong: `isCardDisplayed` + - For a function to check if a card is displayed: + - Correct: `isCardDisplayed()` + - Wrong: `cardIsDisplayed()` - - For a function to check if a card is displayed: - - Correct: `isCardDisplayed()` - - Wrong: `cardIsDisplayed()` - We have started compiling a [style guide for JavaScript](https://docs.google.com/document/d/1ZDmLN66f53WdDPItFChu9Lr37z0dKoqR-ASX8UM5y60). This is currently a work in progress. However, please use this as the definitive guide when figuring out the correct way to name things (CamelCase, snake_case, etc.) -- The dependencies mentioned in strings and functional parameters of controllers, directives and factories should be in the following manner: dollar imports (e.g. ```$log, $scope``` etc.), regular imports (e.g. ```ContextService, PageService``` etc.), and constant imports (e.g ```COLLECTION_TAGS, DELETE_COLLECTION``` etc.) all in sorted order. - - For Example: - ```javascript - oppia.thing('ThingName', [ - '$sortedDollarImports', 'SortedRegularImports', - 'SORTED_CONSTANT_IMPORTS', - function( - $sortedDollarImports, SortedRegularImports, - SORTED_CONSTANT_IMPORTS) { - // The implementation of `ThingName`. - }]); - ``` +- The dependencies mentioned in strings and functional parameters of controllers, directives and factories should be in the following manner: dollar imports (e.g. `$log, $scope` etc.), regular imports (e.g. `ContextService, PageService` etc.), and constant imports (e.g `COLLECTION_TAGS, DELETE_COLLECTION` etc.) all in sorted order. + + For Example: + + ```javascript + oppia.thing("ThingName", [ + "$sortedDollarImports", + "SortedRegularImports", + "SORTED_CONSTANT_IMPORTS", + function ( + $sortedDollarImports, + SortedRegularImports, + SORTED_CONSTANT_IMPORTS, + ) { + // The implementation of `ThingName`. + }, + ]); + ``` + - For asynchronous functions that return a promise, use the following convention: - At the function declaration, use the keyword `async` (see [here](https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Statements/async_function)). - Add 'Async' to the function name. example: ```ts - const getUserInfoAsync = async function() { - return new Promise(resolve => { - setTimeout(function() { + const getUserInfoAsync = async function () { + return new Promise((resolve) => { + setTimeout(function () { resolve("something"); }, 2000); - }); - } + }); + }; ``` - For functions or variables that are private and that should not be exposed outside their immediate controller/service, prefix their names with an underscore (`_`) and add the `private` keyword. ## Typescript + - Make sure to follow all the javascript rules here as well. - Keep line lengths to at most 80 characters (with the exception of lines containing URLs, which are allowed to have a length of greater than 80 characters). - Declare a variable before usage. For instance: **Wrong usage:** + ```javascript exampleVar = true; if (someCondition) { exampleVar = false; } ``` + **Right usage:** + ```javascript var exampleVar = true; if (someCondition) { exampleVar = false; } ``` + - All loop variables should be declared. For instance: **Wrong usage:** + ```javascript for (item in itemList) { ... } ``` + **Right usage:** + ```javascript for (var item in itemList) { ... @@ -228,45 +257,54 @@ _General note: We use the ES2017 standard for our JavaScript/TypeScript code. (S - Do not add new properties to a declared variable. Ensure that all properties are declared in the variable declaration. For instance: **Wrong usage:** + ```javascript var person = { - name: 'name', - age: 'age' + name: "name", + age: "age", }; if (someCondition) { - person.address = 'address'; + person.address = "address"; } ``` + **Right usage:** + ```javascript var person = { - name: 'name', - age: 'age', - address: null + name: "name", + age: "age", + address: null, }; if (someCondition) { - person.address = 'address'; + person.address = "address"; } ``` + - Always initialize a variable at declaration. If you do not want a specific value at declaration, initialize the variable with a null value. For instance: **Wrong usage:** + ```javascript var person; if (someCondition) { - person = 'name'; + person = "name"; } ``` + **Right usage:** + ```javascript var person = null; if (someCondition) { - person = 'name'; + person = "name"; } ``` + - Do not overwrite the variable with a different type. Instead create a new variable whenever you have a different use case. For instance: **Wrong usage:** + ```javascript var person = { name: 'name', @@ -278,7 +316,9 @@ _General note: We use the ES2017 standard for our JavaScript/TypeScript code. (S officeName: 'office name' }; ``` + **Right usage:** + ```javascript var personForSchool = { name: 'name', @@ -290,7 +330,9 @@ _General note: We use the ES2017 standard for our JavaScript/TypeScript code. (S officeName: 'office name' }; ``` + - If you get compilation error which says that a property does not exist on a particular type, go through the type definitions of the type and do a type casting if required. For instance: + ```javascript var checkMismatch = function(searchQuery) { var isMismatch = true; @@ -302,6 +344,7 @@ _General note: We use the ES2017 standard for our JavaScript/TypeScript code. (S return isMismatch; }; ``` + Here `$(this).val()` is type casted to a string by using `$(this).val()` If we do not use a typecast, typescript will give a error `Property 'trim' does not exist on type 'string | number | string[]'` since val can be a string or a number or a string array. So, to use trim we specifically need it as a string. @@ -310,10 +353,12 @@ _General note: We use the ES2017 standard for our JavaScript/TypeScript code. (S You can add a new custom type definitions if type casting is not possible. In the file `typings/custom-element-defs.d.ts`, we add a new property to `HTMLElement` by adding a custom type defintion. In this type casting cannot be used, since we are adding a new property to the existing type instead of changing it to some other type. ### Karma test specific guidelines + - Use `angular.mock.module` instead of `module` since the typings for angular-mocks does not support the usage of module. - Use `angular.mock.inject` instead of `inject` to maintain a consistent behaviour. ### When to add custom type defintions to the typings folder? + - If you find a missing property in a typings package, create an issue [here](https://github.com/DefinitelyTyped/DefinitelyTyped) and a new file for the custom types with the issue link in the top of the file. - If you add a package for which type definitions are not found [here](https://github.com/DefinitelyTyped/DefinitelyTyped), add it to `third-party-defs.d.ts` - If you add a new property on window which is not present in typings for window, add it to `custom-window-defs.d.ts` @@ -321,6 +366,7 @@ _General note: We use the ES2017 standard for our JavaScript/TypeScript code. (S - Make sure that all files have comments which explain why these custom type defintions are required and additional comments to explain each new added property if required. For example, `typings/custom-scope-defs.d.ts` has a top level comment explaining that the type defintions are needed for properties defined on scope in link function and then there are additional comments with properties added specifying which file they belong to. Go through the existing files and try to follow the same pattern when adding a new file. ### Component Directives + Usage of old-style AngularJS directives is discouraged. Instead, use component directives. Component directives are an advanced version of AngularJS directives and are more preferred because of the "isolated scope" it creates and the reusability it offers across modules. This is also the way forward in Angular 2+. - Do not create standalone controllers. The standalone controllers are those which are associated with the `ng-controller` directive in the HTML file. @@ -332,15 +378,19 @@ Usage of old-style AngularJS directives is discouraged. Instead, use component d In all TypeScript files in `core/templates` we use webpack. That means that instead of including the required files by `` in HTML files we include them by using `require(…)` in the individual TS files. ### Adding `require(…)` to the TypeScript files with service/filter/factory + When you add new service/filter/factory dependency to service/filter/factory, you need to also `require(…)` it at the top of the file. For example if you have this filter: + ```javascript oppia.filter('normalizeWhitespace', [function() { return function(input) {…}; }]); ``` + and need to use `UtilsService` in this filter, you also need to add `require('services/UtilsService.ts');` (the paths are relative to the `core/templates` directory), the final filter will look like this: + ```javascript require('services/UtilsService.ts'); @@ -352,9 +402,11 @@ oppia.filter('normalizeWhitespace', ['UtilsService', function(UtilsService) { **The requires should be sorted in alphabetical order.** ### Adding `require(…)` to the TypeScript files with directive + The rules for directives are little bit more complex. You also need to add `require(…)` for service/filter/factory dependencies, but also if you use custom directive in the HTML you need to `require(…)` it in the TypeScript file too. For example, if you have directive: + ```javascript require('domain/utilities/UrlInterpolationService.ts'); @@ -362,7 +414,9 @@ oppia.directive('storySummaryTile', ['UrlInterpolationService', function(UrlInte return {…}; }]); ``` + and add `` into the **story_summary_directive.html** you need to also add the new `require('components/share/SharingLinksDirective.ts');` into the TypeScript file: + ```javascript require('components/share/SharingLinksDirective.ts'); @@ -378,23 +432,28 @@ oppia.directive('storySummaryTile', ['UrlInterpolationService', function(UrlInte ### Exporting variables and functions from a Typescript file to be imported in another Typescript file. If the file adds variable to the global scope: + ```javascript // functions.ts -var functions = function() { +var functions = function () { // something happens here. -} +}; ``` + We want to isolate that scope, this can be done by exporting the variable using ES6 exports. + ```javascript // functions.ts -var functions = function() { +var functions = function () { // something happens here. -} +}; export default functions; ``` + And then the variable can be loaded by `import functions from 'folder/folder/functions.ts';` ## CSS + - Do not include units if the value is 0. E.g. `margin-left: 0` instead of `margin-left: 0px`. - Within each CSS rule, attributes should be alphabetized (e.g. 'height' before 'margin' before 'top'). This makes it easy to find the value of an attribute if there are lots of them. - Avoid using `!important` as much as possible. @@ -402,20 +461,24 @@ And then the variable can be loaded by `import functions from 'folder/folder/fun - If the CSS class is oppia-specific, prefix it with `oppia-`. This helps distinguish it from CSS classes used by other third-party libraries. - For directives, include the CSS in the directive template file, similar to what we do in [this file](https://github.com/oppia/oppia/blob/b284a23d71133f48aa60d680ea5b72a7b0bbf552/core/templates/components/summary-tile/exploration-summary-tile.component.html). (Note that, in this case, all CSS rules should start with the top-level CSS class of the directive, so that they don't affect other elements outside it.) All other CSS should go in `core/templates/css/oppia.css`. ----- +--- + ### How to ensure that your code follows the coding guidelines: You can invoke the pre-commit script to ensure that your code follows the coding guidelines for a particular file that you've modified by running the following command from the root directory: + ```bash python -m scripts.linters.run_lint_checks --path filepath ``` If you'd like to run the checks for a list of files, run the following command: + ```bash python -m scripts.linters.run_lint_checks --files file_1 file_2 ... file_n ``` If you'd like to run the checks for a list of file-types, run the following command: + ```bash python -m scripts.linters.run_lint_checks --only_check_file_extensions file_extension_type_1 file_extension_type_2 ... file_extension_type_n ``` diff --git a/Conducting-research-with-students.md b/Conducting-research-with-students.md deleted file mode 100644 index b8ba025f..00000000 --- a/Conducting-research-with-students.md +++ /dev/null @@ -1,19 +0,0 @@ -One of Oppia's aims is to make it easy for people to create and share high-quality lessons online, so that others can learn anything they want to in an effective and enjoyable way. In order for this to happen, we need to learn what "high-quality" means, and would like to facilitate research that aims to answer this question. We also care about ensuring that Oppia's lessons are accessible to and effective for all students, regardless of the student's background or individual circumstances. - -If you'd like to help others learn a topic, and you have a hypothesis about the effectiveness of different ways of conveying it, you can test this hypothesis by going to [Oppia.org](https://www.oppia.org) and creating one exploration that exhibits it and another exploration that does not. In order to control the results, you could also incorporate the same "pre" and "post" questions in both explorations. The exploration editor will then allow you to see where learners get stuck, what they reply (in aggregate), and how of them complete your exploration, which could help you improve rough spots and get deeper insights into how learning works. - -We'd like to make these insights available to everyone, so that the quality of learning material on the Internet as a whole improves. As a contributor, you are welcome to do the following individually: - - * **Conduct experiments and tell the Oppia team about your work.** If you have an idea for a research project, you can email admin@oppia.org with a research plan. If there is additional functionality you need in order to conduct such experiments, we may be able to add it, especially if the results have educational value and are likely to be useful for other creators. - - * **Compile best practices (substantiated by research) for creating great learning experiences.** If you do experiments and write them up, please let us know, since we may be able to feature it and it may be useful for other exploration creators. Over time, as we develop our understanding of what ways of creating an exploration work better than others, we hope to improve the Oppia editor to make it easy to create good explorations from the get-go. - -There are also ways to contribute as part of a larger effort. Here is a list of our current projects and how you can help (note that being a professional researcher is not a prerequisite for any of these): - -## Improving Oppia's basic mathematics lessons - -Currently, a large part of the team is focusing on creating a set of lessons to teach basic mathematics, in a way that ensures that everyone in the world can access and learn effectively from these, no matter what their background. (This is important because more than half of children and adolescents worldwide [aren't achieving minimum proficiency in these skills](http://uis.unesco.org/sites/default/files/documents/fs46-more-than-half-children-not-learning-en-2017.pdf).) Here are some of the existing lessons we've created, with more on the way: [Fractions](https://www.oppia.org/learn/maths/fractions), [Ratios](https://www.oppia.org/learn/maths/ratios) and [Negative Numbers](https://www.oppia.org/learn/maths/negative-numbers). - -We'd like to do some **usability studies** to ensure that these lessons are effective for students in various demographics. This involves letting students play through a lesson, and making notes on stumbling blocks and confusion points. Such feedback is very useful to the lesson creators, because it helps ensure that the lessons can be improved for future students. - -If you'd like to help out with this project, please fill out [this form](https://forms.gle/jEytndtgdsx7BrnV6), and we'd be happy to help you get started! diff --git a/Contributing-code-to-Oppia.md b/Contributing-code-to-Oppia.md deleted file mode 100644 index 35307a4e..00000000 --- a/Contributing-code-to-Oppia.md +++ /dev/null @@ -1,212 +0,0 @@ -_These instructions are for developers who'd like to contribute code to improve the Oppia platform. If you'd prefer to help out with other things, please see our [[general contribution guidelines|Home]]._ - -Thanks for your interest in improving the Oppia platform! This page explains how you can get involved. - -If you run into any problems along the way, we're here to help! Check out our [[Getting Help Page|Get-help]] for the communication channels you can use. If you find any bugs, you can also file an issue on our [issue tracker](https://github.com/oppia/oppia/issues). There are also lots of helpful resources in the sidebar, check that out too! Also, if you'd like to get familiar with Oppia from a user's point of view, you can take a look at the [user documentation](http://oppia.github.io/). - -**Important! Please read this page in its entirety before making any code changes.** It contains lots of really important information. You should also read through our [[guide to making pull requests|Rules-for-making-PRs]]. - -## Table of Contents - -* [How to find information in the repo and the wiki](#how-to-find-information-in-the-repo-and-the-wiki) -* [Setting things up](#setting-things-up) -* [Developing your skills](#developing-your-skills) -* [Finding something to do](#finding-something-to-do) - * [Good first issues for new contributors](#good-first-issues-for-new-contributors) - * [How to tackle good first issues](#how-to-tackle-good-first-issues) - * [Contributor Roles](#contributor-roles) - * [Tasks for Existing Contributors](#tasks-for-existing-contributors) -* [Tips for Success](#tips-for-success) -* [Notes](#notes) - -## How to find information in the repo and the wiki - -1. In the top-right corner of the repository page, you'll find a search bar. Click on it to expand the search options. -2. Enter the search query you want to use. You can search for specific keywords, file names, code snippets, or any other relevant information you're looking for. -3. GitHub provides some advanced search filters that you can use to refine your search further. You can filter results by the file type, code language, author, or specific locations within the repository. To use these filters, click on the "Filters" button next to the search bar and select the desired options. -4. Press Enter or click on the search button to initiate the search. -5. GitHub will display the search results based on your query. The results will show matching files, code snippets, issues, pull requests, commits, discussions, wikis and other relevant information which you can see in left side after search. -6. You can click on any search result to view more details or explore the code/files associated with it. - -Additionally, GitHub also provides an advanced search syntax that allows you to construct more complex queries. You can find more information about this syntax in the [GitHub documentation](https://docs.github.com/en/github-ae@latest/search-github/searching-on-github/searching-code). - -[How to find information in the repo and the wiki demonstration](https://github.com/oppia/oppia-web-developer-docs/assets/76530270/5ad36445-61a5-448d-8432-db6fad24c384) - -## Setting things up - -1. Please sign the CLA so that we can accept your contributions. If you're contributing as an individual, use the [individual CLA](https://goo.gl/forms/AttNH80OV0). If your company owns the copyright to your contributions, a company representative should sign the [corporate CLA](https://goo.gl/forms/xDq9gK3Zcv). **If you do not sign the CLA, any PRs you open will be closed.** - -2. Fill in the [Oppia contributor survey](https://goo.gl/forms/otv30JV3Ihv0dT3C3) to let us know what your interests are. (You can always change your responses later.) - -3. Say hi and introduce yourself on [GitHub Discussions](https://github.com/oppia/oppia/discussions/16715)! - -4. Install Oppia following the [[installation instructions|Installing-Oppia]]. If you run into any issues, please check out the [[troubleshooting instructions|Troubleshooting]]. If you want help setting up a code editor, also check out our [[guide to common IDEs|Tips-for-common-IDEs]]. If the above resources don't help and you're still stuck, please check [GitHub Discussions](https://github.com/oppia/oppia/discussions/categories/q-a-installation) to see if any existing threads address the issue. If not, feel free to start a new thread explaining what you've tried and what you're seeing, so that we can try and help you! (You will probably get a faster response if you include a [[debugging doc|Debugging-Docs]] explaining what local investigations you have tried so far.) - -5. Update your GitHub settings: - - * [Set up 2FA]( https://help.github.com/articles/securing-your-account-with-two-factor-authentication-2fa/) on your GitHub account. **This is important to prevent people from impersonating you.** - - When using 2FA, you might need to create a [personal access token](https://help.github.com/articles/creating-a-personal-access-token-for-the-command-line/) so that you can log in from the command line. Alternatively, you can [log in using SSH](https://docs.github.com/en/github/authenticating-to-github/connecting-to-github-with-ssh). - - * Set your GitHub notification preferences [here](https://github.com/oppia/oppia/subscription). The important thing is to make sure you notice when someone replies to a conversation you're part of -- many people choose "Not watching" so that they do not get overwhelmed. Selecting "Watching" will notify you about everything that happens on the Oppia repository (which is a lot!), and notifications specifically addressed to you might end up getting lost in the noise. - - * (Optional) Consider setting up [automatic auth](https://help.github.com/articles/caching-your-github-password-in-git/) so you don't have to type in a username and password each time you push a change. Note that this isn't an issue if you use SSH. - -6. Familiarize yourself with the resources linked to from the sidebar of this page, especially the [[overview of the codebase|Overview-of-the-Oppia-codebase]], the [[coding style guide|Coding-style-guide]], and the [[Frequently Asked Questions|Frequently-Asked-Questions]]. You don't have to read all the other stuff right now, but it's a good idea to be aware of what's available, so that you can refer to it later if needed. - -7. Take up your first Oppia starter issue! (See [below](https://github.com/oppia/oppia/wiki/Contributing-code-to-Oppia#finding-something-to-do) on how to do this.) Make sure to read and follow the [[PR instructions|Rules-for-making-PRs]] closely so that your PR review proceeds smoothly. - - * In your browser, consider bookmarking the [[guide to making pull requests|Rules-for-making-PRs]] for easy reference, as well as the ["my issues" page](https://github.com/issues/assigned) (so that you can keep track of the issues assigned to you). - - * Facing any problems (including non-coding ones)? Please feel free to create a [GitHub Discussion](https://github.com/oppia/oppia/discussions) and get help from the Oppia community. You can use this avenue for asking anything -- questions about any issue, who to contact for specific things, etc. - -8. When you have merged at least one PR, please fill in [this form](https://forms.gle/NxPjimCMqsSTNUgu5) to let us know what team(s) you are interested in joining (to join a team you must have completed at least one of that team's issues). We will look at your contributions and invite you to become an Oppia collaborator! This will grant you access to the repository, and allow you to join a team. (But please don't create your own issues and then make PRs for them -- that won't count.) Since we want you to have a good experience contributing to Oppia, team leads may ask you to develop your skills on some more substantial issues before onboarding you to the team. - -## Developing your skills - -> [!NOTE] -> Oppia's wiki now includes **tutorials**, which are denoted in the sidebar by 👣 icons. Use them to learn key skills that are needed for solving issues! - -The Oppia wiki has lots of useful documentation. We **strongly recommend** looking through the resources under "Developing Oppia" in the wiki sidebar. Good places to start include the [[Overview of the Oppia codebase|Overview-of-the-Oppia-codebase]] and the [[tips on how to find the right code to change|Find-the-right-code-to-change]]. - -Our [[page of learning resources|Learning-Resources]] also has suggestions on how you can improve your development skills. When you take up an issue at Oppia that requires programming languages or tools you are unfamiliar with, check out that page for resources that other developers have found useful when learning. - -## Finding something to do - -### Good first issues for new contributors - -Welcome! Please make sure to follow the instructions above if you haven't already. - -After that, you can choose a good first issue from the [list of unassigned good first issues](https://github.com/oppia/oppia/issues?q=is%3Aopen+label%3A%22good+first+issue%22+no%3Aassignee). These issues are hand-picked to ensure that you don't run into unexpected roadblocks while working on them, and each of them should have clear instructions for new contributors. If you see one that doesn't, please let us know via [GitHub Discussions](https://github.com/oppia/oppia/discussions) and we'll fix it. For other issues, you might need to be more independent because we might not know how to solve them either. - -Please only work on issues that are labelled **"Impact: High"** or **"Impact: Medium"**, and that **have no assignee**. Any such issue is available for contribution (you don't need to ask on the issue thread whether it is available or not). - -> [!CAUTION] -> - Do not work on "Impact: Low" or "Backlog" issues. If you submit a fix for those, you will probably be asked to pick a different issue. Those issues often haven't been looked at closely yet, and might not be priorities for implementation. -> - Do not work on issues with the "triage needed" label, including issues that you recently filed. These issues haven't been vetted yet and might get closed or modified during the weekly triage process. - -As a new contributor, if you run into any problems along the way, we're here to help! Check out our [[Getting Help Page|Get-help]] for the communication channels you can use. Also, it's worth bearing in mind (especially when starting out) that trying to understand the entire project at once is unrealistic -- focus on building up your understanding one small step at a time. - -You can also browse good first issues for each of the core Oppia Web teams to find something you'd enjoy working on! Please only choose issues that have **not yet** been assigned, unless the issue is a "checkbox issue" with multiple claimable parts. Here are the project boards for the different teams: - -- Learner and Creator Experience (LaCE): https://github.com/orgs/oppia/projects/3/views/10, typically frontend or full-stack -- Developer Workflow: https://github.com/orgs/oppia/projects/8/views/10, typically backend or frontend -- Contributor Dashboard: https://github.com/orgs/oppia/projects/18/views/14, typically frontend or full-stack - -### How to tackle good first issues - -When you've found a good first issue you'd like to tackle, please investigate it first to understand why the issue is happening. Here are some things you should do: - -- Read the entire discussion thread to understand what has been tried so far. -- Try to reproduce the issue on your local dev server. (For Contributor Dashboard issues, the [[Contributor Dashboard onboarding guide|Contributor-dashboard]] has some useful setup information. For Learner and Creator Experience related issues, you can refer to the [[LaCE onboarding guide|LaCE-onboarding-guide]].) -- Figure out why the problem is happening, and [find the relevant code in the Oppia repository to change|Find-the-right-code-to-change]. If you have trouble with this, feel free to ask on [GitHub Discussion](https://github.com/oppia/oppia/discussions) and explain what you've tried doing so far. -- Try and get a fix working on your local dev server. You will need to do this in order to claim the issue (see below). - -Once you have a good understanding of the issue, you can ask for it to be assigned to you by leaving a comment as follows: - -- Show a video of the fix working correctly on your local machine. (For user-facing changes, your video should show a URL that starts with localhost:8181.) When creating a video, make sure to follow exactly the same steps as the issue author did in their video/description, so that your video can be compared to theirs to confirm that it fixes the problem. -- (If fixing a bug) Explain clearly what the **root cause** of the bug is, pointing to specific lines of code in the Oppia codebase if needed. -- Explain clearly what you did to tackle the issue (at a minimum, point to which file(s) you modified and describe the changes you made). You can include code snippets if you like. -- @-mention the leads of the corresponding project (you can find their details [here](https://github.com/orgs/oppia/projects)), letting them know you'd like to work on it and when you can submit a PR by. - -If your proof looks good and your explanation makes sense, we'll assign the issue to you. Once assigned, feel free to submit a PR and get it merged by following the [[instructions for making a PR|Rules-for-making-PRs]]. We recommend actively working to merge your PR soon after getting assigned, because we will de-assign contributors if the PR is closed for being stale or there is no activity after the initial assignment. - -> [!IMPORTANT] -> Please follow the [[PR instructions|Rules-for-making-PRs]] carefully, otherwise your PR review may be delayed or your PR may be closed. - -#### Getting help - -Generally, we aim to review PRs within 48 hours. If you have not heard from a reviewer within 48 hours, feel free to leave a comment on the review thread, and also add a message to the "Contacting Folks" section of [GitHub Discussions](https://github.com/oppia/oppia/discussions/categories/q-a-contacting-folks). **Don't wait longer than 2 days to do this.** - -If you run into a general problem, please create a [GitHub Discussion thread](https://github.com/oppia/oppia/discussions) to get help from the Oppia community. - -**Note:** It is important to pick a starter issue that you are able to do! If you find that the issue you are tackling is too difficult, it is fine to choose another one instead and come back to it later. - - -### Contributor Roles - -If you want to play a more integral role in sustaining Oppia, you can look forward to being able to take on more responsibilities as you continue to make quality contributions to the project. Here is a rough outline of the roles developers play at Oppia: - -```mermaid -graph TD; - Everyone("Everyone (read access)") --> Contributors("New Contributors (read access)") --Get 1+ PRs Merged--> Collaborators("Collaborators (triage access)") --Make Sustained Quality Contributions--> Members("Members (write access)") --> Lead(Project Leads
    and
    Core Maintainers); -``` - -As a new contributor, you won't have any permissions on the repository except to read the code, so you'll need to ask other developers (or Oppiabot) to assign reviewers to your PR or add labels to your issue. - -After you've merged a PR into develop, you can fill in [this form](https://forms.gle/NxPjimCMqsSTNUgu5) to become an Oppia collaborator! This will grant you access to the repository, and allow you to join a team of your choice. Once you fill out the form, we'll mail you a collaborator invite link for the Oppia repository -- this is a manual process, and may take up to 48 hours. Please visit [this link](https://github.com/oppia/oppia/invitations) to accept the invitation to collaborate. Feel free to email us at dev-support@oppia.org if you don't receive the email! - -Then you'll be a collaborator with triage access, which lets you assign reviewers and labels. No more asking for reviewers to be assigned! If you continue to make quality contributions, you may be added as a member of the Oppia organization, which grants you write access. Then you'll be able to restart tests, serve as a [code owner](https://docs.github.com/en/articles/about-code-owners), and review pull requests. - -Finally, after you've been contributing to the project for a while, you may become a project lead and/or core maintainer. In those roles, you'll help plan and lead Oppia's development. - -If you ever wonder why you don't have permission to perform some action on the Oppia repository, it might be because of your role. GitHub details each role's privileges in more detail in [their documentation](https://docs.github.com/en/organizations/managing-access-to-your-organizations-repositories/repository-permission-levels-for-an-organization). - -### Tasks for Existing Contributors - -There are lots of options! - -* **Want easy issues?** Check out our [list of "good first issues"](https://github.com/oppia/oppia/labels/good%20first%20issue). - -* **Want to join a team working on a larger effort?** See our [list of projects](https://github.com/oppia/oppia/projects). - -* **Want to practice debugging?** Check out our [list of issues needing debugging help](https://github.com/oppia/oppia/issues?utf8=%E2%9C%93&q=is%3Aissue%20is%3Aopen%20label%3A%22needs%20debugging%22%20). - -* **Want to practice writing a design doc?** Check out the [list of issues requiring a design doc](https://github.com/oppia/oppia/labels/needs%20design%20doc). This is useful for learning how to write good "technical implementation" proposals. - -* **Want to lead a project?** Let us know by emailing dev-support@oppia.org. We may offer you the opportunity to do this once you've sent in several good PRs. - -* **Want help figuring out what to do?** Just ask us on [GitHub Discussions](https://github.com/oppia/oppia/discussions). We'll try to help! - -If an issue hasn't got someone assigned to it, and there's no existing PR for the issue (you can check this by scanning the list of [existing PRs](https://github.com/oppia/oppia/pulls)), feel free to take it up by assigning yourself to it, and let the corresponding team lead know. Also, if you need help or advice on an issue, you can contact the corresponding team lead. See the [Projects page](https://github.com/oppia/oppia/projects) for details of who the team leads are. - -## Tips for Success - -1. Make dependent (“stacked”) PRs to ensure smaller time between reviews and subsequent PRs. A large PR results in difficulty to review for the reviewer as well as difficulty in making changes according to a review for an author. So, it is better to create smaller PRs which deliver a single small goal. If you have code changes dependent on a PR, it is better to create a separate PR for those changes. - -2. Try and follow test-driven development. This is the easiest way to make sure the code you wrote is working fine. The basic idea is to first write tests for the expected behaviour and then write code that will pass those tests. Refer to our [[guides for writing good tests|Tests]]. - -3. If you're stuck on something, ask for help instead of letting it block all your work. It can be difficult to know when to ask for help, so one rule of thumb is to ask whenever you've been unable to make progress for an hour. - -4. When asking questions, follow our guide to [[getting help|Get-help]] to make sure your question gets to the right people and has the information they need to help you. - -5. Remember that you're working with volunteers, nearly all of whom spend most of their time at school or a job. Don't be surprised if it takes a few hours for someone to get back to you (they might be in a different time zone!). - -6. Do a self-review to find your own mistakes. Reviews by other developers take time, so any mistakes you can catch by yourself will speed things up. - -7. Take the time to understand what the code you are changing is doing. Sometimes we see PRs for changes that don't do anything useful or that don't make sense given the context of the code. These won't get merged. - -8. If you are making a contribution which involves changing some user interface or introducing a new feature, it is good to start with a design doc to avoid wasting efforts later. Follow our [[guide for writing design docs|Writing-design-docs]]. - -9. You will almost certainly run into bugs where you aren't sure what the cause is. This is normal! Our [[debugging guides|Debugging]] will help you investigate the problem. - -## Notes - -* Our central development branch is `develop`, which should be clean and ready for release at any time. All changes should be done in feature branches based off of `develop`. - -* Sometimes, GitHub comments in the main conversation thread don't have a reply box. This seems to be a quirk with GitHub: apparently the reply box doesn't show up on outdated threads if you're currently in the middle of a review (or a reply to someone else's review) that you haven't submitted. After you submit the review, the reply field should show up again. - -* If you want to do a codebase change that is large and somewhat repetitive, do a small trial PR first for a limited subset of the change, and check with reviewers whether the approach makes sense. Only after getting that trial PR merged (or at least approved by all reviewers) should you do the full change. - - This helps because, if you just did the full PR at the outset instead, then if a reviewer requests changes to the approach, you'd need to go back and modify all the files. On the other hand, with a trial PR, addressing an initial round of changes is less work, and by the time you get to the full PR, you'd already know what you need to do! - -* To find the author of a particular change in a file, run this command: - - ```console - git blame file-name - ``` - - The output will show the latest commit SHA, author, date, and time of commit for each line. - - To confine the search of an author between particular lines in a file, you can use: - - ```console - git blame -L 40,60 file-name - ``` - The output will then show lines 40 to 60 of the particular file. - - For more `git blame` options, you can visit the [git blame documentation](https://git-scm.com/docs/git-blame). You can also view this information on GitHub. Just navigate to the file you are interested and click the "Blame" button: - - ![Screenshot showing the blame button on GitHub](images/githubBlame.png) - -* **Important** PRs marked with the “critical” label need to be tested in the backup server before being merged. For this, one of the release coordinators (with access to deploy) should checkout a new branch from develop, merge the branch from the PR into the new branch, and initiate deployment to the backup server from this branch. The PR author should give specific testing instructions for the changes (like which job to run, what the expected output is, etc) and the coordinator should verify the same. Once successfully tested, the PR should be merged into develop. This is to prevent cases like exploration migrations which can result in data corruption (as it will auto-migrate) if the migration isn’t safe. The "critical" label needs to be applied on PRs that change data validation checks, and other possibly critical changes which could affect production data. diff --git a/Contributing-to-Oppia's-design.md b/Contributing-to-Oppia's-design.md deleted file mode 100644 index 6821958b..00000000 --- a/Contributing-to-Oppia's-design.md +++ /dev/null @@ -1,18 +0,0 @@ -Thanks for your interest in helping out with Oppia's UX! From user research to product design, UX is an important part of the Oppia project. It’s our priority to provide learning and teaching experiences that are engaging and enjoyable. - -The following instructions explain how to get started, and how to find a fun project to work on: - -## Getting Started ## -1. Sign the CLA so that we can accept your contributions. If you're contributing as an individual, use the [individual CLA](https://goo.gl/forms/AttNH80OV0). If your company owns the copyright to your contributions, a company representative should sign the [corporate CLA](https://goo.gl/forms/xDq9gK3Zcv). -2. Fill in the [Oppia contributor survey](https://goo.gl/forms/otv30JV3Ihv0dT3C3) to let us know your interests. You can always update your responses later. - - *Note that when filling out this survey, be aware that specifying that you want to contribute to the codebase will result in you also being added to the technical onboarding process. If you're not actually interested in contributing code, you should answer 'No' to that question.* -3. Familiarize yourself with [Oppia's design guide](https://xd.adobe.com/view/e54eaf14-243c-4cf4-8b9e-d8ff8c6933cc-b01d/grid/). - -## Ways to contribute… ## -* **Create art for lessons** - * Oppia lessons incorporate colorful illustrations for storytelling and educational purposes. These illustrations are one of the reasons why our lessons are engaging! If you’re interested in helping out with these graphic design efforts, please fill in [this volunteer form](https://forms.gle/GrcrTQTgEj1n9aZn7). -* **Suggest improvements** - * If you notice something on the site that looks a bit off (and that you'd also like to work on), please feel free to log a -[new issue](https://github.com/oppia/oppia/issues/new?title=Describe%20your%20feature%20request%20or%20bug%20report%20succinctly&body=If%20you%27d%20like%20to%20propose%20a%20feature,%20describe%20what%20you%27d%20like%20to%20see.%20Mock%20ups%20would%20be%20great!%0A%0AIf%20you%27re%20reporting%20a%20bug,%20please%20be%20sure%20to%20include%20the%20expected%20behaviour,%20the%20observed%20behaviour,%20and%20steps%20to%20reproduce%20the%20problem.%20Console%20copy-pastes%20and%20any%20background%20on%20the%20environment%20would%20also%20be%20helpful.%0A%0AThanks!). -* **Pick up a UX project** - * We have some UI/UX design projects available. If you are interested in contributing to the UI/UX design team, please fill out our [volunteer form](https://forms.gle/SFGVfU5fViiZjiiH7). However, please note that the team is generally oversubscribed, and might have limited capacity to accept new volunteers. diff --git a/Contributor-dashboard.md b/Contributor-dashboard.md deleted file mode 100644 index a000b6c2..00000000 --- a/Contributor-dashboard.md +++ /dev/null @@ -1,133 +0,0 @@ -## Overview -The [contributor dashboard page](https://www.oppia.org/contributor-dashboard) on the Oppia site allows users to submit content suggestions (currently translations and practice questions) directly to lessons. These suggestions are then reviewed, and either accepted, or sent back for revision. See the [user docs](https://oppia-user-guide.readthedocs.io/en/latest/contributor/contribute.html) for step-by-step instructions how to contribute content suggestions. - -## How items for contribution are populated - -### Translate text tab -Every list item on the "Translate Text" tab corresponds to a particular lesson and all of its pieces of text content. In the codebase, we refer to each of these list items as _opportunities_. The contributor dashboard automatically shows a translation opportunity for a lesson when the following are true: - -1. The lesson (also called an _exploration_) corresponds to a _chapter_ in a _story_ of a published classroom subject, e.g. "Decimals" (also called a _topic_). See the [user docs](https://oppia-user-guide.readthedocs.io/en/latest/keyconcepts.html) for an overview of these terms. -1. There is at least one piece of text content in the lesson that does not yet have an accepted translation. - -### Submit question tab -Every list item on the "Submit Question" tab corresponds to a particular _skill_, e.g. "Adding Decimals", in a topic. The contributor dashboard automatically shows a question opportunity for a skill when the following are true: - -1. The skill is part of a published classroom topic. -1. The skill does not yet have 10 accepted practice questions. - -## Additional feature behavior -- Unlike with submitting translation suggestions, users need to be allowlisted by an admin before being able to submit question suggestions. Until then, the user will not see the "Submit Question" tab on the contributor dashboard page: - - ![Question tab](https://oppia-user-guide.readthedocs.io/en/latest/_images/submit_question.png "a title") - -- Users cannot review their own suggestions. -- Users must be allowlisted by an admin to be able to review translation suggestions in a particular language or to review question suggestions. -- Reviewers can edit a suggestion and accept the edited version. -- Only subjects/topics associated with a classroom, e.g. Math, are surfaced in the topic/subject selector of the "Translate Text" tab. - -## Admin page -There exists a separate admin page for the contributor dashboard at /contributor-admin-dashboard. There, an admin user can: - -1. allowlist a user to submit question suggestions -1. allowlist a user to review translation suggestions in a particular language -1. allowlist a user to review question suggestions -1. remove rights from a user for any of the above - -See [this](https://docs.google.com/document/d/1VqNiJttq85YyR6cQkd8M9lGGkOP8OlUlkI37Xw6SovM/edit) doc for step-by-step admin instructions. This may be useful for developing locally as a coder as well. - -## Local development -Some setup is usually required when developing locally for the contributor dashboard since before a user can submit a content suggestion to a lesson, a lesson needs to exist. Additionally, the requirements outlined in [How items for contribution are populated](#how-items-for-contribution-are-populated) must be satisfied. - -To populate contributor dashboard data, first start your local server using: - -``` -python -m scripts.start -``` - -And then navigate to Admin page and go to the "Roles" tab. Assign yourself the "Curriculum Admin" role. - -Now go back to the "Activities" tab on the Admin page and click on "Load Data" as shown in the below screenshot. - -![Screenshot of Admin page](images/CdOnboardingGuide/Admin-Page.png) - -This will generate three translatable opportunities, for which suggestions can be made through the "Translate Text" tab of Contributor Dashboard page (http://localhost:8181/contributor-dashboard). - -If you need to generate more sample data, follow the step-by-step instructions on how to generate sample data manually. See -[this doc](https://docs.google.com/document/d/1JYX4nvTcblaVVYAlTi7rApE0lWSBx0v_ZCCr_8WW4Wc/edit#). - -### Accepted translation suggestions -Upon accepting a translation suggestion, the translation becomes a part of the target exploration. To view these translations as the exploration creator, turn on the feature flag named "exploration_editor_can_modify_translations". See [this page](https://github.com/oppia/oppia/wiki/Launching-new-features#changing-value-of-feature-flags) for instructions to turn on feature flags. - -Then, go to the exploration editor page corresponding to the target exploration ID of the accepted suggestion. Go to the translations tab. Switch to translate mode and select the target language of the suggestion. On the graph, click on the content card corresponding to the suggestion. The translation should appear in the edit translation text area. - -## Code pointers - -See the [Oppia codebase overview](https://github.com/oppia/oppia/wiki/Overview-of-the-Oppia-codebase) for a general overview of Oppia's code structure. - -### Frontend -- [core/templates/pages/contributor-dashboard-page/](https://github.com/oppia/oppia/tree/develop/core/templates/pages/contributor-dashboard-page): Main directory of Angular components, frontend services, HTML, CSS. -- core/templates/domain/opportunity/: Frontend opportunity models. -- core/templates/domain/suggestion/: Frontend suggestion models. - -Highlights: -- core/templates/pages/contributions-and-review/: Component for the "My Contributions" tab. Handles viewing and reviewing suggestions. -- core/templates/pages/modal-templates/: Templates for pop-up modals, e.g. for submitting/reviewing a question/translation suggestion. -- core/templates/pages/question-opportunities/: Component for showing question opportunity list items on the "Submit Question" tab. -- core/templates/pages/translation-opportunities/: Component for showing translation opportunity list items on the "Translate Text" tab. - -### Backend - -#### Controllers -- [core/controllers/contributor_dashboard.py](https://github.com/oppia/oppia/blob/develop/core/controllers/contributor_dashboard.py): Handles fetching opportunities and contributor dashboard metadata such as eligible translatable text content. -- [core/controllers/suggestion.py](https://github.com/oppia/oppia/blob/develop/core/controllers/suggestion.py): Handles everything suggestion related, e.g. submitting and reviewing suggestions. -- [core/controllers/contributor_dashboard_admin.py](https://github.com/oppia/oppia/blob/develop/core/controllers/contributor_dashboard_admin.py): Handles admin actions. - -#### Domain services -- [core/domain/opportunity_services.py](https://github.com/oppia/oppia/blob/develop/core/domain/opportunity_services.py): Backend services for operating over opportunities. -- [core/domain/suggestion_services.py](https://github.com/oppia/oppia/blob/develop/core/domain/suggestion_services.py): Backend services for operating over suggestions. -- [core/domain/email_manager.py](https://github.com/oppia/oppia/blob/develop/core/domain/email_manager.py): Contains services for sending contributor dashboard related emails, e.g. for notifying users when they have been added as a reviewer. - -#### Domain models -- [core/domain/opportunity_domain.py](https://github.com/oppia/oppia/blob/develop/core/domain/opportunity_domain.py): Domain models for opportunities. -- [core/domain/suggestion_registry.py](https://github.com/oppia/oppia/blob/develop/core/domain/suggestion_registry.py): Domain models for suggestions. - -#### Storage -- [core/storage/opportunity/gae_models.py](https://github.com/oppia/oppia/blob/develop/core/storage/opportunity/gae_models.py): Storage models for opportunities. -- [core/storage/suggestion/gae_models.py](https://github.com/oppia/oppia/blob/develop/core/storage/suggestion/gae_models.py): Storage models for suggestions. - -### E2E Tests -- [core/tests/webdriverio_desktop/contributorDashboard.js](https://github.com/oppia/oppia/blob/develop/core/tests/webdriverio_desktop/contributorDashboard.js): E2E tests for contributor dashboard CUJs. -- [core/tests/webdriverio_utils/ContributorDashboardAdminPage.js](https://github.com/oppia/oppia/blob/develop/core/tests/webdriverio_utils/ContributorDashboardAdminPage.js): Utilities for navigating/asserting on the admin page. -- [core/tests/webdriverio_utils/ContributorDashboardPage.js](https://github.com/oppia/oppia/blob/develop/core/tests/webdriverio_utils/ContributorDashboardPage.js): Utilities for navigating/asserting on the contributor dashboard page. -- [core/tests/webdriverio_utils/ContributorDashboardTranslateTextTab.js](https://github.com/oppia/oppia/blob/develop/core/tests/webdriverio_utils/ContributorDashboardTranslateTextTab.js): Utilities for navigating/asserting on the "Translate Text" tab. - -## Sample coding exercise -To familiarize ourselves with the codebase, let's go through an exercise to show custom description text for the translation opportunity subheading: - -![Custom description](images/contributorDashboardCustomDescription.png) - -This text will be populated in the backend and propagated to the frontend. - -1. First, start a local server and follow the steps outlined in the doc linked in [Local development](#local-development) to populate translation opportunities. Then, navigate to /contributor-dashboard and click on the "Translate Text" tab. You should see something like the following: - -![Translate text tab](images/contributorDashboardTranslateTextTab.png) - -Notice the subheadings are formatted [TOPIC NAME - CHAPTER TITLE]. Now let's make our code changes. - -2. Add a new field `description` of type `str` to the `PartialExplorationOpportunitySummaryDict` backend model in core/domain/opportunity_domain.py. This will allow us to pass a description from the backend to the frontend. - -3. Populate the backend `description` field with some custom text in the returned translation opportunities in core/controllers/contributor_dashboard.py like so: - -![Description backend field](images/contributorDashboardDescriptionBackendField.png) - -4. Add a `description` field to the `ExplorationOpportunitySummaryBackendDict` and `ExplorationOpportunitySummary` classes in core/templates/domain/opportunity/exploration-opportunity-summary.model.ts. Make sure to update the constructor definitions as well. - -5. Populate the `description` field in the `_getExplorationOpportunityFromDict()` method of the frontend API service: core/templates/pages/contributor-dashboard-page/services/contribution-opportunities-backend-api.service.ts. This step adds the description field from the backend dict to the frontend model. - -6. Finally, go back to core/templates/domain/opportunity/exploration-opportunity-summary.model.ts and modify `getOpportunitySubheading()` to return the description instead of the topic and chapter title. Make sure all your changes are saved, refresh the page, and you should see your custom description in all the opportunity subheadings! - -> [!NOTE] -> For this example, our description field was not fetched from persisted storage and was instead manually set in the backend controller. - -## Appendix -1. [Contributor dashboard overview](https://docs.google.com/document/d/1wM9cQzq1-3nbEhZliRlpnGDXbM_HspNkY16CYnA6lWg/edit#): More in-depth developer focused overview of the system design of the contributor dashboard. diff --git a/Create a Good PR.md b/Create a Good PR.md deleted file mode 100644 index 12caddb3..00000000 --- a/Create a Good PR.md +++ /dev/null @@ -1,114 +0,0 @@ -## Table of contents - -- [Table of contents](#table-of-contents) -- [Why It Matters](#why-it-matters) -- [Before You Open Your PR](#before-you-open-your-pr) -- [What Makes a PR “Great”](#what-makes-a-pr-great) - - [1. Clear Purpose](#1-clear-purpose) - - [2. Focused Scope](#2-focused-scope) - - [3. Helpful Description](#3-helpful-description) - - [4. Show, Don’t Tell](#4-show-dont-tell) - - [5. Tested \& Verified](#5-tested--verified) -- [Common Mistakes to Avoid](#common-mistakes-to-avoid) -- [Final Checklist Before You Request Review](#final-checklist-before-you-request-review) -- [Reviewers Appreciate...](#reviewers-appreciate) -- [Need Help?](#need-help) - -This page is an introduction to creating a great Pull Request (PR). A great Pull Request (PR) is more than just code—it’s clean, clear, and easy for reviewers to understand. This guide shares best practices for writing high-quality PRs that are easy to review and quick to merge. - -## Why It Matters - -A good pull request isn’t just about working code—it’s about making collaboration easier. Clear, well-structured PRs save reviewers time, reduce confusion, and help maintain high code quality. In open source, thoughtful PRs build trust and keep the project maintainable for everyone. - -For the essential checklist and best practices, be sure to follow [Rules for making PRs](https://github.com/oppia/oppia/wiki/Rules-for-making-PRs)—every good PR starts there. - -## Before You Open Your PR - -Make sure you’ve: - -- Created a branch off the latest `develop` -- Pulled the latest code: `git pull upstream develop` -- Tested your changes locally -- Passed all relevant tests -- Cleaned up your code (no stray debug logs or commented code) -- Written a fix plan (if required) and got it approved - -## What Makes a PR “Great” - -Here’s what makes a PR stand out: - -### 1. Clear Purpose - -Use a **descriptive PR title** and mention the issue number. - -**Good:** -`Fix #4567: Align navbar correctly on small screens` - -**Bad:** -`Update UI stuff` - -### 2. Focused Scope - -- Stick to **one topic per PR** -- Don’t sneak in unrelated changes -- Keep your PRs small when possible—**smaller PRs are faster to review** - -### 3. Helpful Description - -Fill out the PR template completely: - -- What did you change? -- Why was it needed? -- Any tricky parts? -- Screenshots for UI changes -- Questions you want feedback on - -### 4. Show, Don’t Tell - -If your PR changes the UI, **include before-and-after screenshots**. This helps reviewers instantly understand the visual impact. - -### 5. Tested & Verified - -Before requesting review: - -- All tests pass (unit, integration, lint) -- You've tested manually (especially for UI or logic changes) -- Edge cases are handled - -## Common Mistakes to Avoid - -| ❌ Don’t Do This | ✅ Do This Instead | -|------------------|--------------------| -| "Fix some stuff" in title | "Fix #1234: Add padding to dashboard cards" | -| Combine unrelated fixes | Separate into focused PRs | -| Skip writing a description | Clearly explain what and why | -| Add unfinished code | Only push code that’s ready | -| Forget screenshots | Add visual proof for UI changes | - -## Final Checklist Before You Request Review - -- [ ] My PR addresses **one issue only** -- [ ] I followed **Oppia’s code style** -- [ ] All **tests pass** -- [ ] I filled out the **PR template** -- [ ] I included **screenshots** (if needed) -- [ ] My commit message is clear and formatted -- [ ] I pushed the **latest code** -- [ ] I’m ready to **respond to reviews** promptly - -## Reviewers Appreciate... - -- Clean, readable code -- Clear commit history -- Context provided in comments if something’s non-obvious -- Respect for their time - -## Need Help? - -Don’t be afraid to ask! You can: - -- Tag reviewers in comments -- Use [GitHub Discussions](https://github.com/oppia/oppia/discussions) - -Thanks for taking the time to make your PR shine. -High-quality PRs make Oppia better for everyone! diff --git a/Creating-Dependencies.md b/Creating-Dependencies.md deleted file mode 100644 index c610c556..00000000 --- a/Creating-Dependencies.md +++ /dev/null @@ -1,3 +0,0 @@ -A dependency allows you to specify JavaScript and CSS code that you want to be available to your interaction at runtime. - -To add a dependency called my\_dependency, create the file extensions/dependencies/my\_dependency.html. Inside this file you can write lines of the form `` specifying CSS or JS code. Alternatively you can use `