Feature TR-4685 NRPS membership difference #136
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
feat: introduce members' soft-delete
wazelin
requested review from
kilatib,
peetya,
poyuki and
mike-ionut-mihai-sandu-tao
kilatib
reviewed
Nov 22, 2022
| array_filter( | ||
| $membership->getMembers()->all(), | ||
| static function (MemberInterface $member) { | ||
| return $member->getStatus() !== MemberInterface::STATUS_DELETED; |
There was a problem hiding this comment.
That filters not deleted ones.
kilatib
reviewed
Nov 22, 2022
| } | ||
|
|
||
| public function __invoke(Request $request, string $membershipIdentifier): Response | ||
| { | ||
| $membership = $this->repository->find($membershipIdentifier); | ||
| $membership = $this->service->findMembership($membershipIdentifier); |
There was a problem hiding this comment.
Here you take only
MemberInterface::STATUS_ACTIVE, MemberInterface::STATUS_INACTIVE
statuses end then the filter deletes, is it ok?
I think you coyuld provide second params and not doit any filtering
There was a problem hiding this comment.
No, that retrieves all records regardless of their state if I'm not mistaken.
kilatib
reviewed
Nov 22, 2022
| } | ||
|
|
||
| public function __invoke(Request $request, string $membershipIdentifier): Response | ||
| { | ||
| $membership = $this->repository->find($membershipIdentifier); | ||
| $membership = $this->service->findMembership($membershipIdentifier); |
There was a problem hiding this comment.
Suggested change
| $membership = $this->service->findMembership($membershipIdentifier); | |
| $membership = $this->service->findMembership($membershipIdentifier, [MemberInterface::STATUS_DELETED]); |
There was a problem hiding this comment.
That would fetch only deleted ones. I don't think this would be intentional.
poyuki
approved these changes
Nov 23, 2022
There was a problem hiding this comment.
- New code is covered by tests (if applicable)
- Tests are running successfully (old and new ones) on my local machine (if applicable)
- New code is respecting code style rules
- New code is respecting best practices
- New code is not subject to concurrency issues (if applicable)
- Feature is working correctly on my local machine (if applicable)
- Acceptance criteria are respected
- Pull request title and description are meaningful
- Pull request's target is not
master - Commits are following conventional commits
- Changelog is updated according to changes (if applicable)
- Documentation is updated according to changes (if applicable)
kilatib
approved these changes
Nov 28, 2022
# for free
to join this conversation on GitHub.
Already have an account?
# to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
TR-4685
Screen.Recording.2022-11-21.at.16.17.18.mov
How to test
rel="difference"valueThis PR aims to bring Membership Differences to the NRPS implementation in DevKit.
This feature is required to re-submit for the LTI Advantage Platform certification.
The validation with the IMS' test tool went well, will merge once the final confirmation from IMS has been received.