Show pending training review message on project landing page

[Chrystinne]

This PR updates the restricted-access message shown in the Files section of a project landing page when the user has already submitted the required training report and it is still under review.

This avoids confusing users about whether they still need to submit a training report. It also adds a link to the Certification page, where users can check the status of their training submissions.

[lukepayyapilli]

Thanks for this, @Chrystinne! This is a genuinely helpful UX improvement, and telling users their training is under review instead of asking them to submit again will save a lot of confusion (and probably some support emails). Moving the "You may submit your training here" text into a proper <li> is a nice cleanup too.

I found one behavioral issue and a few things I'd like to see addressed before merging:

1. Multi-training projects can hide unmet requirements (blocker)

has_training_under_review is true if any required training is in REVIEW, and the template then replaces the whole requirements list with the "under review" message. For a project that requires two trainings, a user who has submitted training A but never started training B will see only "have your submitted training report approved." The list of required trainings and the submit link disappear, so they have no way to discover training B until after A is approved.

I think the condition should be "all trainings the user is still missing are under review" rather than "at least one is." Alternatively, showing per-training status in the list would be even clearer.

2. Reuse the existing get_review() queryset method (blocker)

TrainingQuerySet already has a get_review() method (user/managers.py) that encapsulates what "under review" means, including the DOCUMENT/URL type filtering. Using Training.objects.get_review().filter(...) here would avoid hand-rolling the status filter and importing TrainingStatus into views.py, and it keeps that domain rule in one place.

3. Extract the duplicated template block (blocker)

The new conditional block is identical in both the self-managed and non-self-managed sections, so the template now has two 13-line copies that must be edited in lockstep. Pulling the training requirements block into an {% include %} partial would make future edits much safer. Happy to have this done as part of this PR since it's the change that grew the duplication.

4. Please add a test for the new branch (blocker)

A test covering a user with a training under review (and ideally the multi-training case from point 1) would lock in the behavior and would have caught the issue above. Two small assertions should do it.

Minor, non-blocking suggestions:

• The wording "Have your submitted training report approved" reads as a task the user can't act on. Something like "Your training report is under review. You can check its status on the Certification page" might land better.
• The view now runs two similar queries against the user's trainings for this project. Fetching them once and answering both questions from that queryset would trim a query on a hot page.

Thanks again, the underlying change is a good one and I'm looking forward to seeing it land!

[Chrystinne]

Thank you for your review, @lukepayyapilli ! I’ve addressed the blockers. Could you please have another look?

[lukepayyapilli]

Thanks @Chrystinne, this addresses all four blockers and I'm happy to see it go in.

I checked the new logic against both test cases rather than just reading it. Partial case: no valid trainings, so missing = {reviewed, outstanding}, under_review = {reviewed}, the difference is non-empty, so False, and the full list renders. Full case: both missing, both under review, difference empty, so True, and the message renders. That is exactly right, and the two tests pin it down. Using get_review() keeps the DOCUMENT/URL rule in one place, and the partial means the next person to touch this only has to edit one thing.

Approving. Everything below is a suggestion, not a gate. I would take the first one before merge if you have a minute, since it is a two-line change to a hot path, but I will leave the call to you.

One thing worth naming rather than silently leaving: published_project_unauthorized.html has three access-policy branches, and this converts two. The RESTRICTED branch (line 10) still renders finish required training with no awareness of review state, so a user on a restricted-access project with a submitted report still gets told to go submit one, which is the exact confusion this PR removes everywhere else. Not this PR's job to fix (that branch has a different shape and would need its own design), but I don't want it to read as "done" when it is done for two of three policies. Happy to open a follow-up issue myself if you would rather not carry it.

[lukepayyapilli]

Two things, one change: this runs unguarded, and the name does not match the value.

As written this executes on every authenticated hit to published_project: every open-access project, every project with zero required trainings, every user who already has access and never sees the unauthorized template. That is two extra queries on the highest-traffic page on the site to compute a value the template will not read. You already have both guard variables on the lines directly above.

On the name: has_training_under_review is false for a user who has a training under review but has not started another one, which is the precise case this PR exists to handle. Your own commit message uses the accurate phrasing ("only when all trainings are under review"), so I would just use it in the code, since the template reader cannot see this definition.

Suggested change

	has_training_under_review = False
	if user.is_authenticated:
	valid_training_type_ids = Training.objects.get_valid().filter(
	training_type__in=project.required_trainings.all(), user=user
	).values('training_type_id')
	missing_training_types = project.required_trainings.exclude(id__in=valid_training_type_ids)
	under_review_training_type_ids = Training.objects.get_review().filter(
	training_type__in=missing_training_types,
	user=user,
	).values('training_type_id')
	has_training_under_review = missing_training_types.exists() and not missing_training_types.exclude(
	id__in=under_review_training_type_ids
	).exists()
	all_trainings_under_review = False
	if user.is_authenticated and requires_training and not has_required_training:
	valid_training_type_ids = Training.objects.get_valid().filter(
	training_type__in=project.required_trainings.all(), user=user
	).values('training_type_id')
	missing_training_types = project.required_trainings.exclude(id__in=valid_training_type_ids)
	under_review_training_type_ids = Training.objects.get_review().filter(
	training_type__in=missing_training_types,
	user=user,
	).values('training_type_id')
	all_trainings_under_review = missing_training_types.exists() and not missing_training_types.exclude(
	id__in=under_review_training_type_ids
	).exists()

Behaviour is otherwise identical. I deliberately kept the missing_training_types.exists() check rather than leaning on not has_required_training to imply it, because those two are not quite equivalent (see my other comment).

[lukepayyapilli]

Not introduced by you, and I am not asking you to fix it here, but your new code makes it visible, so I want it written down somewhere.

has_required_training answers "does the user owe any required training?" by counting valid rows and comparing to a total. missing_training_types on the lines below answers the same question by computing the actual set difference. Training has no uniqueness constraint on (user, training_type): in user/models.py the Meta carries only default_permissions = (), and no migration adds one. So two ACCEPTED rows of the same type is a legal state. When it happens, count() == required_trainings.count() holds while a required type is genuinely still missing, and the user is told they are done.

The one-liner is has_required_training = user.is_authenticated and not missing_training_types.exists() (it is only ever read under a requires_training guard, so the no-trainings-required case is unobservable, and I checked all four call sites). Take it here if you want it gone, otherwise I will file it separately. Your call, genuinely not a blocker.

[lukepayyapilli]

Follows the rename above.

Suggested change

	'has_training_under_review': has_training_under_review,
	'all_trainings_under_review': all_trainings_under_review,

[lukepayyapilli]

Three small things bundled so it is one apply: the rename, a header noting the contract, and the list nesting.

On the contract: this partial reads four variables from ambient context and emits bare <li> elements, so it is only valid inside a <ul>. That is a real coupling with nothing anywhere that states it. Both current call sites happen to be right; a comment makes the third one right too.

On the nesting: I called moving "You may submit your training here" out of the bare text node a nice cleanup last time and I stand by that. It just landed one level too high. The outer list is introduced with "you must fulfill all of the following requirements:", so it is now a peer bullet of "sign the data use agreement", and the page lists, as a requirement for access, "You may submit your training here." That is an offer, not a requirement. Tucking it inside the complete required training <li> keeps it valid markup (the thing we actually fixed) and puts it back where it belongs. The same edit also nests the inner <ul> inside an <li>, since a <ul> as a direct child of a <ul> is invalid. That was true before too, but you have just collapsed this into one file, so it is now a one-place fix instead of a two-place one.

Suggested change

	{% if requires_training and not has_required_training %}
	{% if has_training_under_review %}
	<li>
	Your training report is under review. You can check its status on the <a href="{% url 'edit_certification' %}">Certification</a> page.
	</li>
	{% else %}
	<li>complete required training:</li>
	<ul>
	{% for required_training in project.required_trainings.all %}
	<li><a href="{% url 'published_project_required_training' project.slug project.version %}#{{ required_training.pk }}">{{ required_training.name }}</a></li>
	{% endfor %}
	</ul>
	<li>You may submit your training <a href="{% url 'edit_training' %}">here</a>.</li>
	{% endif %}
	{% endif %}
	{% comment %}
	Renders the "required training" bullet(s) for a project's access requirements.
	Emits <li> elements, so it must be included inside a <ul>.
	Context: project, requires_training, has_required_training, all_trainings_under_review.
	{% endcomment %}
	{% if requires_training and not has_required_training %}
	{% if all_trainings_under_review %}
	<li>
	Your training report is under review. You can check its status on the <a href="{% url 'edit_certification' %}">Certification</a> page.
	</li>
	{% else %}
	<li>
	complete required training:
	<ul>
	{% for required_training in project.required_trainings.all %}
	<li><a href="{% url 'published_project_required_training' project.slug project.version %}#{{ required_training.pk }}">{{ required_training.name }}</a></li>
	{% endfor %}
	</ul>
	You may submit your training <a href="{% url 'edit_training' %}">here</a>.
	</li>
	{% endif %}
	{% endif %}

[lukepayyapilli]

These are real tests and they cover the branch I asked for, so this is polish rather than a gate.

The gap I would most like closed: test_all_trainings_under_review asserts only that the message appears. The feature is a swap, showing the review message instead of the training list and the submit link, and the test checks the "instead" but not the "of". A regression that renders both blocks at once passes today.

Beyond that: the two bodies are 12 near-identical lines differing in one status, test_pending_training_requirements does not say which case it covers, and reviewer_comments='' is redundant (Django defaults a non-null CharField to '').

I have used the message text rather than reverse('edit_training') in the negative assertions deliberately, since that URL is also referenced by the RESTRICTED branch of the same template, so a text assertion is both more robust and more obvious about intent.

Suggested change

	def test_pending_training_requirements(self):
	project = PublishedProject.objects.get(title='Demo eICU Collaborative Research Database')
	user = User.objects.get(email='rgmark@mit.edu')
	reviewed_training = TrainingType.objects.create(name='Reviewed training')
	outstanding_training = TrainingType.objects.create(name='Outstanding training')
	project.required_trainings.set([reviewed_training, outstanding_training])
	Training.objects.create(
	user=user,
	slug='reviewed-training',
	training_type=reviewed_training,
	status=TrainingStatus.REVIEW,
	reviewer_comments='',
	)
	self.client.login(username=user.username, password='Tester11!')
	response = self.client.get(reverse('published_project', args=(project.slug, project.version)))
	self.assertContains(response, reviewed_training.name)
	self.assertContains(response, outstanding_training.name)
	self.assertNotContains(response, 'Your training report is under review.')
	def test_all_trainings_under_review(self):
	project = PublishedProject.objects.get(title='Demo eICU Collaborative Research Database')
	user = User.objects.get(email='rgmark@mit.edu')
	first_training = TrainingType.objects.create(name='First training')
	second_training = TrainingType.objects.create(name='Second training')
	project.required_trainings.set([first_training, second_training])
	Training.objects.create(
	user=user,
	slug='first-training',
	training_type=first_training,
	status=TrainingStatus.REVIEW,
	reviewer_comments='',
	)
	Training.objects.create(
	user=user,
	slug='second-training',
	training_type=second_training,
	status=TrainingStatus.REVIEW,
	reviewer_comments='',
	)
	self.client.login(username=user.username, password='Tester11!')
	response = self.client.get(reverse('published_project', args=(project.slug, project.version)))
	self.assertContains(response, 'Your training report is under review.')
	TRAINING_UNDER_REVIEW_MESSAGE = 'Your training report is under review.'
	def _require_trainings(self, user, statuses):
	"""
	Make the demo eICU project require one training type per entry in
	`statuses`, creating a Training for `user` in that status. An entry of
	None means the user has not started that training at all.
	Returns the project and the created TrainingTypes, in order.
	"""
	project = PublishedProject.objects.get(title='Demo eICU Collaborative Research Database')
	training_types = []
	for index, status in enumerate(statuses):
	training_type = TrainingType.objects.create(name=f'Training {index}')
	training_types.append(training_type)
	if status is not None:
	Training.objects.create(
	user=user,
	slug=f'training-{index}',
	training_type=training_type,
	status=status,
	)
	project.required_trainings.set(training_types)
	return project, training_types
	def test_partial_training_review_shows_full_requirements(self):
	"""
	A user with one training under review and another not yet started still
	sees every required training and the link to submit one.
	"""
	user = User.objects.get(email='rgmark@mit.edu')
	project, (reviewed, outstanding) = self._require_trainings(user, [TrainingStatus.REVIEW, None])
	self.client.login(username=user.username, password='Tester11!')
	response = self.client.get(reverse('published_project', args=(project.slug, project.version)))
	self.assertContains(response, reviewed.name)
	self.assertContains(response, outstanding.name)
	self.assertContains(response, 'You may submit your training')
	self.assertNotContains(response, self.TRAINING_UNDER_REVIEW_MESSAGE)
	def test_all_trainings_under_review_hides_requirements(self):
	"""
	When every outstanding training is under review, the requirements list
	is replaced by the review message and a link to the certification page.
	"""
	user = User.objects.get(email='rgmark@mit.edu')
	project, (first, second) = self._require_trainings(user, [TrainingStatus.REVIEW, TrainingStatus.REVIEW])
	self.client.login(username=user.username, password='Tester11!')
	response = self.client.get(reverse('published_project', args=(project.slug, project.version)))
	self.assertContains(response, self.TRAINING_UNDER_REVIEW_MESSAGE)
	self.assertContains(response, reverse('edit_certification'))
	self.assertNotContains(response, first.name)
	self.assertNotContains(response, second.name)
	self.assertNotContains(response, 'You may submit your training')

[tompollard]

@Chrystinne could we pause on this before merging? This is important logic and I'd like to make sure we do a very thorough check before deploying.

[Chrystinne]

@tompollard I’ll still go over Luke’s comments

[tompollard]

I had a quick play around with this and overall it works as I'd expect when there is a single training course. I'd be okay with merging, if Luke is happy with the code changes.

If just a single training course is required, we see:

complete required training:

• CITI Data or Specimens Only Research
  You may submit your training here.

If I submit the CITI Data or Specimens Only Research course, I see:

Your training report is under review. You can check its status on the Certification page.

When there are two or more training courses, the behavior seems slightly inconsistent. If two training courses are required, we see:

complete required training:

• CITI Data or Specimens Only Research
• World 101: Introduction to Continents and Countries
  You may submit your training here.

After submitting the CITI Data or Specimens Only Research, the message is unchanged:

complete required training:

• CITI Data or Specimens Only Research
• World 101: Introduction to Continents and Countries
  You may submit your training here.

It does not mention anything about my CITI training being under review. Not major issue, particularly now we have refactored the training/certification pages to make them clearer.

[tompollard]

@Chrystinne, if you do plan on merging, please could you rebase on dev first? We have made several changes to the training content (including merging the training and certification pages).

[Chrystinne]

@tompollard thanks for reviewing it, Tom! I rebased on dev and am now merging.