Skip to content

Conversation

vas3a
Copy link
Collaborator

@vas3a vas3a commented Aug 13, 2025

https://topcoder.atlassian.net/browse/PM-1608 - Issues in WM project search
https://topcoder.atlassian.net/browse/PM-1609 - Add Navigation for User Management in Project Details

@vas3a vas3a requested review from hentrymartin and jmgasper August 13, 2025 18:17
filters['id'] = parseInt(projectNameOrIdFilter, 10)
} else { // text search
filters['keyword'] = decodeURIComponent(projectNameOrIdFilter)
filters['keyword'] = `"${decodeURIComponent(projectNameOrIdFilter)}"`

Choose a reason for hiding this comment

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

Wrapping the decoded project name or ID filter in quotes may affect the search functionality. Ensure that the search backend can handle quoted keywords correctly, as this might change the search results or cause unexpected behavior.

perPage: 20,
page: 1,
keyword
keyword: `"${keyword}"`

Choose a reason for hiding this comment

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

Wrapping the keyword in quotes might lead to unexpected search results if the search system interprets the quotes literally. Ensure that the search backend correctly handles quoted keywords or consider removing the quotes if they are not necessary.

type: PropTypes.string.isRequired,
text: PropTypes.string.isRequired,
link: PropTypes.string,
link: PropTypes.oneOfType([PropTypes.string, PropTypes.object]),

Choose a reason for hiding this comment

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

The change from PropTypes.string to PropTypes.oneOfType([PropTypes.string, PropTypes.object]) for the link prop suggests that link can now be either a string or an object. Ensure that the component logic correctly handles both types, especially if the object type is new and requires additional handling or validation.

<OutlineButton
text={'Users'}
type='info'
submit

Choose a reason for hiding this comment

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

The submit attribute in the OutlineButton component seems to be used incorrectly here. Typically, submit is used for form submission buttons, but this button appears to be a navigation button. Consider removing the submit attribute if it's not necessary for navigation.

paddingLeft: '10px',
border: 'none',
width: '100%',
display: 'grid',

Choose a reason for hiding this comment

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

Consider using display: flex instead of display: grid if the layout does not require grid-specific features. Flexbox might be more appropriate for simpler layouts.

<PrimaryButton
text={'Go To Project'}
type={'info'}
link={`/projects/${projectOption.value}/challenges`} />

Choose a reason for hiding this comment

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

The link prop used in the PrimaryButton component suggests that it might be intended for navigation. Ensure that the PrimaryButton component properly handles navigation if link is used, or consider using a different component designed for navigation, such as a Link component from a routing library.

export const FILE_PICKER_UPLOAD_TIMEOUT = 30 * 60 * 1000 // 30 minutes
export const SPECIFICATION_ATTACHMENTS_FOLDER = 'SPECIFICATION_ATTACHMENTS'
export const MEMBERS_API_URL = process.env.MEMBERS_API_URL
export const MEMBERS_API_URL = process.env.MEMBER_API_URL

Choose a reason for hiding this comment

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

The environment variable name has been changed from MEMBERS_API_URL to MEMBER_API_URL. Ensure that this change is intentional and that the new environment variable MEMBER_API_URL is correctly set in all environments where this code will run.

isAdmin: false,
isLoadingProject: false
isLoadingProject: false,
project: props.location.state && props.location.state.projectId ? {

Choose a reason for hiding this comment

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

Consider simplifying the conditional logic by using optional chaining and nullish coalescing. For example: project: props.location.state?.projectId ? { id: props.location.state.projectId, name: props.location.state.projectName } : null.

isAdmin
})

if (location.state && location.state.projectId) {

Choose a reason for hiding this comment

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

Consider checking if location.state.projectId is a valid and expected type before calling this.loadProject. This can help prevent potential errors if projectId is not a valid identifier.

isSearchingUserProjects
} = this.props
const {
project,

Choose a reason for hiding this comment

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

It seems like the project variable is added to the destructuring assignment, but it's not clear if it's being used in the component. Ensure that project is necessary and utilized in the component logic to avoid unused variables.

}

export default connect(mapStateToProps, mapDispatchToProps)(Users)
export default withRouter(connect(mapStateToProps, mapDispatchToProps)(Users))

Choose a reason for hiding this comment

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

Suggestion

Consider verifying that the withRouter higher-order component is necessary for the Users component. If routing props are not utilized within Users, adding withRouter may be redundant and could be removed to simplify the component.

@jmgasper jmgasper merged commit 1b79e45 into master Aug 13, 2025
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants