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

Jy/view vacant rooms #79

Merged

Conversation

JingYenLoh
Copy link

@JingYenLoh JingYenLoh commented Oct 12, 2020

Closes #10

This PR adds the ability to view vacant rooms with a flag --vacant.

While working on this, I identified several issues.

  1. The tests were not compiling. I have resolved that.
  2. One of the tests, while passing, makes some assumptions about the equality of the objects involved. I haven't added another check for that. (In fact, I haven't added any tests)

Additionally, we never had a UI for displaying rooms, although the models were mostly working underneath. I've added a tabbed based UI for them. Depending on the command, it should navigate to the correct tab. For example, rooms should automatically switch to the Rooms tab.

image

I'm currently fixing a bug where the Rooms are not saved. I will unmark this as a draft when that is done.
EDIT: I have come up with code that works. However, it involves creating additional classes for serialization, which I don't find ideal (see cbd0b8d). I'd be happy to accept suggestions, although I doubt I can implement it before v1.2 closes.

@JingYenLoh JingYenLoh added this to the v1.2 milestone Oct 12, 2020
@JingYenLoh JingYenLoh added this to To Do in ResiReg Kanban Board via automation Oct 12, 2020
@codecov-io
Copy link

codecov-io commented Oct 12, 2020

Codecov Report

Merging #79 into master will decrease coverage by 6.39%.
The diff coverage is 8.29%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master      #79      +/-   ##
============================================
- Coverage     65.57%   59.18%   -6.40%     
- Complexity      450      454       +4     
============================================
  Files            85       92       +7     
  Lines          1592     1781     +189     
  Branches        182      215      +33     
============================================
+ Hits           1044     1054      +10     
- Misses          486      660     +174     
- Partials         62       67       +5     
Impacted Files Coverage Δ Complexity Δ
.../seedu/resireg/logic/commands/AllocateCommand.java 0.00% <0.00%> (ø) 0.00 <0.00> (ø)
...va/seedu/resireg/logic/commands/CommandResult.java 83.33% <0.00%> (-4.91%) 10.00 <0.00> (ø)
.../seedu/resireg/logic/commands/ListRoomCommand.java 0.00% <0.00%> (ø) 0.00 <0.00> (ø)
.../seedu/resireg/logic/parser/AddressBookParser.java 84.21% <0.00%> (ø) 12.00 <0.00> (ø)
...du/resireg/logic/parser/ListRoomCommandParser.java 0.00% <0.00%> (ø) 0.00 <0.00> (?)
src/main/java/seedu/resireg/model/Model.java 100.00% <ø> (ø) 1.00 <0.00> (ø)
...rc/main/java/seedu/resireg/model/ModelManager.java 82.75% <0.00%> (-4.52%) 23.00 <0.00> (ø)
.../seedu/resireg/storage/JsonAdaptedRoomStudent.java 0.00% <0.00%> (ø) 0.00 <0.00> (?)
.../seedu/resireg/storage/JsonAdaptedStudentRoom.java 0.00% <0.00%> (ø) 0.00 <0.00> (?)
src/main/java/seedu/resireg/ui/MainWindow.java 0.00% <0.00%> (ø) 0.00 <0.00> (ø)
... and 15 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 fc806f9...cbd0b8d. Read the comment docs.

Problem:
There is a cyclic dependency between Room and Student - they contain
each other. This causes serialization and toString to stackoverflow.

Current Solution:
Modify their toString not to contain each other, and create additional
classes for Jackson to Serialize (JsonAdaptedRoomStudent and
JsonAdaptedStudentRoom). This solution isn't ideal, but it produces
working code. I'd be happy to accept alternatives.
@JingYenLoh JingYenLoh marked this pull request as ready for review October 12, 2020 10:22
@JingYenLoh
Copy link
Author

Requesting 2 reviewers for this 😠

Copy link

@chloelee767 chloelee767 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
Collaborator

@mkeoliya mkeoliya left a comment

Choose a reason for hiding this comment

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

LGTM!

@JingYenLoh JingYenLoh merged commit d7f672c into AY2021S1-CS2103-T16-3:master Oct 13, 2020
ResiReg Kanban Board automation moved this from To Do to Done Oct 13, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

As a OHS Admin I can view a list of vacant rooms
4 participants