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

Display my alerts #170

Merged
merged 15 commits into from
Feb 18, 2021
Merged

Display my alerts #170

merged 15 commits into from
Feb 18, 2021

Conversation

kryswisnaskas
Copy link
Collaborator

@kryswisnaskas kryswisnaskas commented Feb 15, 2021

Description of change
Displays alerts for managers, authors and collaborators. If there are no alerts, an empty area will be displayed with the text "You're all caught up" (per design).

How to test

  • pull changes
  • enter a couple of reports
  • note that the reports show up in both sections, the "My activity report alerts" and "Activity reports"
  • modify .env file to simulate a different user (one that is not a manager or a collaborator)
  • restart the backend server
  • note that the reports only show up in the lower table and there is a "You're all caught up" section.
  • modify .env file to simulate a user that is either a manager or a collaborator on existing reports
  • restart the backend server
  • verify that the corresponding reports are displayed in the

Issue(s)

Checklist

  • Meets issue criteria
  • Code tested
  • Meets accessibility standards (WCAG 2.1 Levels A, AA)
  • Documentation updated

[Op.and]: [
{
status: {
[Op.ne]: 'approved',
Copy link

Choose a reason for hiding this comment

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

Should approved reports be shown on the alerts? It seems like that would get pretty cluttered over time.

Copy link
Collaborator Author

@kryswisnaskas kryswisnaskas Feb 16, 2021

Choose a reason for hiding this comment

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

True. No need to have approved reports.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Hmm...Op.ne (not equal) should take care of not bringing in approved reports. Do you still see 'approved' in the alerts section?

Copy link

Choose a reason for hiding this comment

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

Screenshot from 2021-02-16 13-07-06

This report was filled out and submitted by userid 5 and approved by userid 1. When I log backin as userid 5 it shows up.

Copy link

@jasalisbury jasalisbury left a comment

Choose a reason for hiding this comment

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

Looks good! The striping from the table causes some of the <Tag> elements to not show up very well (see the status column of the first row).
Screenshot from 2021-02-16 10-28-13


const collaboratorsWithTags = collaborators.map((collaborator) => (
<Tag
key={collaborator.fullName.slice(1, 13)}

Choose a reason for hiding this comment

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

Can we use collaborator.id here?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Changed.

<tr key={`my_alerts_${id}`}>
<td>
<Link
to={`/activity-reports/${id}/activity-summary`}

Choose a reason for hiding this comment

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

If the /activity-summary is removed the user will be redirected to the review page if the report is not in draft mode (the report is waiting for approval or "Needs Action")

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Updated.

@kryswisnaskas
Copy link
Collaborator Author

kryswisnaskas commented Feb 16, 2021

The striping from the table causes some of the elements to not show up very well (see the status column of the first row).

Yes, it blends a bit. Double checked the background color. It matches the design, but I'll run this by @arickalewis1

It also looks like needs_action needs fixing.

Copy link
Collaborator Author

@kryswisnaskas kryswisnaskas left a comment

Choose a reason for hiding this comment

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

@dcmcand Good find! I changed the query condition.

Copy link

@dcmcand dcmcand left a comment

Choose a reason for hiding this comment

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

Looks good!

}

MyAlerts.propTypes = {
reports: PropTypes.node.isRequired,
Copy link

Choose a reason for hiding this comment

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

I'm getting this in the console:

Warning: Failed prop type: Invalid prop reportssupplied toMyAlerts, expected a ReactNode

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Changed.

src/services/activityReports.js Outdated Show resolved Hide resolved
{
[Op.and]: [
{
status: { [Op.ne]: 'approved' },
Copy link

Choose a reason for hiding this comment

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

Do any of these status strings exist in a Constant? It'd be good to refer to the constants rather than hard coding in the query.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Good catch!
Fixed.

@kryswisnaskas kryswisnaskas merged commit 54cff22 into main Feb 18, 2021
@kryswisnaskas kryswisnaskas deleted the kw-my-alerts branch February 18, 2021 16:09
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.

4 participants