Merge qa_loader and create-dev-data; add seen data.

[rmol]

Status

Ready for review

Description of Changes

Fixes #5514.

There was a lot of overlap between qa_loader.py and create-dev-data.py, so we're consolidating them while adding seen data to the populated database.

Testing

• Run make dev and verify that the database is populated correctly.
• Run make build-debs && make staging, log in to the app server, and run:
  • sudo -u www-data bash
  • cd /var/www/securedrop
  • CRYPTOGRAPHY_ALLOW_OPENSSL_102=yes ./loaddata.py --source-count ALL.
    The default three journalists and a number of sources should be created without any errors.
  • Run loaddata.py a few more times, varying the number of journalists, sources, and files, messages and replies per source, to be created. Confirm the results are as expected.

Deployment

This is dev/test only.

Checklist

If you made changes to the server application code:

• Linting (make lint) and tests (make test) pass in the development container

If you made non-trivial code changes:

• I have written a test plan and validated it for this PR

Choose one of the following:

• I have opened Replace qa_loader.py with loaddata.py securedrop-docs#108 in the docs repo for these changes
• I would appreciate help with the documentation
• These changes do not require documentation

[zenmonkeykstop]

Couple of issues as per below:

dev

• Run make dev and verify that the database is populated correctly. *Only 2 journalists left of 3 created - as expected as one is deleted after the source replies are created

staging

• The default three journalists and a number of sources should be created without any errors. same 2 journalists created.
• Run loaddata.py a few more times, varying the number of journalists, sources, and files, messages and replies per source, to be created. Confirm the results are as expected. subsequent runs just add extra records instead of resetting database first - this could be addressed in documentation by asking dev to run ./manage.py reset between runs.

[sssoleileraaa]

make dev shows all three sources as unseen now. i think it would be useful to go back to showing the default sources with more variation, e.g. the way it is on develop:

1. one source with all conversation items seen
2. one source with all conversation items unseen
3. one source with some conversation items seen (so the source will show up as unseen)

also, it looks like you show some sources without replies.

this is how it looks like on develop when you run NUM_SOURCES=10 make dev:

now it looks like:

did you intend to change this?

[zenmonkeykstop]

@creviera I got 2 unseen sources and one seen on my make dev run, I think that's just down to the roll of the dice. On the replies front, it looks like once the fraction of sources that are supposed to get replies has been reached, all others afterwards get zero.

[zenmonkeykstop]

Script populates test data as expected, with the exception of the Death Of Superman. One worthwhile change would be to reset the database before loading data, though this could also be addressed by documenting the existing manage.py action for this.

reviewing new changes

[sssoleileraaa]

✔️ cassette generation for seen/unseen in the client is working again
✔️ a couple client functional tests (that are more brittle) are failing during cassette generation

• there is now 1 file attachment instead of 2 when running make dev which affects some functional tests, e.g. test_file_download in the client
• this is fine (not great but fine) since CI will not fail on the client repo. Client functional tests can be updated any time after this is merged to work with the new default config

❌ some SDK functional tests (that are more brittle) are failing during cassette generation

• now, not every source has a reply be default when running make dev which affects some tests, e.g. test_get_all_replies and test_get_replies_from_source in the sdk
• there will be CI failures since we have an SDK CI job that tests cassette generation. I think if we just add replies to each source by default again (the way it is on main) when running make dev, the SDK functional tests will work again, but it would be best to make sure by following these steps to test cassette generation: https://github.com/freedomofpress/securedrop-sdk#generating-cassettes-for-api-calls-over-http. Another idea: If you don't want to add replies to every source by default, then you'll have to make a PR in the SDK repo with the functional test changes needed in order to work with this new default config. This will need to go out fairly quickly after this PR is merged and will take some time, so it should be done in parallel.

[rmol]

The deleted journalist is expected; sorry for not noting that in the test plan.

There's a couple of axes of tension here: between dev and staging (the current create-dev-data.py and qa_loader.py scripts), and between randomness and predictability in the test dataset.

We shouldn't fix the seed by default when running loaddata.py manually, because when you're iteratively populating a database, perhaps for load testing at various sizes, it's not obvious that the duplicate records are caused by the fixed seed (#5236).

That same scenario is why loaddata.py shouldn't reset the database.

I think we should try to have less predictability in the dev and test database by default. Tests that don't rely on cassettes should either establish the state they need (which could be done with fixtures -- #3836) or work with what they find, without brittle expectations. It's more work, but it's more robust, and it'll be important when we try to tackle #2535. I've also found variable data useful in interactive use; when changing the UI it can turn up things you didn't account for.

To support fixture generation, I could modify dev-shell, dev-deps and the Makefile to add a controlled configuration, always using the same seed and counts, and have the SDK CI job use that.

Thoughts?

[sssoleileraaa]

That makes sense, John. What do you think about doing things in the following order (this PR should tackle the first step):

1. Merge the two scripts into one without breaking CI and ideally without breaking any cassette generation test in the client or in the SDK. (Currently, cassette generation is broken with the changes in this PR.)
2. Change the functional tests that would break based on the changes you want to make.
3. Make changes such as adding ways to configure how we initialize the dev server, e.g. sometimes we want each initialization to look the same during UI and UX development so we can meticulously compare, sometimes we want data that looks more like a real server for live demos, sometimes we want a lot of randomness and variation to catch issues we might not see otherwise. (This is a good topic for our engineering meeting.)
4. More changes to how we do functional tests and cassette generation

[rmol]

I've changed the loaddata.py defaults to match the expectations of the client and SDK tests. I tested cassette generation in securedrop-sdk and had no trouble.

I've changed dev-shell to support passing the LOADDATA_ARGS environment variable, which dev-deps will use if set. This will let us lock down parameters in things like the SDK cassette generation CI test, so that down the road we can change the default test dataset as we like without affecting those.

[codecov-io]

Codecov Report

Merging #5645 (cad70db) into develop (5e5baa8) will increase coverage by 28.04%.
The diff coverage is 0.51%.

@@             Coverage Diff              @@
##           develop    #5645       +/-   ##
============================================
+ Coverage    53.10%   81.14%   +28.04%     
============================================
  Files           50       51        +1     
  Lines         5928     3872     -2056     
  Branches       530      482       -48     
============================================
- Hits          3148     3142        -6     
+ Misses        2678      631     -2047     
+ Partials       102       99        -3     

Impacted Files	Coverage Δ
securedrop/loaddata.py	0.00% <0.00%> (ø)
securedrop/models.py	92.24% <100.00%> (+29.65%)	⬆️
securedrop/secure_tempfile.py	100.00% <0.00%> (+5.17%)	⬆️
securedrop/journalist_app/decorators.py	100.00% <0.00%> (+12.50%)	⬆️
...s/2d0ce3ee5bdc_added_passphrase_hash_column_to_.py	53.33% <0.00%> (+13.33%)	⬆️
...ions/523fff3f969c_add_versioned_instance_config.py	66.66% <0.00%> (+13.33%)	⬆️
.../b58139cfdc8c_add_checksum_columns_revoke_table.py	36.36% <0.00%> (+15.31%)	⬆️
...rsions/3da3fcab826a_delete_orphaned_submissions.py	31.91% <0.00%> (+15.61%)	⬆️
securedrop/source_app/api.py	100.00% <0.00%> (+19.04%)	⬆️
... and 33 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5e5baa8...cad70db. Read the comment docs.

[rmol]

Rebased and ready for review again.

[sssoleileraaa]

lgtm

lgtm