Skip to content

Update media linking based on a recent return and add conversation view - #587

Open
snoop168 wants to merge 3 commits into
abrignoni:mainfrom
snoop168:main
Open

snoop168 wants to merge 3 commits into
abrignoni:mainfrom
snoop168:main

Conversation

@snoop168

Copy link
Copy Markdown
Contributor

What this changes

Update media linking based on a recent return and add conversation view

For a new or changed artifact

  • Ran the tool against a real extraction and confirmed the row counts, not just that it imports.
  • Ran python admin/scripts/check_artifact_output.py <report folder> on that report and fixed or documented every finding. An empty or constant column is often a real result (no group chats, coarse location denied); the fix for those is a sentence in the artifact's notes naming the column and saying it was empty or uniform, which is also what stops the checker reporting it.
  • Checked it against a second app data directory where the platform provides one (a second user, account, or container). --compare <multi-container report> reads the scaling for you: it should be exactly double.
  • notes, description and sample_data say only what the data shows, and the numbers were re-derived from the finished run.

Anything reviewers should know

Based on a discord return produced in the last few days

@github-actions

Copy link
Copy Markdown

Thanks for the contribution!

This PR changes artifact modules without test data for them. A small fixture with each artifact change lets reviewers run the module against real data, and the committed case keeps guarding the module after merge.

  • discordReturnsdms.py: please include a fixture with this PR.

Adding a fixture

Generate it from your extraction with the helper (details in create_module_test_cases.md):

python admin/test/scripts/make_test_data.py <module> --case <case_number> --input <extraction.zip>

It writes admin/test/cases/testdata.<module>.json and one zip per artifact under admin/test/cases/data/<module>/.

Size rules:

  • Under 10 MB per zip: commit the files in this PR.
  • 10 to 25 MB: commit the case JSON in the PR and attach the zip to a comment here.
  • Over 25 MB: say so here and a maintainer will arrange a handoff.

If your extraction cannot be shared:

  • If the app appears on a public research image, generate the fixture from that instead. public_corpus_images.md lists the images and where to download them.
  • Or sanitize the real file in place: keep the file the app wrote and overwrite only the personal values, which keeps the format honest.
  • Or script a known session: install the app on a test device with a throwaway account, perform documented actions, and extract that.

If none of those fit, say so here and we will work it out. The PR can still be reviewed and merged with the gap recorded in the artifact's notes.

This is a request, not a gate. Nothing here blocks review.

@github-actions github-actions Bot added the needs-test-data Artifact PR without test data for the changed modules label Oct 13, 2026
@snoop168

Copy link
Copy Markdown
Contributor Author

extraction cannot be shared

@abrignoni

Copy link
Copy Markdown
Owner

Thanks for this, @snoop168. Having a current return to work from is exactly what this module was missing, and the conversation view is a nice add.

A few things before I merge:

  1. _media_refs reads segs[-3] with no length check. I ran it against a few made-up values. A full CDN URL works fine, but a value with fewer than three segments (pic.png, 222/pic.png) raises IndexError, and that takes down the whole artifact. Can you guard on len(segs) >= 3 and skip the ones that do not fit?

  2. Direction compares the id from user.json to the Author ID from the CSV. The CSV side is always a string. If the JSON side is ever a number, every row comes out as Received and nothing warns you. Wrapping both in str() covers it. Is the id a string in your return?

  3. The notes still say no test return is available and that the column order was not checked. That is no longer true thanks to you. Please update them to say what you confirmed on your return: the column order, the attachment folder layout, and that Direction comes from user.json and reads Unknown when that file is missing. Shapes and counts only, no content from the return.

  4. The server and unknown channel modules (discordReturnsserver.py, discordReturnsunkn.py) still use the old attachment id lookup. Does your return store their attachments the same way as the DMs? If so they need the same fix. Happy to take that here or in a follow up, your call.

Small stuff: the first loop skips the str(file_found) the second one does, and there is a stray blank line after the with open.

Understood on the return not being shareable. No problem there.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

needs-test-data Artifact PR without test data for the changed modules

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants