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

#165053196 admin is able to view department details. #244

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from

Conversation

gbols
Copy link
Collaborator

@gbols gbols commented Jun 25, 2019

What does this PR do?

View Assets assigned to a department.

Description of Task to be completed?

Allow Admin to view departments details or appropriate message.

How should this be manually tested?

Login with an admin email(The properties of this user should read as folllows: admin: true, superuser: false)
Visit Home -> Departments

What are the relevant pivotal tracker stories?

#165053196

Any background context you want to add?

N/A

Important notes

N/A

Packages installed

N/A

Deployment note

Related PRs branch | PR (branch_name) | (pr_link)

Branch PR
other_pr_production link
other_pr_master link

Todos

  • Raise PR
  • Test
  • Documentation

Screenshots

@HawiCaesar HawiCaesar temporarily deployed to art-dashboard-staging-pr-244 June 25, 2019 15:04 Inactive
@gbols gbols force-pushed the ft-department-assets-165053196 branch from 0da71c2 to 337cebb Compare June 25, 2019 16:08
@HawiCaesar HawiCaesar temporarily deployed to art-dashboard-staging-pr-244 June 25, 2019 16:08 Inactive
@gbols gbols requested review from fojuri and skboadu June 25, 2019 16:09
@coveralls
Copy link

coveralls commented Jun 25, 2019

Coverage Status

Coverage increased (+0.004%) to 86.89% when pulling bbd4a9f on ft-department-assets-165053196 into 366a69b on develop.

Copy link
Collaborator

@fojuri fojuri left a comment

Choose a reason for hiding this comment

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

You also need to update the getAssignedAssets().
The Test Coverage decreased. Update the test as well.

@@ -19,7 +19,7 @@ export class DepartmentDetailComponent extends React.Component {


assetsAssigned = (assets) => {
if (isEmpty(assets.assets_assigned)) {
if (isEmpty(assets.assets_assigned.result)) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

This should be assets.assets_assigned.results

@gbols gbols force-pushed the ft-department-assets-165053196 branch from 337cebb to 08ce4ea Compare June 26, 2019 10:19
@HawiCaesar HawiCaesar temporarily deployed to art-dashboard-staging-pr-244 June 26, 2019 10:19 Inactive
@@ -15,11 +15,11 @@ export class DepartmentDetailComponent extends React.Component {
}

getAssignedAssets = assets =>
this.props.getAssetsSuccess({ results: assets.assets_assigned, count: assets.assets_assigned.length }, '');
this.props.getAssetsSuccess({ results: assets.assets_assigned.results, count: assets.assets_assigned.results.length }, '');
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
this.props.getAssetsSuccess({ results: assets.assets_assigned.results, count: assets.assets_assigned.results.length }, '');
this.props.getAssetsSuccess({ results: assets.assets_assigned.results, count: assets.assets_assigned.count }, '');

</Header>
<Header>
Total Assets Assigned: {details.assets_assigned.length}
Total Assets Assigned: {assets_assigned.results.length}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
Total Assets Assigned: {assets_assigned.results.length}
Total Assets Assigned: {assets_assigned.count}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants