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

Add class groups #92

Merged
merged 11 commits into from
Oct 16, 2021
Merged

Add class groups #92

merged 11 commits into from
Oct 16, 2021

Conversation

chaiwanlin
Copy link

closes #25

@chaiwanlin chaiwanlin added this to the v1.2 milestone Oct 14, 2021
@codecov-commenter
Copy link

codecov-commenter commented Oct 14, 2021

Codecov Report

Merging #92 (96c1677) into master (4196474) will decrease coverage by 1.41%.
The diff coverage is 45.12%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master      #92      +/-   ##
============================================
- Coverage     62.27%   60.86%   -1.42%     
- Complexity      458      507      +49     
============================================
  Files            92      100       +8     
  Lines          1654     1855     +201     
  Branches        179      216      +37     
============================================
+ Hits           1030     1129      +99     
- Misses          582      661      +79     
- Partials         42       65      +23     
Impacted Files Coverage Δ
src/main/java/seedu/edrecord/MainApp.java 0.00% <0.00%> (ø)
...java/seedu/edrecord/logic/commands/AddCommand.java 80.00% <0.00%> (-12.31%) ⬇️
...du/edrecord/logic/commands/ListModulesCommand.java 100.00% <ø> (ø)
...eedu/edrecord/logic/commands/MakeGroupCommand.java 0.00% <0.00%> (ø)
...va/seedu/edrecord/logic/parser/EdRecordParser.java 72.72% <0.00%> (-3.47%) ⬇️
...seedu/edrecord/logic/parser/EditCommandParser.java 86.66% <0.00%> (-6.20%) ⬇️
.../edrecord/logic/parser/MakeGroupCommandParser.java 0.00% <0.00%> (ø)
...edrecord/logic/parser/MakeModuleCommandParser.java 0.00% <0.00%> (ø)
src/main/java/seedu/edrecord/model/Model.java 100.00% <ø> (ø)
...odel/group/exceptions/DuplicateGroupException.java 0.00% <0.00%> (ø)
... and 25 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4196474...96c1677. Read the comment docs.

@caipng caipng changed the title Add class Add group feature Oct 16, 2021
@caipng caipng changed the title Add group feature Add groups Oct 16, 2021
@FergusMok
Copy link

There's no checks to create a person to an existing class for now

Sample test case:

  1. add n/Betsy Crowe p/1234567 e/betsycrowe@example.com m/CS2103 c/W-14-3 i/telegram @betyoass t/strong
    (W-14-3 is not held by the Module, but the Person has the class)
  2. add n/Bob p/1234567 e/betsycrowe@example.com m/CS2103 c/W-3 i/telegram @betyoass t/strong
  3. edit c/W-14-3

Error Message: Class with that code has yet to be created.

If we're rushing, we can shift this to another issue in 1.3

@marcusc55 marcusc55 added the v1.2 label Oct 16, 2021
@chaiwanlin
Copy link
Author

chaiwanlin commented Oct 16, 2021

There's no checks to create a person to an existing class for now

Sample test case:

  1. add n/Betsy Crowe p/1234567 e/betsycrowe@example.com m/CS2103 c/W-14-3 i/telegram @betyoass t/strong
    (W-14-3 is not held by the Module, but the Person has the class)
  2. add n/Bob p/1234567 e/betsycrowe@example.com m/CS2103 c/W-3 i/telegram @betyoass t/strong
  3. edit c/W-14-3

Error Message: Class with that code has yet to be created.

If we're rushing, we can shift this to another issue in 1.3

@FergusMok These test cases are all under Add..Test or Edit..Test already but I didn't have to change them because the TypicalModules and PersonBuilder takes class into consideration already. However I haven't added test cases for making mods and class I'll put those under v1.3 for now since I might not be able to rush them out yet

Copy link

@marcusc55 marcusc55 left a comment

Choose a reason for hiding this comment

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

LGTM!

@FergusMok
Copy link

@chaiwanlin Sorry I meant non-existing class, I think it's a non-intended bug. I think test cases not super impt for now

Copy link

@caipng caipng left a comment

Choose a reason for hiding this comment

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

Some minor nits.

@caipng caipng changed the title Add groups Add class groups Oct 16, 2021
Copy link

@FergusMok FergusMok left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link

@caipng caipng left a comment

Choose a reason for hiding this comment

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

LGTM!

@caipng caipng merged commit a603690 into AY2122S1-CS2103-W14-3:master Oct 16, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Create class
5 participants