Conversation
jmgasper
commented
Mar 10, 2026
- Engagements v3 updates
- Fun challenge flag display
- wiproAllowed flag
What was broken\nMarathon tournament fun challenges still rendered the standard "1st " header prize display on challenge details, which implies a direct payout that does not exist.\n\nRoot cause\nHeader prize rendering had no branch for a fun challenge flag and always rendered placement prize cards.\n\nWhat was changed\nAdded fun-challenge-aware rendering in header prizes to show "No individual prize - leaderboard scoring" when the flag is true, wired funChallenge from header props, and added matching styles.\n\nAny added/updated tests\nAdded header prize component tests and snapshots for both fun-challenge and standard prize rendering paths.
What was broken:\nCommunity App allowed @wipro.com users to start challenge registration flow even when a challenge disallowed Wipro participants.\n\nRoot cause:\nThe challenge registration entrypoint in challenge detail did not check the new challenge-level wiproAllowed flag against the logged-in member email domain before continuing.\n\nWhat was changed:\nAdded a Wipro eligibility helper in challenge detail, wired the registration click handler to block when email is @wipro.com and wiproAllowed is false, and surfaced the required user message in CA.\n\nAny added/updated tests:\nAdded unit tests for the Wipro registration guard helper covering blocked and allowed scenarios, including case-insensitive domain matching.
What was broken: Some older challenges did not show the Winners tab even when final winners existed. Root cause: Winner filtering in challenge detail accepted only lowercase 'final' (w.type === 'final'). Legacy payloads can contain 'Final', which got filtered out, making winner count zero and hiding the tab. What was changed: Added getDisplayWinners() in challenge detail container to normalize winner type matching case-insensitively for non-task challenges. Replaced inline winners filtering with getDisplayWinners() in the render path so tab visibility uses normalized winners. Any added/updated tests: Updated __tests__/shared/containers/challenge-detail/index.jsx with regression coverage for legacy 'Final' winners on non-task challenges and preserved task challenge behavior.
What was broken: Review opportunity details always fetched /v6/challenges/:id and rejected the page load when that request was forbidden, so grouped challenges in the review feed stayed stuck/loading or showed stale prior details. Root cause: Grouped challenges can expose public review opportunities while blocking direct challenge detail access. The details service treated any non-200 challenge response as fatal. What was changed: Added a fallback path in review opportunity details loading: when challenge details return 403, build challenge details from the review opportunity payload (challenge/challengeData) with safe defaults for id/name/type/phases/terms. Kept the existing challenge API path for normal successful responses and preserved existing error behavior for non-403 failures. Any added/updated tests: Added __tests__/shared/services/reviewOpportunities.js covering successful challenge fetch, 403 fallback behavior, and opportunity-fetch failure handling.
What was broken Open for Registration and My Challenges badges could show counts that did not match rendered cards for users with assigned Task challenges. Root cause The listing filter compared assignee and current user using numeric coercion (Number(memberId) !== Number(userId)). With non-numeric/string IDs this comparison failed and hid valid assigned Task cards. What was changed Updated assigned-task visibility check to compare IDs as strings in challenge listing bucket filtering. Updated related prop types to accept string or numeric user IDs in listing bucket and challenge card components. Any added/updated tests Updated __tests__/shared/components/challenge-listing/Listing/Bucket.jsx with focused tests verifying assigned Task cards are shown when memberId/userId strings match and hidden when they do not.
Hotfix review summations
What was broken Fun challenges in the /home opportunities feed displayed "$0" instead of a fun-specific label. Root cause The dashboard opportunities component always rendered the summed placement prize value and ignored the funChallenge flag. What was changed - Added fun challenge handling in dashboard opportunities prize rendering so challenge.funChallenge === true displays "Fun". - Preserved existing USD/POINT amount rendering for non-fun challenges. - Replaced Array.prototype.flatMap with _.flatMap in this component for Node 10 compatibility. Any added/updated tests - Added __tests__/shared/components/Dashboard/Challenges/index.jsx with regression coverage for fun challenge label rendering and regular prize rendering.
What was broken:\nFun challenges in the challenge listing feed displayed a blank prize amount area.\n\nRoot cause:\nThe prize renderer only handled placement prize sets and returned null when none were present, which is the case for fun challenges.\n\nWhat was changed:\nUpdated getPrizePointsUI to return "Fun" when challenge.funChallenge is true, before placement-prize handling.\n\nAny added/updated tests:\nAdded __tests__/shared/utils/challenge-detail/helper.jsx to verify fun challenges without placement prizes render "Fun".
| ], | ||
| }); | ||
|
|
||
| const text = JSON.stringify(view); |
There was a problem hiding this comment.
[maintainability]
Using JSON.stringify to convert the component's output to a string for assertions may lead to brittle tests. Consider using a more robust method to inspect the rendered output, such as querying the rendered component tree for specific elements or text.
| ], | ||
| }); | ||
|
|
||
| expect(JSON.stringify(view)).toContain('$44'); |
There was a problem hiding this comment.
[maintainability]
Similar to the previous test, using JSON.stringify for assertions can make tests fragile. It's better to directly query the rendered component for specific content to ensure the test is more reliable and less prone to false positives.
| @@ -0,0 +1,30 @@ | |||
| import React from 'react'; | |||
| import Renderer from 'react-test-renderer/shallow'; | |||
There was a problem hiding this comment.
[maintainability]
Consider using @testing-library/react for testing React components instead of react-test-renderer/shallow. It provides more robust testing capabilities and better simulates user interactions.
| }); | ||
| }); | ||
|
|
||
| function countElementsByType(element, type) { |
There was a problem hiding this comment.
[performance]
The countElementsByType function uses recursion to count elements, which could lead to a stack overflow if the component tree is too deep. Consider using an iterative approach to avoid potential performance issues.
| return count; | ||
| } | ||
|
|
||
| test('Shows assigned task when memberId and userId are matching strings', () => { |
There was a problem hiding this comment.
[correctness]
The test case for showing assigned tasks when memberId and userId match is correct, but it would be more robust to also test for cases where memberId is null or undefined to ensure the component handles these edge cases gracefully.
| expect(countElementsByType(renderer.getRenderOutput(), ChallengeCard)).toBe(1); | ||
| }); | ||
|
|
||
| test('Hides assigned task when memberId and userId do not match', () => { |
There was a problem hiding this comment.
[correctness]
The test case for hiding assigned tasks when memberId and userId do not match is correct, but consider adding a test for when memberId is null or undefined to ensure the component behaves as expected in these scenarios.
| @@ -0,0 +1,50 @@ | |||
| import { getDisplayWinners, isWiproRegistrationBlocked } from 'containers/challenge-detail'; | |||
There was a problem hiding this comment.
[correctness]
Consider adding tests for edge cases, such as when the email is null or an empty string, to ensure isWiproRegistrationBlocked handles these scenarios correctly.
|
|
||
| describe('Challenge detail winners filter', () => { | ||
| test('includes legacy winners with "Final" type for non-task challenges', () => { | ||
| const winners = getDisplayWinners({ |
There was a problem hiding this comment.
[correctness]
The test for getDisplayWinners assumes that the type field is case-sensitive. If the type field is intended to be case-insensitive, additional tests should be added to verify this behavior.
| }); | ||
|
|
||
| afterEach(() => { | ||
| global.fetch = originalFetch; |
There was a problem hiding this comment.
[maintainability]
Restoring global.fetch in afterEach might lead to issues if other tests in the suite rely on a mocked fetch. Consider using beforeEach to mock fetch and afterEach to restore it, ensuring isolation between tests.
| terms: [], | ||
| }; | ||
|
|
||
| global.fetch = jest.fn() |
There was a problem hiding this comment.
[💡 readability]
Consider using a more descriptive variable name for global.fetch mocks to improve readability and maintainability. For example, mockFetch could clarify its purpose.
| }); | ||
| }); | ||
|
|
||
| test('rejects when review opportunity request fails', async () => { |
There was a problem hiding this comment.
[correctness]
The test case for a failed review opportunity request should verify that getDetails does not make a second fetch call, ensuring that the function handles early failures correctly.
| async function getEngagementsDone(uuid, page, filters, tokenV3) { | ||
| try { | ||
| const { engagements, meta } = await getEngagements(page, PAGE_SIZE, filters, tokenV3); | ||
| const publicFilters = { ...filters, status: 'OPEN' }; |
There was a problem hiding this comment.
[correctness]
The publicFilters object is created by spreading filters and adding a status property. Ensure that filters does not already contain a status property, as this will overwrite it without warning. Consider adding a check or warning if filters contains a status key to avoid unintended behavior.
| .filter(set => set.type === 'PLACEMENT') | ||
| .flatMap(item => item.prizes); | ||
| const isFunChallenge = challenge.funChallenge === true; | ||
| const placementPrizes = _.flatMap( |
There was a problem hiding this comment.
[💡 performance]
The use of _.flatMap is appropriate here, but consider using native JavaScript methods like Array.prototype.flatMap if you are targeting environments that support it. This can reduce dependency on lodash and improve performance slightly.
| Prizes.propTypes = { | ||
| isFunChallenge: PT.bool, | ||
| pointPrizes: PT.arrayOf(PT.number), | ||
| prizes: PT.arrayOf(PT.shape()), |
There was a problem hiding this comment.
[maintainability]
The prizes prop type is defined as PT.arrayOf(PT.shape()), which does not specify the shape of the objects within the array. Consider defining the shape to ensure that the expected properties (e.g., value, type) are present and correctly typed. This will improve type safety and maintainability.
| <div styleName="prizes-outer-container"> | ||
| <Prizes prizes={prizes && prizes.length ? prizes : [0]} pointPrizes={pointPrizes} /> | ||
| <Prizes | ||
| isFunChallenge={funChallenge === true} |
There was a problem hiding this comment.
[💡 readability]
The isFunChallenge prop is being set using funChallenge === true. Consider using a more concise expression like !!funChallenge to ensure it's a boolean, which also handles cases where funChallenge might be undefined or null.
|
|
||
| color: #2a2a2a; | ||
| font-size: 24px; | ||
| font-weight: 500; |
There was a problem hiding this comment.
[💡 style]
The font-weight property is set to 500, which is a medium weight. Since @include roboto-bold is used, it might be more appropriate to use a font-weight of 700 to ensure consistency with the bold style.
| isMM, | ||
| isRDM, | ||
| prizes, | ||
| isFunChallenge, |
There was a problem hiding this comment.
[maintainability]
The isFunChallenge prop is added but not documented in the component's JSDoc or any other documentation. Ensure that any new props are documented for clarity and maintainability.
|
|
||
| // Handle point prizes on the winners display | ||
| // Hide prize text for fun challenges, as they do not have individual payouts. | ||
| let prizeText = ''; |
There was a problem hiding this comment.
[💡 readability]
The logic for determining prizeText is now nested inside a conditional check for isFunChallenge. Consider extracting this logic into a separate function to improve readability and maintainability.
| sampleWinnerProfile: PT.shape(), | ||
| selectChallengeDetailsTab: PT.func.isRequired, | ||
| userId: PT.number, | ||
| userId: PT.oneOfType([PT.number, PT.string]), |
There was a problem hiding this comment.
[❗❗ correctness]
Changing userId to accept both number and string types could lead to potential issues if the type is not consistently handled throughout the application. Ensure that all usages of userId are updated to handle both types appropriately to avoid type-related bugs.
| && ch.task.isTask | ||
| && ch.task.isAssigned | ||
| && Number(ch.task.memberId) !== Number(userId)) { | ||
| && `${ch.task.memberId}` !== `${userId}`) { |
There was a problem hiding this comment.
[correctness]
Using template literals for type conversion from Number to String is unconventional and may lead to unexpected results if either ch.task.memberId or userId is null or undefined. Consider using String(ch.task.memberId) and String(userId) for clarity and safety.
| setSort: PT.func.isRequired, | ||
| sort: PT.string, | ||
| userId: PT.number, | ||
| userId: PT.oneOfType([PT.number, PT.string]), |
There was a problem hiding this comment.
[maintainability]
Changing userId to accept both number and string types can introduce subtle bugs if not handled carefully throughout the codebase. Ensure that all usages of userId are compatible with both types.
| * @return {Boolean} | ||
| */ | ||
| function isWiproRegistrationBlockedError(error) { | ||
| const title = error && error.title ? error.title : ''; |
There was a problem hiding this comment.
[💡 readability]
Consider using optional chaining (error?.title) to simplify the null check for error and error.title.
| <div> | ||
| {error ? ( | ||
| <Modal theme={{ container: style.container }}> | ||
| <p styleName="title">{error.title}</p> |
There was a problem hiding this comment.
[correctness]
The styleName attribute is used here, which is not a standard React attribute. Ensure that a CSS module or a similar solution is correctly configured to handle this.
| @@ -41,7 +112,7 @@ ErrorMessageContainer.propTypes = { | |||
| clearError: PT.func.isRequired, | |||
| error: PT.shape({ | |||
| title: PT.string.isRequired, | |||
There was a problem hiding this comment.
[correctness]
The details property in error is now optional. Ensure that all parts of the application that rely on error.details handle the case where it might be undefined.
| @include roboto-regular; | ||
|
|
||
| overflow: hidden; | ||
| padding: 8 * $base-unit; |
There was a problem hiding this comment.
[❗❗ correctness]
The padding calculation 8 * $base-unit should be enclosed in parentheses to ensure correct calculation order. Consider using (8 * $base-unit) to avoid potential issues with operator precedence.
| */ | ||
| export function isWiproRegistrationBlocked(email, wiproAllowed) { | ||
| if (wiproAllowed !== false) return false; | ||
| return /@wipro\.com$/i.test(_.trim(email || '')); |
There was a problem hiding this comment.
[security]
The regular expression used here to check for Wipro email addresses is case-insensitive, which is good. However, it might be more robust to use a stricter pattern that ensures the email is fully validated before checking the domain. Consider using a comprehensive email validation function before this check.
| } | ||
|
|
||
| return winners.filter((winner = {}) => { | ||
| const winnerType = _.toLower(_.trim(_.toString(winner.type))); |
There was a problem hiding this comment.
[correctness]
The filtering logic for winners assumes that the type property is always present and correctly formatted. Consider adding a check to ensure winner.type is a string before calling _.toLower and _.trim to avoid potential runtime errors.
| const userEmail = _.get(auth, 'user.email'); | ||
| const wiproAllowed = _.get(challenge, 'wiproAllowed'); | ||
|
|
||
| if (isWiproRegistrationBlocked(userEmail, wiproAllowed)) { |
There was a problem hiding this comment.
[❗❗ correctness]
The function isWiproRegistrationBlocked is used here to determine if a user can register. Ensure that this function is thoroughly tested, especially with edge cases like null or undefined email and wiproAllowed values, to prevent unexpected behavior.
| if (challengeRes.ok) { | ||
| challengeData = await challengeRes.json(); | ||
| } else if (challengeRes.status === 403) { | ||
| challengeData = buildChallengeFallback(opportunityDetails, challengeId); |
There was a problem hiding this comment.
[correctness]
The fallback mechanism using buildChallengeFallback is a good approach for handling 403 errors. However, ensure that the opportunityDetails passed to buildChallengeFallback contains all necessary fields to construct a valid challenge object, as this could lead to incomplete data being used elsewhere.
| * @param {Object} challenge challenge info | ||
| */ | ||
| export function getPrizePointsUI(challenge) { | ||
| if (challenge.funChallenge === true) { |
There was a problem hiding this comment.
[💡 style]
Consider using challenge.funChallenge directly instead of challenge.funChallenge === true for a more concise and idiomatic check.