Skip to content

[PROD] release - Minor navigation enhancement & search fix #1671

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 15 commits into from
Aug 13, 2025
Merged

[PROD] release - Minor navigation enhancement & search fix #1671

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
15 commits
Select commit Hold shift + click to select a range
15f3784
PM-1608 - update project search: use exact match
vas3a Aug 11, 2025
7a1ed52
PM-1608 - project search: update search input
vas3a Aug 11, 2025
7c76389
PM-1609 - add navigation for user management
vas3a Aug 11, 2025
16fea50
Merge pull request #1663 from topcoder-platform/PM-1608_project-searc…
vas3a Aug 12, 2025
8e91f82
Merge pull request #1664 from topcoder-platform/PM-1609_navigation-fo…
vas3a Aug 12, 2025
9cd8413
Members api fix
vas3a Aug 12, 2025
de10459
Merge pull request #1666 from topcoder-platform/PM-1609_navigation-fo…
vas3a Aug 12, 2025
0110fb3
PM-1609 - update button colors
vas3a Aug 13, 2025
c807031
Merge pull request #1667 from topcoder-platform/PM-1609_navigation-fo…
vas3a Aug 13, 2025
94cdb18
show users button for all users
vas3a Aug 13, 2025
48346d2
Merge pull request #1668 from topcoder-platform/PM-1609_navigation-fo…
vas3a Aug 13, 2025
5fff264
Fix button width & color
vas3a Aug 13, 2025
506c195
Merge pull request #1669 from topcoder-platform/PM-1609_navigation-fo…
vas3a Aug 13, 2025
184e715
make users button available to everyone
vas3a Aug 13, 2025
754a730
Merge pull request #1670 from topcoder-platform/PM-1609_navigation-fo…
vas3a Aug 13, 2025
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 src/actions/projects.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,7 @@ function _loadProjects (projectNameOrIdFilter = '', paramFilters = {}) {
if (!isNaN(projectNameOrIdFilter)) { // if it is number
filters['id'] = parseInt(projectNameOrIdFilter, 10)
} else { // text search
filters['keyword'] = decodeURIComponent(projectNameOrIdFilter)
filters['keyword'] = `"${decodeURIComponent(projectNameOrIdFilter)}"`
Copy link

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.

}
}

Expand Down
2 changes: 1 addition & 1 deletion src/actions/users.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -82,7 +82,7 @@ export function searchUserProjects (isAdmin = true, keyword) {
sort: 'updatedAt desc',
perPage: 20,
page: 1,
keyword
keyword: `"${keyword}"`
Copy link

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.

}
if (!isAdmin) {
filters['memberOnly'] = true
Expand Down
4 changes: 2 additions & 2 deletions src/components/Buttons/OutlineButton/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ const OutlineButton = ({ type, text, link, onClick, url, className, submit, disa

if (!_.isEmpty(link)) {
return (
<Link className={cn(styles.container, styles[type], className)} to={`${link}`}>
<Link className={cn(styles.container, styles[type], className)} to={link}>
<span>{text}</span>
</Link>
)
Expand All @@ -38,7 +38,7 @@ const OutlineButton = ({ type, text, link, onClick, url, className, submit, disa
OutlineButton.propTypes = {
type: PropTypes.string.isRequired,
text: PropTypes.string.isRequired,
link: PropTypes.string,
link: PropTypes.oneOfType([PropTypes.string, PropTypes.object]),
Copy link

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.

url: PropTypes.string,
className: PropTypes.string,
onClick: PropTypes.func,
Expand Down
10 changes: 10 additions & 0 deletions src/components/ChallengesComponent/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -91,6 +91,16 @@ const ChallengesComponent = ({
</div>
{activeProject && activeProject.id && !isReadOnly ? (
<div className={styles.projectActionButtonWrapper}>
<OutlineButton
text={'Users'}
type='info'
submit
Copy link

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.

link={{
pathname: '/users',
state: { projectId: activeProjectId, projectName: activeProject.name }
}}
className={styles.btnOutline}
/>
{isAdminOrCopilot && (
<OutlineButton
text={'Assets Library'}
Expand Down
3 changes: 3 additions & 0 deletions src/components/Select/styles.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,9 @@ export default {
paddingRight: '6px',
paddingLeft: '10px',
border: 'none',
width: '100%',
display: 'grid',
Copy link

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.

gridTemplateColumns: '1fr',
input: {
width: '100% !important',
height: 'auto !important',
Expand Down
4 changes: 1 addition & 3 deletions src/components/Users/Users.module.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,8 +52,6 @@
align-items: center;

input {
max-width: 280px;

@include upto-sm {
display: block;
padding-bottom: 10px;
Expand Down Expand Up @@ -413,7 +411,7 @@
margin-bottom: 20px;
gap: 8px;
> * {
width: 125px;
width: max-content;
}
}

Expand Down
51 changes: 36 additions & 15 deletions src/components/Users/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,14 @@ class Users extends Component {

this.debouncedOnInputChange = _.debounce(this.onInputChange, AUTOCOMPLETE_DEBOUNCE_TIME_MS)
}
componentDidMount () {
if (this.props.initialProject && this.props.initialProject.id) {
this.setProjectOption({
value: this.props.initialProject.id,
label: this.props.initialProject.name
})
}
}

setProjectOption (projectOption) {
this.setState({ projectOption })
Expand Down Expand Up @@ -165,7 +173,8 @@ class Users extends Component {
isLoadingProject
} = this.props
const {
searchKey
searchKey,
projectOption
} = this.state
const projectOptions = ((searchKey ? resultSearchUserProjects : projects) || []).map(p => {
return {
Expand Down Expand Up @@ -204,20 +213,28 @@ class Users extends Component {
</div>
</div>

{
showAddUser && (
<div className={styles.addButtonContainer}>
<PrimaryButton
text={'Add User'}
type={'info'}
onClick={() => this.onAddUserClick()} />
<PrimaryButton
text={'Invite User'}
type={'info'}
onClick={() => this.onInviteUserClick()} />
</div>
)
}
<div className={styles.addButtonContainer}>
{
showAddUser && (
<>
<PrimaryButton
text={'Add User'}
type={'info'}
onClick={() => this.onAddUserClick()} />
<PrimaryButton
text={'Invite User'}
type={'info'}
onClick={() => this.onInviteUserClick()} />
</>
)
}
{projectOption && (
<PrimaryButton
text={'Go To Project'}
type={'info'}
link={`/projects/${projectOption.value}/challenges`} />
Copy link

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.

)}
</div>
{
this.state.showAddUserModal && (
<UserAddModalContent
Expand Down Expand Up @@ -318,6 +335,10 @@ class Users extends Component {
}

Users.propTypes = {
initialProject: PropTypes.shape({
id: PropTypes.string,
name: PropTypes.string
}),
loadProject: PropTypes.func.isRequired,
updateProjectMember: PropTypes.func.isRequired,
removeProjectMember: PropTypes.func.isRequired,
Expand Down
2 changes: 1 addition & 1 deletion src/config/constants.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,7 +59,7 @@ export const FILE_PICKER_PROGRESS_INTERVAL = 100
export const FILE_PICKER_UPLOAD_RETRY = 2
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
Copy link

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.


export const getAWSContainerFileURL = (key) => `https://${FILE_PICKER_CONTAINER_NAME}.s3.amazonaws.com/${key}`

Expand Down
4 changes: 4 additions & 0 deletions src/containers/Projects/styles.module.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,10 @@
border-radius: 3px;
border: 1px solid $light-gray;
background-color: $lighter-gray;

> div {
width: 100%;
}
}

.tcCheckbox {
Expand Down
18 changes: 15 additions & 3 deletions src/containers/Users/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ import React, { Component } from 'react'
import { connect } from 'react-redux'
import _ from 'lodash'
import PT from 'prop-types'
import { withRouter } from 'react-router-dom'
import UsersComponent from '../../components/Users'
import { PROJECT_ROLES } from '../../config/constants'
import { fetchInviteMembers, fetchProjectById } from '../../services/projects'
Expand All @@ -22,7 +23,11 @@ class Users extends Component {
projectMembers: null,
invitedMembers: null,
isAdmin: false,
isLoadingProject: false
isLoadingProject: false,
project: props.location.state && props.location.state.projectId ? {
Copy link

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.

id: props.location.state && props.location.state.projectId,
name: props.location.state && props.location.state.projectName
} : null
}
this.loadProject = this.loadProject.bind(this)
this.updateProjectMember = this.updateProjectMember.bind(this)
Expand All @@ -33,7 +38,7 @@ class Users extends Component {
}

componentDidMount () {
const { token, isLoading, loadAllUserProjects, page } = this.props
const { token, isLoading, loadAllUserProjects, page, location } = this.props
if (!isLoading) {
const isAdmin = checkAdmin(token)
const isManager = checkManager(token)
Expand All @@ -44,6 +49,10 @@ class Users extends Component {
this.setState({
isAdmin
})

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

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.

this.loadProject(location.state.projectId)
}
}
}

Expand Down Expand Up @@ -157,13 +166,15 @@ class Users extends Component {
isSearchingUserProjects
} = this.props
const {
project,
Copy link

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.

projectMembers,
invitedMembers,
isAdmin,
isLoadingProject
} = this.state
return (
<UsersComponent
initialProject={project}
projects={projects}
loadProject={this.loadProject}
updateProjectMember={this.updateProjectMember}
Expand Down Expand Up @@ -201,6 +212,7 @@ const mapStateToProps = ({ users, auth }) => {
}

Users.propTypes = {
location: PT.object.isRequired,
projects: PT.arrayOf(PT.object),
resultSearchUserProjects: PT.arrayOf(PT.object),
auth: PT.object,
Expand All @@ -220,4 +232,4 @@ const mapDispatchToProps = {
loadNextProjects
}

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

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.

Loading