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

Change null to mdash on empty student ids in the All results table #1284

Conversation

ashi006
Copy link
Contributor

@ashi006 ashi006 commented Oct 11, 2023

Description

What?

Empty students IDs were displayed as null in the All results table

How?

Replace null with an em dash (—) for a user friendly view

Fixes #1272

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.

[ADD A DESCRIPTION ABOUT WHAT YOU TESTED MANUALLY]

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!

@markkuriekkinen markkuriekkinen self-requested a review October 11, 2023 11:27
Copy link
Contributor

@markkuriekkinen markkuriekkinen left a comment

Choose a reason for hiding this comment

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

Great that you have gotten started! I made multiple comments about the details of the coding style and also about ideas for a cleaner solution. Respond to the comments if something is unclear or feels wrong.

The git commit message needs to be rewritten. Take a look at our git history to get a grasp of the expected style. The commit message needs to start with a concise line that says what is done. For example, "Change null to mdash in the All results table"

One "commit message guide" from the web:
https://gist.github.com/robertpainsi/b632364184e70900af4ab688decf6f53

We also want the "Fixes #1272" issue link in the git commit message.
https://docs.github.com/en/issues/tracking-your-work-with-issues/linking-a-pull-request-to-an-issue

You can also squash the two commits into one. In this case, there is no need to keep multiple commits since it is a very small fix.

The author of the commits is not connected to your Github profile. In GitHub, the author becomes a link to the profile when it works. The connection depends on the email address. The email address in the git commit (defined when you run the "git commit" command) should match one of the email addresses you have added to your Github profile.
Did you set your name and email to the global git config before getting started?

https://docs.github.com/en/account-and-profile/setting-up-and-managing-your-personal-account-on-github/managing-email-preferences/setting-your-commit-email-address#setting-your-email-address-for-every-repository-on-your-computer

exercise/static/exercise/results_staff.js Outdated Show resolved Hide resolved
exercise/static/exercise/results_staff.js Outdated Show resolved Hide resolved
exercise/static/exercise/results_staff.js Outdated Show resolved Hide resolved
exercise/static/exercise/results_staff.js Outdated Show resolved Hide resolved
exercise/static/exercise/results_staff.js Outdated Show resolved Hide resolved
exercise/static/exercise/results_staff.js Outdated Show resolved Hide resolved
@markkuriekkinen
Copy link
Contributor

markkuriekkinen commented Oct 11, 2023

Once you have defined a better git commit message, you can also modify the pull request title. The pull request title is also not good.

@ashi006 ashi006 closed this Oct 12, 2023
@ashi006 ashi006 force-pushed the Issue-#-1272-The-All-Results-page-describes-missing-student-IDs branch from 0019cf7 to 46dba40 Compare October 12, 2023 10:57
@ashi006 ashi006 changed the title Issue # 1272 the all results page describes missing student ids Change null to mdash in the All results table Oct 12, 2023
@ashi006 ashi006 reopened this Oct 12, 2023
@markkuriekkinen markkuriekkinen changed the title Change null to mdash in the All results table Change null to mdash on empty student ids in the All results table Oct 12, 2023
Copy link
Contributor

@markkuriekkinen markkuriekkinen left a comment

Choose a reason for hiding this comment

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

Excellent! I like the code more than the first version. The git commit message is still missing the "Fixes link" to the issue. I also thought that the commit message should emphasize that the change affects the students with no student id. Here's a new suggested git commit message:

Change null to mdash on empty student ids in the All results table

Fixes #1272

The Selenium test error seems to be unrelated to the changes in this pull request. Something else is wrong there. I suspect that the test data has dates that have passed. That is to say, the course is supposed to be open in the test, but its ending date has been set to 27.9.2023. It is not open any longer now in October 2023.

Could you fix the Selenium test data? The ending dates could be moved to the year 2030. You can run the tests locally in your laptop.

Selenium test data:

"ending_time": "2023-09-27T09:00:00Z",

Running tests with Docker:
https://github.com/apluslms/develop-aplus/blob/master/docker-compose.test.yml
docker-compose -f docker-compose.test.yml up plus_selenium
You need to fix the file path to the A-plus source code under the volumes field.

@ashi006 ashi006 force-pushed the Issue-#-1272-The-All-Results-page-describes-missing-student-IDs branch 2 times, most recently from a299b1c to 46a5007 Compare October 17, 2023 08:06
@ashi006 ashi006 removed their assignment Oct 17, 2023
@markkuriekkinen markkuriekkinen self-assigned this Oct 17, 2023
Selenium tests were failing because the course closing dates
in the test have passed. The course is supposed to be open
in order to run the tests successfully so the dates were moved
to the future in the test data JSON and teacher_list_test.py files.
@markkuriekkinen markkuriekkinen force-pushed the Issue-#-1272-The-All-Results-page-describes-missing-student-IDs branch from 8f2b32d to f15f357 Compare October 17, 2023 16:43
Copy link
Contributor

@markkuriekkinen markkuriekkinen left a comment

Choose a reason for hiding this comment

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

Great, thanks! I changed the commit message of the Selenium test fix since you had used very long lines there. Commit messages are not supposed to have lines longer than about 70-80 characters. Long URLs are an exception. You should write commit messages in a text editor so that it is easy to edit multi-line text. When you don't use the -m option in the git commit command, git will open a text editor for writing the commit message. You can change the preferred editor in the settings. If you don't like advanced command-line text editors (vim and emacs), then nano is a very simple editor. Or you could use a graphical editor (then it is better to use a simple text editor, not something complex like VS Code). On the other hand, if you make git commits inside a tool like VS Code, then the user experience is very different than the command-line usage.

@markkuriekkinen markkuriekkinen merged commit dd06696 into apluslms:master Oct 17, 2023
8 checks passed
@ashi006 ashi006 deleted the Issue-#-1272-The-All-Results-page-describes-missing-student-IDs branch October 18, 2023 12:24
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.

The All Results page describes missing student IDs as "null"
2 participants