[hma] Add note field to BankContent with storage layer support

[sreeprasad]

Part of #1925 - Add notes to hashbank content

Changes:

• Add column (VARCHAR 255) to bank_content table
• Add database migration
• Update BankContentConfig dataclass with note field
• Update storage implementation (add, get, update)
• Update mocked storage
• Add unit tests for note CRUD operations
• Fix devcontainer Dockerfile yarn GPG key issue

Summary

Adds a note field to BankContent allowing users to attach context when adding content to a bank (e.g., "Reported by user X" or "Part of campaign ABC").

This PR implements the database and storage layer changes:

• New note column (VARCHAR 255) on bank_content table
• Database migration
• Updated storage interface and implementation
• Unit tests for note CRUD operations

Also fixes a yarn GPG key issue in the devcontainer Dockerfile.

Part of #1925

Test Plan

• All 46 existing tests pass
• Added 2 new tests:
  • test_bank_content_note - tests add, read, update, and clear note
  • test_bank_content_note_max_length - tests 255 character limit
• Tested locally in devcontainer with py.test

[meta-cla]

Hi @sreeprasad!

Thank you for your pull request and welcome to our community.

Action Required

In order to merge any pull request (code, docs, etc.), we require contributors to sign our Contributor License Agreement, and we don't seem to have one on file for you.

Process

In order for us to review and merge your suggested changes, please sign at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need to sign the corporate CLA.

Once the CLA is signed, our tooling will perform checks and validations. Afterwards, the pull request will be tagged with CLA signed. The tagging process may take up to 1 hour after signing. Please give it that time before contacting us about it.

If you have received this in error or have any questions, please contact us at cla@meta.com. Thanks!

[Dcallies]

Looks good, would be willing to merge as is, just some questions inline, and you'll need to accept CLA.

If you want to send back to me without making a code change, just hit "Request review" in the upper right hand.

[Dcallies]

blocking q: What is this change for? Consider documenting what this is for.

[Dcallies]

ignorable: You probably could get away with adding setting the note to the existing tests.

A pattern I usually think works and is compact is:

1. Test creating with all defaults
2. Test setting all settable fields
3. Test setting one field, all other fields are unset
4. Test creating with all field set

[Dcallies]

blocking: Add a case that demonstrates the behavior creating over the max length. The test as written I don't think tests anymore more than the previous test does

[aokolish]

@Dcallies I wonder if this PR as-is would be a breaking change for folks. My thinking is - when folks update they first need to run the migrations before we can update the code to use the new field; we can't assume in the code that the migration has already ran. What do you think?

[Dcallies]

@aokolish - making sure I understand your question correctly, are you asking if this change will require a database schema update to work?

If so, then yes, it will, and that's what we use the alembic library for. The expectation is that you use alembic to upgrade the schemas of your database when upgrading to new versions. You can see the created migration file in the PR.

[aokolish]

Nope, I know that it requires the schema change. I'm wondering about downtime / errors.

My question is - is there a way we can we ship this with no down time? Here's what I think will happen if we merge this and an HMA user updates:

1. HMA user updates to the latest image w/this code
2. The code immediately tries to use the note field
3. This causes errors because the migrations haven't run yet
4. Few minutes later, HMA user runs the migrations and things begin working again

There may not be a good way to avoid this, but lmk if you think I'm missing something.

If HMA were not open source, what I would do is...

1. Deploy a PR with just the DB migration
2. Run the migration in prod (and verify nothing breaks)
3. Deploy subsequent PRs to use the new field

Maybe we could avoid the errors by splitting this into 2 PRs and 2 different HMA tags (then we could deploy them separately). You think it's worth the effort? Or, am I missing something?