Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Rename Discussion to Welcome Session, remove deprecated TrainingRequirements #2420

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 15 additions & 0 deletions .github/workflows/cicd_feature_branch.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
# CI for feature branches - contains only test runs

name: CI (feature)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍🏼


# don't run CI for every push of any feature branch
# but do run CI if a PR is made with any feature branch as a base
on:
push:
branches: [ 'feature/instructor-checkout-changes' ]
pull_request:
branches: [ 'feature/**' ]

jobs:
test:
uses: ./.github/workflows/test.yml
17 changes: 14 additions & 3 deletions .github/workflows/test.yml
Original file line number Diff line number Diff line change
Expand Up @@ -5,11 +5,12 @@ on:

jobs:
test:
name: Test on Python ${{ matrix.python-version }}
name: ${{ matrix.test-type }} tests on Python ${{ matrix.python-version }}
runs-on: ubuntu-latest
strategy:
matrix:
python-version: [ '3.11' ]
test-type: ["Unit", "Migration"]
elichad marked this conversation as resolved.
Show resolved Hide resolved

services:
postgres:
Expand Down Expand Up @@ -82,12 +83,22 @@ jobs:
exit 1;
fi;

- name: Test
run: pipenv run python manage.py test
- name: Unit tests
run: pipenv run python manage.py test --exclude-tag=migration_test
env:
AMY_DATABASE_HOST: localhost
AMY_DATABASE_PORT: 5432
AMY_DATABASE_NAME: test_amy
AMY_DATABASE_USER: postgres
AMY_DATABASE_PASSWORD: postgres
if: matrix.test-type == 'Unit'

- name: Migration tests
run: pipenv run python manage.py test --tag=migration_test
env:
AMY_DATABASE_HOST: localhost
AMY_DATABASE_PORT: 5432
AMY_DATABASE_NAME: test_amy
AMY_DATABASE_USER: postgres
AMY_DATABASE_PASSWORD: postgres
if: matrix.test-type == 'Migration'
12 changes: 8 additions & 4 deletions Makefile
Original file line number Diff line number Diff line change
Expand Up @@ -11,17 +11,21 @@ all : commands
commands : Makefile
@sed -n 's/^## //p' $<

## test : run all tests.
## test : run all tests except migration tests.
test :
${MANAGE} test
${MANAGE} test --exclude-tag migration_test

## fast_test : run all tests really fast.
fast_test:
${MANAGE} test --keepdb --parallel
${MANAGE} test --keepdb --parallel --exclude-tag migration_test

## fast_test_fail : run all tests really fast, fails as soon as any test fails.
fast_test_fail:
${MANAGE} test --keepdb --parallel --failfast
${MANAGE} test --keepdb --parallel --failfast --exclude-tag migration_test

## test_migrations : test database migrations only
test_migrations:
${MANAGE} test --parallel --tag migration_test

## dev_database : re-make database using saved data
dev_database :
Expand Down
1 change: 1 addition & 0 deletions Pipfile
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,7 @@ social-auth-app-django = "~=5.0.0"
gunicorn = "~=20.1.0"
whitenoise = "~=6.1"
django-better-admin-arrayfield = "==1.4.2"
django-test-migrations = "~=1.3.0"

[dev-packages]
django-webtest = "~=1.9.8"
Expand Down
31 changes: 27 additions & 4 deletions Pipfile.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

4 changes: 2 additions & 2 deletions amy/api/tests/test_export.py
Original file line number Diff line number Diff line change
Expand Up @@ -180,7 +180,7 @@ def prepare_data(self, user):
# add some training progress
TrainingProgress.objects.create(
trainee=self.user,
requirement=TrainingRequirement.objects.get(name="Discussion"),
requirement=TrainingRequirement.objects.get(name="Welcome Session"),
state="p", # passed
event=event,
url=None,
Expand Down Expand Up @@ -504,7 +504,7 @@ def test_relational_fields_structure(self):
"created_at": data["training_progresses"][0]["created_at"],
"last_updated_at": data["training_progresses"][0]["last_updated_at"],
"requirement": {
"name": "Discussion",
"name": "Welcome Session",
"url_required": False,
"event_required": False,
},
Expand Down
22 changes: 9 additions & 13 deletions amy/dashboard/tests/test_instructor_dashboard.py
Original file line number Diff line number Diff line change
Expand Up @@ -36,10 +36,6 @@ def setUp(self):
self._setUpUsersAndLogin()
self._setUpBadges()
self.progress_url = reverse("training-progress")
TrainingRequirement.objects.create(
name="Lesson Contribution", url_required=True
)
TrainingRequirement.objects.create(name="Demo")

def test_instructor_badge(self):
"""When the trainee is awarded both Carpentry Instructor badge,
Expand Down Expand Up @@ -67,7 +63,7 @@ def test_eligible_but_not_awarded(self):
requirements = [
"Training",
"Lesson Contribution",
"Discussion",
"Welcome Session",
"Demo",
]
for requirement in requirements:
Expand Down Expand Up @@ -175,31 +171,31 @@ def test_submission_form(self):
self.assertEqual(got, expected)


class TestDiscussionSessionStatus(TestBase):
"""Test that trainee dashboard displays status of passing Discussion
class TestWelcomeSessionStatus(TestBase):
"""Test that trainee dashboard displays status of passing Welcome
Session. Test whether we display instructions for registering for a
session."""

def setUp(self):
self._setUpUsersAndLogin()
self.discussion = TrainingRequirement.objects.get(name="Discussion")
self.welcome = TrainingRequirement.objects.get(name="Welcome Session")
self.progress_url = reverse("training-progress")

def test_session_passed(self):
TrainingProgress.objects.create(trainee=self.admin, requirement=self.discussion)
TrainingProgress.objects.create(trainee=self.admin, requirement=self.welcome)
rv = self.client.get(self.progress_url)
self.assertContains(rv, "Discussion Session passed")
self.assertContains(rv, "Welcome Session passed")

def test_session_failed(self):
TrainingProgress.objects.create(
trainee=self.admin, requirement=self.discussion, state="f"
trainee=self.admin, requirement=self.welcome, state="f"
)
rv = self.client.get(self.progress_url)
self.assertContains(rv, "Discussion Session not passed yet")
self.assertContains(rv, "Welcome Session not passed yet")

def test_no_participation_in_a_session_yet(self):
rv = self.client.get(self.progress_url)
self.assertContains(rv, "Discussion Session not passed yet")
self.assertContains(rv, "Welcome Session not passed yet")


class TestDemoSessionStatus(TestBase):
Expand Down
17 changes: 9 additions & 8 deletions amy/scripts/seed_training_requirements.py
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,14 @@
# If an entry needs to be removed from the database, remove it from e.g.
# `EMAIL_TEMPLATES`, and put its' ID in `DEPRECATED_EMAIL_TEMPLATES`.

DEPRECATED_TRAINING_REQUIREMENTS: list[str] = []
DEPRECATED_TRAINING_REQUIREMENTS: list[str] = [
"DC Homework",
"SWC Homework",
"LC Homework",
"DC Demo",
"SWC Demo",
"LC Demo",
]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just want to make sure: were these migrated in production? I'm not sure what would happen if these were removed for existing users.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, I saw a migration removing these entries. We should not keep 2 systems for managing "static" (seeded) objects. I would suggest sticking to the seeding script, unless a data migration is actually required.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, I can remove that step from the migration. This is one thing I am uncertain of - are the seeding scripts run when deploying new versions to production?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good question. Let me see... I think they are not run during production deployment (but they definitely are run when deploying to test stage).

We can run them manually after deploying to production, or make sure there's a step that runs them in Ansible.

Once we switch to Docker-based production, we will run the seeds just like we do in test-amy.


TrainingRequirementDef = TypedDict(
"TrainingRequirementDef",
Expand All @@ -22,13 +29,7 @@

TRAINING_REQUIREMENTS: list[TrainingRequirementDef] = [
{"name": "Training", "url_required": False, "event_required": True},
{"name": "DC Homework", "url_required": True, "event_required": False},
{"name": "SWC Homework", "url_required": True, "event_required": False},
{"name": "Discussion", "url_required": False, "event_required": False},
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"Discussion" is not being deprecated as it's being switched to "Welcome Session". The migration 0259_... should probably only migrate the name "Discussion" <-> "Welcome Session", without changes the seeding manages.

{"name": "DC Demo", "url_required": False, "event_required": False},
{"name": "SWC Demo", "url_required": False, "event_required": False},
{"name": "LC Demo", "url_required": False, "event_required": False},
{"name": "LC Homework", "url_required": True, "event_required": False},
{"name": "Welcome Session", "url_required": False, "event_required": False},
{"name": "Lesson Contribution", "url_required": True, "event_required": False},
{"name": "Demo", "url_required": False, "event_required": False},
]
Expand Down
10 changes: 5 additions & 5 deletions amy/templates/dashboard/training_progress.html
Original file line number Diff line number Diff line change
Expand Up @@ -81,13 +81,13 @@ <h2>Your progress of becoming The Carpentries Instructor</h2>
</td>
</tr>
<tr>
<th>3. Discussion Session</th>
<th>3. Welcome Session</th>
<td>
{% if user.passed_discussion %}
<p class="text-success">Discussion Session passed.</p>
{% if user.passed_welcome %}
<p class="text-success">Welcome Session passed.</p>
{% else %}
<p>Discussion Session not passed yet.</p>
<p>Register for a Discussion Session on <a href="https://pad.carpentries.org/community-discussions">this Etherpad</a>. Register for only one session even if you want to become an Instructor for more than one Carpentry lesson program.</p>
<p>Welcome Session not passed yet.</p>
<p>Register for a Welcome Session on <a href="https://pad.carpentries.org/community-discussions">this Etherpad</a>. Register for only one session even if you want to become an Instructor for more than one Carpentry lesson program.</p>
{% endif %}
</td>
</tr>
Expand Down
13 changes: 2 additions & 11 deletions amy/trainings/forms.py
Original file line number Diff line number Diff line change
@@ -1,7 +1,6 @@
from crispy_forms.layout import Layout
from django import forms
from django.core.exceptions import ValidationError
from django.db.models import Q
from django.forms import RadioSelect, TextInput

# this is used instead of Django Autocomplete Light widgets
Expand All @@ -19,11 +18,7 @@ class TrainingProgressForm(forms.ModelForm):
widget=ModelSelect2Widget(data_view="person-lookup"),
)
requirement = forms.ModelChoiceField(
queryset=TrainingRequirement.objects.exclude(
Q(name__startswith="SWC")
| Q(name__startswith="DC")
| Q(name__startswith="LC")
),
queryset=TrainingRequirement.objects.all(),
label="Type",
required=True,
)
Expand Down Expand Up @@ -92,11 +87,7 @@ class BulkAddTrainingProgressForm(forms.ModelForm):
trainees = forms.ModelMultipleChoiceField(queryset=Person.objects.all())

requirement = forms.ModelChoiceField(
queryset=TrainingRequirement.objects.exclude(
Q(name__startswith="SWC")
| Q(name__startswith="DC")
| Q(name__startswith="LC")
),
queryset=TrainingRequirement.objects.all(),
label="Type",
required=True,
)
Expand Down
Loading