Move the exceptions raised in student.rb to a localization file, and add test coverage for cases where Group and Grouping save return false #7218

anubhajoshi01 · 2024-09-14T16:29:10Z

Proposed Changes

(Describe your changes here. Also describe the motivation for your changes: what problem do they solve, or how do they improve the application or codebase? If this pull request fixes an open issue, use a keyword to link this pull request to the issue.)

Must move the exceptions raised in student.rb to a localization file, and add test coverage for the case where Group or Grouping save fail.

Type of Change

(Write an X or a brief description next to the type or types that best describe your changes.)

First, I added a localization file config/locales/models/students/en.yml with the text string of the Exceptions raised in when Group or Grouping save fails, and switched to using I18n to lookup those strings when raising the exception

I also added two tests in student_spec.rb , where it stubs the Group and Grouping save methods to return false, and ensures that the exception is raised in these cases as expected.

Verify new line coverage:

Type	Applies?
🚨 Breaking change (fix or feature that would cause existing functionality to change)
✨ New feature (non-breaking change that adds functionality)
🐛 Bug fix (non-breaking change that fixes an issue)
🎨 User interface change (change to user interface; provide screenshots)
♻️ Refactoring (internal change to codebase, without changing functionality)
🚦 Test update (change that only adds or modifies tests)	X
📦 Dependency update (change that updates a dependency)
🔧 Internal (change that only affects developers or continuous integration)

Checklist

(Complete each of the following items for your pull request. Indicate that you have completed an item by changing the [ ] into a [x] in the raw text, or by clicking on the checkbox in the rendered description on GitHub.)

Before opening your pull request:

I have performed a self-review of my changes.
- Check that all changed files included in this pull request are intentional changes.
- Check that all changes are relevant to the purpose of this pull request, as described above.
I have added tests for my changes, if applicable.
- This is required for all bug fixes and new features.
I have updated the project documentation, if applicable.
- This is required for new features.
If this is my first contribution, I have added myself to the list of contributors.

After opening your pull request:

I have updated the project Changelog (this is required for all changes).
I have verified that the pre-commit.ci checks have passed.
I have verified that the CI tests have passed.
I have reviewed the test coverage changes reported by Coveralls.
I have requested a review from a project maintainer.

Questions and Comments

(Include any questions or comments you have regarding your changes.)

for more information, see https://pre-commit.ci

anubhajoshi01 · 2024-09-14T16:35:05Z

config/locales/models/students/en.yaml

+en:
+  students:
+    errors:
+      group_creation_failure: "Sorry! For some reason, your group could not be created. Please wait a few seconds, then hit refresh to try again. If you come back to this page, you should inform the course instructor."


From the other localizations files I saw, the text strings were generally one sentence. Also, since the Please wait a few seconds... part is repeated, would it be better if I extract that into a seperate text? I wasn't sure since they each represent different exceptions

anubhajoshi01

I also have to still modify the changelog, is New Features and Improvements the correct section I should add it under

EDIT Pranav suggested to add it under Internal changes

…ithub.com/anubhajoshi01/Markus-Anubha into add-test-coverage-save-fail-student-model

coveralls · 2024-09-14T18:03:23Z

Pull Request Test Coverage Report for Build 10913837694

Details

12 of 12 (100.0%) changed or added relevant lines in 2 files are covered.
No unchanged relevant lines lost coverage.
Overall coverage increased (+0.02%) to 91.541%

Totals
Change from base Build 10876839680:	0.02%
Covered Lines:	40311
Relevant Lines:	43365

💛 - Coveralls

david-yz-liu

@anubhajoshi01 good work. I left one inline comment about the localization strings. Also:

It's okay for the strings to contain some duplicated text; there's no super elegant want of decomposing locale strings into sub-strings other than just concatenation, and that's not super typical in the MarkUs codebase.
Please add your name to the list of contributors (see my comment on your other PR).
When filling out the checklist, make sure not to include a space in [X], otherwise the formatting isn't parsed correctly.
You received a commit from our pre-commit.ci bot. This suggests you haven't set up your pre-commit hooks properly. See this section of the setup guide: https://github.com/MarkUsProject/Wiki/blob/master/Developer-Guide--Set-Up-With-Docker.md#installing-pre-commit-hooks

david-yz-liu · 2024-09-14T18:56:41Z

config/locales/models/students/en.yml

@@ -0,0 +1,6 @@
+---
+en:


This change overall is good, but there's a better (existing) location: locales/views/users/en.yml. There's already a students key there you can add onto (but the errors is good).

While you've correctly noted that the text is used in a model method, in general most user-facing strings are put under views; the exceptions are strings inherent to model the models themselves: class name, attribute names, and validation error messages.

Separately, we already have users which actually combines messages for all of: users, roles, instructors, students, tas. This is more of a historical artifact than anything else.

ohh ok I made the change! out of curiosity, what type of text do the config files in locales/models store?

@anubhajoshi01 I answered this question in my previous comment. See "the exceptions are..."

sorry, i missed that

david-yz-liu · 2024-09-16T03:00:22Z

doc/markus-contributors.txt

@@ -212,3 +212,4 @@ Yujin Cho
 Yusi Fan
 Zachary Munro-Cape
 Ziyuan (Jerry) Zhang
+Anubha Joshi


Make sure to respect the existing order of names

ohh sorry, did you mean alphabetical order?

Yes, that's correct

david-yz-liu · 2024-09-16T03:00:51Z

.pre-commit-config.yaml

@@ -55,3 +55,6 @@ exclude: vendor

 ci:
  autoupdate_schedule: monthly
+


Do not include this in this branch

doc/markus-contributors.txt

for more information, see https://pre-commit.ci

david-yz-liu · 2024-09-18T00:37:21Z

Changelog.md

@@ -16,6 +16,8 @@
 - Upgrade Docker environment to use Ruby v3.3 (#7185)
 - Upgrade to Rails v7.2 (#7185)
 - Manually specify chromedriver port number in Github actions (#7209)
+- Move Exception message in student model to a localization file


@anubhajoshi01 oh I didn't notice this last time, but you should also make sure to add the pull request number here (7218) for both entries. This is the standard practice for this changelog.

I made the change

david-yz-liu · 2024-09-18T01:53:25Z

Great job, @anubhajoshi01 🎉

anubhajoshi01 and others added 2 commits September 14, 2024 11:59

add localization file for students and add test coverage

ddc90f7

[pre-commit.ci] auto fixes from pre-commit.com hooks

9e0e0f3

for more information, see https://pre-commit.ci

anubhajoshi01 commented Sep 14, 2024

View reviewed changes

anubhajoshi01 added 2 commits September 14, 2024 12:41

add to changelog

ac8972d

Merge branch 'add-test-coverage-save-fail-student-model' of https://g…

0407559

…ithub.com/anubhajoshi01/Markus-Anubha into add-test-coverage-save-fail-student-model

anubhajoshi01 marked this pull request as ready for review September 14, 2024 16:46

anubhajoshi01 added 2 commits September 14, 2024 13:13

reame yaml to yml

de00e68

normalized localizatio file

2b7132c

anubhajoshi01 requested a review from david-yz-liu September 14, 2024 18:19

anubhajoshi01 changed the title ~~add localization file for students and add test coverage~~ Move the exceptions raised in student.rb to a localization file, and add test coverage for cases where Group and Grouping save return false Sep 14, 2024

david-yz-liu reviewed Sep 14, 2024

View reviewed changes

anubhajoshi01 added 4 commits September 15, 2024 14:17

use users localization file for exception

59e0c05

use users localization file for exception

a275e2a

restore gemfile lock from master

06ed3b5

restore gemfile lock from master

fbae051

anubhajoshi01 force-pushed the add-test-coverage-save-fail-student-model branch from 86cdd27 to fbae051 Compare September 16, 2024 01:48

anubhajoshi01 added 2 commits September 15, 2024 21:56

untrack vendor files

31f9e01

restore vendor folder from master

5feb5a1

anubhajoshi01 force-pushed the add-test-coverage-save-fail-student-model branch from d77597c to 5feb5a1 Compare September 16, 2024 01:59

anubhajoshi01 added 2 commits September 15, 2024 22:02

restore pre commit config yaml from master

f005f01

add default language version for ruby in pre commt config yaml

bd55c43

david-yz-liu reviewed Sep 16, 2024

View reviewed changes

anubhajoshi01 added 2 commits September 16, 2024 21:07

fix order contributors

0508d39

restore master pre config yaml

b92a873

anubhajoshi01 force-pushed the add-test-coverage-save-fail-student-model branch from b5cae4c to b92a873 Compare September 17, 2024 01:11

resolve changelog conflict

d9226b7

david-yz-liu reviewed Sep 17, 2024

View reviewed changes

doc/markus-contributors.txt Show resolved Hide resolved

fix contributor ordering

2b01e0b

anubhajoshi01 force-pushed the add-test-coverage-save-fail-student-model branch 2 times, most recently from ed84755 to 2b01e0b Compare September 17, 2024 14:27

[pre-commit.ci] auto fixes from pre-commit.com hooks

084bd9f

for more information, see https://pre-commit.ci

david-yz-liu reviewed Sep 18, 2024

View reviewed changes

add PR number to changelog

70449d4

david-yz-liu approved these changes Sep 18, 2024

View reviewed changes

david-yz-liu merged commit db6b76d into MarkUsProject:master Sep 18, 2024
6 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Move the exceptions raised in student.rb to a localization file, and add test coverage for cases where Group and Grouping save return false #7218

Move the exceptions raised in student.rb to a localization file, and add test coverage for cases where Group and Grouping save return false #7218

anubhajoshi01 commented Sep 14, 2024 •

edited

Loading

anubhajoshi01 Sep 14, 2024

anubhajoshi01 left a comment •

edited

Loading

coveralls commented Sep 14, 2024 •

edited

Loading

david-yz-liu left a comment

david-yz-liu Sep 14, 2024

anubhajoshi01 Sep 16, 2024

david-yz-liu Sep 16, 2024

anubhajoshi01 Sep 17, 2024

david-yz-liu Sep 16, 2024

anubhajoshi01 Sep 16, 2024

david-yz-liu Sep 16, 2024

david-yz-liu Sep 16, 2024

david-yz-liu Sep 18, 2024

anubhajoshi01 Sep 18, 2024

david-yz-liu commented Sep 18, 2024

		@@ -55,3 +55,6 @@ exclude: vendor

		ci:
		autoupdate_schedule: monthly

		@@ -0,0 +1,6 @@
		---
		en:

Move the exceptions raised in student.rb to a localization file, and add test coverage for cases where Group and Grouping save return false #7218

Move the exceptions raised in student.rb to a localization file, and add test coverage for cases where Group and Grouping save return false #7218

Conversation

anubhajoshi01 commented Sep 14, 2024 • edited Loading

Proposed Changes

Type of Change

Checklist

Questions and Comments

Choose a reason for hiding this comment

anubhajoshi01 left a comment • edited Loading

Choose a reason for hiding this comment

coveralls commented Sep 14, 2024 • edited Loading

Pull Request Test Coverage Report for Build 10913837694

Details

💛 - Coveralls

david-yz-liu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

david-yz-liu commented Sep 18, 2024

anubhajoshi01 commented Sep 14, 2024 •

edited

Loading

anubhajoshi01 left a comment •

edited

Loading

coveralls commented Sep 14, 2024 •

edited

Loading