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

Personalized points goal #1387

Merged
merged 15 commits into from
Oct 9, 2024
Merged

Conversation

mikaelGusse
Copy link
Contributor

@mikaelGusse mikaelGusse commented Aug 7, 2024

Description

What?

Add a feature to the progress bar for modules which allows you to set a points goal for that particular module. When this goal is reached, the points progress bar changes color to blue.
image
image
image
image

Why?

Was asked for and improves the student experience of using APlus.

How?

Changed templates, added a new class to store points goals, added a modal to the interface to modify and create goals.
Also changed how the points required vertical bar is shown to be clearer.

Fixes #1332

Testing

Remember to add or update unit tests for new features and changes.

What type of test did you run?

  • Accessibility test using the WAVE extension.
  • Django unit tests.
  • Selenium tests.
  • Other test. (Add a description below)
  • Manual testing.

Added playwright tests to test this feature. Manually tested different combinations of required points, points goals etc.

Did you test the changes in

  • Chrome
  • Firefox
  • This pull request cannot be tested in the browser.

Think of what is affected by these changes and could become broken

Translation

Programming style

  • Did you follow our style guides?
  • Did you use Python type hinting in all functions that you added or edited? (type hints for function parameters and return values)

Have you updated the README or other relevant documentation?

  • documents inside the doc directory.
  • README.md.
  • Aplus Manual.
  • Other documentation (mention below which documentation).

Is it Done?

  • Reviewer has finished the code review
  • After the review, the developer has made changes accordingly
  • Customer/Teacher has accepted the implementation of the feature

Clean up your git commit history before submitting the pull request!

@mikaelGusse mikaelGusse force-pushed the personalized_points_goal branch 4 times, most recently from 1d124db to a5103c2 Compare August 8, 2024 10:33
@mikaelGusse mikaelGusse force-pushed the personalized_points_goal branch from a5103c2 to c694072 Compare August 8, 2024 13:08
@mikaelGusse mikaelGusse force-pushed the personalized_points_goal branch 3 times, most recently from c6e7c85 to 2dc4c8e Compare August 9, 2024 07:44
@mikaelGusse mikaelGusse force-pushed the personalized_points_goal branch from 2dc4c8e to c07dbdb Compare August 9, 2024 07:53
@mikaelGusse mikaelGusse requested a review from ihalaij1 August 9, 2024 08:49
@mikaelGusse mikaelGusse force-pushed the personalized_points_goal branch 3 times, most recently from e8a83ba to bd05872 Compare August 12, 2024 13:31
@mikaelGusse mikaelGusse force-pushed the personalized_points_goal branch from bd05872 to 145ac2f Compare August 12, 2024 13:40
@mikaelGusse mikaelGusse self-assigned this Aug 13, 2024
@mikaelGusse mikaelGusse requested a review from murhum1 August 13, 2024 07:53
Copy link
Contributor

@ihalaij1 ihalaij1 left a comment

Choose a reason for hiding this comment

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

Nice!

A couple of comments from me, and a question:
Is it currently possible to remove points goals at all?
If not, it should be added as an option for the user.
Maybe setting the goal to zero should remove it completely?

e2e_tests/test_points_goal_set.py Outdated Show resolved Hide resolved
exercise/static/exercise/personalized_points_goal.js Outdated Show resolved Hide resolved
locale/fi/LC_MESSAGES/django.po Outdated Show resolved Hide resolved
locale/en/LC_MESSAGES/django.po Outdated Show resolved Hide resolved
locale/en/LC_MESSAGES/django.po Outdated Show resolved Hide resolved
locale/en/LC_MESSAGES/django.po Outdated Show resolved Hide resolved
locale/en/LC_MESSAGES/django.po Outdated Show resolved Hide resolved
locale/fi/LC_MESSAGES/django.po Outdated Show resolved Hide resolved
locale/fi/LC_MESSAGES/django.po Outdated Show resolved Hide resolved
locale/fi/LC_MESSAGES/django.po Show resolved Hide resolved
@mikaelGusse mikaelGusse force-pushed the personalized_points_goal branch from b93cbd1 to 3a4d0d1 Compare August 15, 2024 11:17
@mikaelGusse mikaelGusse force-pushed the personalized_points_goal branch from 3a4d0d1 to debfa75 Compare August 15, 2024 11:20
exercise/views.py Fixed Show fixed Hide fixed
@mikaelGusse mikaelGusse force-pushed the personalized_points_goal branch from 7e200b7 to 67f5158 Compare August 15, 2024 12:15
@mikaelGusse mikaelGusse requested a review from ihalaij1 August 15, 2024 13:56
Copy link
Contributor

@ihalaij1 ihalaij1 left a comment

Choose a reason for hiding this comment

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

Good job on the fixes!

Is it possible to check if there exists a points goal when clicking the button, and if it exists, show the Remove-button in the modal? If there is no points goal the button wouldn't be shown.

Also, if a previous goal exists, it would be nice if it was auto-filled into the points goal input field.

locale/fi/LC_MESSAGES/django.po Outdated Show resolved Hide resolved
exercise/static/exercise/personalized_points_goal.js Outdated Show resolved Hide resolved
@mikaelGusse mikaelGusse requested a review from ihalaij1 August 19, 2024 09:37
Copy link
Contributor

@ihalaij1 ihalaij1 left a comment

Choose a reason for hiding this comment

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

Can the Remove button be hidden after it has been clicked? Currently, it's possible to click it more than once, which results in a failure message.

exercise/templatetags/exercise.py Outdated Show resolved Hide resolved
Copy link
Contributor

@ihalaij1 ihalaij1 left a comment

Choose a reason for hiding this comment

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

Nice! Let's wait for @murhum1 to review this too.

@ihalaij1 ihalaij1 assigned murhum1 and unassigned mikaelGusse Aug 26, 2024
Copy link
Contributor

@murhum1 murhum1 left a comment

Choose a reason for hiding this comment

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

Added some comments and change requests; might still be missing some, but will check the rest tomorrow!

exercise/views.py Outdated Show resolved Hide resolved
exercise/views.py Outdated Show resolved Hide resolved
exercise/views.py Outdated Show resolved Hide resolved
course/models.py Outdated Show resolved Hide resolved
course/models.py Show resolved Hide resolved
exercise/templates/exercise/_points_progress.html Outdated Show resolved Hide resolved
@mikaelGusse mikaelGusse force-pushed the personalized_points_goal branch from 8326413 to 8caddf7 Compare September 25, 2024 09:39
@mikaelGusse mikaelGusse requested a review from murhum1 September 25, 2024 10:27
if ($progressDiv.length) {
const tooltipTitle = $progressDiv.attr('data-original-title');
const parser = new DOMParser();
const doc = parser.parseFromString(tooltipTitle, 'text/html');

Check warning

Code scanning / CodeQL

DOM text reinterpreted as HTML Medium

DOM text
is reinterpreted as HTML without escaping meta-characters.
// Update tooltip
const tooltipTitle = $progressDiv.attr('data-original-title');
const parser = new DOMParser();
const doc = parser.parseFromString(tooltipTitle, 'text/html');

Check warning

Code scanning / CodeQL

DOM text reinterpreted as HTML Medium

DOM text
is reinterpreted as HTML without escaping meta-characters.
@murhum1 murhum1 force-pushed the personalized_points_goal branch from 867c68a to 2f99612 Compare October 8, 2024 09:06
@murhum1
Copy link
Contributor

murhum1 commented Oct 8, 2024

I added some more fixes:

  • Renamed the instances of personalized_points_goal_* to module_goal_* to make things more understandable (e.g. personalized_points_module_goal_points is really difficult to mentally parse). I still kept references to "personalized points goal" in UI text as they were since it works better in that context.
  • Fixed some progress-bar CSS classes to use aplus-progress-bar instead
    • Related to that, fixed e2e test CSS selector
  • Squashed migrations into a single one by deleting the migration files and re-running makemigrations

Is the renaming operation ok for you @mikaelGusse? If so, I'll proceed with the merge.

@mikaelGusse
Copy link
Contributor Author

Yes. All of these changes seem sensible to me. Go ahead and merge 👍

@murhum1 murhum1 merged commit 7f509ef into apluslms:master Oct 9, 2024
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

Allow setting personal points goals for modules
3 participants