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 functionality to whitelist and blacklist commands #156

Merged
merged 12 commits into from
Nov 4, 2024

Conversation

Dinoman44
Copy link

@Dinoman44 Dinoman44 commented Nov 3, 2024

Previously, the whitelist and blacklist commands could only whitelist/blacklist clients. Now, their functionality has been extended to all users to also list out all whitelisted/blacklisted clients through these commands.

Fixes #151

Fixes #149

@Dinoman44 Dinoman44 added type.Enhancement An enhancement to an existing story priority.Medium Nice to have labels Nov 3, 2024
@Dinoman44 Dinoman44 added this to the v1.5 milestone Nov 3, 2024
@Dinoman44 Dinoman44 requested a review from a team November 3, 2024 18:52
Copy link

codecov bot commented Nov 3, 2024

Codecov Report

Attention: Patch coverage is 85.39326% with 13 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
.../address/model/person/ArgumentPredicateToFail.java 81.48% 0 Missing and 10 partials ⚠️
...u/address/logic/commands/BlacklistListCommand.java 66.66% 2 Missing ⚠️
...u/address/logic/parser/WhitelistCommandParser.java 88.88% 0 Missing and 1 partial ⚠️
Files with missing lines Coverage Δ Complexity Δ
...seedu/address/logic/commands/BlacklistCommand.java 74.19% <100.00%> (+74.19%) 5.00 <0.00> (+5.00)
...seedu/address/logic/commands/WhitelistCommand.java 78.94% <100.00%> (+78.94%) 7.00 <0.00> (+7.00)
...u/address/logic/commands/WhitelistListCommand.java 100.00% <100.00%> (ø) 2.00 <2.00> (?)
...u/address/logic/parser/BlacklistCommandParser.java 100.00% <100.00%> (+100.00%) 3.00 <0.00> (+3.00)
.../seedu/address/model/person/ArgumentPredicate.java 82.00% <100.00%> (+0.36%) 22.00 <1.00> (+1.00)
...u/address/logic/parser/WhitelistCommandParser.java 84.21% <88.88%> (+84.21%) 6.00 <0.00> (+6.00)
...u/address/logic/commands/BlacklistListCommand.java 66.66% <66.66%> (ø) 2.00 <2.00> (?)
.../address/model/person/ArgumentPredicateToFail.java 81.48% <81.48%> (ø) 24.00 <24.00> (?)

... and 2 files with indirect coverage changes

Copy link

@rahula1008 rahula1008 left a comment

Choose a reason for hiding this comment

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

Few comments, and perhaps add some testcases, but overall looks good 👍

// make the find command do the hard work
return (new FindCommandParser().parse("find cs/blacklisted")).execute(model);
} catch (ParseException pe) {
return null; // this will never happen

Choose a reason for hiding this comment

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

By any chance would it be good to have a good error message/log message anyways? In case find command changes or other code is modified? But for now, I agree, it won't happen.

Copy link
Author

Choose a reason for hiding this comment

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

This portion of the find command is reliant on the ArgumentPredicate being changed, which is highly unlikely. But in the future I will be decoupling the blacklist command from the find command entirely; this was just a quick addition that I did not want to initially spend too much time on, so I could focus on the test cases and the whitelist command.

// here, the predicate dictates that the client status should be blacklisted;
// hence if the predicate fails (ensured by the ArgumentPredicateToFail object),
// the client is not blacklisted => they are whitelisted
model.updateFilteredPersonList(ArgumentPredicateToFail.getNotBlacklistedPredicate());

Choose a reason for hiding this comment

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

Just for my understanding, how come there is a different execution for whitelistcommand and blacklistcommand? Couldn't it be easier if there was a predicate 'getBlacklistedPredicate' that kept things consistent and reduces Blacklist's dependence on other code? That being said, I think this still works 👍

Copy link
Author

Choose a reason for hiding this comment

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

Yep, I will be decoupling the blacklist command to make it work more like the whitelist command later on, that was just an easy temporary fix.

@jktang14
Copy link

jktang14 commented Nov 4, 2024

LGTM

@jktang14 jktang14 merged commit 491d857 into AY2425S1-CS2103T-F14A-3:master Nov 4, 2024
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
priority.Medium Nice to have type.Enhancement An enhancement to an existing story
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Update blacklist command to display the blacklist Blacklist and Whitelist commands have no testcases yet
3 participants