[PROD RELEASE] - Q2 #7101
[PROD RELEASE] - Q2 #7101
Conversation
add support for fetching and displaying copilot opportunities within the challenge feed, including dynamic sorting and pagination handling. refs pm-1188
apply consistent formatting and fix linting errors in all scss files to maintain code quality. no issue
remove commented code, add prod copilots url. no issue
PM-1188 copilot opportunities on challenge feed
Removed the unused search bar from the opportunities section and corrected minor typos in labels and headings. Fixes PM-1188
do not drop the copilot opportunities on bucket change
…urls PM-967 - expose "TOPGEAR_ALLOWED_SUBMISSIONS_DOMAINS" to app
…urls PM-967 - keep env var as string
fix(PM-1311): canceled status color
Grab the Topgear submission changes
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| import { errors, services } from 'topcoder-react-lib'; | ||
| import { BUCKETS } from 'utils/challenge-listing/buckets'; | ||
| import SORT from 'utils/challenge-listing/sort'; | ||
| import getCopilotOpportunities from '../../services/copilotOpportunities'; |
There was a problem hiding this comment.
Consider verifying if the getCopilotOpportunities function is used within this file after being imported. If it is not used, the import statement may be unnecessary and could be removed to keep the code clean.
| * @return {Promise<{uuid: string, loaded: object}>} Action result | ||
| */ | ||
| function getCopilotOpportunitiesDone(uuid, page) { | ||
| return getCopilotOpportunities(page, COPILOT_OPPORTUNITY_PAGE_SIZE) |
There was a problem hiding this comment.
Consider handling the case where getCopilotOpportunities might return a non-object or unexpected data structure. This can prevent potential runtime errors when destructuring or accessing properties of loaded.
| return getCopilotOpportunities(page, COPILOT_OPPORTUNITY_PAGE_SIZE) | ||
| .then(loaded => ({ uuid, loaded })) | ||
| .catch((error) => { | ||
| fireErrorMessage('Error Getting Copilot Opportunities', error.content || error); |
There was a problem hiding this comment.
Ensure that fireErrorMessage is defined and properly handles the error.content or error object. It might be beneficial to log additional error details for debugging purposes.
| } | ||
|
|
||
| CopilotOpportunityCard.propTypes = { | ||
| opportunity: PT.shape().isRequired, |
There was a problem hiding this comment.
The opportunity prop type should be more specific to ensure the component receives the expected structure. Consider defining the shape with expected properties like id, name, type, status, numHoursPerWeek, skills, and startDate.
| display: inline-block; | ||
| font-size: 13px; | ||
| font-weight: 500; | ||
| color: green; |
There was a problem hiding this comment.
Avoid using hard-coded color values like green. Consider using a variable from your color palette for consistency and easier maintenance.
| font-size: 12px; | ||
| color: $tc-gray-60; | ||
| margin-right: $challenge-space-10; | ||
| line-height: ($challenge-space-10) + 2; |
There was a problem hiding this comment.
The line-height calculation ($challenge-space-10) + 2 might not be necessary. Consider using a consistent unit or variable for line-height to maintain uniformity.
| color: green; | ||
| line-height: $challenge-space-20; | ||
| margin-right: $challenge-space-20; | ||
| min-width: $challenge-space-50 + 2; |
There was a problem hiding this comment.
The calculation for min-width using $challenge-space-50 + 2 could lead to unexpected results. Ensure that the addition is intentional and that it aligns with the design requirements.
|
|
||
| function CopilotOpportunityHeader() { | ||
| return ( | ||
| <div styleName="copilotOpportunityHeader"> |
There was a problem hiding this comment.
Consider using a more descriptive class name for styleName="copilotOpportunityHeader" to ensure clarity and maintainability, especially if this component is part of a larger system.
| return ( | ||
| <div styleName="copilotOpportunityHeader"> | ||
| <div styleName="left-panel"> | ||
|
|
There was a problem hiding this comment.
The styleName="left-panel" class name might be too generic. Consider using a more specific name to avoid potential conflicts with other components.
| </div> | ||
| </div> | ||
|
|
||
| <div styleName="right-panel"> |
There was a problem hiding this comment.
The styleName="right-panel" class name might be too generic. Consider using a more specific name to avoid potential conflicts with other components.
| padding: $challenge-space-20 0; | ||
| color: $tc-gray-60; | ||
| font-size: 12px; | ||
| margin-left: 24px; |
There was a problem hiding this comment.
Consider using a variable for the margin-left value to maintain consistency and ease of maintenance.
| .left-panel { | ||
| display: flex; | ||
| justify-content: flex-start; | ||
| width: 45.5%; |
There was a problem hiding this comment.
The width of 45.5% might lead to layout issues due to potential rounding errors in some browsers. Consider using a more standard value or ensuring it aligns well with other elements.
| .right-panel { | ||
| display: flex; | ||
| justify-content: space-between; | ||
| width: 50%; |
There was a problem hiding this comment.
Consider using a variable for the width value to maintain consistency and ease of maintenance.
| margin-right: $challenge-space-30; | ||
|
|
||
| @include md { | ||
| margin-right: 180px; |
There was a problem hiding this comment.
Consider using a variable for the margin-right value to maintain consistency and ease of maintenance.
| } | ||
|
|
||
| .type { | ||
| width: 40%; |
There was a problem hiding this comment.
Consider using a variable for the width value to maintain consistency and ease of maintenance.
| } | ||
|
|
||
| .status { | ||
| width: 30%; |
There was a problem hiding this comment.
Consider using a variable for the width value to maintain consistency and ease of maintenance.
| } | ||
|
|
||
| .numHours { | ||
| width: 30%; |
There was a problem hiding this comment.
Consider using a variable for the width value to maintain consistency and ease of maintenance.
| /> | ||
| </div> | ||
| </div> | ||
| { !isCopilotOpportunitiesBucket && ( |
There was a problem hiding this comment.
Consider adding a descriptive variable name for isCopilotOpportunitiesBucket to improve readability and maintainability of the code. This will help other developers understand the purpose of this condition without needing to look up its definition.
| )} | ||
|
|
||
| { !isReviewOpportunitiesBucket | ||
| { !isReviewOpportunitiesBucket && !isCopilotOpportunitiesBucket |
There was a problem hiding this comment.
Consider adding a comment explaining the logic behind the condition !isReviewOpportunitiesBucket && !isCopilotOpportunitiesBucket to ensure future maintainability and clarity for other developers.
| } | ||
| </div> | ||
|
|
||
| { !isCopilotOpportunitiesBucket && ( |
There was a problem hiding this comment.
The condition !isCopilotOpportunitiesBucket is added here, but it's not clear from the diff whether this condition is necessary or if it could potentially hide the buttons when they should be displayed. Ensure that this condition aligns with the intended functionality and does not inadvertently restrict access to the buttons.
| auth: PT.shape().isRequired, | ||
| // isSavingFilter: PT.bool, | ||
| isReviewOpportunitiesBucket: PT.bool, | ||
| isCopilotOpportunitiesBucket: PT.bool, |
There was a problem hiding this comment.
The new prop isCopilotOpportunitiesBucket is added to FiltersPanel.propTypes, but there is no description or context provided for its use. Ensure that this prop is being used correctly in the component logic and that its purpose is clear from the implementation.
| const sortedOpportunities = _.clone(opportunities); | ||
| sortedOpportunities.sort(Sort[activeSort].func); | ||
|
|
||
| const filteredOpportunities = sortedOpportunities; |
There was a problem hiding this comment.
The variable filteredOpportunities is simply assigned the value of sortedOpportunities without any filtering logic. If filtering is intended, consider implementing the filtering logic or remove the unnecessary variable.
| {cards} | ||
| {!loading && filteredOpportunities.length === 0 && ( | ||
| <div> | ||
| <div styleName="copilot-opportunity-bucket"> |
There was a problem hiding this comment.
The nested div with styleName="copilot-opportunity-bucket" seems redundant as it duplicates the outer div with the same style. Consider removing the nested div to simplify the structure.
| challengesUrl: PT.string.isRequired, | ||
| expandedTags: PT.arrayOf(PT.number), | ||
| expandTag: PT.func, | ||
| // filterState: PT.shape().isRequired, |
There was a problem hiding this comment.
The filterState prop type is commented out. If this prop is not needed, consider removing it entirely. If it is needed, uncomment and define the shape properly.
| display: flex; | ||
| align-items: center; | ||
| justify-content: center; | ||
| background-color: $tc-white; |
There was a problem hiding this comment.
The background-color property is set to $tc-white, which is the same as the parent .copilot-opportunity-bucket. Consider removing it to avoid redundancy unless there's a specific reason to override the parent's background.
| color: $tco-black; | ||
| padding-top: 80px; | ||
| border-top: 2px solid $listing-gray; | ||
| border-radius: 1px; |
There was a problem hiding this comment.
The border-radius is set to 1px, which might not be visually noticeable. Consider increasing the value if a rounded effect is intended.
| import { connect } from 'react-redux'; | ||
| import { | ||
| BUCKETS, isReviewOpportunitiesBucket, NO_LIVE_CHALLENGES_CONFIG, | ||
| BUCKETS, isReviewOpportunitiesBucket, isCopilotOpportunitiesBucket, NO_LIVE_CHALLENGES_CONFIG, |
There was a problem hiding this comment.
The import statement for isCopilotOpportunitiesBucket has been added, but ensure that it is actually used in the code. If it's not used, consider removing it to keep the code clean.
| // import { challenge as challengeUtils } from 'topcoder-react-lib'; | ||
| import Bucket from './Bucket'; | ||
| import ReviewOpportunityBucket from './ReviewOpportunityBucket'; | ||
| import CopilotOpportunityBucket from './CopilotOpportunityBucket'; |
There was a problem hiding this comment.
The import statement for CopilotOpportunityBucket has been added. Verify that this component is utilized in the code. If it is not used, it should be removed to avoid unnecessary imports.
| setSearchText={setSearchText} | ||
| /> | ||
| ); | ||
| } else if (isCopilotOpportunitiesBucket(bucket)) { |
There was a problem hiding this comment.
The new CopilotOpportunityBucket component is introduced here. Ensure that this component is properly defined and imported in the file. If it is a new component, verify that it has been tested and functions as expected.
| filterState={filterState} | ||
| keepPlaceholders={keepPastPlaceholders} | ||
| needLoad={needLoad} | ||
| loading={loadingCopilotOpportunities} |
There was a problem hiding this comment.
The variable loadingCopilotOpportunities is used here. Make sure this variable is defined and correctly managed in the component's state or props.
| keepPlaceholders={keepPastPlaceholders} | ||
| needLoad={needLoad} | ||
| loading={loadingCopilotOpportunities} | ||
| loadMore={loadMoreCopilotOpportunities} |
There was a problem hiding this comment.
The function loadMoreCopilotOpportunities is used here. Ensure that this function is defined and correctly implemented to handle loading more copilot opportunities.
| needLoad={needLoad} | ||
| loading={loadingCopilotOpportunities} | ||
| loadMore={loadMoreCopilotOpportunities} | ||
| opportunities={copilotOpportunities} |
There was a problem hiding this comment.
The variable copilotOpportunities is used here. Verify that this variable is defined and contains the expected data structure for copilot opportunities.
| keepPastPlaceholders: PT.bool.isRequired, | ||
| needLoad: PT.bool.isRequired, | ||
| loadingCopilotOpportunities: PT.bool.isRequired, | ||
| loadMoreCopilotOpportunities: PT.func, |
There was a problem hiding this comment.
The loadMoreCopilotOpportunities prop is not marked as required. If this is intentional, consider providing a default value to ensure consistent behavior.
| needLoad: PT.bool.isRequired, | ||
| loadingCopilotOpportunities: PT.bool.isRequired, | ||
| loadMoreCopilotOpportunities: PT.func, | ||
| copilotOpportunities: PT.arrayOf(PT.shape()), |
There was a problem hiding this comment.
The copilotOpportunities prop is defined as an array of shapes, but the shape is not specified. Consider defining the shape of the objects within the array for better type checking and clarity.
| allChallengesLoaded: cl.allChallengesLoaded, | ||
| allPastChallengesLoaded: cl.allPastChallengesLoaded, | ||
| allOpenForRegistrationChallengesLoaded: cl.allOpenForRegistrationChallengesLoaded, | ||
| // allCopilotOpportunitiesLoaded: cl.allCopilotOpportunitiesLoaded, |
There was a problem hiding this comment.
It seems like the line allCopilotOpportunitiesLoaded: cl.allCopilotOpportunitiesLoaded, is commented out. If this is intentional for debugging or temporary removal, ensure to track this change for future reference. If it is not needed, consider removing it entirely to keep the code clean.
| activeBucket={activeBucket} | ||
| auth={props.auth} | ||
| challenges={challenges} | ||
| copilotOpportunities={props.copilotOpportunities} |
There was a problem hiding this comment.
Ensure that props.copilotOpportunities is being passed correctly and is defined in the parent component. If copilotOpportunities is a new prop, verify that it is handled appropriately within the ChallengeListing component to avoid potential runtime errors.
| auth: null, | ||
| // communityFilter: null, | ||
| communityName: null, | ||
| copilotOpportunities: [], |
There was a problem hiding this comment.
Consider checking if copilotOpportunities should have a default value of [] or if it should be null to align with other default props that are set to null.
| loadMoreMy: null, | ||
| loadMoreMyPast: null, | ||
| loadMoreAll: null, | ||
| loadMoreCopilotOpportunities: null, |
There was a problem hiding this comment.
Ensure that loadMoreCopilotOpportunities is being used correctly in the component logic, as it is added here as a default prop.
| activeBucket: PT.string.isRequired, | ||
| expanding: PT.bool, | ||
| challenges: PT.arrayOf(PT.shape()).isRequired, | ||
| copilotOpportunities: PT.arrayOf(PT.shape()), |
There was a problem hiding this comment.
The prop type PT.shape() is used here, which means the shape of the objects in the copilotOpportunities array is not defined. Consider specifying the shape of the objects for better type checking and clarity.
| placeholder={isForReviewOpportunities ? 'Search Review Opportunities' : searchPlaceholderText} | ||
| query={searchText} | ||
| /> | ||
| !isForCopilotOpportunities |
There was a problem hiding this comment.
Consider using a more descriptive variable name for isForCopilotOpportunities to clearly convey its purpose and improve code readability.
| placeholder={isForReviewOpportunities ? 'Search Review Opportunities' : searchPlaceholderText} | ||
| query={searchText} | ||
| /> | ||
| !isForCopilotOpportunities |
There was a problem hiding this comment.
The conditional rendering logic for ChallengeSearchBar could be simplified by using a ternary operator directly in the return statement, which might enhance readability.
| selectedCommunityId={selectedCommunityId} | ||
| loadMorePast={loadMorePast} | ||
| loadMoreReviewOpportunities={loadMoreReviewOpportunities} | ||
| loadMoreCopilotOpportunities={loadMoreCopilotOpportunities} |
There was a problem hiding this comment.
Ensure that loadMoreCopilotOpportunities is defined and properly passed as a prop to the component. Check if it is being used correctly within the component logic.
| loadingUuid: PT.string.isRequired, | ||
| timestamp: PT.number.isRequired, | ||
| }).isRequired, | ||
| copilotOpportunities: PT.arrayOf(PT.shape()).isRequired, |
There was a problem hiding this comment.
The copilotOpportunities prop is defined as an array of shapes, but the shape is not specified. Consider defining the shape of the objects within the array to ensure type safety and clarity.
| lastRequestedPageOfAllChallenges: PT.number.isRequired, | ||
| lastRequestedPageOfPastChallenges: PT.number.isRequired, | ||
| lastRequestedPageOfReviewOpportunities: PT.number.isRequired, | ||
| lastRequestedPageOfCopilotOpportunities: PT.number.isRequired, |
There was a problem hiding this comment.
Consider ensuring that lastRequestedPageOfCopilotOpportunities is being used appropriately in the component logic, as it has been added as a required prop.
| lastRequestedPageOfCopilotOpportunities: PT.number.isRequired, | ||
| // lastUpdateOfActiveChallenges: PT.number.isRequired, | ||
| loadingActiveChallengesUUID: PT.string.isRequired, | ||
| loadingCopilotOpportunitiesUUID: PT.string.isRequired, |
There was a problem hiding this comment.
Ensure that loadingCopilotOpportunitiesUUID is being utilized correctly in the component logic, as it has been added as a required prop.
| return { | ||
| auth: state.auth, | ||
| // allActiveChallengesLoaded: cl.allActiveChallengesLoaded, | ||
| allCopilotOpportunitiesLoaded: cl.allCopilotOpportunitiesLoaded, |
There was a problem hiding this comment.
It seems like allCopilotOpportunitiesLoaded is being added to the props. Ensure that this property is being used in the component, otherwise it might be unnecessary.
| allReviewOpportunitiesLoaded: cl.allReviewOpportunitiesLoaded, | ||
| filter: cl.filter, | ||
| challenges: cl.challenges, | ||
| copilotOpportunities: cl.copilotOpportunities, |
There was a problem hiding this comment.
The copilotOpportunities property is being added to the props. Make sure that this new property is utilized within the component to avoid unused props.
| dispatch(a.getReviewOpportunitiesDone(uuid, page, token)); | ||
| }, | ||
| getCopilotOpportunities: (page) => { | ||
| const uuid = shortId(); |
There was a problem hiding this comment.
The shortId() function is used here to generate a UUID. Ensure that shortId() is imported and defined correctly in the scope of this file.
| getCopilotOpportunities: (page) => { | ||
| const uuid = shortId(); | ||
| dispatch(a.getCopilotOpportunitiesInit(uuid, page)); | ||
| dispatch(a.getCopilotOpportunitiesDone(uuid, page)); |
There was a problem hiding this comment.
The getCopilotOpportunitiesDone function is called without a token parameter, unlike getReviewOpportunitiesDone. Verify if this is intentional or if a token is required for this dispatch as well.
| * @return {Object} New state | ||
| */ | ||
| function onGetCopilotOpportunitiesDone(state, { payload, error }) { | ||
| if (error) return state; |
There was a problem hiding this comment.
Consider handling the error case more explicitly. Currently, the function returns the unchanged state if there's an error, but it might be beneficial to log the error or update the state to reflect that an error occurred.
|
|
||
| if (uuid !== state.loadingCopilotOpportunitiesUUID) return state; | ||
|
|
||
| const ids = new Set(); |
There was a problem hiding this comment.
The use of Set is appropriate for ensuring unique IDs, but consider adding a comment to explain why duplicates might occur in loaded and why it's necessary to filter them out.
| myPastChallenges: [], | ||
| openForRegistrationChallenges: [], | ||
| pastChallenges: [], | ||
| // copilotOpportunities: [], |
There was a problem hiding this comment.
The line for copilotOpportunities is commented out. If this is intentional for debugging or temporary removal, consider adding a TODO or tracking this change elsewhere to ensure it is revisited later.
| lastRequestedPageOfAllChallenges: -1, | ||
| lastRequestedPageOfRecommendedChallenges: -1, | ||
| lastRequestedPageOfPastChallenges: -1, | ||
| // lastRequestedPageOfCopilotOpportunities: -1, |
There was a problem hiding this comment.
It seems like the line lastRequestedPageOfCopilotOpportunities: -1, has been commented out. If this is intentional and the functionality is no longer needed, consider removing the line entirely to keep the code clean. If it's commented out for debugging or temporary reasons, ensure to track it for future cleanup.
| * @param {string} sort - Sort order (e.g., 'createdAt desc'). | ||
| * @returns {Promise<Object>} The fetched data. | ||
| */ | ||
| export default function getCopilotOpportunities(page, pageSize = 20, sort = 'createdAt desc') { |
There was a problem hiding this comment.
Consider adding input validation for the page, pageSize, and sort parameters to ensure they are of the expected types and within valid ranges. This can help prevent potential errors or unexpected behavior when the function is used.
|
|
||
| return fetch(url, { | ||
| method: 'GET', | ||
| }).then(res => res.json()); |
There was a problem hiding this comment.
It's a good practice to handle potential errors from the fetch call. Consider adding error handling to manage cases where the network request fails or the response is not in the expected format.
| name: 'Review start date', | ||
| }, | ||
| [SORTS.COPILOT_OPPORTUNITIES_STATUS]: { | ||
| func: (a, b) => a.status.localeCompare(b.status), |
There was a problem hiding this comment.
Consider handling the case where a.status or b.status might be undefined or null to prevent potential runtime errors.
| name: 'Status', | ||
| }, | ||
| [SORTS.COPILOT_OPPORTUNITIES_TITLE_A_TO_Z]: { | ||
| func: (a, b) => a.project.name.localeCompare(b.project.name), |
There was a problem hiding this comment.
Consider handling the case where a.project.name or b.project.name might be undefined or null to prevent potential runtime errors.
| name: 'Title A-Z', | ||
| }, | ||
| [SORTS.COPILOT_OPPORTUNITIES_TYPE]: { | ||
| func: (a, b) => a.type.localeCompare(b.type), |
There was a problem hiding this comment.
Consider handling the case where a.type or b.type might be undefined or null to prevent potential runtime errors.
| }, | ||
| [SORTS.COPILOT_OPPORTUNITIES_START_DATE]: { | ||
| func: (a, b) => { | ||
| const statusPriority = status => (status === 'active' ? 1 : 0); |
There was a problem hiding this comment.
Consider handling the case where a.status or b.status might be undefined or null to prevent potential runtime errors.
| const statusDiff = statusPriority(b.status) - statusPriority(a.status); | ||
| if (statusDiff !== 0) return statusDiff; | ||
| // Then: sort by createdAt descending | ||
| return moment(b.createdAt) - moment(a.createdAt); |
There was a problem hiding this comment.
Consider handling the case where a.createdAt or b.createdAt might be undefined or null to prevent potential runtime errors.
No description provided.