Skip to content

Course Credit Calculator #2118

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

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open

Conversation

A-Wheeto
Copy link
Contributor

@A-Wheeto A-Wheeto commented Aug 6, 2024

Status

Review progress:

  • Browser tested
  • Front-end review completed
  • Tech review completed

What's changed?

  • Added new method in activity model to calculate credits for a course seed based on the duration in hours
  • Logic to calculate credits if an existing activities programme is changed

@tc-deploybot tc-deploybot temporarily deployed to teachcomputing-pr-2118 August 6, 2024 14:24 Inactive
@A-Wheeto A-Wheeto self-assigned this Aug 6, 2024
@A-Wheeto A-Wheeto temporarily deployed to teachcomputing-pr-2118 August 6, 2024 14:57 Inactive
@A-Wheeto A-Wheeto requested a review from msquance-stem August 7, 2024 06:52
Copy link
Contributor

@msquance-stem msquance-stem left a comment

Choose a reason for hiding this comment

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

Few little bits that would make this a little bit safer, but looking really good now

@@ -11,6 +11,7 @@
always_on { false }
retired { false }
self_verification_info { nil }
duration_in_hours { 2.5 }
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if we should have this default to nil, as that currently the default behaviour we have on seeds, then maybe have a trait :with_duration?

Copy link
Contributor Author

@A-Wheeto A-Wheeto Aug 22, 2024

Choose a reason for hiding this comment

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

Just having a think about this. I need to test cs_accelerator cases with different durations (less than a day, more than one day).
Would it be cleaner to define the duration in hours when creating the activity for the individual test, rather than a trait? Otherwise I'd have to create multiple traits.
Eg. activity = create(:activity, programmes: [cs_accelerator_programme], duration_in_hours: 10)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

...and have the default to nil as suggested.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually, ignore the above comments. I was repeating a lot of code when defining the duration on individual tests.

@@ -231,4 +231,65 @@
expect(activity.active_course?).to eq(false)
end
end

Copy link
Contributor

Choose a reason for hiding this comment

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

We currently do not test that the credits are not changed if there is no duration in hours. As we use that as the primary criteria in the callback in the programme activity, would be good to test that behaviour

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've added a test for this on line 246

Copy link

@A-Wheeto A-Wheeto force-pushed the 2737-course-credit-calculator-V2 branch from 8e98ab5 to 6055b30 Compare September 24, 2024 14:54
Copy link

@tc-deploybot tc-deploybot temporarily deployed to teachcomputing-pr-2118 September 24, 2024 15:07 Inactive
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