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

Update DG #240

Merged
merged 8 commits into from
Nov 7, 2020
Merged

Update DG #240

merged 8 commits into from
Nov 7, 2020

Conversation

nweiyue
Copy link

@nweiyue nweiyue commented Nov 6, 2020

No description provided.

@codecov-io
Copy link

Codecov Report

Merging #240 (abc74fa) into master (a1334fa) will increase coverage by 0.06%.
The diff coverage is 0.00%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master     #240      +/-   ##
============================================
+ Coverage     71.89%   71.96%   +0.06%     
- Complexity      989      990       +1     
============================================
  Files           136      136              
  Lines          3010     3010              
  Branches        306      306              
============================================
+ Hits           2164     2166       +2     
+ Misses          754      753       -1     
+ Partials         92       91       -1     
Impacted Files Coverage Δ Complexity Δ
src/main/java/atas/ui/MainWindow.java 0.00% <0.00%> (ø) 0.00 <0.00> (ø)
.../java/atas/model/statistics/SessionStatistics.java 92.45% <0.00%> (-3.78%) 22.00% <0.00%> (-1.00%)
.../java/atas/model/statistics/StudentStatistics.java 95.83% <0.00%> (+8.33%) 20.00% <0.00%> (+2.00%)

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 a1334fa...abc74fa. Read the comment docs.

@erisjacey erisjacey added this to the v1.4 milestone Nov 7, 2020
Copy link

@erisjacey erisjacey left a comment

Choose a reason for hiding this comment

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

LGTM, just a few nitpicks

1. This product should be able to function without being connected to a network.
1. This product should be able to work without requiring an installer.
1. Data generated by the product should be stored locally in human-editable file.
1. This product Should not contain very large file sizes (JAR files - 100Mb, PDF files - 15Mb/file).

Choose a reason for hiding this comment

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

Maybe "Should" should be de-capitalized here?

Copy link
Author

Choose a reason for hiding this comment

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

Good spot! Thank you.

Comment on lines 1210 to 1222
1. Test case: `addstu n/Alvin Boon m/A0123456X e/alvinboon@u.nus.edu`<br>
Expected: A student is added to the bottom of the student list. A success message including the particulars of the added student is shown in the result box.

1. Test case: `addstu n/Cathy Duigan m/A1123456X e/cathyduigan@u.nus.edu t/helpful`<br>
Expected: Similar to previous.

1. Test case: `addstu m/A2123456X n/Elbert Foo e/elbertfoo@u.nus.edu`<br>
Expected: Similar to previous.

1. Test case: `addstu n/Gina Ho m/A3123456X`<br>
Expected: No student is added. Error message indicating an invalid command format is shown in the result box.

1. Test case: `addstu n/Gina Ho e/ginaho@u.nus.edu`<br>

Choose a reason for hiding this comment

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

Maybe pictures could be provided for at least one positive test case and one negative test case each?

Copy link
Author

Choose a reason for hiding this comment

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

Nice suggestion! Will do!

@nweiyue nweiyue merged commit 9a741f2 into AY2021S1-CS2103T-W16-4:master Nov 7, 2020
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.

None yet

3 participants