Aplus orientation

Equipment

• For interns: you need at least a laptop, and, depending on your needs, also a screen, mouse, keyboard, maybe something else
  
  • Contact CS IT at guru@cs.aalto.fi to tell them what you would need and for agreeing on when to pick them up
  • Pick up the equipment from CS IT in CS building room A243
• what your laptop should include:
  • Ubuntu Linux (20.04 LTS for new computers)
  • You should apply for the workstation administrator (sudo) privileges so that you can install software and tools yourself, though you can ask CS IT to install software too. Ask CS IT about the admin privileges.
  • CS IT should install Docker (package docker-ce) and add the user to the docker group. (https://docs.docker.com/engine/install/ubuntu/ and https://docs.docker.com/engine/install/linux-postinstall/)
  • CS IT should also install Docker Compose (package docker-compose, https://packages.ubuntu.com/focal/docker-compose). (It is also possible to install docker-compose by simply downloading one binary file from Github: https://docs.docker.com/compose/install/ and https://github.com/docker/compose/releases) (If you need to install Docker Compose yourself, read the "installing Docker Compose" section below on this page.)
  • The preinstalled editors are Eclipse and Vim. You can can install other editors with instructions behind the next link to scicomp (section "Ubuntu packages").
  • We recommend the code editor VS Code: https://code.visualstudio.com/download. They have their own .deb package. We have requested that CS IT would install VS code too to the computers of A+ developers.
  • More instructions for using Aalto Linux and how to install software: https://scicomp.aalto.fi/aalto/linux/
  • https://scicomp.aalto.fi/training/linux-shell-tutorial/
  • Aalto VPN (always included in Aalto/CS Linux)
  • Zoom and Teams clients for online meetings
• also macOS is an easy alternative, if CS IT has some "on the shelf". With Windows, a Linux virtual machine is the best choice.

Technical guides

Best practices

• Coding style guide: https://apluslms.github.io/contribute/styleguides/
• Contribution guide: https://apluslms.github.io/contribute/
• Git and pull request guides are on this wiki page
• Definition of Done

Editors

• Visual Studio Code (vscode)
  • Download .deb package: https://go.microsoft.com/fwlink/?LinkID=760868

  • Then run sudo apt install ./<file>.deb in the Downloads folder (or where the file is)

  • We also have an extension for VS Code: Aplus Tools Official
  • The VS Code extension is helpful for writing A+ course materials in RST. It does not affect Python programming.
  • https://github.com/apluslms/vscode-aplus-tools
  • https://marketplace.visualstudio.com/items?itemName=apluslms.aplus-tools-official
• Eclipse (available by default)
• Atom
• others? Inform preferably CS IT in advance

Links to git repositories

Quick start guide explains how to start A+ course development with the Aplus-manual course. You can run the course in your computer with the A+ Docker containers.

• https://apluslms.github.io/guides/quick/
• https://plus.cs.aalto.fi/aplus-manual/master/
• https://github.com/apluslms/aplus-manual

The Aplus-manual can be used for testing A+ features. There is also a separate test-course that focuses on using all A+ features with many different settings so that it is easier to manually start testing them. The test-course can of course be expanded with new A+ features (and many features are still missing there). You can can create pull requests for the test-course too!

• https://github.com/apluslms/test-course

The A+ architecture is briefly explained in these pages:

• https://apluslms.github.io/architecture/
• https://apluslms.github.io/components/

Our work is mostly located under the apluslms organisation in github.com. The Acos server has its own organisation. Course (content) repositories are private and located in the Aalto GitLab server https://version.aalto.fi. Many course repos are under the course group https://version.aalto.fi/gitlab/course.

You don't need to clone or fork all of them. Do that when you need to. Many repositories are listed here, so you can find them when needed.

• https://github.com/apluslms/a-plus
• https://github.com/apluslms/mooc-grader
• https://github.com/apluslms/a-plus-rst-tools
• https://github.com/apluslms/aplus-manual/
• https://github.com/apluslms/mooc-jutut
• https://github.com/apluslms/radar
• https://github.com/apluslms/rubyric/
• https://github.com/apluslms/moodle-mod_astra/
• https://github.com/acos-server/acos-server
  

Installing Docker Compose

Docker Compose ("docker-compose") is another tool besides Docker ("docker"). CS IT should have installed Docker into Aalto work laptops for you (i.e. there should be a package "docker-ce" and a command "docker"). Unfortunately, Ubuntu repositories have outdated versions of docker-compose, so it should be installed manually. It can be installed into the user's home directory without administration privileges. Docker-compose is just a single binary executable file.

NOTE: this is not needed on macOS as docker-compose is included in the Docker for Mac application.

Installation can be completed by copy-pasting and executing following in a terminal window. If the last lines are ok, then do logout from the desktop and log back in to make the command work without full path.

set -k

## Prepare:
# next will create location for binaries in your home
mkdir -p "$HOME/.local/bin"
# then we make executables in that folder visible as commands by adding the above location to the PATH variable
# we do this by changing PATH variable in .profile, which executed by login terminals (e.g. when you login via GUI)
echo 'export PATH="$HOME/.local/bin:$PATH"' >> "$HOME/.profile"
# zsh (shell you are using) doesn't read .profile, so will make it do that (optional step).
echo "[ -e \"\$HOME/.profile\" ] && emulate sh -c '. \"\$HOME/.profile\"'" >> "$HOME/.zprofile"

## Download docker-compose
# find out the newest version
compose_version=$(curl -LSs https://api.github.com/repos/docker/compose/releases/latest | grep -F '"tag_name":' | cut -d'"' -f4)
# download the docker-compose
curl -LSs -o "$HOME/.local/bin/docker-compose" "https://github.com/docker/compose/releases/download/$compose_version/docker-compose-$(uname -s)-$(uname -m)"
# make the binary executable
chmod +x "$HOME/.local/bin/docker-compose"

## Verify
~/.local/bin/docker-compose -v
md5sum ~/.local/bin/docker-compose
# should print:
#   docker-compose version 1.25.5, build 8a1c60f6
#   3485ce0470f084d338732c541873339a  /u/XX/<user>/unix/.local/bin/docker-compose
# or:
#   docker-compose version 1.26.2, build eefe0d31
#   218a4d71308268cd4b9c31e208c9bf4c  /u/XX/<user>/unix/.local/bin/docker-compose

Following example shows what it should look like. Note that the script is pasted one block at a time and only last part provides any feedback. Furthermore, if the version number is the same, then the hash on the last line should be the same for you too. However, if you get a newer version, then it will be different.

Everything should work now, but if you are unsure, you can compare your local files to these examples:

# this is ~/.profile

# add ~/.local/bin to PATH
export PATH="$HOME/.local/bin:$PATH"

# set your terminal editors to nano (default would be vim)
export EDITOR=nano
export VISUAL=nano

# this is ~/.zprofile

# zsh doesn't load .profile by default, so do that
[ -e "$HOME/.profile" ] && emulate sh -c '. "$HOME/.profile"'

References:

• The official Docker Compose installation guide: https://docs.docker.com/compose/install/#install-compose.
  
• About .zprofile and .profile with zsh (the default shell in Aalto linuxes): https://superuser.com/q/187639

Show exit codes in the terminal (OPTIONAL)

This configuration is not required for your environment to work for A+ development, but it might still be useful.

Sometimes crashed and failed commands go unnoticed. To help with that, you can include program exit codes in the prompt. If you drop this ..zshrc into your home folder, then your shell will start to look like the following image.

After downloading the that file, move it to your home folder, and then rename it from home.zshrc to .zshrc (yes, it starts with a dot).

NOTE: this only works for zsh, thus bash users require something different (e.g. macOS users).

Use nano (i.e. not vim) for git commits with nice colors (OPTIONAL)

In this part, we configure our terminal text editor to nano. which might be easier to use than the typical default vim. In addition, we add custom syntax coloring to git operations.

First, let's set our default editor

echo 'export EDITOR=nano' >> "$HOME/.profile"
echo 'export VISUAL=nano' >> "$HOME/.profile"

Now, your ~/.profile should look something like the example file in the seciton Installing Docker Compose. You need to log out and in for above changes to take effect.

Second, let's configure nano to show some colors and to react to mouse clicks. To do that, create following files:

# See /etc/nanorc for more default settings, which can be copied here.

## Remember the used search/replace strings for the next session.
set historylog

## Display line numbers to the left of the text.
#set linenumbers
# instead use: nano -l file.txt
# or press: alt+shift+3 (alt+#)

## Enable mouse support, if available for your system.  When enabled,
## mouse clicks can be used to place the cursor, set the mark (with a
## double click), and execute shortcuts.  The mouse will work in the X
## Window System, and on the console when gpm is running.
set mouse

# system styles
include "/usr/share/nano/*.nanorc"
# my styles
include "~/.nano/git.nanorc"

If you need to copy text from open nano session, then you need to press SHIFT while selecting text with the mouse. You could also disable mouse by commenting out the corresponding line. Or you can add line numbers. Both parameters have comments in the file above.

For the second file, you might need to create the folder ~/.nano.

# This code is free software; you can redistribute it and/or modify it under
# the terms of the new BSD License.
#
# Copyright (c) 2010, Sebastian Staud
# Copyright (c) 2020, Jaakko Kantojärvi

# A nano configuration file to enable syntax highlighting of some Git specific
# files with the GNU nano text editor (http://www.nano-editor.org)


## This syntax format is used for git config files
syntax "git-config" "git(config|modules)$|\.git/config$"

color brightcyan "\<(true|false)\>"
color cyan "^[[:space:]]*[^=]*="
color brightmagenta "^[[:space:]]*\[.*\]$"
color yellow ""(\\.|[^"])*"|'(\\.|[^'])*'"
color brightblack "(^|[[:space:]]+)#([^{].*)?$"
color ,green "[[:space:]]+$"


## This syntax format is used for commit (and tag) messages
syntax "git-commit" "COMMIT_EDITMSG|TAG_EDITMSG"

# Commit message
#color yellow ".*"
# Warn when a commit message exceeds 50 and 72 chars per line
#color brightyellow "^(.{1,50})$"
color brightwhite "^(.{1,50})$"
color yellow "^(.{51}).*$"
color red "^(.{73}).*$"

# Headers in the end of the message:
color white         "^[A-Za-z_-]+: .*$"
color brightblue    "^[A-Za-z_-]+: "
color brightgreen   "^([Cc]lose[sd]?|[Ff]ix(e[sd])?|[Rr]esolve[sd]?) ((#|GH-)[0-9]+|https?://[^ ]*)$"
color green         "^([Cc]lose[sd]?|[Ff]ix(e[sd])?|[Rr]esolve[sd]?)"

# Comments
color brightblack "^#.*"

# Branch status
color brightmagenta "^# [A-Za-z]+[^:]*:$"
color brightred   "^#[[:space:]]Your branch and '[^']+"
color brightblack "^#[[:space:]]Your branch and '"
color brightwhite "^#[[:space:]]On branch [^ ]+"
color brightblack "^#[[:space:]]On branch "
color brightwhite "^#[[:space:]]Your branch is up to date with '[^']+"
color brightblack "^#[[:space:]]Your branch is up to date with '"

# Files changes
color white       "#[[:space:]](deleted|modified|new file|renamed):[[:space:]].*"
color red         "#[[:space:]]deleted:"
color green       "#[[:space:]]modified:"
color brightgreen "#[[:space:]]new file:"
color brightblue  "#[[:space:]]renamed:"

# Untracked filenames
color white "^#	[^?*:;{}\\]+(/|\.?[^/?*:;{}\\]+)$"

# Rebase actions:
color brightyellow  "^# interactive rebase in progress.*"
color yellow        "^#[[:space:]]+(pick|reword|edit|squash|fixup|exec|break|drop|label|reset|merge) .*"
color brightwhite   "^#[[:space:]]+(pick|reword|edit|squash|fixup|exec|break|drop|label|reset|merge)"

# Commit IDs
color brightblue "[0-9a-f]{7,40}"

# Recolor hash symbols
color brightblack "^#"

# Trailing spaces (+LINT is not ok, git uses tabs)
color ,red "[[:space:]]+$"


## This syntax format is used for interactive rebasing
syntax "git-rebase-todo" "git-rebase-todo"

# Default
color yellow ".*"

# Comments
color brightblack "^#.*"

# Rebase commands
color green         "^(e|edit) [0-9a-f]{7,40}"
color green         "^# e, edit"
color brightgreen   "^(f|fixup) [0-9a-f]{7,40}"
color brightgreen   "^# f, fixup"
color brightwhite   "^(p|pick) [0-9a-f]{7,40}"
color brightwhite   "^# p, pick"
color blue          "^(r|reword) [0-9a-f]{7,40}"
color blue          "^# r, reword"
color brightred     "^(s|squash) [0-9a-f]{7,40}"
color brightred     "^# s, squash"
color brightblue    "^(m|merge) (-[Cc] [0-9a-f]{7,40}|[^ ]+)"
color brightred     "^(m|merge) "
color brightred     "^# m, merge"
color red           "^(d|drop) [0-9a-f]{7,40}"
color red           "^# d, drop"
color white         "^(x|exec)[[:space:]]+[^ ]+.*$"
color brightyellow  "^(x|exec) "
color brightyellow  "^# x, exec"
color white         "^(b|break)"
color white         "^# b, break"
color brightblue    "^(l|r|label|reset) [^ ]+"
color white         "^(l|r|label|reset) "
color white         "^# l, label"
color white         "^# t, reset"

# Commit IDs
color brightblue "[0-9a-f]{7,40}"

# Recolor hash symbols
color brightblack "^#"

Now, when you issue a command, which uses editor (e.g. git commit), you should see nano with some nice colors.

Creating Python virtual environments

Python virtual environments allow you to install Python packages (libraries and tools) without interfering with the system-wide Python installation. Each virtual environment is isolated from each other, so you can, for example, install different versions of the same package in different environments. You typically create a new venv for each app that requires a lot of dependencies, for example, a venv for a-plus and a venv for the mooc-grader, assuming you develop both systems. We recommend that the venv directory is created outside the application directory, e.g., outside the a-plus source code directory.

Since we can run A+ and the MOOC-Grader in Docker containers, we typically only need virtual environments for running certain Django (manage.py) commands like "test", "makemigrations", "makemessages", and "compilemessages".

The example below creates a new venv in the directory venv_aplus under the current working directory.

aptdcon --install python3-venv
python3 -m venv venv_aplus
source venv_aplus/bin/activate
pip install --upgrade pip setuptools
pip install wheel
# You have created the virtual environment now, but usually you also want to install dependencies from some app to the venv.
# For example for A+, change directory to the a-plus (source code) directory that you cloned from git. It contains the required pip packages in the file requirements.txt.
pip install -r requirements.txt

The source command is used to activate the venv again at another time (in a new terminal window or after rebooting the machine): source venv_aplus/bin/activate

A+ local development setup

Course developers usually do not modify A+ source code and they do not need to clone the a-plus git repo. They can run the latest released versions of the A+ Docker containers in order to test their courses. The container version is set in docker-compose.yml.

https://github.com/apluslms/develop-aplus is a utility repo for debugging A+ and mooc-grader using VSCode in a docker container. The src/srv part below applies to it too, it is set to /srv/ in the repo.

A+ and MOOC-grader developers need to mount the source code to the Docker containers so that they can run their own version of the source code in the container. Example paths for the volumes field in docker-compose.yml (the full example docker-compose.yml file is also below):

• /home/user/a-plus/:/src/aplus/:ro
• /home/user/mooc-grader/:/src/grader/:ro
• The first path before the colon is the local path that you need to fix for your own computer.
• A-plus is installed in /srv/aplus in the run-aplus-front container. You can mount development version of the source code to /src/aplus. The container will then copy it to /srv/aplus and compile the translation file (django.mo). If you mount directly to /srv/aplus, you need to manually compile the translation file beforehand, but on the other hand, Django can reload the code and restart the server without restarting the whole container when you edit the source code files.
• The same applies to the run-mooc-grader container, but the paths inside the container are /src/grader and /srv/grader.
• You should add the following symbolic link to the root of the mooc-grader directory in your computer: ln -s /srv/courses/ exercises
  • With the symbolic link, the run-mooc-grader container finds the course directory even when you mount your own mooc-grader source code into the container.
• The run-mooc-grader container normally mounts the course directory /srv/courses/default
• In the run-mooc-grader container, if you do not use the symbolic link "exercises → /srv/courses" and if you mount the mooc-grader code to /srv/grader, then you need to change the course directory mount to /srv/grader/courses/default in docker-compose.yml.

version: '3'

volumes:
  data:
services:
  grader:
    image: apluslms/run-mooc-grader:1.8
    volumes:
      - data:/data
      - /var/run/docker.sock:/var/run/docker.sock
      - /tmp/aplus:/tmp/aplus
      # This file should be in a course directory and
      # the course directory is mounted to the MOOC-Grader container.
      - .:/srv/courses/default:ro
      # mount the mooc-grader source code to one of the two options:
      - /home/user/mooc-grader:/src/grader:ro
      #- /home/user/mooc-grader:/srv/grader
      #- .:/srv/grader/courses/default:ro
    ports:
      - "8080:8080"
  plus:
    image: apluslms/run-aplus-front:1.8
    volumes:
      - data:/data
      # mount the A+ source code to one of these:
      - /home/user/a-plus:/src/aplus:ro
      #- /home/user/a-plus:/srv/aplus:ro
    ports:
      - "8000:8000"
    depends_on:
      - grader

aplus/local_settings.py is needed when you run Django manage.py commands (such as makemessages, compilemessages, makemigrations). It should contain the settings below. Note that when you run the run-aplus-front container, it does not use the local_settings.py file from the root of the a-plus source code.

DEBUG = True
BASE_URL = 'http://localhost:8000/'

Git tutorials and guides

• Linux Shell tutorial from the CS IT: https://scicomp.aalto.fi/training/linux-shell-tutorial/
• We recommend that everyone knows how to use git in the command-line. GUI's support most basic features, but some more complex operations are typically missing.
• Basics in the first two chapters of the Pro Git book: https://git-scm.com/book/en/v2
• Set your name and email to git: https://git-scm.com/book/en/v2/Customizing-Git-Git-Configuration
• You might want to change the default text editor for writing commit messages. Nano is a simple editor if you are not very familiar with the command line: git config --global core.editor "nano" (or see Use nano for git commits above).
• Reminder of git commands (and some workflows to manage changes):
  
• You must understand the following commands before you can do anything: git status, git log, git add, git commit, git pull, git push, git diff (--cached)
• Branches and merging (entire chapter 3): https://git-scm.com/book/en/v2/Git-Branching-Branches-in-a-Nutshell
• Branches are crucially important for separating your work from the upstream master version while you are working on your feature in a topic branch. You must understand the commands: git fetch, git checkout, git branch, git merge

• Alias git lol for visualizing the branches: git config --global --add alias.lol "log --graph --decorate --pretty=oneline --abbrev-commit --all" (source)

• Rebasing: https://git-scm.com/book/en/v2/Git-Branching-Rebasing
• Rewriting history (squash, change commit messages, reorder commits, interactive rebase): https://git-scm.com/book/en/v2/Git-Tools-Rewriting-History
• git reset can be used to:
  • remove staged changes (reverse of git add)
  • remove changes in working tree (git reset --hard), though use git checkout filename, so you don't make mistakes
  • "delete" your local commits from a branch when the local branch has a different history than the remote and you want to use the remote version (i.e. change where the branch points to)
• Git submodules (course repositories add the a-plus-rst-tools repo as a submodule): https://git-scm.com/book/en/v2/Git-Tools-Submodules
• Stashing, i.e., saving work and removing it from the work directory without committing: https://git-scm.com/book/en/v2/Git-Tools-Stashing-and-Cleaning
• GitHub.com pull requests: https://help.github.com/en/github/collaborating-with-issues-and-pull-requests/about-pull-requests
• Git tutorial from the DigitalOcean community: https://www.digitalocean.com/community/tutorials/how-to-contribute-to-open-source-getting-started-with-git
• Git does not remove commits, so if you lose changes, but did commit them at some point, you have ~30 days to get them back. Contact a git guru!
• Finally, check pull-request guides below.

VSCode partial staging

VSCode allows partial staging. That is, it allows you to stage (add to the next commit) specific lines:

1.  In the Source Control tab: click on the file you want to stage lines from under "Changes"
   • This should open a diff view with the original file on left and changed on the right
2. Select the lines you want to stage from the changed file
3. Right-click and choose "Stage Selected Ranges"
   • Whole lines are staged at once, you cannot only stage half of a line
   • You don't need to select the whole line to stage it, the whole line is staged as long as part of it selected
   • Single lines can be staged by just right clicking on them without needing to select.

You can also use this to revert a change by choosing "Revert Selected Ranges".

Similar process can be used to unstage changes from the files under "Staged Changes" in the Source Control tab.

Django tutorials (Python web framework)

• Django is the Python web framework used in A+ and the MOOC-Grader. A web framework contains interfaces to functionalities that are often needed in the web (backend) development, such as HTTP requests and responses, databases (object-relational mapper) and rendering (HTML) templates.
• Web software development course at Aalto: https://mycourses.aalto.fi/course/view.php?id=24331 and https://plus.cs.aalto.fi/wsd/2019-2020/
• https://docs.djangoproject.com/en/2.2/
• Basic tutorial: https://docs.djangoproject.com/en/2.2/intro/
• Topics that most people should probably understand:
  
  • database, models, making queries
  • HTTP requests, view classes, generic view classes
  • user authentication
  • Forms, form validation
  • (HTML) templates, Django template language
  • Migrations, how to generate migrations after you change the fields in model classes
  • unit testing
  • security (HTML escaping in templates, XSS, CSRF, SQL injection, access control): https://docs.djangoproject.com/en/2.2/topics/security/

Testing

Manual testing

Manual testing is always required as well even if you had a lot of automated tests. Mount your source code into the A+ and MOOC-Grader Docker containers and run them in a test course. Think of what can be tested automatically and what should be tested manually.

Unit testing

• It is easier to run these Django commands in your host machine than in a container. Thus, you need to create a Python virtual environment (instructions are located in this page) and install the requirements (pip packages) from the project (A+ or MOOC-Grader) into the venv.
• Running unit tests in a Django project: python manage.py test
  • To save time, you can run only a certain test suite with parameters: python manage.py test exercise.tests.ExerciseTest.test_base_exercise_absolute_url
• New features should be accompanied with adequate unit tests. Create tests for at least the most important cases and also edge cases.
• Unit tests are made for the backend Django code. We don't have unit tests for frontend JavaScript code (and it is not possible to add any JS unit tests now).
• We don't have enough automatic tests for our old code base, but at least we should test new features better and always include automatic tests.

Selenium tests

Selenium tests run a browser such as Firefox and execute interactions in the web page according to the test code. They test if the page behaves correctly. Selenium tests are a form of functional testing that tests the system from the end users perspective instead of testing internal functions of the system like unit testing does.

• A+ has some old Selenium tests. They cover only a small part of the system.
• Selenium tests are a bit difficult to write and they may easily break when the HTML code of the pages is changed again later.
• Let's use Selenium tests for only the most important parts of the system, especially when bugs are hard to find manually. For example, end-of-course features, results/points page that teachers use for final grading of the course.
• A+ has some documentation about its Selenium tests: https://github.com/apluslms/a-plus/tree/master/selenium_test

Git and pull request guidelines

Read these:

• git commit messages are important and they should explain the reason and background for the changes: https://mislav.net/2014/02/hidden-documentation/
• Pull Request Etiquette: https://gist.github.com/mikepea/863f63d6e37281e329f8
  • Responsibilities of author (person who creates the pull request) and reviewer (who accepts it)
  • Includes good reasoning why good commit messages are important (in addition to the "hidden documentation" above)
  • Though, doesn't include how to give positive comments
• About the style of comments and behaviour on pull request https://medium.com/better-programming/pull-request-etiquettes-for-reviewer-and-author-f4e80360f92c
• There is a pull request template in the A+ repositories. It is used in the pull request description when you make a new pull request. The template reminds you to write clear a description and to discuss how the code has been and should be tested.
• The pull request title should be written in the imperative form: "Add this feature", NOT "Adds some feature" or "This PR adds a feature".
• The pull request description is written like normal text. Be clear and concise, but include all relevant information so that readers understand the topic and the need for the changes. Compare writing the description to writing email messages.
• Naming the git branch for the pull request: the name does not technically affect anything, but it is best to use very short and clear names that describe the topic of the pull request. You will more easily recognize your own git branches later when they have descriptive names. You may include the issue number in the branch name if you like that.

Markku's comments:

• You create new PRs for new features and fixes that logically belong together. So, one PR for one feature. The contents of one PR should constitute a logical set of changes for the whole feature. A pull request does not always add a new feature, since there are also bug fixes and other necessary changes without adding new features.
• You modify the PR by pushing new commits when you fix bugs and other issues that have been pointed out in the code review and testing.
• You should clean up the git history before creating the PR. While the PR is being reviewed, you make changes in new commits and do not squash so that reviewers can easily follow the latest changes (the newest commits). At the end when everything has been polished, you clean up the git history again (git rebase, squash etc.) and force-push.
• There can be more than one commit. Each commit should constitute a sensible set of changes that somewhat belong together. The code should usually always "work" after each commit, so don't leave the code in a completely broken state after a commit (syntax errors, immediate crashes when executing the code). Of course, not every detail can work after a single commit when large features are split into many commits, but there are usually logical subcomponents or such.
• Massive commits are hard to read when developers track changes afterwards (for example, while bug hunting). A large number of very small commits is also hard to digest.

Tuomas's comments on new features that require refactoring:

• Split the commits in two steps: first make the necessary changes and then implement the new feature
  
  • This is not an iron-clad rule, sometimes it is difficult to split the changes like this 
  • That is, a single commit should only contain refactoring or new feature code, not both.
  • E.g. you could have first 3 commits that refactor the old code and then 2 commits that implement the new feature
  • This documents the code well and makes reviewing the PR much easier
  • The VSCode partial staging would probably help in this
• The product breaking due to a commit does not make the commit automatically bad
  • The purpose of a commit is to document the code and changes
  • Whatever breaks, must be fixed later in the same PR
  • If a PR contains commits that break the product, it must be merged using a merge commit to mark the commits in the PR as belonging to a bigger whole. 

Reviewing someone else's pull request:

• Start a review in the pull request (Github has a button for that) and add comments to the code lines that you want to discuss. You may, for example, suggest changes, ask about unclear things (that should maybe be implemented better or clarified with a comment), and point out possible bugs.
• It is possible to write comments in Github without starting a review, but we prefer to start a review when you are really reviewing the whole pull request. Individual comments could be used if you had to comment on something without reviewing the whole pull request.
• The pull request discussion tab is used for comments that are not tied to any specific line of code.
• https://help.github.com/en/github/collaborating-with-issues-and-pull-requests/about-pull-request-reviews
• When there are conversations on certain code lines, either the developer or the reviewer could resolve the conversation, which will collapse it (it can still be opened later, however). There is no strict rule who should resolve those conversations. If the developer can be certain that he/she has finished the requested changes correctly, then the developer could resolve it after pushing the changes. Otherwise, the reviewer or the manager could resolve the conversation after verifying that the code has been fixed correctly.

UPDATE 26.6.2020 based on discussion (TODO: clean up the pull request and issue guide)

• There should normally be a Github issue about something before you implement the changes and submit a pull request.
• Github issue should describe a problem and usecase (what the user needs or wants and why). The issue should not be a direct statement "do this".
  • We have many old issues that are orders ("do this"), but then it is important to find out the actual problem. That can be discussed in the issue. The original idea ("do this in that way") may actually be bad and the problem should be solved with a different approach.
• In the issue, discuss ideas about how to solve the problem and what kind of implementation would be good. Describe the overall idea of the implementation before you start coding. This way, we can agree on the approach before you write a lot of code and submit a pull request.
• Since we have agreed about the approach for implementation before submitting the PR, we won't argue about the approach in the PR discussion.
• The PR discussion and review can concentrate on smaller issues and details in the code (like coding style, inefficient code, or security problems). The overall architecture should already be good since it has been agreed on in the issue discussion. Remember to link the PR to the issue.
• The PR discussion should stay on topic and discuss the changes of the PR. Discussion about the actual problem/feature is done in the issue, not the PR. Usually, the discussion about the problem should be completed before submitting the PR and there should be no need to debate about the original problem anymore in the PR discussion.
• If new concerns appear during the PR discussion that are not directly related to the original issue, then a new issue can be created from those. Let's keep the PR discussion on topic!
• Draft pull requests can be used for sharing coding ideas at an early stage. For example, if you want to discuss some code design details before finishing the code, then a draft PR is a good way to share the code for others and discuss about it.
• Draft PRs are not ready for normal, full reviews and no such reviews should be done on them.
• If you have a draft PR, you can force-push finished code to the git branch in the end and convert it to a normal pull request.
• Normal pull requests are ready for review. Reviews may suggest changes to the code, but massive changes should not be needed at this stage since the problem has been discussed in the issue beforehand.
• After reviews have been done and the fixed code is finished, the git history shall be cleaned. Small fixes should usually be squashed into the appropriate commits and the code may need to be rebased onto the upstream master.

Jaakko's comment on 26.6.2020:

Goal of the process is to ensure, there wouldn't be huge problems at the PR time, but that still can happen. Sometimes it might happen that we need to move back to the issue, discuss it with the new information in mind, and choose what should be done if anything. With a clear idea about the problem, use case and an implementation idea, that should happen too often though.
In short, the development, implementation or the review can find information, which was not available during the discussion about the issue. If that happens, I think a good idea would be commenting in the issue "The code/thing/idea (link to code if relevant) made me think that have we have not consider thing Y" or such. This can mean: PR is closed (if it's way of the new plan), PR is redirected, PR is accepted, but it doesn't close the issue and another PR is created to continue. Keep in mind that new issues should be created if that is more suitable.
(Interestingly, original paper about waterfall development model described a process, where you can return to previous steps if that is need  )