Skip to content

Conversation

@b0ink
Copy link

@b0ink b0ink commented Apr 16, 2025

Description

This feature will allow staff to enable the "Tutorial self enrolment" feature for a designated task. Staff will select which tutorial stream(s) to fetch the tutorials from, and students will be prompted with the list of tutorials they must choose from to complete the task.

Type of change

  • New feature (non-breaking change which adds functionality)
  • This change requires a documentation update

How Has This Been Tested?

Ensure you are also using the API branch:
thoth-tech/doubtfire-api#64

  1. Login as aadmin
  2. Select any unit and any task to edit
  3. Scroll to the "Tutorial enrolment form" and enable it
  4. Select a tutorial stream that has a atleast a few tutorials
  5. If there are no tutorial streams to choose from or the stream has no tutorials, manually add them in the in the tutorials tab.
  6. Save the changes to the task after enabling the self enrolment form.
  7. Now login as student_1, and navigate to the same unit and task you enabled the form for.
  8. Click on "Begin enrolment", and confirm the list of tutorials show up. Do note that some tutorials may not show up for a student based on their campus. Eg. if a student is based on campus X, but the tutorial is only available for campus Y, the student will not see it.
  9. Select a tutorial and click enrol.
  10. If it says you have already been enrolled, you can navigate to the Tutorial List and withdraw yourself from the unit, then go back to the task to begin tutorial enrolment, and select a tutorial

Testing Checklist:

  • Tested in latest Chrome
  • Tested in latest Safari
  • Tested in latest Firefox

Checklist:

  • My code follows the style guidelines of this project
  • I have performed a self-review of my own code
  • I have commented my code in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • I have requested a review from @macite and @jakerenzella on the Pull Request

@returnMarcco
Copy link

returnMarcco commented Apr 20, 2025

Hi @b0ink,

Good work!
In reviewing your PR, I've completed the following:

Checklist

  • Tested in Chrome
  • Tested in Firefox
  • Tested in Safari (no access to a Mac)
  • No obvious UX issues
  • No obvious UI issues
  • No devtools console errors
  • No devtools network tab errors

Screenshots

Screenshot 2025-04-20 131448

Suggested Changes

  • The wording in the following screenshot could probably be simplified. I suggest something along the lines of "Please select a tutorial".
    Screenshot 2025-04-20 131448

  • Question: When enrolling in a tutorial, should every task then show the green 'You're enrolled...' text as it does now?

Summary

  • The feature functions as expected, no obvious errors or design decisions. I was able to successfully enrol/withdraw from a tutorial. Each task correctly shows when a tutorial has been enrolled in. Good work. I'll approve this once I've been assigned as a reviewer for this PR.

@EkamBhullar
Copy link

Hey @b0ink, I have reviewed the PR, and the UI and functionality is working as expected. I have tested in following web browsers

  • Tested in latest Chrome
  • Tested in latest Safari

Screenshot

Screenshot 2025-04-20 at 8 32 56 pm
  • Great work overall! I was able to enroll and then withdraw without much hassle.

@b0ink b0ink force-pushed the feat/tutorial-self-enrolment branch from de37b87 to 15ba574 Compare May 7, 2025 06:18
@b0ink b0ink marked this pull request as ready for review May 14, 2025 06:40
@returnMarcco
Copy link

returnMarcco commented May 16, 2025

Hi @b0ink,

I've been assigned to re-review your PR as a 'senior reviewer'.

I've completed the following steps:

Checklist

  • Tested in Chrome
  • Tested in Firefox
  • Tested in Safari (no access to a Mac)
  • No obvious UX issues
  • No obvious UI issues
  • No devtools console errors
  • No devtools network tab errors

Findings

I've come across some potential issues. The main thing here is to determine whether the issue is in my development environment, or there's actually bugs in the code.

  • Potential error: When I enable Tutorial Enrolment and select a Tutorial Stream, I can save it with a 200 response. The problem is, when I reload the page, the checkbox seems to have reverted to not being selected anymore as if no changes were saved.

  • Pontential error: When logging in as student_1 and navigating to the task and assignment I've configured the Tutorial Enrolment for in the previous step, the Begin enrolment button doesn't seem to be visible anywhere. Please see the screenshot below.
    potential_error_cannot_see_begin_enrolment

  • Potential syntax issue: Seemingly incorrect event binding here. It seems to me that the square brackets around the selectionchange attribute should be parenthesis, which would bind the onSelectStream() event handler to the mat-select component the attribute belongs to. Please correct me if I'm wrong.
    potentially_incorrect_event_binding

  • Potential unrelated error: When logging in as a student and navigating a unit, the following type errors are thrown from the component in the screenshot. I am unsure if this has anything to do with your changes, or if this will affect your changes at all.
    error_logged_in_as_student

  • As a side note, there are multiple console.logs remaining in the task-definition-tutorial-enrolment.component.ts file.

Can you please see if you are able to reproduce the above errors on your end and get back to me, then we can go from there.

Cheers.

public onSelectStream(): void {
const selectedStream = this.tutoralStreamControl.value;
if (selectedStream) {
console.log(selectedStream);

Choose a reason for hiding this comment

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

This log should probably be removed

this._tutorialStreams.push(stream);
}

console.log(this.taskDefinition.tutorialSelfEnrolmentStream);

Choose a reason for hiding this comment

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

This log should probably be removed.

const tutorialStreams = this.taskDefinition.unit.tutorialStreamsCache.currentValues;
console.log(tutorialStreams);
for (const stream of tutorialStreams) {
console.log(stream);

Choose a reason for hiding this comment

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

This log should probably be removed.


public ngOnInit(): void {
const tutorialStreams = this.taskDefinition.unit.tutorialStreamsCache.currentValues;
console.log(tutorialStreams);

Choose a reason for hiding this comment

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

This log should probably be removed.

@returnMarcco
Copy link

I've accidentally approved these changes and cannot revert this due to not having write access. I'll reach out to Nebula to revert this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants