Skip to content
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion .circleci/config.yml
Original file line number Diff line number Diff line change
Expand Up @@ -160,7 +160,7 @@ workflows:
context: org-global
filters: &filters-dev
branches:
only: ["develop", "pm-2917", "points"]
only: ["develop", "pm-2917", "points", "pm-3270"]

# Production builds are exectuted only on tagged commits to the
# master branch.
Expand Down
76 changes: 66 additions & 10 deletions src/components/ChallengeEditor/ChallengeReviewer-Field/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -152,9 +152,9 @@ class ChallengeReviewerField extends Component {
this.loadWorkflows()
}

updateAssignedMembers (challengeResources, challenge) {
updateAssignedMembers (challengeResources, challenge, prevChallenge = null) {
const reviewersWithPhaseName = challenge.reviewers.map(item => {
const phase = challenge.phases && challenge.phases.find(p => p.phaseId === item.phaseId)
const phase = challenge.phases && challenge.phases.find(p => (p.id === item.phaseId) || (p.phaseId === item.phaseId))
Copy link

Choose a reason for hiding this comment

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

[💡 maintainability]
The condition (p.id === item.phaseId) || (p.phaseId === item.phaseId) might be redundant if p.id and p.phaseId are always the same. Ensure that both properties are necessary and distinct. If not, consider simplifying the condition.

return {
...item,
name: phase && phase.name
Expand All @@ -170,22 +170,78 @@ class ChallengeReviewerField extends Component {
})

const assignedMembers = {}

const unchangedReviewers = new Set()
if (prevChallenge && prevChallenge.reviewers) {
const prevReviewers = prevChallenge.reviewers || []
challenge.reviewers.forEach((reviewer, index) => {
const prevReviewer = prevReviewers[index]
Copy link

Choose a reason for hiding this comment

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

[⚠️ correctness]
The logic assumes that prevReviewers and challenge.reviewers have the same order and length. If this assumption is not guaranteed, it could lead to incorrect behavior. Consider using a more robust matching strategy, such as matching by a unique identifier.

if (prevReviewer &&
prevReviewer.phaseId === reviewer.phaseId &&
(reviewer.isMemberReview !== false) &&
(prevReviewer.isMemberReview !== false)) {
unchangedReviewers.add(index)
if (this.state.assignedMembers[index]) {
assignedMembers[index] = [...this.state.assignedMembers[index]]
}
}
})
}

challengeResources.forEach((resource) => {
const indices = reviewerIndex[ResourceToPhaseNameMap[resource.roleName]] || []
const phaseName = ResourceToPhaseNameMap[resource.roleName]
if (!phaseName) return

const indices = reviewerIndex[phaseName] || []

// Distribute resources across all reviewers with the same phase name
indices.forEach((index) => {
if (!assignedMembers[index]) {
assignedMembers[index] = []
const reviewer = challenge.reviewers[index]
if (!reviewer || (reviewer.isMemberReview === false)) return

if (unchangedReviewers.has(index)) {
const existing = assignedMembers[index] || []
const alreadyAssigned = existing.some(m =>
m && (m.userId === resource.memberId || m.handle === resource.memberHandle)
)
if (!alreadyAssigned) {
if (!assignedMembers[index]) {
assignedMembers[index] = []
}
assignedMembers[index].push({
handle: resource.memberHandle,
userId: resource.memberId
})
}
} else {
if (!assignedMembers[index]) {
assignedMembers[index] = []
}
const existing = assignedMembers[index]
const alreadyAssigned = existing.some(m =>
m && (m.userId === resource.memberId || m.handle === resource.memberHandle)
)
if (!alreadyAssigned) {
assignedMembers[index].push({
handle: resource.memberHandle,
userId: resource.memberId
})
}
}
assignedMembers[index].push({
handle: resource.memberHandle,
userId: resource.memberId
})
})
})

// Clean up assignments for reviewers that no longer exist or are no longer member reviewers
Object.keys(assignedMembers).forEach(indexStr => {
const index = parseInt(indexStr, 10)
const reviewer = challenge.reviewers[index]
if (index >= challenge.reviewers.length ||
Copy link

Choose a reason for hiding this comment

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

[💡 readability]
The condition index >= challenge.reviewers.length is redundant because index is derived from Object.keys(assignedMembers), which should not exceed the length of challenge.reviewers. Consider removing this check for clarity.

!reviewer ||
(reviewer.isMemberReview === false)) {
delete assignedMembers[index]
}
})

if (!isEqual(this.state.assignedMembers, assignedMembers)) {
this.setState({
assignedMembers
Expand Down Expand Up @@ -222,7 +278,7 @@ class ChallengeReviewerField extends Component {
})()

if (challenge && this.doUpdateAssignedMembers && reviewersChanged) {
this.updateAssignedMembers(challengeResources, challenge)
this.updateAssignedMembers(challengeResources, challenge, prevChallenge)
}

if (challenge && prevChallenge &&
Expand Down
Loading