PM-1188 copilot opportunities on challenge feed #7094
Conversation
add support for fetching and displaying copilot opportunities within the challenge feed, including dynamic sorting and pagination handling. refs pm-1188
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| * @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 could prevent potential runtime errors if the API response changes.
| 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.
The fireErrorMessage function is called with error.content || error. Ensure that error.content is a valid property and consider logging the entire error object for better debugging information.
| import './style.scss'; | ||
|
|
||
| const PROJECT_TYPE_LABELS = { | ||
| dev: 'Development', |
There was a problem hiding this comment.
Consider adding a default label for project types that are not listed in PROJECT_TYPE_LABELS to handle unexpected values gracefully.
src/shared/components/challenge-listing/CopilotOpportunityCard/index.jsx
Show resolved
Hide resolved
|
|
||
| <div styleName="right-panel"> | ||
| <div styleName="type"> | ||
| <span>{PROJECT_TYPE_LABELS[opportunity.type]}</span> |
There was a problem hiding this comment.
Consider adding a fallback for PROJECT_TYPE_LABELS[opportunity.type] in case opportunity.type is not a key in PROJECT_TYPE_LABELS to prevent rendering undefined.
| } | ||
|
|
||
| CopilotOpportunityCard.propTypes = { | ||
| opportunity: PT.shape().isRequired, |
There was a problem hiding this comment.
The opportunity prop type should be more specific. Define the shape with expected properties and their types to improve type safety and documentation.
| 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 work as intended. Consider using calc() for arithmetic operations in CSS, like calc(#{$challenge-space-10} + 2px).
| 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 min-width property value $challenge-space-50 + 2 might not be calculated correctly. Consider using calc() for arithmetic operations, like calc(#{$challenge-space-50} + 2px).
|
|
||
| function CopilotOpportunityHeader() { | ||
| return ( | ||
| <div styleName="copilotOpportunityHeader"> |
There was a problem hiding this comment.
Consider using a more descriptive class name for copilotOpportunityHeader to avoid potential conflicts with other styles and to improve readability.
| function CopilotOpportunityHeader() { | ||
| return ( | ||
| <div styleName="copilotOpportunityHeader"> | ||
| <div styleName="left-panel"> |
There was a problem hiding this comment.
The left-panel class name could be more descriptive to better convey its purpose or content.
| </div> | ||
| </div> | ||
|
|
||
| <div styleName="right-panel"> |
There was a problem hiding this comment.
Consider using a more descriptive class name for right-panel to improve clarity and maintainability.
| </div> | ||
|
|
||
| <div styleName="right-panel"> | ||
| <div styleName="type"> |
There was a problem hiding this comment.
The type class name is quite generic. Consider using a more specific name to avoid potential conflicts and improve readability.
| <div styleName="type"> | ||
| <span>Type</span> | ||
| </div> | ||
| <div styleName="status"> |
There was a problem hiding this comment.
The status class name is very generic. Consider using a more descriptive name to avoid potential conflicts and improve maintainability.
| <div styleName="status"> | ||
| <span>Status</span> | ||
| </div> | ||
| <div styleName="numHours"> |
There was a problem hiding this comment.
Consider using a more descriptive class name for numHours to better convey its purpose and improve code readability.
| @@ -0,0 +1,93 @@ | |||
| @import '~styles/mixins'; | |||
|
|
|||
| $challenge-space-20: $base-unit * 4; | |||
There was a problem hiding this comment.
Consider renaming $challenge-space-20 to something more descriptive, such as $challenge-padding-small, to better convey its purpose.
| @import '~styles/mixins'; | ||
|
|
||
| $challenge-space-20: $base-unit * 4; | ||
| $challenge-space-30: $base-unit * 6; |
There was a problem hiding this comment.
Consider renaming $challenge-space-30 to something more descriptive, such as $challenge-padding-large, to better convey its purpose.
| .copilotOpportunityHeader { | ||
| @include roboto-medium; | ||
|
|
||
| display: flex; |
There was a problem hiding this comment.
The justify-content: flex-start; property is redundant here since flex-start is the default value for justify-content. Consider removing it for cleaner code.
| padding: $challenge-space-20 0; | ||
| color: $tc-gray-60; | ||
| font-size: 12px; | ||
| margin-left: 24px; |
There was a problem hiding this comment.
The margin-left: 24px; might cause alignment issues on smaller screens. Ensure this is the intended behavior or consider using a responsive unit.
| .left-panel { | ||
| display: flex; | ||
| justify-content: flex-start; | ||
| width: 45.5%; |
There was a problem hiding this comment.
The width of 45.5% for .left-panel might lead to layout issues when combined with the 50% width of .right-panel. Ensure this is the intended design or adjust the widths to fit within 100%.
| .challenge-details { | ||
| display: inline-block; | ||
| vertical-align: baseline; | ||
| width: 82%; |
There was a problem hiding this comment.
The width of 82% for .challenge-details might cause layout issues when combined with the margin-right. Ensure this is the intended design or adjust the width to fit within the container.
| @@ -216,6 +217,7 @@ export default function FiltersPanel({ | |||
| /> | |||
| </div> | |||
| </div> | |||
| { !isCopilotOpportunitiesBucket && ( | |||
There was a problem hiding this comment.
It seems like the closing parenthesis for the conditional rendering is missing. Ensure that the JSX block is properly closed with a parenthesis to avoid syntax errors.
| @@ -257,8 +259,9 @@ export default function FiltersPanel({ | |||
| </div> | |||
| </div> | |||
| </div> | |||
| )} | |||
|
|
|||
There was a problem hiding this comment.
The closing parenthesis for the conditional rendering block seems to be misplaced. Ensure that the conditional logic is correctly structured to avoid rendering issues.
| @@ -456,7 +459,7 @@ export default function FiltersPanel({ | |||
| ) | |||
| } | |||
| </div> | |||
|
|
|||
| { !isCopilotOpportunitiesBucket && ( | |||
There was a problem hiding this comment.
The condition !isCopilotOpportunitiesBucket is added here to wrap the buttons div. Ensure that this condition is necessary and correctly implemented to avoid hiding buttons unintentionally when isCopilotOpportunitiesBucket is true.
| // import { challenge as challengeUtils } from 'topcoder-react-lib'; | ||
| import CopilotOpportunityHeader from 'components/challenge-listing/CopilotOpportunityHeader'; | ||
| import CardPlaceholder from '../../placeholders/ChallengeCard'; | ||
| import CopilotOpportunityCard from '../../CopilotOpportunityCard'; // <== Replace with your actual Copilot Card component |
There was a problem hiding this comment.
The comment // <== Replace with your actual Copilot Card component suggests that CopilotOpportunityCard might be a placeholder or temporary component. Ensure that this is the intended component for production use.
| sort, | ||
| setSearchText, | ||
| }) { | ||
| if (!opportunities.length && !loadMore) return null; |
There was a problem hiding this comment.
The condition if (!opportunities.length && !loadMore) return null; might cause the component to return null even when loading is true. Consider revising this logic if you want to show a loading state when there are no opportunities but data is still being fetched.
| {cards} | ||
| {!loading && filteredOpportunities.length === 0 && ( | ||
| <div> | ||
| <div styleName="copilot-opportunity-bucket"> |
There was a problem hiding this comment.
The nested SortingSelectBar within the !loading && filteredOpportunities.length === 0 condition seems redundant since a similar component is rendered above. Consider removing this duplication unless there is a specific reason to render it twice.
| CopilotOpportunityBucket.propTypes = { | ||
| bucket: PT.string.isRequired, | ||
| challengesUrl: PT.string.isRequired, | ||
| expandedTags: PT.arrayOf(PT.number), |
There was a problem hiding this comment.
The expandedTags prop is defined as PT.arrayOf(PT.number), but it defaults to an empty array. Ensure that this type definition aligns with the expected data structure, especially if expandedTags might contain non-numeric values.
| expandedTags: PT.arrayOf(PT.number), | ||
| expandTag: PT.func, | ||
| // filterState: PT.shape().isRequired, | ||
| opportunities: PT.arrayOf(PT.shape()).isRequired, |
There was a problem hiding this comment.
The opportunities prop is defined as PT.arrayOf(PT.shape()).isRequired, which lacks specific shape details. Consider defining the expected shape of the objects within the opportunities array for better type checking and documentation.
| margin-left: 0; | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
It is a good practice to ensure that files end with a newline character. This can prevent issues with certain tools and improve readability when concatenating files.
| setSearchText={setSearchText} | ||
| /> | ||
| ); | ||
| } else if (isCopilotOpportunitiesBucket(bucket)) { |
There was a problem hiding this comment.
The function isCopilotOpportunitiesBucket is used here, but it's not clear from the diff whether this function is defined elsewhere or imported. Ensure that this function is properly defined or imported to avoid runtime errors.
| needLoad={needLoad} | ||
| loading={loadingCopilotOpportunities} | ||
| loadMore={loadMoreCopilotOpportunities} | ||
| opportunities={copilotOpportunities} |
There was a problem hiding this comment.
The variable loadMoreCopilotOpportunities is used here, but it's not clear from the diff whether this variable is defined elsewhere or imported. Ensure that this variable is properly defined or imported to avoid runtime errors.
| needLoad={needLoad} | ||
| loading={loadingCopilotOpportunities} | ||
| loadMore={loadMoreCopilotOpportunities} | ||
| opportunities={copilotOpportunities} |
There was a problem hiding this comment.
Consider renaming loadMoreCopilotOpportunities to loadMoreOpportunities for consistency with the naming convention used for loadMoreReviewOpportunities.
| filterState={filterState} | ||
| keepPlaceholders={keepPastPlaceholders} | ||
| needLoad={needLoad} | ||
| loading={loadingCopilotOpportunities} |
There was a problem hiding this comment.
The variable loadingCopilotOpportunities is used here, but it's not clear from the diff whether this variable is defined elsewhere or imported. Ensure that this variable is properly defined or imported to avoid runtime errors.
| needLoad={needLoad} | ||
| loading={loadingCopilotOpportunities} | ||
| loadMore={loadMoreCopilotOpportunities} | ||
| opportunities={copilotOpportunities} |
There was a problem hiding this comment.
The variable copilotOpportunities is used here, but it's not clear from the diff whether this variable is defined elsewhere or imported. Ensure that this variable is properly defined or imported to avoid runtime errors.
| @@ -335,6 +362,9 @@ Listing.propTypes = { | |||
| filterState: PT.shape().isRequired, | |||
| keepPastPlaceholders: PT.bool.isRequired, | |||
| needLoad: PT.bool.isRequired, | |||
| loadingCopilotOpportunities: PT.bool.isRequired, | |||
| loadMoreCopilotOpportunities: PT.func, | |||
| copilotOpportunities: PT.arrayOf(PT.shape()), | |||
There was a problem hiding this comment.
Consider specifying the shape of the objects within the copilotOpportunities array for better type checking and clarity. This will help ensure that all necessary properties are present and correctly typed.
| @@ -377,6 +407,7 @@ const mapStateToProps = (state) => { | |||
| allChallengesLoaded: cl.allChallengesLoaded, | |||
| allPastChallengesLoaded: cl.allPastChallengesLoaded, | |||
| allOpenForRegistrationChallengesLoaded: cl.allOpenForRegistrationChallengesLoaded, | |||
| // allCopilotOpportunitiesLoaded: cl.allCopilotOpportunitiesLoaded, | |||
There was a problem hiding this comment.
The line for allCopilotOpportunitiesLoaded is commented out. If this is intentional, consider removing it if it's not needed, or uncommenting it if it should be part of the functionality.
| @@ -601,6 +606,13 @@ export class ListingContainer extends React.Component { | |||
| ); | |||
| } | |||
|
|
|||
| let loadMoreCopilotOpportunities; | |||
There was a problem hiding this comment.
Consider renaming loadMoreCopilotOpportunities to handleLoadMoreCopilotOpportunities to better convey that this is a function handling an action.
| @@ -601,6 +606,13 @@ export class ListingContainer extends React.Component { | |||
| ); | |||
| } | |||
|
|
|||
| let loadMoreCopilotOpportunities; | |||
| if (!allCopilotOpportunitiesLoaded) { | |||
| loadMoreCopilotOpportunities = () => getCopilotOpportunities( | |||
There was a problem hiding this comment.
It's unclear what the argument 1 + lastRequestedPageOfCopilotOpportunities represents. Ensure that the logic for incrementing the page number is correct and consider using a named constant or variable for clarity.
| @@ -768,6 +784,7 @@ ListingContainer.propTypes = { | |||
| loadingUuid: PT.string.isRequired, | |||
| timestamp: PT.number.isRequired, | |||
| }).isRequired, | |||
| copilotOpportunities: PT.arrayOf(PT.shape()).isRequired, | |||
There was a problem hiding this comment.
The PT.shape() used for copilotOpportunities is currently empty. Consider defining the expected shape of the objects within the copilotOpportunities array to ensure proper type checking and validation.
| @@ -872,8 +894,10 @@ const mapStateToProps = (state, ownProps) => { | |||
| lastRequestedPageOfAllChallenges: cl.lastRequestedPageOfAllChallenges, | |||
| lastRequestedPageOfPastChallenges: cl.lastRequestedPageOfPastChallenges, | |||
| lastRequestedPageOfReviewOpportunities: cl.lastRequestedPageOfReviewOpportunities, | |||
| lastRequestedPageOfCopilotOpportunities: cl.lastRequestedPageOfCopilotOpportunities, | |||
There was a problem hiding this comment.
Consider verifying if lastRequestedPageOfCopilotOpportunities is correctly initialized and used throughout the application to prevent potential undefined behavior.
| // lastUpdateOfActiveChallenges: cl.lastUpdateOfActiveChallenges, | ||
| loadingActiveChallengesUUID: cl.loadingActiveChallengesUUID, | ||
| loadingCopilotOpportunitiesUUID: cl.loadingCopilotOpportunitiesUUID, |
There was a problem hiding this comment.
Ensure that loadingCopilotOpportunitiesUUID is properly managed and updated in the application state to avoid any inconsistencies or errors during data fetching.
| @@ -898,7 +922,8 @@ const mapStateToProps = (state, ownProps) => { | |||
| loading: Boolean(cl.loadingActiveChallengesUUID) | |||
| || Boolean(cl.loadingOpenForRegistrationChallengesUUID) | |||
| || Boolean(cl.loadingMyChallengesUUID) || Boolean(cl.loadingAllChallengesUUID) | |||
| || Boolean(cl.loadingPastChallengesUUID) || cl.loadingReviewOpportunitiesUUID, | |||
| || Boolean(cl.loadingPastChallengesUUID) || cl.loadingReviewOpportunitiesUUID | |||
| || cl.loadingCopilotOpportunitiesUUID, | |||
There was a problem hiding this comment.
Consider checking if cl.loadingCopilotOpportunitiesUUID needs to be wrapped with Boolean() for consistency with the other loading flags.
| getCopilotOpportunities: (page) => { | ||
| const uuid = shortId(); | ||
| dispatch(a.getCopilotOpportunitiesInit(uuid, page)); | ||
| dispatch(a.getCopilotOpportunitiesDone(uuid, page)); |
There was a problem hiding this comment.
It seems that the getCopilotOpportunitiesDone function is being dispatched without a token argument, unlike getReviewOpportunitiesDone which includes it. If authentication is required for fetching copilot opportunities, consider adding the token parameter.
| * @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 rather than returning the current state. This could involve logging the error or updating the state to reflect the error condition.
| if (uuid !== state.loadingCopilotOpportunitiesUUID) return state; | ||
|
|
||
| const ids = new Set(); | ||
| loaded.forEach(item => ids.add(item.id)); |
There was a problem hiding this comment.
Using a Set to track IDs is efficient for ensuring uniqueness. However, consider adding a comment explaining why this approach is chosen, especially if the order of items is important.
| const ids = new Set(); | ||
| loaded.forEach(item => ids.add(item.id)); | ||
| const copilotOpportunities = state.copilotOpportunities | ||
| .filter(item => !ids.has(item.id)) |
There was a problem hiding this comment.
The filter and concat operations could be optimized by using a single reduce operation to build the new array, which might improve performance slightly if the arrays are large.
| * @returns {Promise<Object>} The fetched data. | ||
| */ | ||
| export default function getCopilotOpportunities(page, pageSize = 20, sort = 'createdAt desc') { | ||
| const url = `${v5ApiUrl}/projects/copilots/opportunities?page=${page}&pageSize=${pageSize}&sort=${encodeURIComponent(sort)}`; |
There was a problem hiding this comment.
Consider adding error handling for the fetch request to manage potential network errors or unsuccessful responses. This will improve the robustness of the function.
apply consistent formatting and fix linting errors in all scss files to maintain code quality. no issue
| @@ -102,6 +102,7 @@ module.exports = { | |||
| /* This is the same value as above, but it is used by topcoder-react-lib, | |||
| * as a more verbose name for the param. */ | |||
| COMMUNITY_APP: 'https://community-app.topcoder-dev.com', | |||
| COPILOTS_URL: 'https://copilots.topcoder-dev.com', | |||
There was a problem hiding this comment.
As this is hard-coded value we need to add it also in https://github.com/topcoder-platform/community-app/blob/develop/config/production.js
| const sortedOpportunities = _.clone(opportunities); | ||
| sortedOpportunities.sort(Sort[activeSort].func); | ||
|
|
||
| // const filteredOpportunities = sortedOpportunities.filter( |
There was a problem hiding this comment.
Do we need this?
Let's remove commented out code for going upstream with this.
remove commented code, add prod copilots url. no issue
https://topcoder.atlassian.net/browse/PM-1188
Add support for fetching and displaying copilot opportunities within the challenge feed, including dynamic sorting and pagination handling.
This excludes search over opportunities. We have added sorting over title, project track, and status.
Deafult sort prioritises active oportunities and sorts them on created Date.