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 reports #150

Merged
merged 40 commits into from
Feb 12, 2021
Merged

Display reports #150

merged 40 commits into from
Feb 12, 2021

Conversation

kryswisnaskas
Copy link
Collaborator

@kryswisnaskas kryswisnaskas commented Feb 5, 2021

Description of change
Reports are displayed under "Activity Reports". In this PR the table contains all reports in smart hub. The UI contains a "New report" button that allows to create a new report. If there are no reports, an empty row will be displayed in the table.

How to test
Local:

  • pull changes
  • yarn deps
  • yarn db:migrate
  • yarn db:seed
  • yarn start:local
  • click on "Activity Reports"
  • verify the landing page has a table with "Activity reports"
  • create a couple of reports
  • verify they are displayed

Docker:

  • same as above, but with the docker commands

Issue(s)

Checklist

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

Note - "Fix Cucumber tests" branch was merged in to potentially avoid cucumber test failures.

src/routes/activityReports/handlers.js Show resolved Hide resolved
frontend/src/pages/Landing/index.js Show resolved Hide resolved
frontend/src/pages/Landing/index.js Outdated Show resolved Hide resolved
frontend/src/pages/Landing/index.js Outdated Show resolved Hide resolved
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!

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.

Nice job Krys!

@rahearn
Copy link

rahearn commented Feb 10, 2021

I'll do an in-depth review of this tomorrow morning, but I wanted to flag that when I run yarn docker:deps it's changing yarn.lock (the backend one). Please update that to be in-sync with package.json

Copy link

@rahearn rahearn left a comment

Choose a reason for hiding this comment

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

Nice stuff, Krys.

There's two minor accessibility concerns I have with the table, but the biggest issue is that I can't actually see the whole table on my laptop:

table_cutoff


function LandingLayout({ children }) {
return (
<Grid row>
Copy link

Choose a reason for hiding this comment

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

Does this need a GridContainer around it?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The idea was to get away from the GridContainer due to layout constraints.

frontend/src/pages/Landing/__tests__/index.js Outdated Show resolved Hide resolved
frontend/src/pages/Landing/index.js Show resolved Hide resolved
frontend/src/pages/Landing/index.js Show resolved Hide resolved
frontend/src/pages/Landing/index.js Outdated Show resolved Hide resolved
src/models/user.js Outdated Show resolved Hide resolved
@kryswisnaskas
Copy link
Collaborator Author

What's the purpose of this test vs. the tests that validate each column individually? This feels like maybe it's over-specified

Removed this test.

Please switch this column to be a

Switched.

This is causing a 500 error when updating users with a null role.

Good catch!
Modified.

when I run yarn docker:deps it's changing yarn.lock (the backend one). Please update that to be in-sync with package.json

Updated.

@kryswisnaskas
Copy link
Collaborator Author

the biggest issue is that I can't actually see the whole table on my laptop:

I was going to handle that in a separate PR, but since it's already an issue, I added scrollbars now. This wasn't an attempt to hide the right side of the table.

Copy link

@rahearn rahearn left a comment

Choose a reason for hiding this comment

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

Code looks good, but the tests failures are consistent for whatever reason. Once that's cleaned up this'll be good to merge

@kryswisnaskas kryswisnaskas merged commit 5896310 into main Feb 12, 2021
@kryswisnaskas kryswisnaskas deleted the kw-display-reports branch February 12, 2021 22:39
dcmcand pushed a commit that referenced this pull request Feb 15, 2021
commit 5896310
Merge: 55a2a37 c0394f1
Author: kryswisnaskas <[email protected]>
Date:   Fri Feb 12 17:39:27 2021 -0500

    Merge pull request #150 from adhocteam/kw-display-reports

    Display reports

commit c0394f1
Merge: 9189f61 55a2a37
Author: kryswisnaskas <[email protected]>
Date:   Fri Feb 12 17:13:40 2021 -0500

    Merge branch 'main' into kw-display-reports

commit 55a2a37
Merge: e985f60 61cb04e
Author: kryswisnaskas <[email protected]>
Date:   Fri Feb 12 17:11:28 2021 -0500

    Merge pull request #163 from adhocteam/kw-secure-cookie

    Add explicit cookie-session options

commit 61cb04e
Merge: 4ba76a9 6740fd5
Author: kryswisnaskas <[email protected]>
Date:   Fri Feb 12 16:56:17 2021 -0500

    Merge branch 'kw-secure-cookie' of https://github.com/adhocteam/Head-Start-TTADP into kw-secure-cookie

commit 4ba76a9
Author: kryswisnaskas <[email protected]>
Date:   Fri Feb 12 16:55:58 2021 -0500

    Remove one of secure cookie settings

commit 6740fd5
Merge: fa3f81e e985f60
Author: kryswisnaskas <[email protected]>
Date:   Fri Feb 12 16:54:01 2021 -0500

    Merge branch 'main' into kw-secure-cookie

commit 9189f61
Merge: c1f7ba4 e985f60
Author: Ryan Ahearn <[email protected]>
Date:   Fri Feb 12 14:23:56 2021 -0500

    Merge branch 'main' into kw-display-reports

commit c1f7ba4
Merge: f3881d1 79a4808
Author: kryswisnaskas <[email protected]>
Date:   Fri Feb 12 12:57:14 2021 -0500

    Merge branch 'main' into kw-display-reports

commit f3881d1
Author: kryswisnaskas <[email protected]>
Date:   Fri Feb 12 12:26:40 2021 -0500

    Fix lint errors

commit 112ccd6
Author: kryswisnaskas <[email protected]>
Date:   Fri Feb 12 11:10:37 2021 -0500

    Update yarn.lock

commit cdc95f1
Merge: 1b1ce8c 8751dcf
Author: kryswisnaskas <[email protected]>
Date:   Fri Feb 12 10:18:13 2021 -0500

    Merge branch 'kw-display-reports' of https://github.com/adhocteam/Head-Start-TTADP into kw-display-reports

commit 1b1ce8c
Author: kryswisnaskas <[email protected]>
Date:   Fri Feb 12 10:17:49 2021 -0500

    Added scrollbars; removed test; modified styles

commit fa3f81e
Author: kryswisnaskas <[email protected]>
Date:   Wed Feb 10 16:22:08 2021 -0500

    Add explicit cookie session options

commit 8751dcf
Merge: 3d8382b c5b47a7
Author: kryswisnaskas <[email protected]>
Date:   Wed Feb 10 12:51:09 2021 -0500

    Merge branch 'main' into kw-display-reports

commit 3d8382b
Author: kryswisnaskas <[email protected]>
Date:   Wed Feb 10 12:25:43 2021 -0500

    Fix merge issue

commit 3d4b1ac
Merge: 6c5e287 16c743f
Author: kryswisnaskas <[email protected]>
Date:   Wed Feb 10 11:50:02 2021 -0500

    Merge branch 'main' of https://github.com/adhocteam/Head-Start-TTADP into kw-display-reports

commit 6c5e287
Author: kryswisnaskas <[email protected]>
Date:   Wed Feb 10 11:46:58 2021 -0500

    Remove blank lines

commit ff3b8b5
Author: kryswisnaskas <[email protected]>
Date:   Wed Feb 10 11:21:03 2021 -0500

    Remove sorting

commit 1da28b0
Merge: c3b37b4 d1c5786
Author: kryswisnaskas <[email protected]>
Date:   Wed Feb 10 09:32:54 2021 -0500

    Merge branch 'main' of https://github.com/adhocteam/Head-Start-TTADP into kw-display-reports

commit c3b37b4
Author: kryswisnaskas <[email protected]>
Date:   Tue Feb 9 17:47:17 2021 -0500

    Adjust test

commit 4af7bd8
Author: kryswisnaskas <[email protected]>
Date:   Tue Feb 9 17:33:38 2021 -0500

    Remove whitespace to satisfy eslint

commit 253e568
Author: kryswisnaskas <[email protected]>
Date:   Tue Feb 9 17:10:15 2021 -0500

    Combine values from multiples

commit 23cf654
Author: kryswisnaskas <[email protected]>
Date:   Tue Feb 9 14:31:56 2021 -0500

    Fix new eslint errors

commit e4e51ec
Author: kryswisnaskas <[email protected]>
Date:   Tue Feb 9 14:31:34 2021 -0500

    Fix new eslint errors

commit 4b8a95d
Author: kryswisnaskas <[email protected]>
Date:   Tue Feb 9 13:42:36 2021 -0500

    Add role acronyms to display

commit 809a57b
Author: kryswisnaskas <[email protected]>
Date:   Tue Feb 9 12:23:42 2021 -0500

    Update open api doc

commit 55c78db
Author: kryswisnaskas <[email protected]>
Date:   Tue Feb 9 09:02:27 2021 -0500

    Fix cucumber test

commit f106cc2
Author: kryswisnaskas <[email protected]>
Date:   Mon Feb 8 20:26:38 2021 -0500

    Modified cucumber test

commit a21b9d2
Author: kryswisnaskas <[email protected]>
Date:   Mon Feb 8 20:12:33 2021 -0500

    Modified cucumber test

commit 0561da4
Merge: 664b52f a99fe60
Author: kryswisnaskas <[email protected]>
Date:   Mon Feb 8 19:40:25 2021 -0500

    Merge branch 'kw-display-reports' of https://github.com/adhocteam/Head-Start-TTADP into kw-display-reports

commit 664b52f
Merge: 158b2cd 0277d73
Author: kryswisnaskas <[email protected]>
Date:   Mon Feb 8 19:39:54 2021 -0500

    Merge branch 'js-282-fix-cucumber-tests' of https://github.com/adhocteam/Head-Start-TTADP into kw-display-reports

commit a99fe60
Merge: 158b2cd 9f5bb95
Author: kryswisnaskas <[email protected]>
Date:   Mon Feb 8 19:30:27 2021 -0500

    Merge branch 'main' into kw-display-reports

commit 158b2cd
Author: kryswisnaskas <[email protected]>
Date:   Mon Feb 8 19:15:28 2021 -0500

    Fix lint errors

commit e4dae31
Author: kryswisnaskas <[email protected]>
Date:   Mon Feb 8 18:08:49 2021 -0500

    Added more tests

commit 04bba1c
Author: kryswisnaskas <[email protected]>
Date:   Fri Feb 5 16:51:33 2021 -0500

    Fix lint errors

commit 95b3a38
Author: kryswisnaskas <[email protected]>
Date:   Fri Feb 5 15:05:51 2021 -0500

    Fix merge conflict

commit 7d9805f
Merge: 4b6a542 5a31b94
Author: kryswisnaskas <[email protected]>
Date:   Fri Feb 5 14:39:14 2021 -0500

    Merge branch 'main' of https://github.com/adhocteam/Head-Start-TTADP into kw-display-reports

commit 4b6a542
Author: kryswisnaskas <[email protected]>
Date:   Fri Feb 5 13:59:37 2021 -0500

    Remove unused module

commit 67566d3
Author: kryswisnaskas <[email protected]>
Date:   Fri Feb 5 13:58:32 2021 -0500

    Fix lint errors

commit 6de8f8e
Author: kryswisnaskas <[email protected]>
Date:   Fri Feb 5 12:24:20 2021 -0500

    Fix merge issue

commit 6431825
Author: kryswisnaskas <[email protected]>
Date:   Fri Feb 5 11:48:46 2021 -0500

    Correct merge error

commit 75f7639
Author: kryswisnaskas <[email protected]>
Date:   Fri Feb 5 11:46:04 2021 -0500

    Added tests, empty report

commit 69ccf13
Merge: a2bd6c0 8e5d3ef
Author: kryswisnaskas <[email protected]>
Date:   Wed Feb 3 17:18:26 2021 -0500

    Merge branch 'main' of https://github.com/adhocteam/Head-Start-TTADP into kw-display-reports

commit a2bd6c0
Merge: a716599 4c3a436
Author: kryswisnaskas <[email protected]>
Date:   Sat Jan 30 10:07:19 2021 -0500

    Merge branch 'main' of https://github.com/adhocteam/Head-Start-TTADP into kw-display-reports

commit a716599
Author: kryswisnaskas <[email protected]>
Date:   Fri Jan 29 13:50:53 2021 -0500

    Added sorting

commit cedfeb0
Author: kryswisnaskas <[email protected]>
Date:   Wed Jan 27 12:47:28 2021 -0500

    Landing page checkpoint commit
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