Skip to content
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.

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

Displaying identifiers for project drop downs #1352

Merged
merged 21 commits into from
Aug 23, 2022
Merged
Show file tree
Hide file tree
Changes from 12 commits
Commits
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
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@
* [REST]: Updated synchronizing of sample data to remove sequencing objects and assemblies that no longer exist on the remote sample. See [PR 1345](https://github.com/phac-nml/irida/pull/1345)
* [UI]: Fixed issue with filtering samples by files using a windows encoded text file causing sample name truncation. See [PR 1346](https://github.com/phac-nml/irida/pull/1346)
* [Developer]: Fixed deleting a project with project subscriptions. See [PR 1348](https://github.com/phac-nml/irida/pull/1348)
* [Developer]: Add identifier to project drop-downs. See [PR 1352](https://github.com/phac-nml/irida/pull/1352)

## [22.05.5] - 2022/06/28
* [UI]: Fixed bug preventing export of project samples table due to invalid url. [PR 1331](https://github.com/phac-nml/irida/pull/1331)
Expand Down
3 changes: 3 additions & 0 deletions src/main/resources/i18n/messages.properties
Original file line number Diff line number Diff line change
Expand Up @@ -692,6 +692,9 @@ project.nav.details=Recent Activity
project.nav.settings=Settings
project.nav.samples.import-metadata=Import Sample Metadata

# Project select
ProjectSelect.label.id=ID: {0}

# KEPT FOR LEGACY PURPOSES (still used on user_details.html and groups.html)
project.table.collaborator.name=Name
project.table.collaborator.role=Project Role
Expand Down
80 changes: 80 additions & 0 deletions src/main/webapp/resources/js/components/project/ProjectSelect.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,80 @@
import React from "react";
import { Select, SelectProps, Tag, Typography } from "antd";
import { LabeledValue } from "antd/lib/select";

export type Project = { identifier: number; name: string };
joshsadam marked this conversation as resolved.
Show resolved Hide resolved

export interface ProjectSelectProps extends SelectProps {
projects: Project[];
Copy link
Contributor

Choose a reason for hiding this comment

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

You need to describe all you parameters

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Reached out about this comment. Instructed to disregard.

Copy link
Contributor

Choose a reason for hiding this comment

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

I though you had decided to go with ProjectMinimal here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Please see 76ba866.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

After generalising the component, I think picking id and name form IridaBase makes the most sense now. Please see 9942971.

}

/**
* React component for displaying a project drop-down menu.
* @param projects - list of projects that is to be displayed
* @param onChange - function that is called when select option has changed
* @param defaultValue - project identifier of the project that is to be displayed by default
* @constructor
*/
export function ProjectSelect({
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder we could generalize this to be a select for anything that has an id?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think so... I just need a good name. Please see d19ce2b.

projects,
onChange = () => {},
Copy link
Contributor

Choose a reason for hiding this comment

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

I would just not set a default value for the onChange since it really cannot function without one.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Please see 8023bed.

defaultValue = null,
}: ProjectSelectProps): JSX.Element {
const [options, setOptions] = React.useState<LabeledValue[]>(() =>
formatOptions(projects)
);

function formatOptions(values: Project[]) {
if (!values) return [];
return values.map((project) => ({
label: (
<div
style={{
display: "flex",
justifyContent: "space-between",
width: "100%",
}}
>
<Typography.Text ellipsis={{ tooltip: true }}>
{project.name}
</Typography.Text>
<Tag>{i18n("ProjectSelect.label.id", project.identifier)}</Tag>
</div>
),
value: project.identifier,
selected: project.name,
}));
}

React.useEffect(() => {
setOptions(formatOptions(projects));
}, [projects]);

const handleSearch = (value: string) => {
const lowerValue = value.toLowerCase();

const available = projects.filter(
(project) =>
project.name.toLowerCase().includes(lowerValue) ||
project.identifier.toString() === value
);
const formatted = formatOptions(available);
setOptions(formatted);
};

return (
<Select
optionLabelProp="selected"
autoFocus
showSearch
size="large"
style={{ width: `100%` }}
options={options}
className="t-project-select"
filterOption={false}
onSearch={handleSearch}
onChange={onChange}
defaultValue={defaultValue}
/>
);
}
45 changes: 6 additions & 39 deletions src/main/webapp/resources/js/pages/projects/share/ShareProject.jsx
Original file line number Diff line number Diff line change
@@ -1,7 +1,8 @@
import { Select, Space, Typography } from "antd";
import { Space, Typography } from "antd";
import React from "react";
import { useDispatch, useSelector } from "react-redux";
import { ProjectSelect } from "../../../components/project/ProjectSelect";
import { setProject } from "./shareSlice";
import { useDispatch, useSelector } from "react-redux";

/**
* React component for selecting the project to share a sample with.
Expand All @@ -12,33 +13,6 @@ import { setProject } from "./shareSlice";
export function ShareProject({ projects }) {
const dispatch = useDispatch();
const { targetProject } = useSelector((state) => state.shareReducer);
const [options, setOptions] = React.useState(() => formatOptions(projects));

function formatOptions(values) {
if (!values) return [];
return values.map((project) => ({
label: project.name,
value: project.identifier,
}));
}

React.useEffect(() => {
setOptions(
projects.map((project) => ({
label: project.name,
value: project.identifier,
}))
);
}, [projects]);

const handleSearch = (value) => {
const lowerValue = value.toLowerCase();
const available = projects.filter((project) =>
project.name.toLowerCase().includes(lowerValue)
);
const formatted = formatOptions(available);
setOptions(formatted);
};

function onChange(projectId) {
const project = projects.find((p) => p.identifier === projectId);
Expand All @@ -50,17 +24,10 @@ export function ShareProject({ projects }) {
<Typography.Title level={5}>
{i18n("ShareSamples.projects")}
</Typography.Title>
<Select
autoFocus
showSearch
size="large"
style={{ width: `100%` }}
options={options}
className="t-share-project"
filterOption={false}
onSearch={handleSearch}
<ProjectSelect
projects={projects}
onChange={onChange}
defaultValue={targetProject ? targetProject.identifier : null}
defaultValue={targetProject?.identifier}
/>
</Space>
);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@
import static ca.corefacility.bioinformatics.irida.ria.integration.pages.AbstractPage.waitForTime;

public class ShareSamplesPage {
@FindBy(css = ".t-share-project .ant-select-selection-search-input")
@FindBy(css = ".t-project-select .ant-select-selection-search-input")
private WebElement shareProjectSelect;

@FindBy(className = "ant-select-dropdown")
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you add testing for searching for a project by it's identifier please

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Please see 3aa96c6.

Expand Down Expand Up @@ -147,15 +147,15 @@ public boolean isShareSingleSuccessDisplayed() {
public boolean isSomeSamplesSameIdsWarningDisplayed() {
try {
return someSamplesSameIdsWarning.isDisplayed();
} catch(Exception e) {
} catch (Exception e) {
return false;
}
}

public boolean isSomeSamplesSameNamesWarningDisplayed() {
try {
return someSamplesSameNamesWarning.isDisplayed();
} catch(Exception e) {
} catch (Exception e) {
return false;
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,8 @@ public void testShareSamplesAsManager() {
samplesPage.shareSamples();

assertFalse(shareSamplesPage.isNextButtonEnabled(), "");
shareSamplesPage.searchForProject("2");
Copy link
Contributor

Choose a reason for hiding this comment

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

This does not really count as searching for a project by it's identifier since the project name also contains a 2. What I would like to see is a completely different name from the identifier and confirm that the id is present in the tag.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Please see 0d340a1.

assertTrue(shareSamplesPage.isNextButtonEnabled(), "Next button should be enabled");
shareSamplesPage.searchForProject("project2");
assertTrue(shareSamplesPage.isNextButtonEnabled(), "Next button should be enabled");
shareSamplesPage.gotToNextStep();
Expand Down Expand Up @@ -55,7 +57,8 @@ public void testShareSamplesAsManager() {
assertTrue(shareSamplesPage.isSomeSamplesSameIdsWarningDisplayed(),
"Should display an expandable warning which lists the samples that will not be copied over as the samples with the same identifiers already exist in the target project");
shareSamplesPage.expandSameSampleIdsWarning();
assertEquals(1, shareSamplesPage.numberOfSamplesWithSameIds(), "There should be one sample listed which exists in the target project with the same identifier");
assertEquals(1, shareSamplesPage.numberOfSamplesWithSameIds(),
"There should be one sample listed which exists in the target project with the same identifier");

assertFalse(shareSamplesPage.isSomeSamplesSameNamesWarningDisplayed(),
"Shouldn't display an expandable warning which lists the samples that will not be copied over as the samples with the same names but different identifiers exist in the target project");
Expand Down Expand Up @@ -112,6 +115,7 @@ public void testShareSamplesAsManager() {
assertTrue(shareSamplesPage.isSomeSamplesSameNamesWarningDisplayed(),
"Should display an expandable warning which lists the samples that will not be copied over as the samples with the same names but different identifiers exist in the target project");
shareSamplesPage.expandSameSampleNamesWarning();
assertEquals(1, shareSamplesPage.numberOfSamplesWithSameNames(), "There should be one sample listed which exists in the target project with the same name and different identifier");
assertEquals(1, shareSamplesPage.numberOfSamplesWithSameNames(),
"There should be one sample listed which exists in the target project with the same name and different identifier");
}
}