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 StatusCommand #100

Merged

Conversation

ivanleekk
Copy link

No description provided.

@netlify
Copy link

netlify bot commented Oct 19, 2023

Deploy Preview for shiny-daffodil-45f25f ready!

Name Link
🔨 Latest commit 33c27a5
🔍 Latest deploy log https://app.netlify.com/sites/shiny-daffodil-45f25f/deploys/653615b874bc170008cc8e97
😎 Deploy Preview https://deploy-preview-100--shiny-daffodil-45f25f.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@codecov
Copy link

codecov bot commented Oct 19, 2023

Codecov Report

Merging #100 (bd3e250) into master (f472d4a) will increase coverage by 3.98%.
The diff coverage is 73.46%.

❗ Current head bd3e250 differs from pull request most recent head 33c27a5. Consider uploading reports for the commit 33c27a5 to get more accurate results

@@             Coverage Diff              @@
##             master     #100      +/-   ##
============================================
+ Coverage     70.24%   74.22%   +3.98%     
- Complexity      472      473       +1     
============================================
  Files            81       80       -1     
  Lines          1623     1540      -83     
  Branches        156      148       -8     
============================================
+ Hits           1140     1143       +3     
+ Misses          440      355      -85     
+ Partials         43       42       -1     
Files Coverage Δ
src/main/java/seedu/staffsnap/logic/Messages.java 88.88% <100.00%> (+1.38%) ⬆️
...ava/seedu/staffsnap/model/applicant/Applicant.java 89.09% <100.00%> (+7.27%) ⬆️
...du/staffsnap/logic/parser/ApplicantBookParser.java 71.87% <0.00%> (+2.17%) ⬆️
...rc/main/java/seedu/staffsnap/ui/ApplicantCard.java 0.00% <0.00%> (ø)
...du/staffsnap/logic/parser/StatusCommandParser.java 81.81% <81.81%> (ø)
...n/java/seedu/staffsnap/model/applicant/Status.java 75.00% <75.00%> (ø)
.../seedu/staffsnap/logic/commands/StatusCommand.java 66.66% <66.66%> (ø)

... and 15 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

This was linked to issues Oct 19, 2023
@ivanleekk ivanleekk removed a link to an issue Oct 19, 2023
@ivanleekk ivanleekk marked this pull request as ready for review October 20, 2023 04:39
@ivanleekk ivanleekk added this to the v1.3 milestone Oct 20, 2023
Copy link

@craigtonlian craigtonlian left a comment

Choose a reason for hiding this comment

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

Looks good!

But I think that resetting the status when Edit is called will break some user flow down the road. Let's discuss how to iron this logic out 😃

@@ -109,7 +109,7 @@ private static Applicant createEditedApplicant(
List<Interview> updatedInterviews = editApplicantDescriptor
.getInterviews().orElse(applicantToEdit.getInterviews());

return new Applicant(updatedName, updatedPhone, updatedEmail, updatedPosition, updatedInterviews);
return new Applicant(updatedName, updatedPhone, updatedEmail, updatedPosition, updatedInterviews, null);

Choose a reason for hiding this comment

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

Will setting applicant.status to null every time we edit the Applicant cause the status value to be reset when an Edit command is executed?

Copy link
Author

Choose a reason for hiding this comment

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

have replaced this with a proper integration, so now it will retain its status when edited if untouched

break;
}
}
System.out.println(result);

Choose a reason for hiding this comment

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

Is this print statement intended?

Copy link
Author

Choose a reason for hiding this comment

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

oops this was meant for debugging, has been removed in the latest commit

@@ -46,7 +46,7 @@ public AddCommand parse(String args) throws ParseException {
Position position = ParserUtil.parsePosition(argMultimap.getValue(PREFIX_POSITION).get());
List<Interview> interviewList = new ArrayList<>();

Applicant applicant = new Applicant(name, phone, email, position, interviewList);
Applicant applicant = new Applicant(name, phone, email, position, interviewList, null);

Choose a reason for hiding this comment

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

Refer to above 😄


@Override
public String toString() {
return "StatusCommand{" + "index=" + index + ", status=" + status + '}';

Choose a reason for hiding this comment

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

Perhaps can use ToStringBuilder() to generate the String representation similar to other how the other commands implement toString() 🐶

Copy link
Author

Choose a reason for hiding this comment

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

done too in the latest commit

*/
model.updateFilteredApplicantList(PREDICATE_HIDE_ALL_APPLICANTS);
model.updateFilteredApplicantList(PREDICATE_SHOW_ALL_APPLICANTS);
model.refreshApplicantList();

Choose a reason for hiding this comment

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

Niceeeee ❤️

Copy link

@craigtonlian craigtonlian left a comment

Choose a reason for hiding this comment

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

Looks great! Thanks, Ivan 😄

@craigtonlian craigtonlian merged commit 3fa1722 into AY2324S1-CS2103T-W08-1:master Oct 23, 2023
7 checks passed
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.

Add applicant status command
2 participants