Require authentication for facility CSV downloads and log requests

[jwalgran]

Overview

Require a login to download a facility CSV from the map page and log all CSV download requests to the database.

Connects #668

Demo

Notes

We have kept the method, model, and model attribute names relatively generic so we have the option of logging other downloads using the same code while avoiding the need to change names.

Testing Instructions

This assumes the fixture data has been loaded and processed with ./scripts/resetdb

• Log in as c2@example.com
• Search for shoes
• Click "Download CSV" and verify that is behaves as before.
• Run ./scripts/manage dbshell
• Run select * from api_downloadlog and verify that the path and record_count columns match.
• Stop ./scripts/server. Click "Download CSV" and verify that error toast is shown. Start ./scripts/server
• Log out
• Search for "shoes" again and click "Download CSV." Verify that the "Log In To Download" dialog is shown.
  • Verify each button behavior (Cancel, Register, and Log In).

Checklist

• fixup! commits have been squashed
• CI passes after rebase
• CHANGELOG.md updated with summary of features or fixes, following Keep a Changelog guidelines

[kellyi]

Setting this up to take a look!

[kellyi]

Since the facilities data lives in the app's Redux store state, conceivably we could change this function to actually perform the download directly rather than having to pass rows and callback in as arguments. If we handled it this way, we could get the facilities data here via getState and then directly import the downloadFacilitiesCSV utility function to execute it in the promise chain.

[jwalgran]

We discussed this in person and it does make sense given the future refactoring of the search result handling to have this action fetch the results from the state rather than having the caller pass them in.

[jwalgran]

Implemented in fixup c53103b

[kellyi]

Nitpick but consider

.then(() => callback(rows))
.then(() => dispatch(completeLogDownload())
.catch....

Or whatever similar construction would work with a potential revision to use getState.

[jwalgran]

👍

[jwalgran]

Implemented in fixup c53103b

[kellyi]

Is 2083 a maximum value for paths?

[jwalgran]

There is a well researched StackOverflow answer that contributed to this choice of maximum value
https://stackoverflow.com/a/417184

In summary, URLs larger than this bump into some limits in browsers and search crawling infrastructure so it is a reasonable maximum value.

[kellyi]

👍 This worked well! I saw the dialog prompting me to login when I tried to download the CSV without logging in, and I see the downloadlog record created after logging in & downloading the CSV:

openapparelregistry=# SELECT * FROM api_downloadlog;
 id |    path     | record_count |          created_at           |          updated_at           | user_id 
----+-------------+--------------+-------------------------------+-------------------------------+---------
  1 | /facilities |          500 | 2019-07-24 18:37:56.752855+00 | 2019-07-24 18:37:56.752905+00 |       1
(1 row)

I made a comment about a small refactor that I think might clean up the download code: essentially, since the CSV rows originate from the Redux store data, we can do all of the CSV downloading work in the new logDownload action by accessing the store with getState and then calling the proper utility functions. On one hand it'd help make the code a little easier to reason about; on the other I don't know how likely it is that we'll add more new features around this part of the app -- so it may not be beneficial.

Feel free either to do that or not, depending on how soon you'd like to deploy the new release!

[kellyi]

I tested this, btw without reconciling the merge conflict, which is just that one of the reducers on develop now has a different name than it does on this branch.

[jwalgran]

Rebased to resolve the merge conflict.

[jwalgran]

Thanks for the great architecture suggestions. The implementation is better for it.