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.

Sign up for GitHub

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

Jump to bottom

Add delete attendanceRecord command #152

Merged
merged 18 commits into from
Oct 18, 2020
Merged

Add delete attendanceRecord command #152

merged 18 commits into from
Oct 18, 2020

Conversation

junlong4321
Copy link

Resolves #138

@junlong4321
Add preliminary delete command code
0311804
@junlong4321
Merge branch 'master' of https://github.com/AY2021S1-CS2103T-T10-4/tp …
b8dd0d1 
…into delete-attendancerecord-command

# Conflicts:
#	src/main/java/seedu/address/commons/core/Messages.java
#	src/main/java/seedu/address/logic/parser/CliSyntax.java

Resolved by including conflicted lines into code
@junlong4321
Add test cases
a1a6db4
@junlong4321
Merge branch 'master' of https://github.com/AY2021S1-CS2103T-T10-4/tp …
d7bf7c6 
…into delete-attendancerecord-command
@junlong4321
Introduce delete-attendance command
3569ff6
@junlong4321
Merge branch 'master' of https://github.com/AY2021S1-CS2103T-T10-4/tp …
0d9ec4d 
…into delete-attendancerecord-command
@junlong4321 junlong4321 requested review from dextertanyj, a team and ruixuantan and removed request for a team October 17, 2020 13:45
@codecov-io
Copy link

codecov-io commented Oct 17, 2020 •
edited

Codecov Report

Merging #152 into master will increase coverage by 0.68%.
The diff coverage is 88.48%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master     #152      +/-   ##
============================================
+ Coverage     80.80%   81.48%   +0.68%     
- Complexity      937      953      +16     
============================================
  Files           138      142       +4     
  Lines          2745     2808      +63     
  Branches        321      324       +3     
============================================
+ Hits           2218     2288      +70     
- Misses          440      441       +1     
+ Partials         87       79       -8
Impacted Files Coverage Δ Complexity Δ
...ddress/logic/commands/DeleteAttendanceCommand.java 80.55% <80.55%> (ø) 8.00 <8.00> (?)
...ddress/logic/util/ModuleClassModificationUtil.java 87.50% <87.50%> (ø) 11.00 <11.00> (?)
...edu/address/logic/util/LessonModificationUtil.java 92.30% <92.30%> (ø) 5.00 <5.00> (?)
...ss/logic/parser/DeleteAttendanceCommandParser.java 94.11% <94.11%> (ø) 3.00 <3.00> (?)
...u/address/logic/commands/AddAttendanceCommand.java 92.10% <100.00%> (+8.52%) 13.00 <0.00> (-3.00) ⬆️
...seedu/address/logic/commands/AddLessonCommand.java 94.73% <100.00%> (+8.52%) 8.00 <0.00> (-2.00) ⬆️
...du/address/logic/commands/DeleteLessonCommand.java 95.45% <100.00%> (+17.67%) 9.00 <0.00> (-2.00) ⬆️
...eedu/address/logic/commands/EditLessonCommand.java 97.05% <100.00%> (+5.29%) 12.00 <0.00> (-5.00) ⬆️
...va/seedu/address/logic/parser/TutorsPetParser.java 100.00% <100.00%> (ø) 31.00 <0.00> (+1.00)
... and 6 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 32abd0a...d57909b. Read the comment docs.

dextertanyj
dextertanyj requested changes Oct 17, 2020
View reviewed changes
Copy link

@dextertanyj dextertanyj left a comment

Choose a reason for hiding this comment

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

Nice work! Looks good to me overall, just requires some small changes.

src/main/java/seedu/address/logic/commands/DeleteAttendanceCommand.java Outdated Show resolved Hide resolved
src/main/java/seedu/address/logic/parser/DeleteAttendanceCommandParser.java Outdated Show resolved Hide resolved
src/test/java/seedu/address/logic/commands/DeleteAttendanceCommandTest.java Outdated Show resolved Hide resolved
ruixuantan
ruixuantan reviewed Oct 18, 2020
View reviewed changes
Copy link

@ruixuantan ruixuantan left a comment

Choose a reason for hiding this comment

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

Apart from the suggestions, looks good overall!

src/main/java/seedu/address/logic/commands/DeleteAttendanceCommand.java Outdated Show resolved Hide resolved
src/main/java/seedu/address/logic/commands/DeleteAttendanceCommand.java Outdated Show resolved Hide resolved
src/test/java/seedu/address/logic/commands/DeleteAttendanceCommandTest.java Outdated Show resolved Hide resolved
src/main/java/seedu/address/logic/commands/DeleteAttendanceCommand.java Outdated Show resolved Hide resolved
dextertanyj
dextertanyj reviewed Oct 18, 2020
View reviewed changes
Copy link

@dextertanyj dextertanyj left a comment

Choose a reason for hiding this comment

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

Overall looks good! Just mainly needs one important change for checking student existence and the rest are all minor suggestions for documentation.

src/main/java/seedu/address/logic/commands/DeleteAttendanceCommand.java Outdated Show resolved Hide resolved
src/main/java/seedu/address/logic/commands/DeleteAttendanceCommand.java Show resolved Hide resolved
src/main/java/seedu/address/logic/commands/DeleteAttendanceCommand.java Outdated Show resolved Hide resolved
src/main/java/seedu/address/logic/util/LessonModificationUtil.java Outdated Show resolved Hide resolved
src/main/java/seedu/address/logic/util/LessonModificationUtil.java Outdated Show resolved Hide resolved
src/main/java/seedu/address/logic/util/ModuleClassModificationUtil.java Outdated Show resolved Hide resolved
src/main/java/seedu/address/logic/util/ModuleClassModificationUtil.java Outdated Show resolved Hide resolved
src/main/java/seedu/address/logic/util/ModuleClassModificationUtil.java Outdated Show resolved Hide resolved
src/main/java/seedu/address/logic/util/ModuleClassModificationUtil.java Outdated Show resolved Hide resolved
src/main/java/seedu/address/logic/util/ModuleClassModificationUtil.java Outdated Show resolved Hide resolved
dextertanyj
dextertanyj approved these changes Oct 18, 2020
View reviewed changes
Copy link

@dextertanyj dextertanyj left a comment

Choose a reason for hiding this comment

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

LGTM!

@junlong4321
Merge branch 'master' of https://github.com/AY2021S1-CS2103T-T10-4/tp …
462791a 
…into delete-attendancerecord-command

# Conflicts:
#	src/main/java/seedu/address/logic/parser/TutorsPetParser.java
#	src/test/java/seedu/address/logic/commands/AddAttendanceCommandTest.java
#	src/test/java/seedu/address/logic/parser/TutorsPetParserTest.java

Resolved
@junlong4321
Remove extra imports
d57909b
ruixuantan
ruixuantan approved these changes Oct 18, 2020
View reviewed changes
Copy link

@ruixuantan ruixuantan left a comment

Choose a reason for hiding this comment

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

LGTM!

@junlong4321 junlong4321 merged commit 0e0dc6e into AY2021S1-CS2103T-T10-4:master Oct 18, 2020
@junlong4321 junlong4321 modified the milestones: v1.3, v1.3-core Oct 19, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dextertanyj dextertanyj dextertanyj approved these changes

@ruixuantan ruixuantan ruixuantan approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
v1.3-core
Development

Successfully merging this pull request may close these issues.

Implement command and parser support for delete attendenceRecord
4 participants
@junlong4321 @codecov-io @dextertanyj @ruixuantan