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

Saving activity report - frontend #118

Merged
merged 11 commits into from
Jan 20, 2021
Merged

Conversation

jasalisbury
Copy link

@jasalisbury jasalisbury commented Jan 15, 2021

Description of change
Can save an activity report on the frontend

  • Reports are saved anytime a user navigates to a new page
  • Reports are automatically saved every 2 minutes (separate timer from the save on navigation)
  • New reports are created, previously created reports are updated on save
  • Multiselects can handle objects in addition to strings
  • Names of fields updated to match database field names
  • Last save time displayed under the stickied report navigator
  • Previously saved reports can be viewed by entering their id in the path of the url (/activity-reports/{id}...)

How to test

  1. Browse to sandbox
  2. Start filling out a report (you have to fill out at least one grant or nonGrantee for the report to save)
  3. Pull up the console and switch pages of the report, note the id of the saved report
  4. Browse to https://tta-smarthub-sandbox.app.cloud.gov/activity-reports/{id}/activity-summary (fill in the id from previous step)
  5. Note the info you previously entered is populated in the report

The only fields that do not save are the file upload fields and collaborating users.

Issue(s)

Checklist

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

 * Reports are saved anytime a user navigates to a new page
 * Reports are automattically saved every 2 minutes (separate timer from
the save on navigation)
 * New reports are created, previously created reports are updated on
save
 * Multiselects can handle objects in addition to strings
 * Names of fields updated to match database field names
 * Last save time displayed under the sticked report navigator
 * Previously saved reports can be viewed by entering their id in the
path of the url (`/activity-reports/{id}...`)
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.

Tested and everything works as described.

Are you planning on future changes to update the url after it saves (change form /activity-report/new to /activity-report/$REPORTID)?

Copy link
Collaborator

@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.

Fantastic work! 🎉

Couple of questions and comments:

Is the plan to use the group feature from react-select for the Grants/Grantee display to match the design? Or did we clear with Aricka that we won't be showing the grouping with bold grantee names for the MVP?

I noticed that the ids of ActivityParticipants db table change on every save. Is that intended?

This multiselect component uses react-select. React select expects options and selected
values to be in a specific format, arrays of `{ label: x, value: y }` items. Sometimes
we want to just push in and pull out simple arrays of strings instead of these objects.
Dealing with arrays of strings is easier then arrays of objects. The `simple` prop being
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think "then" was meant to be "than" (same comment for line11)

const onSave = async (data) => {
const { participantType, activityParticipants } = data;
if (reportId.current === 'new') {
if (participantType && activityParticipants.length > 0) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should we check if activityParticipants are not null?
(I got an error when grantees/non-grantees were not filled in time for the timer to autosave)

@jasalisbury
Copy link
Author

I noticed that the ids of ActivityParticipants db table change on every save. Is that intended?

Yeah that is because when an activity report is saved all activity-participants are removed and the currently selected activity-participants added back to the activity report. We could be smarter about removing only activity-participants that have been removed from the list and adding only new activity-participants but the current way it works is simpler.

@jasalisbury
Copy link
Author

Tested and everything works as described.

Are you planning on future changes to update the url after it saves (change form /activity-report/new to /activity-report/$REPORTID)?

Yeah I tried for a little while to update the URL but coudn't get it to update properly without a page reload. I'll probably revisit at some point in the future. In the meantime the reportId could definitely make it down as a prop.

@jasalisbury
Copy link
Author

Is the plan to use the group feature from react-select for the Grants/Grantee display to match the design? Or did we clear with Aricka that we won't be showing the grouping with bold grantee names for the MVP?

I just pushed up a change that should group grants by grantee.

The API now returns generic objects instead of objects setup for
`react-select`. The frontend is responsible for formatting the objects
into the proper format
Conflicts:
	docs/openapi/paths/activity-reports/activity-recipients.yaml
	src/services/activityReports.js
@kryswisnaskas
Copy link
Collaborator

One other comment I forgot to mention was that the browser navigation seemed to have changed when trying to get to "Home" from "ActivityReports" using the browser's back button. The other way it worked perfectly fine. You could always got from "Home" to "ActivityReports". I am fine with merging this PR, but keeping in mind that might be something to re-visit.

@jasalisbury jasalisbury merged commit 8a2df69 into main Jan 20, 2021
@jasalisbury jasalisbury deleted the js-saving-activity-report-frontend branch January 20, 2021 15:13
@kryswisnaskas
Copy link
Collaborator

The grouping of the grantees looks great, btw!

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.

3 participants