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.

Sign up for GitHub

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

Jump to bottom

Improve event validation and option filtering for training progress and instructor badge awarding #2471

Merged
merged 19 commits into from
Sep 15, 2023
Merged

Improve event validation and option filtering for training progress and instructor badge awarding #2471

merged 19 commits into from
Sep 15, 2023

Conversation

elichad
Copy link
Contributor

@elichad elichad commented Jul 14, 2023 •
edited
Loading

Fixes #2342, fixes #2343 :

  • when awarding instructor badge, auto-fill badge and event selection where possible. Where multiple passed Trainings exist for a trainee, filter event field options to only include those events (though this generally shouldn't happen as only one passed Training should exist)
  • when creating a training progress, filter event field options to include only events where the selected trainee has a learner task
  • when validating training progress, ensure that
    1. trainee has a learner task for the chosen event
    2. trainee does not already have a progress for the chosen event
  • when adding training progress in bulk, if there are some failures due to the above validation, create all progresses that can be created. Instead of the usual success message, show the errors and info about how many trainee progresses were created/skipped (screenshot).
  • start some documentation for where we use design patterns that might be useful elsewhere (motivated by how it took me a while to realise the community role creation view had the dynamic behaviour I was looking for)

Screenshot of Trainees page, with messages at the top: "Error: Training: Trainee Jay Dean already has a training progress for event 2023-05-29-hodgefort-ttt", repeat for Wanda Higgins, and "Changed progress of 2 trainees. 2 trainees were skipped due to errors."

@elichad elichad added this to the v4.2 milestone Jul 14, 2023
@elichad elichad self-assigned this Jul 14, 2023
pbanaszkiewicz
pbanaszkiewicz requested changes Jul 18, 2023
View reviewed changes
Copy link
Contributor

@pbanaszkiewicz pbanaszkiewicz left a comment

Choose a reason for hiding this comment

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

I left some comments. I think this is heading in a right direction, but I would like you to consider some of my proposals.

amy/workshops/lookups.py Show resolved Hide resolved
amy/workshops/lookups.py Outdated Show resolved Hide resolved
amy/workshops/lookups.py Show resolved Hide resolved
amy/workshops/models.py Outdated Show resolved Hide resolved
amy/workshops/models.py Outdated Show resolved Hide resolved
amy/workshops/models.py Outdated Show resolved Hide resolved
amy/workshops/views.py Outdated Show resolved Hide resolved
amy/workshops/views.py Outdated Show resolved Hide resolved
amy/workshops/models.py Outdated Show resolved Hide resolved
amy/trainings/views.py Outdated
except ValidationError as e:
errors.append(e)

if len(errors) > 0:
Copy link
Contributor

Choose a reason for hiding this comment

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

Idiomatic Python version:

Suggested change
if len(errors) > 0:
if errors:

@elichad elichad modified the milestones: v4.2, v4.3 Aug 2, 2023
@elichad
Copy link
Contributor Author

elichad commented Aug 2, 2023

I'm bumping this to the 4.3 milestone for time reasons. There's some more work needed here to address Piotr's review comments, and it's only a convenient extra to the checkout project rather than anything essential.

elichad and others added 17 commits August 23, 2023 15:38
@elichad
add trainee filter & js trigger to TTT lookup
117e1dd
@elichad
remove debug print
9ad3352
@elichad
ensure cannot create two progresses with same trainee and event
14a78b5
@elichad
add documentation of useful patterns
370b5d2
@elichad
add extra validation and get bulk errors to show
d513c69
@elichad
use correct lookup & add help text
35b068b
@elichad
add event filtering/autofill to award form
09fd48d
@elichad
check for passed progress rather than learner task
1e8553b
@elichad
update tests for award view & event lookup
644f33d
@elichad
tidy up
e267c47
@elichad
use separate lookup for events in award view; ensure instructor badge…
894e618 
… checked for filter
@elichad
ignore ordering in assertQuerysetEqual
219e774
@elichad @pbanaszkiewicz
code style improvements
c97515b 
Co-authored-by: Piotr Banaszkiewicz <piotr@banaszkiewicz.org>
@elichad
more code style improvements
7cd1dc1
@elichad
use unique constraint in DB for trainee + event
f3d0899
@elichad
rebase migration
d273983
@elichad
tidy up
de08d5f
@elichad elichad force-pushed the feature/autofill-training-events branch from e6f916d to de08d5f Compare August 23, 2023 14:54
@elichad
Copy link
Contributor Author

elichad commented Aug 23, 2023

@pbanaszkiewicz I finally got around to addressing your comments on this PR. Ready for a re-review!

pbanaszkiewicz
pbanaszkiewicz approved these changes Aug 26, 2023
View reviewed changes
Copy link
Contributor

@pbanaszkiewicz pbanaszkiewicz left a comment

Choose a reason for hiding this comment

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

Looks good to me!

amy/trainings/utils.py Outdated
Task.objects.get(
person=trainee,
event=event,
role=Role.objects.get(name="learner"),
Copy link
Contributor

Choose a reason for hiding this comment

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

1 query less if you switch to role__name="learner".

amy/trainings/tests/test_training_progress.py
) # do not save to DB as this violates the unique constraint we want to test
p1.full_clean() # should be no error if only this progress exists
with self.assertValidationErrors(["__all__"]):
p2.full_clean()
Copy link
Contributor

@pbanaszkiewicz pbanaszkiewicz Aug 26, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Can you check the validation error message here?

@elichad elichad merged commit b4dbfef into develop Sep 15, 2023
8 checks passed
@elichad elichad mentioned this pull request Oct 24, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pbanaszkiewicz pbanaszkiewicz pbanaszkiewicz approved these changes

Assignees

@elichad elichad

Labels
component: forms component: user interface (UI) type: enhancement
Projects
None yet
Milestone
v4.3
Development

Successfully merging this pull request may close these issues.

Recording event associated with instructor badge Recording trainee participation at TTT event
2 participants
@elichad @pbanaszkiewicz