diff --git a/Procfile b/Procfile index 5b40f65f..aa92c141 100644 --- a/Procfile +++ b/Procfile @@ -1 +1 @@ -web: gunicorn --bind=0.0.0.0:8000 --workers=4 --forwarded-allow-ips=* app:app +web: gunicorn --bind=0.0.0.0:8000 --workers=4 --timeout=120 --forwarded-allow-ips=* app:app diff --git a/README.md b/README.md index 85ca3741..44c6d800 100644 --- a/README.md +++ b/README.md @@ -11,7 +11,14 @@ series of photography competitions. Montage provides a configurable workflow that adapts to the conventions of all groups. - [montage on Wikimedia Commons](https://commons.wikimedia.org/wiki/Commons:Montage) -- [montage on Phabricator](https://phabricator.wikimedia.org/project/view/2287/) +- [montage on GitHub Issues](https://github.com/hatnote/montage/issues) +- [montage on Phabricator](https://phabricator.wikimedia.org/project/view/2287/) (archived) + +## Running locally + +See [dev.md](dev.md) for full setup instructions. + +--- ## Development diff --git a/config.default.yaml b/config.default.yaml index dbbfd7c0..31b1b53f 100644 --- a/config.default.yaml +++ b/config.default.yaml @@ -6,6 +6,12 @@ db_url: "sqlite:///tmp_montage.db" cookie_secret: ReplaceThisWithSomethingSomewhatSecret superuser: Slaporte +# Optional: set to true to bypass OAuth and log in automatically (dev only) +debug: false +# Optional: set these to log in as yourself in debug mode (defaults to __montage_debug__ / 0) +# debug_username: Slaporte +# debug_userid: 6024474 + dev_local_cookie_value: "contact maintainers for details" dev_remote_cookie_value: "contact maintainers for details" oauth_client_id: "register an OAuth 2.0 consumer at https://meta.wikimedia.org/wiki/Special:OAuthConsumerRegistration/propose" diff --git a/deployment.md b/deployment.md index 5dd27067..7c1cc5e1 100644 --- a/deployment.md +++ b/deployment.md @@ -41,6 +41,26 @@ toolforge envvars create MONTAGE_DB_URL # mysql+pymysql://:@ toolforge envvars create MONTAGE_SUPERUSERS # OPTIONAL — enables su-impersonation only, NOT admin access (see "Maintainer vs superuser" below) toolforge envvars create MONTAGE_API_LOG_PATH # e.g. /data/project/montage-beta/montage_api.log toolforge envvars create MONTAGE_REPLAY_LOG_PATH +toolforge envvars create MONTAGE_IMPORT_CHECK_PATH # the tool's own folder, e.g. /data/project/montage-beta/import_checks (see below) +``` + +`MONTAGE_IMPORT_CHECK_PATH` is required everywhere except local development: Montage refuses to +start without it. It is the folder for import checks (hatnote/montage#510): each check of a +first-round import source is written there as a JSON file (mode 0600). A check can be used to +save a round for 1 hour. The file is deleted after at most 7 days: every check, import, download +and app start deletes older files, and at most 5,000 are kept. The files hold the source link, +the rows and Commons' data per file, including Commons uploader names. + +- It must be a folder **of its own**: Montage refuses to start if it holds other files, because + the cleanup deletes old check files there. +- It must be absolute and on the tool's NFS share (`/data/project//...`, mounted with + `--mount all`), so that every pod and a restarted pod see the same files. + +Create the folder first, in the tool's home (`$HOME` is `/data/project/`), and print the +value to enter at the `envvars create` prompt: + +```bash +mkdir -m 700 "$HOME/import_checks" && ls -ld "$HOME/import_checks" && echo "$HOME/import_checks" ``` Optional env vars (all have sensible defaults): @@ -191,7 +211,25 @@ git -C ~/www/python/src checkout master git -C ~/www/python/src pull --ff-only ``` -#### 4. Run the deploy script +#### 4. Check new required settings + +A release can add a required environment variable; the app then refuses to start until it is +set. Since hatnote/montage#510 that is `MONTAGE_IMPORT_CHECK_PATH` (see Fresh install, step 3). +`deploy.sh` checks the required variables before it builds, and stops if one is missing. +Before the first deploy that includes it, create the folder and set the variable on that tool +(as the tool, after `become`): + +```bash +mkdir -m 700 "$HOME/import_checks" && ls -ld "$HOME/import_checks" && echo "$HOME/import_checks" +``` + +```bash +toolforge envvars create MONTAGE_IMPORT_CHECK_PATH +``` + +Enter the path the first command printed (e.g. `/data/project/montage-beta/import_checks`). + +#### 5. Run the deploy script Which ref to deploy depends on the tool (see [Multi-environment setup](#multi-environment-setup)): @@ -233,6 +271,24 @@ from `toolforge envvars`. A tool that worked on the legacy service can therefore buildservice if the `MONTAGE_*` env vars (Fresh install step 3) are not set — and its database may need migrating (see "Startup crash: missing column" below). +#### Import checks: troubleshooting and rollback + +- **The pod does not start and its log says `MONTAGE_IMPORT_CHECK_PATH ...`:** create the folder + and the variable (step 4), then `toolforge webservice buildservice restart --mount all`. No + rebuild and no rollback are needed. +- **A check of a large source fails with a server error after about 30 s:** the request hit gunicorn's + worker timeout. Nothing was saved; try a smaller source (split a list, or a sub-category). +- **Old check files:** every use of the folder (and every app start) deletes files older than + 7 days. To delete them by hand: + +```bash +find "$HOME/import_checks" -name '*.json' -mtime +7 -delete +``` + +- **Rolling back to a version before #510:** the variable and the folder can stay (older + versions ignore them). Delete the check files (`rm "$HOME"/import_checks/*.json`): they hold + Commons uploader names that nothing reads any more. Entries already imported stay. + --- ## Debugging diff --git a/dev.md b/dev.md index 859ed336..2a7da965 100644 --- a/dev.md +++ b/dev.md @@ -96,6 +96,8 @@ Other frontend development commands: ### 6. Use the Makefile to start the backend * Open a new terminal tab and change directory to root of repo * Copy and edit `config.dev.yaml` based on `config.default.yaml` +* (Optional) Set `debug: true` in `config.dev.yaml` to bypass OAuth entirely. This is the easiest way to get started locally without setting up OAuth. To log in, navigate to http://localhost:5001/complete_login directly — the "Login with Wikimedia" button still requires OAuth and won't work locally. +* (Optional) Set `debug_userid: ` and `debug_username: ` to be auto-logged in as yourself in debug mode. Your CentralAuth ID can be found at https://en.wikipedia.org/wiki/Special:CentralAuth/YOUR_USERNAME. * (Optional) In `config.dev.yaml` there is a line for `dev_local_cookie_value`. To get it, log in to the local app in your browser, and then copy the value from the `clastic_cookie` in the apps' cookies. This is your login cookie. @@ -106,11 +108,17 @@ juror.) This will give your user top-level permissions in the full app, so you can view some logs (audit logs, active users), add/remove organizers, and get a coordinator view into all campaigns. +* Build the frontend for the backend (run from the `frontend/` directory): +```bash +npm run toolforge:build +``` * Start the montage backend ```bash make start ``` -This will build the docker image for the montage backend and start the container. Apart from `make start`, these are other commands: +The backend is ready when you see `Debugger PIN` in the logs. It is available at http://localhost:5001. + +Apart from `make start`, these are other commands: * `make start-detached` : Start the backend container in detached mode * `make stop` : Stop the backend container * `make logs` : Stream the backend container logs in real-time. @@ -195,8 +203,8 @@ npm run build ## Project structure ```bash -├── DEV.md -├── Dockerfile +├── dev.md +├── dockerfile ├── LICENSE ├── MANIFEST.in ├── Makefile @@ -240,7 +248,6 @@ npm run build │   ├── templates │   ├── tests │   └── utils.py -├── report.html ├── requirements-dev.txt ├── requirements.in ├── requirements.txt @@ -304,5 +311,5 @@ These provides a detailed explanation of the main components in the **Montage Pr #### Docker Files -- **`Dockerfile`**: Builds the backend container. +- **`dockerfile`**: Builds the backend container. - **`docker-compose.yml`**: Orchestrates service for backend. \ No newline at end of file diff --git a/frontend/cypress/e2e/campaign.cy.js b/frontend/cypress/e2e/campaign.cy.js index bbfcf20e..2e8e1bf3 100644 --- a/frontend/cypress/e2e/campaign.cy.js +++ b/frontend/cypress/e2e/campaign.cy.js @@ -1,3 +1,39 @@ +// #510: a clean check of one file, valid from now on +const stubCheck = () => { + const now = Date.now() + cy.intercept('POST', '/v1/admin/campaign/*/import/check', { + body: { + status: 'success', + data: { + token: 'cypress-check-token-000000000000', + checked_at: new Date(now).toISOString().replace(/\.\d+Z$/, 'Z'), + expires_at: new Date(now + 3600 * 1000).toISOString().replace(/\.\d+Z$/, 'Z'), + import_method: 'selected', + source: { file_names: ['Example.jpg'] }, + columns: { name: null, file_id: null, ignored: [] }, + counts: { + ok: 1, renamed: 0, duplicate: 0, unknown_name: 0, + unknown_file_id: 0, malformed_file_id: 0, same_name: 0 + }, + blocking: false, + total_rows: 1, + importable_count: 1, + issues: [], + issues_total: 0, + issues_truncated: false, + same_name_groups: [] + } + } + }).as('checkImport') +} + +const savedRound = () => ({ + id: 999, + name: 'My Test Round', + import: { new_round_entry_count: 1, warnings: [], disqualified: [] } +}) + + describe('Campaign Details Page', () => { beforeEach(() => { cy.setCookie('clastic_cookie', ''); @@ -84,8 +120,53 @@ describe('Campaign Details Page', () => { .find('li') .first() .click(); - cy.get('.button-group button').contains('Add Round').click().click(); - cy.log(' Round created successfully'); + // #510 / #447: the source is checked first; Save sends the round together + // with its checked import, once + stubCheck() + cy.intercept('POST', '/v1/admin/campaign/*/add_round', (req) => { + req.reply({ delay: 500, body: { status: 'success', data: savedRound() } }) + }).as('addRound') + cy.get('.form-container').contains('label', 'File List').click() + cy.get('.form-container textarea').first().type('Example.jpg') + cy.get('[data-testid="add-round-button"]').should('be.disabled') + cy.get('[data-testid="check-source-button"]').click() + cy.wait('@checkImport') + cy.get('[data-testid="import-check-result"]').should('be.visible') + // a double click must not save twice + cy.get('[data-testid="add-round-button"]').click().click({ force: true }) + cy.wait('@addRound').its('request.body').should((body) => { + expect(body.import).to.deep.equal({ + import_method: 'selected', + check_token: 'cypress-check-token-000000000000' + }) + expect(body).not.to.have.property('file_names') + }) + cy.get('@addRound.all').should('have.length', 1) + cy.get('.juror-campaign-round-card').should('not.exist') +}) + +it('keeps the form open when the server refuses the import', () => { + cy.get('.add-round-button').click() + cy.get('.form-container input[type="text"]').first().clear().type('My Test Round') + cy.get('.form-container').within(() => { + cy.get('input[placeholder="YYYY-MM-DD"]').first().clear().type('2025-08-15') + }) + cy.get('[data-testid="userlist-search"] input').type('AadarshM07') + cy.get('[data-testid="userlist-search"]').find('li').first().click() + stubCheck() + cy.intercept('POST', '/v1/admin/campaign/*/add_round', { + statusCode: 400, + body: { status: 'failure', error_type: 'import_check_expired', detail: 'check expired' } + }).as('addRound') + cy.get('.form-container').contains('label', 'File List').click() + cy.get('.form-container textarea').first().type('Example.jpg') + cy.get('[data-testid="check-source-button"]').click() + cy.wait('@checkImport') + cy.get('[data-testid="add-round-button"]').click() + cy.wait('@addRound') + cy.get('.juror-campaign-round-card').should('exist') + cy.get('[data-testid="import-check-result"]').should('contain', 'expired') + cy.get('[data-testid="add-round-button"]').should('be.disabled') }) diff --git a/frontend/src/components/Round/ImportCheckResult.vue b/frontend/src/components/Round/ImportCheckResult.vue new file mode 100644 index 00000000..2bf8d88d --- /dev/null +++ b/frontend/src/components/Round/ImportCheckResult.vue @@ -0,0 +1,316 @@ + + + + + diff --git a/frontend/src/components/Round/RoundEdit.vue b/frontend/src/components/Round/RoundEdit.vue index 467249e3..22395a36 100644 --- a/frontend/src/components/Round/RoundEdit.vue +++ b/frontend/src/components/Round/RoundEdit.vue @@ -114,7 +114,7 @@ const formData = ref({ deadline_date: props.round.deadline_date.split('T')[0], config: props.round.config, show_stats: props.round.show_stats, - jurors: props.round.jurors.map((juror) => juror.username) + jurors: props.round.jurors.filter((juror) => juror.is_active).map((juror) => juror.username) }) const showStatsOptions = ref([ @@ -169,6 +169,7 @@ const saveRound = () => { .editRound(round.id, round) .then(() => { emit('update:isRoundEditing', false) + window.location.reload() }) .catch(alertService.error) } diff --git a/frontend/src/components/Round/RoundNew.vue b/frontend/src/components/Round/RoundNew.vue index b3e4d129..ee92643d 100644 --- a/frontend/src/components/Round/RoundNew.vue +++ b/frontend/src/components/Round/RoundNew.vue @@ -1,5 +1,7 @@ + - + - + +
+ + {{ + isChecking + ? $t('montage-round-check-running') + : $t('montage-round-check-source') + }} + +

+ {{ $t('montage-round-check-required') }} +

+ +
@@ -177,17 +218,40 @@ + +
+ +

{{ saveError }}

+

+ {{ saveErrorDetail }} +

+ +
+

{{ saveHint }}

- {{ $t('montage-round-add') }} + + {{ + isLoading && roundIndex === 0 + ? $t('montage-round-saving-with-files') + : $t('montage-round-add') + }} @@ -201,7 +265,7 @@ - - \ No newline at end of file diff --git a/montage/static/dist/images/logo_white_fat.svg b/montage/static/dist/images/logo_white_fat.svg deleted file mode 100644 index 2f486607..00000000 --- a/montage/static/dist/images/logo_white_fat.svg +++ /dev/null @@ -1,71 +0,0 @@ - - - - - - - - - - image/svg+xml - - - - - - - - - - diff --git a/montage/tests/conftest.py b/montage/tests/conftest.py index 69cc6c4d..d903c02f 100644 --- a/montage/tests/conftest.py +++ b/montage/tests/conftest.py @@ -156,6 +156,27 @@ def _offline_mw_userid(username): def _no_live_mw_userid(monkeypatch): monkeypatch.setattr('montage.rdb.get_mw_userid', _offline_mw_userid) + +# --------------------------------------------------------------------------- +# Import sources are fetched only from public addresses (loaders.py). Tests +# never resolve real hostnames: every name resolves to a documentation-range +# stand-in for a public address; IP literals resolve to themselves. +# --------------------------------------------------------------------------- +PUBLIC_TEST_ADDRESS = '93.184.216.34' + + +def _offline_host_addresses(host): + import ipaddress + try: + return [str(ipaddress.ip_address(host.strip('[]')))] + except ValueError: + return [PUBLIC_TEST_ADDRESS] + + +@pytest.fixture(autouse=True) +def _offline_dns(monkeypatch): + monkeypatch.setattr('montage.loaders._host_addresses', _offline_host_addresses) + # --------------------------------------------------------------------------- # Wikimedia API callback -- returns a plausible user record for any username # --------------------------------------------------------------------------- diff --git a/montage/tests/test_dao_unit.py b/montage/tests/test_dao_unit.py new file mode 100644 index 00000000..1d925a33 --- /dev/null +++ b/montage/tests/test_dao_unit.py @@ -0,0 +1,93 @@ +# -*- coding: utf-8 -*- + +import datetime +from sqlalchemy import create_engine +from sqlalchemy.orm import sessionmaker + +from montage.rdb import (Base, + UserDAO, + CoordinatorDAO, + Round, + Campaign, + User, + CANCELLED_STATUS, + ACTIVE_STATUS, + FINALIZED_STATUS) + +def test_get_campaign_rounds_dao(): + """ + Unit test for get_campaign_rounds logic. + Verified fix for PR #500 / Issue #399. + """ + # 1. Setup in-memory SQLite database + engine = create_engine('sqlite:///:memory:') + Base.metadata.create_all(engine) + Session = sessionmaker(bind=engine) + session = Session() + + # 2. Populate with test data + user = User(username='test_coord') + session.add(user) + + campaign = Campaign(name='Test Campaign') + session.add(campaign) + session.flush() + + # We add rounds out of chronological order to verify ordering logic + # Round 1: Finalized, earliest + r1 = Round(name='Round 1', + status=FINALIZED_STATUS, + campaign=campaign, + create_date=datetime.datetime(2023, 1, 1)) + + # Round 3: Cancelled, latest + r3 = Round(name='Round 3', + status=CANCELLED_STATUS, + campaign=campaign, + create_date=datetime.datetime(2023, 1, 10)) + + # Round 2: Active, middle + r2 = Round(name='Round 2', + status=ACTIVE_STATUS, + campaign=campaign, + create_date=datetime.datetime(2023, 1, 5)) + + session.add_all([r1, r2, r3]) + session.commit() + + # 3. Instantiate DAO + user_dao = UserDAO(session, user) + dao = CoordinatorDAO(user_dao, campaign) + + # 4. Assertions: Default behavior (with_cancelled=False) + rounds = dao.get_campaign_rounds(campaign, with_cancelled=False) + + assert len(rounds) == 2, "Should return exactly 2 non-cancelled rounds" + assert r3 not in rounds, "Cancelled round should be filtered out" + + # Check ordering (earliest first) + assert rounds[0].id == r1.id + assert rounds[1].id == r2.id + assert rounds[0].create_date < rounds[1].create_date + + # 5. Assertions: Including cancelled rounds + all_rounds = dao.get_campaign_rounds(campaign, with_cancelled=True) + assert len(all_rounds) == 3, "Should return all 3 rounds when with_cancelled=True" + assert r3 in all_rounds + + # Check ordering across all 3 + assert all_rounds[0].id == r1.id + assert all_rounds[1].id == r2.id + assert all_rounds[2].id == r3.id + + print("DAO get_campaign_rounds unit test passed successfully.") + +if __name__ == "__main__": + try: + test_get_campaign_rounds_dao() + except AssertionError as e: + print(f"Test Failed: {e}") + exit(1) + except Exception as e: + print(f"An error occurred: {e}") + exit(1) diff --git a/montage/tests/test_import_check.py b/montage/tests/test_import_check.py new file mode 100644 index 00000000..ca5d4ebe --- /dev/null +++ b/montage/tests/test_import_check.py @@ -0,0 +1,1120 @@ +# -*- coding: utf-8 -*- +"""The import check (hatnote/montage#510): classification, the same-name +rule, check files. Commons lookups are stand-ins; all data is synthetic, +except the е/ё pair from #645 (public Commons file names and ids).""" +import csv +import io +import json +import os +import time +import datetime + +import pytest +import responses as responses_lib + +from montage import import_check +from montage.import_check import (run_check, counts, blocking_rows, importable, + raise_if_blocked, import_warnings, summarize, + upload_csv, save, load, same_name_keys) +from montage.loaders import parse_source_rows +from montage.utils import (ImportCheckBlocked, ImportCheckExpired, + ImportSourceInvalid) + +CAMPAIGN = 7 +VOLOCHEK = u'Церковь_Богоявления,_Вышний_Волочек.jpg' +VOLOCHYOK = u'Церковь_Богоявления,_Вышний_Волочёк.jpg' + + +def info(name, file_id, **kw): + rec = {'img_name': name, 'img_width': 3000, 'img_height': 2000, + 'img_major_mime': 'image', 'img_minor_mime': 'jpeg', + 'img_user': 1, 'img_user_text': 'Uploader', + 'img_timestamp': '20260910120000', + 'rec_img_timestamp': '20260910120000', 'rec_img_user': 1, + 'rec_img_text': 'Uploader', 'oi_archive_name': None, + 'file_id': file_id} + rec.update(kw) + return rec + + +class FakeCommons(object): + """Stands in for the wikireplica lookups (local mode).""" + def __init__(self, files, other_ids=None): + self.by_name = {f['img_name']: f for f in files} + self.by_id = {f['file_id']: f for f in files} + self.other_ids = other_ids or {} + self.calls = [] + + def names(self, names, source='local'): + self.calls.append(('names', sorted(set(names)))) + return {n: self.by_name[n] for n in names if n in self.by_name} + + def ids(self, ids): + self.calls.append(('ids', sorted(set(ids)))) + return {i: self.by_id[i] for i in ids if i in self.by_id} + + def others(self, numbers): + self.calls.append(('other_ids', sorted(set(numbers)))) + return {n: self.other_ids[n] for n in numbers if n in self.other_ids} + + def category(self, name, source='local'): + self.calls.append(('category', name)) + return sorted(self.by_name.values(), key=lambda f: f['img_name'].encode('utf8')) + + +@pytest.fixture +def commons(monkeypatch): + fake = FakeCommons([]) + + def install(files, other_ids=None): + fake.__init__(files, other_ids) + return fake + monkeypatch.setattr(import_check, 'lookup_by_names', fake.names) + monkeypatch.setattr(import_check, 'lookup_by_ids', fake.ids) + monkeypatch.setattr(import_check, 'category_records', fake.category) + monkeypatch.setattr(import_check, 'lookup_other_ids', fake.others) + return install + + +def csv_check(monkeypatch, text, method='csv'): + monkeypatch.setattr(import_check, 'fetch_source_text', lambda url: text) + return run_check({'import_method': method, 'csv_url': 'https://example.org/x.csv'}, + CAMPAIGN, source='local') + + +def statuses(result): + return [(r['row'], r['status']) for r in result['rows']] + + +# --------------------------------------------------------------------------- +# Classification +# --------------------------------------------------------------------------- + +def test_classify_every_status(monkeypatch, commons): + commons([info('A.jpg', 1), info('B_new.jpg', 2), info('C.jpg', 3)]) + result = csv_check(monkeypatch, 'filename,file_id\n' + 'A.jpg,1\n' # 2 ok + 'B old.jpg,2\n' # 3 renamed + 'C.jpg,\n' # 4 ok by name + 'Gone.jpg,\n' # 5 unknown name + 'X.jpg,99\n' # 6 unknown file_id + 'Y.jpg,1.2E+3\n' # 7 malformed + 'File:A.jpg,\n' # 8 duplicate (same name) + ',3\n') # 9 duplicate (id only) + assert statuses(result) == [(2, 'ok'), (3, 'renamed'), (4, 'ok'), + (5, 'unknown_name'), (6, 'unknown_file_id'), + (7, 'malformed_file_id'), (8, 'duplicate'), + (9, 'duplicate')] + renamed = result['rows'][1] + assert renamed['commons_name'] == 'B_new.jpg' and renamed['file_id'] == 2 + assert u'B_old.jpg → B_new.jpg' in renamed['reason'] + assert result['rows'][7]['reason'] == 'the same file as row 4; imported once' + assert (result['rows'][7]['reason_code'], result['rows'][7]['reason_params']) == ('duplicate', ['4']) + assert [r['commons_name'] for r in importable(result)] == ['A.jpg', 'B_new.jpg', 'C.jpg'] + assert [r['row'] for r in blocking_rows(result)] == [6, 7] + assert counts(result)['duplicate'] == 2 + + +def test_old_name_and_its_file_id_are_one_file(monkeypatch, commons): + commons([info('New.jpg', 5)]) + result = csv_check(monkeypatch, 'filename,file_id\nNew.jpg,\nOld.jpg,5\n') + assert statuses(result) == [(2, 'ok'), (3, 'duplicate')] + + +def test_no_name_with_a_known_file_id_is_imported_and_named(monkeypatch, commons): + """A row with only a file_id is imported; the result names the file it + found, so a wrong number is seen before saving.""" + commons([info('A.jpg', 1)]) + result = csv_check(monkeypatch, 'filename,file_id\n,1\n') + assert statuses(result) == [(2, 'by_file_id')] + assert result['rows'][0]['reason_params'] == ['1', 'A.jpg'] + assert importable(result)[0]['commons_name'] == 'A.jpg' + + +def test_full_csv_metadata_is_ignored(monkeypatch, commons): + """#509: Commons' metadata wins over the CSV's.""" + commons([info('A.jpg', 1, img_width=4000)]) + result = csv_check(monkeypatch, 'img_name,img_width,img_user_text\nA.jpg,1,Someone\n') + assert importable(result)[0]['commons']['img_width'] == 4000 + assert result['columns']['ignored'] == ['img_width', 'img_user_text'] + + +def test_blocking_per_method(monkeypatch, commons): + commons([info('A.jpg', 1)]) + result = csv_check(monkeypatch, 'filename,file_id\nA.jpg,77\n') + assert result['rows'][0]['status'] == 'unknown_file_id' + with pytest.raises(ImportCheckBlocked) as exc: + raise_if_blocked(result) + assert exc.value.detail.startswith('1 row blocks the import.') + assert 'row 2: no file on Commons has file ID 77' in exc.value.detail + + +def test_unknown_names_do_not_block(monkeypatch, commons): + commons([info('A.jpg', 1)]) + result = csv_check(monkeypatch, 'A.jpg\nGone.jpg\n') + raise_if_blocked(result) + warnings = import_warnings(result) + assert list(warnings[0]) == ['import issues'] + assert 'Gone.jpg' in warnings[0]['import issues'] + + +def test_blocked_detail_lists_ten_rows_count_first(monkeypatch, commons): + commons([]) + result = csv_check(monkeypatch, 'filename,file_id\n' + + ''.join('F%d.jpg,%d\n' % (i, 500 + i) for i in range(30))) + with pytest.raises(ImportCheckBlocked) as exc: + raise_if_blocked(result) + assert exc.value.detail.startswith('30 rows block the import.') + assert '... and 20 more' in exc.value.detail + + +def test_lookups_are_batched_not_per_row(monkeypatch, commons): + fake = commons([info('A.jpg', 1), info('B.jpg', 2)]) + csv_check(monkeypatch, 'filename,file_id\nA.jpg,1\nB.jpg,\nC.jpg,\n') + assert fake.calls == [('ids', [1]), ('names', ['B.jpg', 'C.jpg'])] + + +def test_file_list_rows_are_lines(commons): + commons([info('A_b.jpg', 1)]) + result = run_check({'import_method': 'selected', + 'file_names': ['File:A b.jpg', '', 'Gone.jpg']}, + CAMPAIGN, source='local') + assert statuses(result) == [(1, 'ok'), (3, 'unknown_name')] + assert result['source'] == {'file_names': ['File:A b.jpg', 'Gone.jpg']} + + +def test_missing_source_is_import_source_invalid(): + for request in [{'import_method': 'selected'}, + {'import_method': 'nonsense'}, + {'import_method': 'csv'}, + {'import_method': 'category', 'category': ' '}]: + with pytest.raises(ImportSourceInvalid): + run_check(request, CAMPAIGN, source='local') + + +# --------------------------------------------------------------------------- +# dev (remote): no lookup by file_id; compared by name +# --------------------------------------------------------------------------- + +def test_remote_compares_file_ids_by_name(monkeypatch, commons): + fake = commons([info('A.jpg', 1), info('B.jpg', 2)]) + monkeypatch.setattr(import_check, 'fetch_source_text', + lambda url: 'filename,file_id\nA.jpg,1\nB.jpg,3\n') + result = run_check({'import_method': 'csv', 'csv_url': 'x'}, CAMPAIGN, source='remote') + assert statuses(result) == [(2, 'ok'), (3, 'unknown_file_id')] + assert result['rows'][1]['reason'].startswith('dev: compared by name only') + assert [c[0] for c in fake.calls] == ['names'] # never by id + + +# --------------------------------------------------------------------------- +# The gist fix: a gist is looked up where the instance looks up (no POST to +# production's utils endpoint from beta/prod) +# --------------------------------------------------------------------------- + +def test_gist_local_makes_no_remote_lookup(monkeypatch): + from montage import labs + monkeypatch.setattr(labs, 'get_files_info_by_names', + lambda names: {'A.jpg': info('A.jpg', 1)}) + monkeypatch.setattr('montage.loaders.get_files_info_by_names', + labs.get_files_info_by_names) + with responses_lib.RequestsMock() as rsps: + rsps.add(responses_lib.GET, 'https://gist.githubusercontent.com/u/abc/raw', + body='filename\nA.jpg\n', status=200) + result = run_check({'import_method': 'gistcsv', + 'gist_url': 'https://gist.github.com/u/abc'}, + CAMPAIGN, source='local') + assert len(rsps.calls) == 1 # the gist itself, no utils POST + assert statuses(result) == [(2, 'ok')] + + +# --------------------------------------------------------------------------- +# Same name (#645): names the database treats as equal +# --------------------------------------------------------------------------- + +def test_same_name_pair_blocks_a_list_import(monkeypatch, commons): + commons([info(VOLOCHEK, 149673020), info(VOLOCHYOK, 149673022), + info('Other.jpg', 3)]) + result = csv_check(monkeypatch, u'filename,file_id\n"%s",149673020\n"%s",\nOther.jpg,\n' + % (VOLOCHEK, VOLOCHYOK)) + assert statuses(result) == [(2, 'same_name'), (3, 'same_name'), (4, 'ok')] + assert result['rows'][0]['group'] == result['rows'][1]['group'] == 1 + assert VOLOCHYOK in result['rows'][0]['reason'] + assert [r['row'] for r in blocking_rows(result)] == [2, 3] + groups = summarize(result, 'x' * 32)['same_name_groups'] + assert [[m['commons_name'] for m in g] for g in groups] == [[VOLOCHEK, VOLOCHYOK]] + + +@pytest.mark.parametrize('pair', [('Photo.jpg', 'Photo.JPG'), + (u'Église.jpg', u'Eglise.jpg'), + (u'Йошкар.jpg', u'Иошкар.jpg')]) +def test_same_name_case_and_accents(monkeypatch, commons, pair): + commons([info(pair[0], 1), info(pair[1], 2)]) + result = csv_check(monkeypatch, 'filename\n%s\n%s\n' % pair) + assert [s for _, s in statuses(result)] == ['same_name', 'same_name'] + + +def test_same_name_in_a_category_keeps_the_first(commons): + commons([info(VOLOCHYOK, 149673022), info(VOLOCHEK, 149673020), info('A.jpg', 1)]) + result = run_check({'import_method': 'category', 'category': 'Cat'}, + CAMPAIGN, source='local') + by_name = {r['commons_name']: r['status'] for r in result['rows']} + # binary order: е (U+0435) sorts before ё (U+0451) + assert by_name == {VOLOCHEK: 'ok', VOLOCHYOK: 'same_name', 'A.jpg': 'ok'} + assert blocking_rows(result) == [] + warning = import_warnings(result)[-1]['same name'] + assert VOLOCHEK in warning and VOLOCHYOK in warning + + +def test_same_name_switch_off(monkeypatch, commons): + monkeypatch.setattr(import_check, 'SAME_NAME_CHECK', False) + commons([info(VOLOCHEK, 1), info(VOLOCHYOK, 2)]) + result = csv_check(monkeypatch, u'filename\n"%s"\n"%s"\n' % (VOLOCHEK, VOLOCHYOK)) + assert [s for _, s in statuses(result)] == ['ok', 'ok'] + + +def test_test_only_key_does_not_cover_expansions(): + """Documented gap of the SQLite stand-in (MariaDB's own keys are used + in production): ß/ss are not grouped.""" + keys = same_name_keys([u'Straße.jpg', u'Strasse.jpg']) + assert keys[u'Straße.jpg'] != keys[u'Strasse.jpg'] + + +def test_same_name_keys_ask_mariadb(monkeypatch): + """On MariaDB the keys come from WEIGHT_STRING, batched, no table.""" + sent = [] + + class Result(object): + def __init__(self, n): + self.n = n + + def fetchone(self): + return self.row + + class Session(object): + def get_bind(self): + class Bind(object): + class dialect(object): + name = 'mysql' + return Bind() + + def execute(self, statement, params): + sent.append((str(statement), params)) + result = Result(len(params)) + # a key per bound name, in column order (case-insensitive) + result.row = [params['n%d' % j].lower().encode('utf8') + for j in range(len(params))] + return result + + monkeypatch.setattr(import_check, 'SAME_NAME_CHUNK_SIZE', 2) + keys = same_name_keys(['c.jpg', 'X.jpg', 'a.jpg', 'x.jpg'], Session()) + assert keys == {'X.jpg': b'x.jpg', 'a.jpg': b'a.jpg', 'c.jpg': b'c.jpg', 'x.jpg': b'x.jpg'} + assert [len(p) for _, p in sent] == [2, 2] # X.jpg and x.jpg in different chunks + sql = sent[0][0] + assert sql.startswith('SELECT WEIGHT_STRING(CONVERT(:n0 USING utf8mb4)' + ' COLLATE utf8mb4_unicode_ci) AS w0') + assert 'FROM' not in sql + + +def _session(dialect): + class Session(object): + def get_bind(self): + class Bind(object): + pass + Bind.dialect = type('Dialect', (), {'name': dialect}) + return Bind() + + def execute(self, statement, params): + class Result(object): + def fetchone(self): + return [None] * len(params) + return Result() + return Session() + + +def test_same_name_keys_never_approximate_silently(monkeypatch): + """B4: the Python stand-in only for SQLite / no session in dev and + tests; another dialect, or any deployed instance, fails loudly.""" + with pytest.raises(RuntimeError): + same_name_keys(['a.jpg'], _session('postgresql')) + with pytest.raises(RuntimeError): # a NULL key from the database + same_name_keys(['a.jpg'], _session('mariadb')) + assert same_name_keys(['a.jpg'], _session('sqlite')) + monkeypatch.setattr(import_check, 'ENV_NAME', 'prod') + with pytest.raises(RuntimeError): + same_name_keys(['a.jpg'], _session('sqlite')) + with pytest.raises(RuntimeError): + same_name_keys(['a.jpg']) + + +# --------------------------------------------------------------------------- +# Summary and download +# --------------------------------------------------------------------------- + +def test_summary_caps_issues(monkeypatch, commons): + monkeypatch.setattr(import_check, 'ISSUES_SHOWN', 3) + commons([]) + result = csv_check(monkeypatch, ''.join('G%d.jpg\n' % i for i in range(5))) + summary = summarize(result, 't' * 32) + assert len(summary['issues']) == 3 and summary['issues_total'] == 5 + assert summary['issues_truncated'] and not summary['blocking'] + assert summary['importable_count'] == 0 and summary['total_rows'] == 5 + assert 'commons' not in summary['issues'][0] + assert summary['expires_at'] > summary['checked_at'] + + +def test_download_is_an_upload_file_that_passes_unchanged(monkeypatch, commons): + commons([info('A.jpg', 1), info('New.jpg', 2), info('=SUM(A1).jpg', 3)]) + first = csv_check(monkeypatch, 'img_name,file_id,img_width\n' + 'A.jpg,1,5\nOld.jpg,2,5\nA.jpg,,5\nGone.jpg,,5\n=SUM(A1).jpg,,5\n') + data = upload_csv(first).decode('utf8') + assert list(csv.reader(io.StringIO(data))) == [ + ['filename', 'file_id'], ['A.jpg', '1'], ['New.jpg', '2'], ["'=SUM(A1).jpg", '3']] + second = csv_check(monkeypatch, data) + assert [s for _, s in statuses(second)] == ['ok', 'ok', 'ok'] + assert upload_csv(second) == upload_csv(first) + + + +def test_download_pasted_into_the_file_list_passes_unchanged(monkeypatch, commons): + """The download can be pasted into the file list box as the next + source (the form sends the box's lines as file_names).""" + commons([info('A.jpg', 1), info('New.jpg', 2), info(VOLOCHEK, 3)]) + first = csv_check(monkeypatch, 'filename,file_id\nA.jpg,1\nOld.jpg,2\n"%s",\nGone.jpg,\n' % VOLOCHEK) + data = upload_csv(first).decode('utf8') + second = run_check({'import_method': 'selected', 'file_names': data.split('\n')}, + CAMPAIGN, source='local') + assert [s for _, s in statuses(second)] == ['ok', 'ok', 'ok'] + assert second['columns']['file_id'] == 'file_id' + assert upload_csv(second) == upload_csv(first) + + +def test_pasted_csv_by_file_id_only_finds_same_name_pairs(commons): + """A list of file_ids alone (header file_id) is looked up by id; two + files whose names the database treats as equal block it.""" + commons([info(VOLOCHEK, 149673020), info(VOLOCHYOK, 149673022)]) + result = run_check({'import_method': 'selected', + 'file_names': ['file_id', '149673020', '149673022']}, + CAMPAIGN, source='local') + assert statuses(result) == [(2, 'same_name'), (3, 'same_name')] + assert [r['commons_name'] for r in result['rows']] == [VOLOCHEK, VOLOCHYOK] + + +def test_file_list_of_bare_file_ids(commons): + """Lines of digits only are file_ids (no header needed); they mix with + names. Two ids whose Commons names the database treats as equal block.""" + commons([info(VOLOCHEK, 149673020), info(VOLOCHYOK, 149673022), info('A.jpg', 1)]) + result = run_check({'import_method': 'selected', + 'file_names': [' 149673020 ', 'A.jpg', '149673022', '999']}, + CAMPAIGN, source='local') + assert statuses(result) == [(1, 'same_name'), (2, 'ok'), (3, 'same_name'), + (4, 'unknown_file_id')] + assert result['rows'][0]['commons_name'] == VOLOCHEK + + +def test_a_number_is_a_file_id_page_and_revision_ids_only_explain_errors(commons): + """A bare number that is a file_id is taken as one, even though almost + every file_id is also some other file page's page ID (Commons, + 2026-10-10: 4 of 4 tested). Only a number that is no file_id is looked + up as a page ID / revision ID, to say so in the error.""" + fake = commons([info('A.jpg', 1), info('B.jpg', 2)], + other_ids={2: {'kind': 'page_id', 'name': 'Other.jpg'}, + 1100000000: {'kind': 'revision_id', 'name': 'C.jpg'}, + 170000000: {'kind': 'page_id', 'name': 'D.jpg'}}) + result = run_check({'import_method': 'selected', + 'file_names': ['1', '2', '1100000000', '170000000', '5', 'B.jpg']}, + CAMPAIGN, source='local') + assert statuses(result) == [(1, 'by_file_id'), (2, 'by_file_id'), (3, 'revision_id'), + (4, 'page_id'), (5, 'unknown_file_id'), (6, 'duplicate')] + reasons = {r['row']: r['reason'] for r in result['rows']} + assert 'revision ID' in reasons[3] and 'C.jpg' in reasons[3] + assert 'page ID' in reasons[4] and 'not its file ID' in reasons[4] and 'D.jpg' in reasons[4] + assert [r['row'] for r in blocking_rows(result)] == [3, 4, 5] + assert ('other_ids', [5, 170000000, 1100000000]) in fake.calls + + +def test_named_rows_are_not_looked_up_as_page_ids(commons): + """A row with a name confirms its file_id; only bare numbers are + checked against page and revision IDs.""" + fake = commons([info('A.jpg', 1)], other_ids={1: {'kind': 'page_id', 'name': 'X.jpg'}}) + result = run_check({'import_method': 'selected', 'file_names': ['A.jpg']}, + CAMPAIGN, source='local') + assert statuses(result) == [(1, 'ok')] + assert not any(c[0] == 'other_ids' for c in fake.calls) + + +def test_rows_carry_a_reason_code_and_params_for_translation(commons): + """The frontend shows montage-round-check-reason- with the params; + the English reason stays for API users.""" + commons([info('A.jpg', 1), info('New.jpg', 2), info(VOLOCHEK, 3), info(VOLOCHYOK, 4)], + other_ids={1100000000: {'kind': 'revision_id', 'name': 'C.jpg'}}) + result = run_check({'import_method': 'selected', 'file_names': [ + 'A.jpg', 'A.jpg', 'Gone.jpg', '99', '1100000000', VOLOCHEK, VOLOCHYOK]}, + CAMPAIGN, source='local') + codes = [(r['row'], r['reason_code'], r['reason_params']) for r in result['rows']] + assert codes == [ + (1, None, []), + (2, 'duplicate', ['1']), + (3, 'unknown-name', ['Gone.jpg']), + (4, 'unknown-file-id', ['99']), + (5, 'revision-id', ['1100000000', 'C.jpg']), + (6, 'same-name', [VOLOCHYOK, '7']), + (7, 'same-name', [VOLOCHEK, '6'])] + issue = summarize(result, 't' * 32)['issues'][0] + assert issue['reason_code'] == 'duplicate' and issue['reason_params'] == ['1'] + + +def test_source_errors_carry_a_reason_code(): + """Source errors reach the client with reason_code / reason_params.""" + from montage.utils import ImportSourceInvalid + err = ImportSourceInvalid('x', reason_code='https-only', reason_params=['http://a']) + assert err.to_dict()['reason_code'] == 'https-only' + assert err.to_dict()['reason_params'] == ['http://a'] + assert err.to_dict()['error_type'] == 'import_source_invalid' + +# --------------------------------------------------------------------------- +# Check files +# --------------------------------------------------------------------------- + +@pytest.fixture +def config(tmpdir): + return {'import_check_path': str(tmpdir.join('checks'))} + + +def _result(campaign_id=CAMPAIGN, method='csv', checked_at=None): + now = checked_at or datetime.datetime.now(datetime.timezone.utc) + return {'format': 1, 'checked_at': now.strftime('%Y-%m-%dT%H:%M:%SZ'), + 'campaign_id': campaign_id, 'import_method': method, + 'source': {'csv_url': 'x'}, 'lookup': 'local', + 'columns': {}, 'rows': []} + + +def test_save_and_load(config): + token = save(_result(), config) + assert len(token) == 32 + folder = config['import_check_path'] + assert os.stat(folder).st_mode & 0o777 == 0o700 + path = os.path.join(folder, token + '.json') + assert os.stat(path).st_mode & 0o777 == 0o600 + assert os.listdir(folder) == [token + '.json'] # no temp file left + assert load(token, config, CAMPAIGN, 'gistcsv')['campaign_id'] == CAMPAIGN + + +def test_load_refuses_bad_tokens(config): + token = save(_result(), config) + old = save(_result(checked_at=datetime.datetime.now(datetime.timezone.utc) + - datetime.timedelta(days=8)), config) + for bad, args in [('../x', (CAMPAIGN,)), + ('../' + token[3:], (CAMPAIGN,)), + ('A' * 32, (CAMPAIGN,)), + (None, (CAMPAIGN,)), + (old, (CAMPAIGN,)), + (token, (CAMPAIGN + 1,)), + (token, (CAMPAIGN, 'category'))]: + with pytest.raises(ImportCheckExpired): + load(bad, config, *args) + + +def test_save_cleans_up_only_old_check_files(config, tmpdir): + keep = save(_result(), config) + stale = save(_result(), config) + folder = config['import_check_path'] + week_ago = time.time() - 8 * 24 * 3600 + for name in (stale + '.json', 'notes.txt', 'export.json', '.tmp-x1.json'): + path = os.path.join(folder, name) + if not os.path.exists(path): + open(path, 'w').close() + os.utime(path, (week_ago, week_ago)) + newest = save(_result(), config) + assert sorted(os.listdir(folder)) == sorted( + [keep + '.json', newest + '.json', 'notes.txt', 'export.json']) + + +def test_save_does_not_change_an_existing_folder(tmpdir): + folder = tmpdir.mkdir('shared') + os.chmod(str(folder), 0o755) + save(_result(), {'import_check_path': str(folder)}) + assert os.stat(str(folder)).st_mode & 0o777 == 0o755 + + +def test_save_keeps_at_most_max_check_files(config, monkeypatch): + monkeypatch.setattr(import_check, 'MAX_CHECK_FILES', 3) + tokens = [] + for i in range(5): + tokens.append(save(_result(), config)) + path = os.path.join(config['import_check_path'], tokens[-1] + '.json') + recent = time.time() - 3600 + i # within 7 days, oldest first + os.utime(path, (recent, recent)) + left = sorted(os.listdir(config['import_check_path'])) + assert left == sorted(t + '.json' for t in tokens[-3:]) + + +def test_check_file_has_no_montage_user(monkeypatch, commons, config): + commons([info('A.jpg', 1, img_user_text='CommonsUploader')]) + token = save(csv_check(monkeypatch, 'A.jpg\n'), config) + with open(os.path.join(config['import_check_path'], token + '.json')) as f: + raw = f.read() + stored = json.loads(raw) + assert set(stored) == {'format', 'checked_at', 'campaign_id', 'import_method', + 'source', 'lookup', 'columns', 'rows'} + assert 'CommonsUploader' in raw # Commons data is kept (approved) + assert 'Yarl' not in raw and 'Slaporte' not in raw + + +def test_check_dir_required_outside_dev(monkeypatch, tmpdir): + monkeypatch.delenv('MONTAGE_IMPORT_CHECK_PATH', raising=False) + good = str(tmpdir.mkdir('checks')) + for env in ('beta', 'prod', 'devlabs', 'staging'): + assert 'MONTAGE_IMPORT_CHECK_PATH' in import_check.check_dir_problem({}, env) + assert import_check.check_dir_problem({'import_check_path': good}, env) is None + for env in ('dev', 'devtest'): + assert import_check.check_dir_problem({}, env) is None + monkeypatch.setenv('MONTAGE_IMPORT_CHECK_PATH', good) + assert import_check.check_dir_problem({}, 'prod') is None + + +def test_check_dir_must_be_a_folder_of_its_own(tmpdir): + """B1: a typo such as the tool's home must not start the app (its + cleanup would delete files there).""" + home = tmpdir.mkdir('home') + home.join('replica.my.cnf').write('x') + for path, why in [('relative/checks', 'absolute'), + (str(home), 'other files'), + (str(tmpdir.join('missing', 'deeper')), 'does not exist')]: + problem = import_check.check_dir_problem({'import_check_path': path}, 'prod') + assert problem and why in problem, (path, problem) + # a folder that does not exist yet, in an existing parent, is fine + assert import_check.check_dir_problem( + {'import_check_path': str(home.join('import_checks'))}, 'prod') is None + + +def test_header_less_list_with_comma_names(monkeypatch, commons): + """A gist that lists the #645 pair one per line, unquoted: the commas + stay in the names and the pair is found.""" + commons([info(VOLOCHEK, 149673020), info(VOLOCHYOK, 149673022)]) + result = csv_check(monkeypatch, u'%s\n%s\n' % (VOLOCHEK.replace('_', ' '), VOLOCHYOK)) + assert [(r['row'], r['commons_name'], r['status']) for r in result['rows']] == [ + (1, VOLOCHEK, 'same_name'), (2, VOLOCHYOK, 'same_name')] + + +# --------------------------------------------------------------------------- +# Endpoints: check, download, import by token +# --------------------------------------------------------------------------- + +from montage.tests.test_web_basic import montage_app, api_client # noqa: E402,F401 +from montage.tests.test_import_entries import ( # noqa: E402,F401 + COORD, coord_client, new_round, db_query, entry_ids, round_rows) + + +def _campaign_of(client, round_id): + return client.fetch('coordinator: get round', '/admin/round/%s' % round_id, + as_user=COORD)['data']['campaign']['id'] + + +@pytest.fixture +def local_commons(monkeypatch, commons): + """Endpoint tests run as 'dev'; look Commons up 'locally' (stand-in) + so lookups by file_id happen, as on Toolforge.""" + monkeypatch.setattr(import_check, 'lookup_source', lambda: 'local') + return commons + + +def _source(monkeypatch, text): + monkeypatch.setattr(import_check, 'fetch_source_text', lambda url: text) + + +def _check(client, campaign_id, error_code=None, **request): + request.setdefault('import_method', 'csv') + request.setdefault('csv_url', 'https://example.org/list.csv') + kw = {'error_code': error_code} if error_code else {} + return client.fetch('coordinator: check import source', + '/admin/campaign/%s/import/check' % campaign_id, + request, as_user=COORD, **kw) + + +def _import(client, round_id, token, method='csv', error_code=None): + data = {'import_method': method} + if token: + data['check_token'] = token + kw = {'error_code': error_code} if error_code else {} + return client.fetch('coordinator: import', '/admin/round/%s/import' % round_id, + data, as_user=COORD, **kw) + + +def _body(resp): + return resp.get_data(as_text=True) + + +def test_check_endpoint_writes_no_rows(montage_app, coord_client, local_commons, + monkeypatch): + local_commons([info('A.jpg', 1)]) + _source(monkeypatch, 'filename,File ID\nA.jpg,x\nGone.jpg,\n') + round_id = new_round(coord_client, 'chk') + data = _check(coord_client, _campaign_of(coord_client, round_id))['data'] + assert len(data['token']) == 32 + assert data['counts']['ok'] == 1 and data['counts']['unknown_name'] == 1 + assert data['columns']['ignored'] == ['File ID'] + assert not data['blocking'] and data['importable_count'] == 1 + assert [i['row'] for i in data['issues']] == [3] + assert db_query(montage_app, 'SELECT id FROM entries') == [] + assert db_query(montage_app, 'SELECT id FROM round_sources') == [] + + +def test_check_and_download_need_a_coordinator(montage_app, coord_client, + local_commons, monkeypatch): + local_commons([info('A.jpg', 1)]) + _source(monkeypatch, 'A.jpg\n') + round_id = new_round(coord_client, 'perm') + campaign_id = _campaign_of(coord_client, round_id) + token = _check(coord_client, campaign_id)['data']['token'] + coord_client.fetch('maintainer: add organizer', '/admin/add_organizer', + {'username': 'Outsider'}) + coord_client.fetch('outsider: check', '/admin/campaign/%s/import/check' % campaign_id, + {'import_method': 'csv', 'csv_url': 'x'}, as_user='Outsider', + error_code=403) + coord_client.fetch('outsider: download', + '/admin/campaign/%s/import/check/%s/download' % (campaign_id, token), + as_user='Outsider', error_code=403) + + +def test_download(montage_app, coord_client, local_commons, monkeypatch): + local_commons([info('A.jpg', 1), info('B_new.jpg', 2)]) + _source(monkeypatch, 'filename,file_id\nA.jpg,1\nB old.jpg,2\nGone.jpg,\n') + round_id = new_round(coord_client, 'dl') + campaign_id = _campaign_of(coord_client, round_id) + token = _check(coord_client, campaign_id)['data']['token'] + resp = coord_client.fetch('coordinator: download check', + '/admin/campaign/%s/import/check/%s/download' + % (campaign_id, token), as_user=COORD) + assert resp.headers['Content-Disposition'].startswith( + 'attachment; filename=montage_import-%s-' % campaign_id) + assert _body(resp) == 'filename,file_id\nA.jpg,1\nB_new.jpg,2\n' + + +def test_import_without_a_token_is_refused(montage_app, coord_client): + round_id = new_round(coord_client, 'notoken') + for method in ('csv', 'gistcsv', 'category', 'selected'): + resp = _import(coord_client, round_id, None, method, error_code=400) + assert 'import_check_required' in _body(resp) + assert db_query(montage_app, 'SELECT id FROM round_sources') == [] + + +def test_import_takes_the_checked_list_without_lookups(montage_app, coord_client, + local_commons, monkeypatch): + """#509 + #510: a full CSV with stale metadata imports Commons' data and + file_ids; the import fetches nothing and asks Commons nothing.""" + local_commons([info('A.jpg', 1, img_width=4000), info('B_new.jpg', 2)]) + _source(monkeypatch, 'img_name,file_id,img_width,img_user_text\n' + 'A.jpg,,10,Stale\nB old.jpg,2,10,Stale\n') + round_id = new_round(coord_client, 'bytoken') + token = _check(coord_client, _campaign_of(coord_client, round_id))['data']['token'] + + def boom(*a, **kw): + raise AssertionError('looked something up during the import') + for name in ('fetch_source_text', 'lookup_by_names', 'lookup_by_ids', + 'category_records'): + monkeypatch.setattr(import_check, name, boom) + from montage import labs + monkeypatch.setattr(labs, '_connect', boom) + with responses_lib.RequestsMock(): # any HTTP request fails + data = _import(coord_client, round_id, token)['data'] + + assert data['new_round_entry_count'] == 2 + assert ['renamed'] in [list(w) for w in data['warnings']] + assert all(len(w) == 1 for w in data['warnings']) # one-key dicts + rows = round_rows(montage_app, round_id) + assert [(r['name'], r['file_id'], r['width'], r['upload_user_text']) + for r in rows] == [('A.jpg', 1, 4000, 'Uploader'), + ('B_new.jpg', 2, 3000, 'Uploader')] + assert rows[0]['source_params']['csv_url'] == 'https://example.org/list.csv' + + +def test_round_source_records_a_check_fingerprint_not_the_token( + montage_app, coord_client, local_commons, monkeypatch): + """B10: round_sources.params is shown on the public /entry page, so the + token itself is not stored; a second check of the same source reuses + the round source.""" + import hashlib + local_commons([info('A.jpg', 1), info('B.jpg', 2)]) + _source(monkeypatch, 'A.jpg\n') + round_id = new_round(coord_client, 'param') + campaign_id = _campaign_of(coord_client, round_id) + token = _check(coord_client, campaign_id)['data']['token'] + _import(coord_client, round_id, token) + _source(monkeypatch, 'A.jpg\nB.jpg\n') + _import(coord_client, round_id, _check(coord_client, campaign_id)['data']['token']) + params = db_query(montage_app, 'SELECT params FROM round_sources WHERE round_id = :r', + r=round_id) + assert len(params) == 1 + assert json.loads(params[0]['params']) == { + 'csv_url': 'https://example.org/list.csv', + 'check_id': hashlib.sha256(token.encode('ascii')).hexdigest()[:12]} + page = coord_client.fetch('public: entry', '/entry/A.jpg') + assert token not in json.dumps(page) + + +def test_blocked_check_writes_nothing_and_retry_uses_the_same_round( + montage_app, coord_client, local_commons, monkeypatch): + files = [info('F%03d.jpg' % i, 1000 + i) for i in range(100)] + local_commons(files) + rows = ''.join('F%03d.jpg,%d\n' % (i, 1000 + i) for i in range(100)) + _source(monkeypatch, 'filename,file_id\n' + rows.replace(',1050\n', ',9999\n')) + round_id = new_round(coord_client, 'blocked') + campaign_id = _campaign_of(coord_client, round_id) + check = _check(coord_client, campaign_id)['data'] + assert check['blocking'] and check['counts']['unknown_file_id'] == 1 + + resp = _import(coord_client, round_id, check['token'], error_code=400) + assert 'import_check_blocked' in _body(resp) + assert 'row 52: no file on Commons has file ID 9999' in _body(resp) + assert db_query(montage_app, 'SELECT id FROM entries') == [] + assert round_rows(montage_app, round_id) == [] + + # the coordinator fixes the source, checks again, imports into the + # round that already exists (the form does not create a second one) + _source(monkeypatch, 'filename,file_id\n' + rows) + check = _check(coord_client, campaign_id)['data'] + data = _import(coord_client, round_id, check['token'])['data'] + assert data['new_round_entry_count'] == 100 + rounds = coord_client.fetch('coordinator: campaign', '/admin/campaign/%s' % campaign_id, + as_user=COORD)['data']['rounds'] + assert len(rounds) == 1 + + +def test_bad_tokens_via_the_endpoint(montage_app, coord_client, local_commons, + monkeypatch): + local_commons([info('A.jpg', 1)]) + _source(monkeypatch, 'A.jpg\n') + round_a = new_round(coord_client, 'tok a') + round_b = new_round(coord_client, 'tok b') + token_b = _check(coord_client, _campaign_of(coord_client, round_b))['data']['token'] + for token, method in [('../../etc/passwd', 'csv'), ('x' * 32, 'csv'), + (token_b, 'csv'), # another campaign + ]: + resp = _import(coord_client, round_a, token, method, error_code=400) + assert 'import_check_expired' in _body(resp) + resp = _import(coord_client, round_b, token_b, 'category', error_code=400) + assert 'import_check_expired' in _body(resp) + assert db_query(montage_app, 'SELECT id FROM round_sources') == [] + + +def _after(monkeypatch, **delta): + later = datetime.datetime.now(datetime.timezone.utc) + datetime.timedelta(**delta) + monkeypatch.setattr(import_check, '_utcnow', lambda: later) + + +def test_a_check_is_valid_for_one_hour(montage_app, coord_client, local_commons, + monkeypatch, tmpdir): + """Files are renamed and deleted on Commons all the time: a check is + used within the hour or done again.""" + local_commons([info('A.jpg', 1)]) + _source(monkeypatch, 'A.jpg\n') + round_id = new_round(coord_client, 'old check') + data = _check(coord_client, _campaign_of(coord_client, round_id))['data'] + assert (import_check._parse_iso(data['expires_at']) + - import_check._parse_iso(data['checked_at'])) == datetime.timedelta(hours=1) + _after(monkeypatch, minutes=61) + resp = _import(coord_client, round_id, data['token'], error_code=400) + assert 'import_check_expired' in _body(resp) and '1 hour' in _body(resp) + _after(monkeypatch, minutes=59) + assert _import(coord_client, round_id, data['token'])['data']['new_round_entry_count'] == 1 + + +def test_downloads_work_until_the_file_is_deleted(montage_app, coord_client, + local_commons, monkeypatch): + """The upload file and the issues report stay available for the file's + lifetime (at most 7 days), also after the check can no longer be + imported.""" + local_commons([info('A.jpg', 1)]) + _source(monkeypatch, 'A.jpg\nGone.jpg\n') + round_id = new_round(coord_client, 'downloads') + campaign_id = _campaign_of(coord_client, round_id) + token = _check(coord_client, campaign_id)['data']['token'] + url = '/admin/campaign/%s/import/check/%s/' % (campaign_id, token) + _after(monkeypatch, days=2) + for kind in ('download', 'issues'): + coord_client.fetch('coordinator: ' + kind, url + kind, as_user=COORD) + _after(monkeypatch, days=8) + resp = coord_client.fetch('coordinator: download', url + 'download', as_user=COORD, + error_code=400) + assert 'import_check_expired' in _body(resp) + + +def test_every_use_of_the_folder_cleans_up(config): + """Files are deleted after at most 7 days whenever the folder is used: + a check, an import, a download, or the app starting.""" + stale, fresh = save(_result(), config), save(_result(), config) + folder = config['import_check_path'] + week_ago = time.time() - 8 * 24 * 3600 + os.utime(os.path.join(folder, stale + '.json'), (week_ago, week_ago)) + with pytest.raises(ImportCheckExpired): + load('x' * 32, config, CAMPAIGN) # a failed load cleans up too + assert os.listdir(folder) == [fresh + '.json'] + os.utime(os.path.join(folder, fresh + '.json'), (week_ago, week_ago)) + import_check.cleanup(config) # what the app runs at start + assert os.listdir(folder) == [] + + +def test_app_start_cleans_up_the_check_folder(tmpdir): + from montage.app import create_app + from montage import utils + folder = tmpdir.mkdir('checks') + old = folder.join('A' * 32 + '.json') + old.write('{}') + week_ago = time.time() - 8 * 24 * 3600 + os.utime(str(old), (week_ago, week_ago)) + config = utils.load_env_config(env_name='devtest') + config['db_url'] = 'sqlite:///' + str(tmpdir.join('app.db')) + config['import_check_path'] = str(folder) + from montage.tests.test_web_basic import _create_schema + _create_schema(config['db_url'], echo=False) + create_app('devtest', config=config) + assert folder.listdir() == [] + + +def test_renamed_file_already_in_montage_keeps_its_row(montage_app, coord_client, + local_commons, monkeypatch): + """Review fix 1: a file Montage already has (found by file_id) keeps its + row and stored name; only a file new to Montage gets Commons' name.""" + local_commons([info('Old_name.jpg', 5)]) + _source(monkeypatch, 'Old_name.jpg\n') + round_a = new_round(coord_client, 'ren a') + _import(coord_client, round_a, _check(coord_client, _campaign_of(coord_client, round_a))['data']['token']) + ids = entry_ids(montage_app, 'Old_name') + + local_commons([info('New_name.jpg', 5)]) + _source(monkeypatch, 'filename,file_id\nOld name.jpg,5\n') + round_b = new_round(coord_client, 'ren b') + check = _check(coord_client, _campaign_of(coord_client, round_b))['data'] + assert check['counts']['renamed'] == 1 + data = _import(coord_client, round_b, check['token'])['data'] + assert data['new_round_entry_count'] == 1 + assert entry_ids(montage_app, 'Old_name') == ids + assert entry_ids(montage_app, 'New_name') == {} + + +def test_category_same_name_pair_keeps_the_first(montage_app, coord_client, + local_commons): + local_commons([info(VOLOCHEK, 149673020), info(VOLOCHYOK, 149673022)]) + round_id = new_round(coord_client, 'cat pair') + check = _check(coord_client, _campaign_of(coord_client, round_id), + import_method='category', category='Cat')['data'] + assert not check['blocking'] and check['importable_count'] == 1 + data = _import(coord_client, round_id, check['token'], 'category')['data'] + assert data['new_round_entry_count'] == 1 + same = [w['same name'] for w in data['warnings'] if 'same name' in w] + assert len(same) == 1 and VOLOCHEK in same[0] and VOLOCHYOK in same[0] + + +def test_name_taken_by_another_file_in_montage_is_left_out(montage_app, coord_client, + local_commons, monkeypatch): + """Where entries.name has a unique index (beta, fresh installs), a new + file whose name the database treats as equal to an existing row of + another file cannot be stored: the save failed with IntegrityError 1062 + (beta, 2026-10-10). The check now finds it: a category leaves it out, + a list blocks. Here: the same name, a different file (re-uploaded).""" + local_commons([info('Photo.jpg', 1)]) + _source(monkeypatch, 'filename,file_id\nPhoto.jpg,1\n') + round_a = new_round(coord_client, 'taken a') + token = _check(coord_client, _campaign_of(coord_client, round_a))['data']['token'] + _import(coord_client, round_a, token) + + local_commons([info('Photo.jpg', 2), info('Other.jpg', 3)]) + round_b = new_round(coord_client, 'taken b') + check = _check(coord_client, _campaign_of(coord_client, round_b), + import_method='category', category='Cat')['data'] + assert check['counts']['same_name_existing'] == 1 + assert not check['blocking'] and check['importable_count'] == 1 + issue = check['issues'][0] + assert issue['reason_code'] == 'same-name-existing-category' + assert issue['reason_params'] == ['Photo.jpg'] + data = _import(coord_client, round_b, check['token'], 'category')['data'] + assert data['new_round_entry_count'] == 1 + + _source(monkeypatch, 'filename,file_id\nPhoto.jpg,2\nOther.jpg,3\n') + check = _check(coord_client, _campaign_of(coord_client, round_b))['data'] + assert check['blocking'] and check['counts']['same_name_existing'] == 1 + assert check['issues'][0]['reason_code'] == 'same-name-existing' + + +def test_name_taken_check_follows_the_database_comparison(commons, monkeypatch): + """The existing rows are those the database matches (on MariaDB: case, + accents, е/ё); the same file (same file_id) or an old row of exactly + this name without a file_id is reused, not a clash.""" + commons([info(u'Ленина_71А.jpg', 10), info('Same.jpg', 11), info('Legacy.jpg', 12), + info('Free.jpg', 13)]) + monkeypatch.setattr(import_check, '_name_must_be_unique', lambda session: True) + monkeypatch.setattr(import_check, '_existing_entries', lambda session, names: [ + (u'Ленина_71а.jpg', 9), ('same.jpg', 11), ('Legacy.jpg', None)]) + monkeypatch.setattr(import_check, 'same_name_keys', + lambda names, session=None: {n: n.lower() for n in names}) + result = run_check({'import_method': 'selected', 'file_names': [ + u'Ленина_71А.jpg', 'Same.jpg', 'Legacy.jpg', 'Free.jpg']}, + CAMPAIGN, source='local', rdb_session=object()) + assert statuses(result) == [(1, 'same_name_existing'), (2, 'ok'), (3, 'ok'), (4, 'ok')] + assert result['rows'][0]['reason_params'] == [u'Ленина_71а.jpg'] + assert [r['row'] for r in blocking_rows(result)] == [1] + + +def test_name_taken_check_is_skipped_without_a_unique_index(commons, monkeypatch): + """Production has no index on entries.name (#650): both rows are stored, + and looking names up there would scan the whole table.""" + commons([info('A.jpg', 1)]) + monkeypatch.setattr(import_check, '_name_must_be_unique', lambda session: False) + monkeypatch.setattr(import_check, '_existing_entries', lambda session, names: 1 / 0) + monkeypatch.setattr(import_check, 'same_name_keys', + lambda names, session=None: {n: n for n in names}) + result = run_check({'import_method': 'selected', 'file_names': ['A.jpg']}, + CAMPAIGN, source='local', rdb_session=object()) + assert statuses(result) == [(1, 'ok')] + + +def test_empty_import_is_not_reported_as_all_disqualified(montage_app, coord_client, + local_commons, monkeypatch): + """#208: an import that brought no files also warned 'all entries + disqualified by round settings' (0 >= 0).""" + local_commons([]) + _source(monkeypatch, 'Gone.jpg\n') + round_id = new_round(coord_client, 'empty') + token = _check(coord_client, _campaign_of(coord_client, round_id))['data']['token'] + warnings = _import(coord_client, round_id, token)['data']['warnings'] + keys = [k for w in warnings for k in w] + assert 'empty import' in keys and 'all disqualified' not in keys + + +def test_app_refuses_to_start_without_a_check_folder(monkeypatch): + from montage.app import create_app + monkeypatch.delenv('MONTAGE_IMPORT_CHECK_PATH', raising=False) + with pytest.raises(ValueError) as exc: + create_app('prod', config={'__file__': 'test', '__env__': 'prod', + 'db_url': 'sqlite://'}) + assert 'MONTAGE_IMPORT_CHECK_PATH' in str(exc.value) + + +# --------------------------------------------------------------------------- +# #447: a first round is created together with its import, or not at all +# --------------------------------------------------------------------------- + +def _new_campaign(client, name): + series_id = client.fetch('get default series', '/series')['data'][0]['id'] + return client.fetch('organizer: create campaign', '/admin/add_campaign', + {'name': name, 'coordinators': [COORD], + 'open_date': '2015-01-01T00:00:00', + 'close_date': '2016-01-01T00:00:00', + 'url': 'http://hatnote.com', 'series_id': series_id}, + as_user=COORD)['data']['id'] + + +def _add_round(client, campaign_id, import_request=None, error_code=None): + rnd = {'name': 'Round 1', 'vote_method': 'yesno', + 'deadline_date': '2016-10-15T00:00:00', + 'jurors': ['Slaporte', 'MahmoudHashemi', 'Effeietsanders']} + if import_request is not None: + rnd['import'] = import_request + kw = {'error_code': error_code} if error_code else {} + return client.fetch('coordinator: create round', + '/admin/campaign/%s/add_round' % campaign_id, + rnd, as_user=COORD, **kw) + + +def _rounds(client, campaign_id): + return client.fetch('coordinator: campaign', '/admin/campaign/%s' % campaign_id, + as_user=COORD)['data']['rounds'] + + +def test_create_round_with_its_import(montage_app, coord_client, local_commons, + monkeypatch): + local_commons([info('A.jpg', 1), info('B.jpg', 2)]) + _source(monkeypatch, 'A.jpg\nB.jpg\nGone.jpg\n') + campaign_id = _new_campaign(coord_client, 'atomic ok') + token = _check(coord_client, campaign_id)['data']['token'] + + data = _add_round(coord_client, campaign_id, + {'import_method': 'csv', 'check_token': token})['data'] + + assert data['import']['new_round_entry_count'] == 2 + assert any('import issues' in w for w in data['import']['warnings']) + assert len(_rounds(coord_client, campaign_id)) == 1 + assert len(round_rows(montage_app, data['id'])) == 2 + + +@pytest.mark.parametrize('case', ['no files', 'blocked', 'unknown token']) +def test_create_round_with_a_failing_import_creates_no_round( + montage_app, coord_client, local_commons, monkeypatch, case): + local_commons([info('A.jpg', 1)]) + _source(monkeypatch, {'no files': 'Gone.jpg\n', + 'blocked': 'filename,file_id\nA.jpg,99\n', + 'unknown token': 'A.jpg\n'}[case]) + campaign_id = _new_campaign(coord_client, 'atomic ' + case) + token = _check(coord_client, campaign_id)['data']['token'] + if case == 'unknown token': + token = 'x' * 32 + + resp = _add_round(coord_client, campaign_id, + {'import_method': 'csv', 'check_token': token}, error_code=400) + + expected = {'no files': 'import_empty', 'blocked': 'import_check_blocked', + 'unknown token': 'import_check_expired'}[case] + assert expected in _body(resp) + assert _rounds(coord_client, campaign_id) == [] + assert db_query(montage_app, 'SELECT id FROM round_sources') == [] + # nothing blocks the next attempt (#447: "one active round at a time") + assert _add_round(coord_client, campaign_id)['data']['id'] + + +def test_create_round_without_import_is_unchanged(montage_app, coord_client): + campaign_id = _new_campaign(coord_client, 'plain') + data = _add_round(coord_client, campaign_id)['data'] + assert 'import' not in data and len(_rounds(coord_client, campaign_id)) == 1 + + +def test_create_round_accepts_only_checked_import_methods(montage_app, coord_client): + """B12: add_round's import goes through the check; import_method + 'round' (advance) would reach a KeyError (500).""" + campaign_id = _new_campaign(coord_client, 'advance') + resp = _add_round(coord_client, campaign_id, + {'import_method': 'round', 'check_token': 'x'}, error_code=400) + assert 'import_method' in _body(resp) + assert _rounds(coord_client, campaign_id) == [] + + +def test_create_round_rolls_back_after_rows_were_written(montage_app, coord_client, + local_commons, monkeypatch): + """B12: a failure after the entries and round entries were inserted + leaves nothing behind either.""" + from montage import admin_endpoints + from montage.utils import InvalidAction + local_commons([info('Wr_A.jpg', 1), info('Wr_B.jpg', 2)]) + _source(monkeypatch, 'Wr_A.jpg\nWr_B.jpg\n') + campaign_id = _new_campaign(coord_client, 'late failure') + token = _check(coord_client, campaign_id)['data']['token'] + + def late_failure(*a, **kw): + raise InvalidAction('failed after the inserts') + monkeypatch.setattr(admin_endpoints, 'autodisqualify', late_failure) + _add_round(coord_client, campaign_id, {'import_method': 'csv', 'check_token': token}, + error_code=400) + monkeypatch.undo() + assert _rounds(coord_client, campaign_id) == [] + assert entry_ids(montage_app, 'Wr_') == {} + assert db_query(montage_app, 'SELECT id FROM round_entries') == [] + assert db_query(montage_app, 'SELECT id FROM round_sources') == [] + assert db_query(montage_app, "SELECT id FROM audit_log_entries WHERE action = 'add_round_entries'") == [] + + +def test_issues_report_download(montage_app, coord_client, local_commons, monkeypatch): + """B13: every row that is not ok, also beyond the 1000 shown in the + form, as a CSV report (the upload file leaves them out).""" + local_commons([info('A.jpg', 1)]) + _source(monkeypatch, 'filename,file_id\nA.jpg,1\n=Gone.jpg,\nX.jpg,99\n') + round_id = new_round(coord_client, 'issues') + campaign_id = _campaign_of(coord_client, round_id) + token = _check(coord_client, campaign_id)['data']['token'] + resp = coord_client.fetch('coordinator: issues report', + '/admin/campaign/%s/import/check/%s/issues' % (campaign_id, token), + as_user=COORD) + assert resp.headers['Content-Disposition'].startswith( + 'attachment; filename=montage_import_issues-%s-' % campaign_id) + rows = list(csv.reader(io.StringIO(_body(resp)))) + assert rows[0] == ['row', 'name', 'file_id', 'commons_name', 'status', 'reason'] + assert [(r[0], r[1], r[4]) for r in rows[1:]] == [ + ('3', "'=Gone.jpg", 'unknown_name'), ('4', 'X.jpg', 'unknown_file_id')] diff --git a/montage/tests/test_import_entries.py b/montage/tests/test_import_entries.py index d9a4f22f..c4b8ccdb 100644 --- a/montage/tests/test_import_entries.py +++ b/montage/tests/test_import_entries.py @@ -189,11 +189,11 @@ def new_round(api_client, name, config=None, quorum=None, open_date=OPEN_DATE, def import_category(api_client, round_id, **kw): - return api_client.fetch('coordinator: import category', - '/admin/round/%s/import' % round_id, - {'import_method': 'category', - 'category': 'Synthetic_test_category'}, - as_user=COORD, **kw) + return api_client.fetch_checked_import('coordinator: import category', + round_id, + {'import_method': 'category', + 'category': 'Synthetic_test_category'}, + as_user=COORD, **kw) def _load_json(raw): @@ -226,6 +226,8 @@ def round_rows(app, round_id, strip_prefix=''): row['flags'] = _load_json(row['flags']) row['re_flags'] = _load_json(row['re_flags']) row['source_params'] = _load_json(row['source_params']) + # random per check (#510); see test_round_source_records_a_check_fingerprint... + row['source_params'].pop('check_id', None) row['name'] = row['name'][len(strip_prefix):] if row['flags'].get('archive_name'): row['flags']['archive_name'] = row['flags']['archive_name'].replace(strip_prefix, '') @@ -481,10 +483,11 @@ def test_mysql_driver_batches_the_inserts(): # --------------------------------------------------------------------------- -# Lookup by file_id for campaigns after the cutoff (#513) +# Lookup by file_id (#513, #654); since #510 in every campaign, not only in +# campaigns opened from 2026-06-01 (the former FILE_ID_LOOKUP_CUTOFF) # --------------------------------------------------------------------------- -AFTER_CUTOFF = rdb.FILE_ID_LOOKUP_CUTOFF + datetime.timedelta(days=30) +RECENT_OPEN_DATE = datetime.datetime(2026, 6, 1) + datetime.timedelta(days=30) def with_file_ids(infos): @@ -495,8 +498,8 @@ def with_file_ids(infos): for info in infos] -def new_round_after_cutoff(api_client, name): - return new_round(api_client, name, open_date=AFTER_CUTOFF, +def new_recent_round(api_client, name): + return new_round(api_client, name, open_date=RECENT_OPEN_DATE, close_date='2026-12-31T00:00:00') @@ -508,11 +511,11 @@ def _no_file_id_lookup(*a, **kw): raise AssertionError('looked up entries by file_id') -def test_file_id_lookup_after_cutoff(montage_app, coord_client, +def test_file_id_lookup_for_a_recent_campaign(montage_app, coord_client, mock_external_apis, monkeypatch): first = with_file_ids(make_file_infos('fid', IMPORT_CHUNK_SIZE + 50)) more = with_file_ids(make_file_infos('fid', 30, start=500)) - round_id = new_round_after_cutoff(coord_client, 'fid') + round_id = new_recent_round(coord_client, 'fid') # production's indexes on entries: file_id yes, name no monkeypatch.setattr(rdb, '_entries_indexes', lambda session: ( {'PRIMARY', 'ix_entries_file_id'}, {'id', 'file_id'})) @@ -531,19 +534,22 @@ def test_file_id_lookup_after_cutoff(montage_app, coord_client, assert len(ids_after) == len(first) + 30 assert {k: ids_after[k] for k in ids_before} == ids_before - # a second campaign after the cutoff reuses the same rows - other = new_round_after_cutoff(coord_client, 'fid other') + # a second campaign reuses the same rows + other = new_recent_round(coord_client, 'fid other') data = import_category(coord_client, other)['data'] assert data['new_round_entry_count'] == len(first) + 30 assert entry_ids(montage_app, 'fid') == ids_after -def test_name_lookup_before_cutoff(montage_app, coord_client, - mock_external_apis, monkeypatch): +def test_file_id_lookup_in_old_campaigns_too(montage_app, coord_client, + mock_external_apis, monkeypatch): + """#510: checked imports match by file_id whatever the campaign's open + date; #654 did so only from 2026-06-01.""" infos = with_file_ids(make_file_infos('old', 40)) round_id = new_round(coord_client, 'old') # opens in 2015 - monkeypatch.setattr(CoordinatorDAO, 'get_entry_file_id_map', - _no_file_id_lookup) + monkeypatch.setattr(rdb, '_entries_indexes', lambda session: ( + {'PRIMARY', 'ix_entries_file_id'}, {'id', 'file_id'})) + monkeypatch.setattr(CoordinatorDAO, 'get_entry_name_map', _no_name_lookup) mock_category(mock_external_apis, infos) import_category(coord_client, round_id) assert len(entry_ids(montage_app, 'old')) == 40 @@ -554,7 +560,7 @@ def test_name_lookup_when_a_file_id_is_missing(montage_app, coord_client, monkeypatch): infos = make_file_infos('nofid', 40) # some have file_id None assert any(info['file_id'] is None for info in infos) - round_id = new_round_after_cutoff(coord_client, 'nofid') + round_id = new_recent_round(coord_client, 'nofid') monkeypatch.setattr(CoordinatorDAO, 'get_entry_file_id_map', _no_file_id_lookup) mock_category(mock_external_apis, infos) @@ -566,13 +572,13 @@ def test_file_id_lookup_keeps_the_row_of_a_renamed_file(montage_app, coord_client, mock_external_apis): infos = with_file_ids(make_file_infos('ren', 3)) - round_a = new_round_after_cutoff(coord_client, 'ren a') + round_a = new_recent_round(coord_client, 'ren a') mock_category(mock_external_apis, infos) import_category(coord_client, round_a) ids = entry_ids(montage_app, 'ren') renamed = [dict(infos[0], img_name='ren_renamed.jpg')] + infos[1:] - round_b = new_round_after_cutoff(coord_client, 'ren b') + round_b = new_recent_round(coord_client, 'ren b') mock_category(mock_external_apis, renamed) data = import_category(coord_client, round_b)['data'] assert data['new_round_entry_count'] == 3 @@ -581,7 +587,7 @@ def test_file_id_lookup_keeps_the_row_of_a_renamed_file(montage_app, def test_file_id_lookup_keeps_the_row_of_a_file_without_file_id( montage_app, coord_client, mock_external_apis): - # imported before the cutoff, so stored without a file_id + # imported before 2026-05-31, so stored without a file_id infos = with_file_ids(make_file_infos('pre', 5)) old_round = new_round(coord_client, 'pre old') mock_category(mock_external_apis, @@ -589,9 +595,9 @@ def test_file_id_lookup_keeps_the_row_of_a_file_without_file_id( import_category(coord_client, old_round) ids = entry_ids(montage_app, 'pre') - # the same files after the cutoff, now with a file_id: where entries.name + # the same files later, now with a file_id: where entries.name # is indexed (here unique, as on beta) they keep their rows - new = new_round_after_cutoff(coord_client, 'pre new') + new = new_recent_round(coord_client, 'pre new') mock_category(mock_external_apis, infos) data = import_category(coord_client, new)['data'] assert data['new_round_entry_count'] == 5 @@ -625,7 +631,7 @@ def test_entries_indexes_reads_the_database(): rdb.Base.metadata.create_all(engine) names, first_columns = rdb._entries_indexes(sessionmaker(bind=engine)()) assert 'name' in first_columns # the model's unique index - assert 'ix_entries_file_id' not in names # only migrate_prod_db.sql adds it + assert 'ix_entries_file_id' in names # declared in the model (#510); prod: migrate_prod_db.sql def test_file_id_lookups_stay_below_the_index_dive_limit( @@ -643,9 +649,25 @@ def recording_lookup(self, file_ids): monkeypatch.setattr(CoordinatorDAO, 'get_entry_file_id_map', recording_lookup) infos = with_file_ids(make_file_infos('dive', 450)) - round_id = new_round_after_cutoff(coord_client, 'dive') + round_id = new_recent_round(coord_client, 'dive') mock_category(mock_external_apis, infos) import_category(coord_client, round_id) assert len(entry_ids(montage_app, 'dive')) == 450 assert sizes and max(sizes) <= rdb.FILE_ID_LOOKUP_CHUNK_SIZE + + +def test_checked_import_keeps_two_files_whose_names_differ_in_case( + montage_app, coord_client, mock_external_apis, monkeypatch): + """Matched by file_id, two different files are two entries, also when + their names differ only in case (the import check, not add_entries, + decides about names the database treats as equal; here it is off).""" + from montage import import_check + monkeypatch.setattr(import_check, 'SAME_NAME_CHECK', False) + infos = [dict(i, img_name=n) for i, n in zip( + with_file_ids(make_file_infos('case', 2)), ['Case_Photo.JPG', 'Case_photo.jpg'])] + round_id = new_round(coord_client, 'case') + mock_category(mock_external_apis, infos) + data = import_category(coord_client, round_id)['data'] + assert data['new_round_entry_count'] == 2 + assert sorted(entry_ids(montage_app, 'Case_')) == ['Case_Photo.JPG', 'Case_photo.jpg'] diff --git a/montage/tests/test_loaders.py b/montage/tests/test_loaders.py index 98e6aa96..9dc2c040 100644 --- a/montage/tests/test_loaders.py +++ b/montage/tests/test_loaders.py @@ -9,77 +9,38 @@ import responses from pytest import raises -from montage.loaders import get_entries_from_gsheet, make_entry +from montage.loaders import (make_entry, parse_source_rows, parse_name_list, + pasted_csv_text, + parse_file_id, normalise_name, fetch_source_text, + guard_cell, unguard_cell) +from montage.utils import ImportSourceInvalid from .conftest import ( FIXTURE_FILE_INFOS, FIXTURE_FULL_CSV, FIXTURE_FILENAME_CSV, - TOOLFORGE_FILE_URL, REUPLOAD_FILE_INFO, ) RESULTS = 'https://docs.google.com/spreadsheets/d/1RDlpT23SV_JB1mIz0OA-iuc3MNdNVLbaK_LtWAC7vzg/edit?usp=sharing' -FILENAME_LIST = 'https://docs.google.com/spreadsheets/d/1Nqj-JsX3L5qLp5ITTAcAFYouglbs5OpnFwP6zSFpa0M/edit?usp=sharing' -GENERIC_CSV = 'https://docs.google.com/spreadsheets/d/1WzHFg_bhvNthRMwNmxnk010KJ8fwuyCrby29MvHUzH8/edit#gid=550467819' FORBIDDEN_SHEET = 'https://docs.google.com/spreadsheets/d/1tza92brMKkZBTykw3iS6X9ij1D4_kvIYAiUlq1Yi7Fs/edit' # Pre-compute the actual fetch URLs that loaders.py constructs from the # doc IDs embedded in the spreadsheet URLs above. _RESULTS_CSV_URL = 'https://docs.google.com/spreadsheets/d/1RDlpT23SV_JB1mIz0OA-iuc3MNdNVLbaK_LtWAC7vzg/gviz/tq?tqx=out:csv' -_FILENAME_CSV_URL = 'https://docs.google.com/spreadsheets/d/1Nqj-JsX3L5qLp5ITTAcAFYouglbs5OpnFwP6zSFpa0M/gviz/tq?tqx=out:csv' -_GENERIC_CSV_URL = 'https://docs.google.com/spreadsheets/d/1WzHFg_bhvNthRMwNmxnk010KJ8fwuyCrby29MvHUzH8/gviz/tq?tqx=out:csv' _FORBIDDEN_CSV_URL = 'https://docs.google.com/spreadsheets/d/1tza92brMKkZBTykw3iS6X9ij1D4_kvIYAiUlq1Yi7Fs/gviz/tq?tqx=out:csv' -@responses.activate -def test_load_results(): - """Full CSV with all required columns -- entries created directly.""" - responses.add( - responses.GET, - _RESULTS_CSV_URL, - body=FIXTURE_FULL_CSV, - status=200, - content_type='text/csv', - ) - imgs, warnings = get_entries_from_gsheet(RESULTS, source='remote') - assert len(imgs) == len(FIXTURE_FILE_INFOS) - - -@responses.activate -def test_load_filenames(): - """Partial CSV with only 'filename' column -- triggers Toolforge lookup.""" - responses.add( - responses.GET, - _FILENAME_CSV_URL, - body=FIXTURE_FILENAME_CSV, - status=200, - content_type='text/csv', - ) - # The filename-only CSV triggers load_partial_csv -> load_name_list - # -> get_by_filename_remote -> POST to Toolforge /file endpoint. - responses.add( - responses.POST, - TOOLFORGE_FILE_URL, - json={'file_infos': FIXTURE_FILE_INFOS, 'no_info': []}, - status=200, - ) - imgs, warnings = get_entries_from_gsheet(FILENAME_LIST, source='remote') - assert len(imgs) == len(FIXTURE_FILE_INFOS) - +# --------------------------------------------------------------------------- +# Fetching a source (hatnote/montage#510): no Commons lookup here +# --------------------------------------------------------------------------- @responses.activate -def test_load_csv(): - """Generic full CSV -- same path as test_load_results.""" - responses.add( - responses.GET, - _GENERIC_CSV_URL, - body=FIXTURE_FULL_CSV, - status=200, - content_type='text/csv', - ) - imgs, warnings = get_entries_from_gsheet(GENERIC_CSV, source='remote') - assert len(imgs) == len(FIXTURE_FILE_INFOS) +def test_fetch_sheet_returns_its_csv_text(): + responses.add(responses.GET, _RESULTS_CSV_URL, body=FIXTURE_FULL_CSV, + status=200, content_type='text/csv') + assert fetch_source_text(RESULTS) == FIXTURE_FULL_CSV + assert len(responses.calls) == 1 @responses.activate @@ -92,8 +53,146 @@ def test_no_persmission(): status=200, content_type='text/html', ) - with raises(ValueError): - get_entries_from_gsheet(FORBIDDEN_SHEET, source='remote') + with raises(ImportSourceInvalid): + fetch_source_text(FORBIDDEN_SHEET) + + +@responses.activate +def test_fetch_gist_rewrites_the_link_and_strips_the_bom(): + responses.add(responses.GET, 'https://gist.githubusercontent.com/u/abc/raw', + body=u'\ufefffilename\nA.jpg\n'.encode('utf8'), status=200) + assert fetch_source_text('https://gist.github.com/u/abc') == 'filename\nA.jpg\n' + + +@responses.activate +def test_fetch_errors_are_import_source_invalid(): + responses.add(responses.GET, 'https://gist.githubusercontent.com/u/gone/raw', + body='Not Found', status=404) + with raises(ImportSourceInvalid) as exc: + fetch_source_text('https://gist.github.com/u/gone') + assert '404' in exc.value.detail + with raises(ImportSourceInvalid): # unregistered: ConnectionError + fetch_source_text('https://gist.github.com/u/unreachable') + with raises(ImportSourceInvalid): + fetch_source_text('https://docs.google.com/spreadsheets/nodoc') + with raises(ImportSourceInvalid): + fetch_source_text(' ') + + +# --------------------------------------------------------------------------- +# Parsing a source +# --------------------------------------------------------------------------- + +def _names(rows): + return [r['name'] for r in rows] + + +def test_parse_filename_csv(): + rows, columns = parse_source_rows(FIXTURE_FILENAME_CSV) + assert _names(rows) == [i['img_name'] for i in FIXTURE_FILE_INFOS] + assert [r['row'] for r in rows[:2]] == [2, 3] # header is row 1 + assert columns == {'name': 'filename', 'file_id': None, 'ignored': []} + + +def test_parse_full_csv_reads_only_the_name_column(): + """#509: metadata columns are ignored; Commons supplies them.""" + rows, columns = parse_source_rows(FIXTURE_FULL_CSV) + assert _names(rows) == [i['img_name'] for i in FIXTURE_FILE_INFOS] + assert columns['name'] == 'img_name' + assert 'img_width' in columns['ignored'] and 'img_name' not in columns['ignored'] + assert set(rows[0]) == {'row', 'name_as_written', 'file_id_as_written', + 'name', 'file_id', 'malformed_file_id'} + + +def test_parse_headers_case_bom_and_preference(): + rows, columns = parse_source_rows(u'\ufeffFile_ID, FileName ,img_name,File ID\n' + u'42,A b.jpg,Other.jpg,x\n') + assert columns == {'name': 'filename', 'file_id': 'file_id', + 'ignored': ['img_name', 'File ID']} + assert rows[0]['name'] == 'A_b.jpg' and rows[0]['file_id'] == 42 + assert rows[0]['name_as_written'] == 'A b.jpg' + + +def test_parse_rejects_columns_without_a_name_column(): + with raises(ImportSourceInvalid) as exc: + parse_source_rows('a,b\n1,2\n') + assert 'no filename or file_id column' in exc.value.detail + assert exc.value.reason_code == 'no-name-column' + + +def test_parse_header_less_lists(): + # a gist: commas in names survive, File: prefixes go, blank lines keep numbers + rows, columns = parse_source_rows('Foo, bar.jpg\n\nFile:Baz qux.png\n') + assert [(r['row'], r['name']) for r in rows] == [(1, 'Foo,_bar.jpg'), + (3, 'Baz_qux.png')] + assert columns['name'] is None + # a one-column Google Sheet export: quoted cells + rows, _ = parse_source_rows('"Foo, bar.jpg"\n"Two.jpg"\n') + assert _names(rows) == ['Foo,_bar.jpg', 'Two.jpg'] + # one column with an unknown header word: read as a name (as before) + rows, _ = parse_source_rows('files\nOne.jpg\n') + assert _names(rows) == ['Files', 'One.jpg'] # first letter canonical + + +def test_parse_file_id_cells(): + assert parse_file_id('149673020') == (149673020, False) + assert parse_file_id(u' 12\xa0') == (12, False) + assert parse_file_id('') == (None, False) + assert parse_file_id(' ') == (None, False) + for bad in ['1.49673E+08', '149,673,020', '12.0', 'abc', '0', '-5', '1 2']: + assert parse_file_id(bad) == (None, True), bad + + +def test_parse_short_rows_and_malformed_ids(): + rows, _ = parse_source_rows('filename,file_id\nA.jpg\nB.jpg,1.2E+3\n,\n,7\n') + assert [(r['row'], r['name'], r['file_id'], r['malformed_file_id']) + for r in rows] == [(2, 'A.jpg', None, False), + (3, 'B.jpg', None, True), + (5, '', 7, False)] + + +def test_parse_name_list(): + rows = parse_name_list(['File:A b.jpg', '', ' ', 'C.jpg']) + assert [(r['row'], r['name']) for r in rows] == [(1, 'A_b.jpg'), (4, 'C.jpg')] + assert _names(parse_name_list('A.jpg\nB.jpg')) == ['A.jpg', 'B.jpg'] + with raises(ImportSourceInvalid): + parse_name_list(None) + + +@pytest.mark.parametrize('pasted, expected', [ + ('filename,file_id,note\nA.jpg,1,x', 'filename,file_id,note\nA.jpg,1,x'), + (['\ufeffFilename', 'A.jpg'], '\ufeffFilename\nA.jpg'), + (['', '"filename","file_id"', '"A, b.jpg",1'], '\n"filename","file_id"\n"A, b.jpg",1'), + ('file_id\n149673020', 'file_id\n149673020'), + (['img_name,img_size', 'A.jpg,10'], 'img_name,img_size\nA.jpg,10'), +]) +def test_pasted_csv_in_a_file_list_is_read_as_csv(pasted, expected): + """A CSV pasted into the file list (e.g. the check's download) is read + as a CSV, not as one file name per line.""" + assert pasted_csv_text(pasted) == expected + + +def test_name_list_keeps_names_that_only_look_like_headers(): + names = ['Filename.jpg', 'File_id_card.jpg', 'Filename,_file_id.jpg'] + assert pasted_csv_text(names) is None + assert pasted_csv_text(['', ' ', None]) is None + assert _names(parse_name_list(names)) == names + + +def test_csv_with_only_a_file_id_column(): + rows, columns = parse_source_rows('file_id\n149673020\n\n7\n') + assert [(r['row'], r['name'], r['file_id']) for r in rows] == [ + (2, '', 149673020), (4, '', 7)] + assert columns == {'name': None, 'file_id': 'file_id', 'ignored': []} + + +def test_formula_guard_round_trip(): + for name in ['=SUM(A1).jpg', '+1.jpg', '-x.jpg', '@me.jpg', "'s-Hertogenbosch.jpg", + "''x.jpg", 'Plain.jpg']: + assert unguard_cell(guard_cell(name)) == name + assert normalise_name(guard_cell(name)) == name + assert guard_cell('=x.jpg') == "'=x.jpg" + assert normalise_name("'s-Hertogenbosch.jpg") == "'s-Hertogenbosch.jpg" def test_make_entry_reupload(): @@ -263,48 +362,25 @@ def fake(query, params, db_host=labs.COMMONS_DB_HOST): return fake -def test_load_by_filename_batches_lookups(monkeypatch): +def test_lookup_by_names_batches_lookups(monkeypatch): from montage import labs, loaders monkeypatch.setattr(labs, 'FILE_LOOKUP_CHUNK_SIZE', 4) - names = ['File %02d.jpg' % i for i in range(10)] - existing = {n.replace(' ', '_') for n in names} + names = ['File_%02d.jpg' % i for i in range(10)] calls = [] - _patch_replicas(monkeypatch, _fake_main_replica(existing, calls)) + _patch_replicas(monkeypatch, _fake_main_replica(set(names), calls)) - entries, warnings = loaders.load_by_filename(names, source='local') + found = loaders.lookup_by_names(names + names[:3], source='local') assert [len(c) for c in calls] == [4, 4, 2] # 3 queries, not 10 - assert [e.name for e in entries] == [n.replace(' ', '_') for n in names] - assert warnings == [] - - -def test_load_by_filename_warns_per_missing_name_in_input_order(monkeypatch): - from montage import labs, loaders - calls = [] - _patch_replicas(monkeypatch, _fake_main_replica({'B.jpg'}, calls)) - - entries, warnings = loaders.load_by_filename(['Z missing.jpg', 'B.jpg', 'A missing.jpg'], - source='local') - - assert [e.name for e in entries] == ['B.jpg'] - assert len(warnings) == 2 - assert '"Z missing.jpg"' in warnings[0] and '"A missing.jpg"' in warnings[1] - - -def test_load_name_list_local(monkeypatch): - """CSV with only file names, local source: used to unpack the file info - dict into (edict, warnings) and fail.""" - from io import StringIO - from montage import labs, loaders - calls = [] - _patch_replicas(monkeypatch, _fake_main_replica({'One.jpg', 'Two_words.jpg'}, calls)) + assert sorted(found) == names - entries, warnings = loaders.load_name_list( - StringIO('File:One.jpg\nTwo words.jpg\nGone.jpg\n'), source='local') - assert [e.name for e in entries] == ['One.jpg', 'Two_words.jpg'] - assert len(calls) == 1 - assert len(warnings) == 1 and '"Gone.jpg"' in warnings[0] +def test_lookup_by_names_remote_keys_by_normalised_name(mock_external_apis): + from montage import loaders + from .conftest import SELECTED_FILE_INFO + name = normalise_name(SELECTED_FILE_INFO['img_name']) # remote: spaces + found = loaders.lookup_by_names([name, 'Not_there.jpg'], source='remote') + assert list(found) == [name] def test_round_source_params_hold_large_file_lists(): @@ -343,19 +419,6 @@ def test_get_files_order_is_binary_like_the_old_query(monkeypatch): assert result == ['A_upper.jpg', 'B_upper.jpg', 'a_lower.jpg', 'b_lower.jpg', 'Ö_umlaut.jpg'] -def test_load_by_filename_duplicates_and_space_underscore_pairs(monkeypatch): - """'A b.jpg' and 'A_b.jpg' are the same Commons file; both requests - resolve, with one query.""" - from montage import labs, loaders - calls = [] - _patch_replicas(monkeypatch, _fake_main_replica({'A_b.jpg'}, calls)) - - entries, warnings = loaders.load_by_filename(['A b.jpg', 'A_b.jpg', 'A b.jpg'], source='local') - - assert [e.name for e in entries] == ['A_b.jpg'] * 3 - assert warnings == [] and len(calls) == 1 and calls[0] == ('A_b.jpg',) - - # --------------------------------------------------------------------------- # One replica connection per lookup (per database host), closed afterwards, # not one per 500-name chunk (hatnote/montage#625). @@ -437,3 +500,210 @@ def test_replica_hosts_can_be_set_by_environment(): assert _labs_hosts({'MONTAGE_COMMONS_DB_HOST': 'main.example', 'MONTAGE_COMMONS_LINKS_DB_HOST': 'links.example'}) == ( 'main.example', 'links.example') + + +# --------------------------------------------------------------------------- +# Lookup by file_id (hatnote/montage#510): same query as by name, other column +# --------------------------------------------------------------------------- + +# _files_by_name's query as on master 939d890, copied verbatim: the shared +# query builder must not change the SQL that the name lookups send. +_MASTER_FILES_BY_NAME_SQL = ''' + SELECT DISTINCT {cols} + FROM commonswiki_p.file AS file + JOIN commonswiki_p.filerevision AS fr ON fr.fr_id = file.file_latest + AND fr.fr_deleted = 0 + LEFT JOIN actor AS ci ON fr.fr_actor = ci.actor_id + LEFT JOIN commonswiki_p.filetypes AS ft ON file.file_type = ft.ft_id + {earliest_rev} + WHERE file.file_name IN ({names}) + AND file.file_deleted = 0 + ''' + + +def test_files_by_name_sql_unchanged(): + from montage import labs + seen = [] + labs._files_by_name(lambda q, p: seen.append((q, p)) or [], ['A.jpg', 'B.jpg']) + expected = _MASTER_FILES_BY_NAME_SQL.format( + cols=', '.join(labs.FILE_COLS), + earliest_rev=labs._EARLIEST_REVISION_SUBQUERY, + names='%s, %s') + assert seen == [(expected, ('A.jpg', 'B.jpg'))] + + +def _fake_main_replica_by_id(existing, calls): + """existing: {file_id: name}; answers lookups by file_id.""" + from montage import labs + + def fake(query, params, db_host=labs.COMMONS_DB_HOST): + assert db_host == labs.COMMONS_DB_HOST + assert 'WHERE file.file_id IN (' in query + assert 'file.file_deleted = 0' in query and 'fr.fr_deleted = 0' in query + calls.append(params) + return [_file_row(existing[i], i) for i in params if i in existing] + return fake + + +def test_get_files_info_by_ids_batches_one_connection(monkeypatch): + from montage import labs + monkeypatch.setattr(labs, 'FILE_LOOKUP_CHUNK_SIZE', 3) + existing = {i: 'F%02d.jpg' % i for i in range(1, 9)} + calls = [] + opened = _patch_replicas(monkeypatch, _fake_main_replica_by_id(existing, calls)) + + found = labs.get_files_info_by_ids(['8', 7, 6, 5, 4, 3, 2, 1, 1, 99]) + + assert [len(c) for c in calls] == [3, 3, 3] # 9 distinct ids, as ints + assert all(isinstance(i, int) for c in calls for i in c) + assert sorted(found) == list(range(1, 9)) + assert found[3]['img_name'] == 'F03.jpg' + assert [(c.db_host, c.queries, c.closed) for c in opened] == [ + (labs.COMMONS_DB_HOST, 3, True)] + + +def test_get_files_info_by_ids_empty(monkeypatch): + from montage import labs + opened = _patch_replicas(monkeypatch, _fake_main_replica_by_id({}, [])) + assert labs.get_files_info_by_ids([]) == {} + assert opened == [] + + +@pytest.mark.xfail( + os.environ.get('TOOLFORGE') != '1', + reason='Requires live wikireplicas (Toolforge); set TOOLFORGE=1 to run', +) +def test_get_files_info_by_ids_live(): + """A file found by name is found again by its file_id, with the same + details.""" + from montage.labs import get_files_info_by_names, get_files_info_by_ids + name = 'Reynisfjara,_Suðurland,_Islandia,_2014-08-17,_DD_164.JPG' + by_name = get_files_info_by_names([name])[name] + by_id = get_files_info_by_ids([by_name['file_id']]) + assert by_id[by_name['file_id']] == by_name + + +@pytest.mark.xfail( + os.environ.get('TOOLFORGE') != '1', + reason='Requires live wikireplicas (Toolforge); set TOOLFORGE=1 to run', +) +def test_get_file_pages_by_other_ids_live(): + """The page ID and a revision ID of a File: page (as shown on Commons, + 2026-10-07) are recognised as such.""" + from montage.labs import get_file_pages_by_other_ids + name = 'Reynisfjara,_Suðurland,_Islandia,_2014-08-17,_DD_164.JPG' + found = get_file_pages_by_other_ids([37348796, 1281700543]) + assert found[37348796] == {'kind': 'page_id', 'name': name} + assert found[1281700543] == {'kind': 'revision_id', 'name': name} + + +def test_source_url_rewrites_only_gist_links(): + """#208: a plain CSV / text link got '/raw' appended and returned 404.""" + from montage.loaders import source_url + assert source_url('https://gist.github.com/u/abc') == ( + 'https://gist.githubusercontent.com/u/abc/raw', False) + assert source_url('https://gist.githubusercontent.com/u/abc/raw/x.csv') == ( + 'https://gist.githubusercontent.com/u/abc/raw/x.csv', False) + assert source_url('https://wikimedia.be/public/wlh/list.txt') == ( + 'https://wikimedia.be/public/wlh/list.txt', False) + + + +# --------------------------------------------------------------------------- +# Review fixes (pr-check 2026-10-06): what the server fetches, and how much +# --------------------------------------------------------------------------- + +@pytest.mark.parametrize('url', [ + 'http://example.org/list.csv', # not https + 'https://127.0.0.1/list.csv', + 'https://169.254.169.254/latest/meta-data/', + 'https://[::1]/list.csv', + 'https://10.0.0.5/list.csv', + 'ftp://example.org/list.csv', + 'https:///nohost', +]) +def test_fetch_refuses_non_public_or_non_https_links(url): + with responses.RequestsMock() as rsps: # nothing may be requested + with raises(ImportSourceInvalid): + fetch_source_text(url) + assert len(rsps.calls) == 0 + + +def test_fetch_follows_redirects_only_to_public_addresses(monkeypatch): + from montage import loaders + with responses.RequestsMock() as rsps: + rsps.add(responses.GET, 'https://example.org/a.csv', status=302, + headers={'Location': 'https://example.org/b.csv'}) + rsps.add(responses.GET, 'https://example.org/b.csv', body='A.jpg\n', status=200) + assert fetch_source_text('https://example.org/a.csv') == 'A.jpg\n' + with responses.RequestsMock() as rsps: + rsps.add(responses.GET, 'https://example.org/c.csv', status=302, + headers={'Location': 'https://169.254.169.254/secret'}) + with raises(ImportSourceInvalid): + fetch_source_text('https://example.org/c.csv') + assert len(rsps.calls) == 1 + with responses.RequestsMock() as rsps: + for i in range(loaders.MAX_REDIRECTS + 1): + rsps.add(responses.GET, 'https://example.org/r%d' % i, status=302, + headers={'Location': 'https://example.org/r%d' % (i + 1)}) + with raises(ImportSourceInvalid): + fetch_source_text('https://example.org/r0') + + +def test_fetch_stops_at_the_size_cap(monkeypatch): + from montage import loaders + monkeypatch.setattr(loaders, 'MAX_SOURCE_BYTES', 1000) + with responses.RequestsMock() as rsps: + rsps.add(responses.GET, 'https://example.org/big.csv', body='A.jpg\n' * 500, status=200) + with raises(ImportSourceInvalid) as exc: + fetch_source_text('https://example.org/big.csv') + assert 'larger than' in exc.value.detail + + +def test_parse_stops_at_the_row_cap(monkeypatch): + from montage import loaders + monkeypatch.setattr(loaders, 'MAX_SOURCE_ROWS', 3) + parse_source_rows('filename\nA.jpg\nB.jpg\nC.jpg\n') + with raises(ImportSourceInvalid): + parse_source_rows('filename\nA.jpg\nB.jpg\nC.jpg\nD.jpg\n') + with raises(ImportSourceInvalid): + parse_name_list(['A.jpg', 'B.jpg', 'C.jpg', 'D.jpg']) + + +def test_source_fields_must_be_text(): + from montage.loaders import category_records + for bad in (['https://example.org/x.csv'], {'u': 1}, 5): + with raises(ImportSourceInvalid): + fetch_source_text(bad) + with raises(ImportSourceInvalid): + category_records(bad) + with raises(ImportSourceInvalid): + parse_name_list(5) + + +@pytest.mark.parametrize('written, canonical', [ + ('file:a b.jpg', 'A_b.jpg'), + ('Image: Foo.jpg', 'Foo.jpg'), + ('File: two spaces__and_.jpg', 'Two_spaces_and_.jpg'), + ('_leading and trailing_ ', 'Leading_and_trailing'), + (u'éclair.jpg', u'Éclair.jpg'), + (b'Bytes.jpg', 'Bytes.jpg'), + # titlecase, like MediaWiki's Language::ucfirst (MB_CASE_TITLE): + # Georgian Mkhedruli has no titlecase form, so Commons keeps it lower + # case; str.upper() would turn it into Mtavruli, a different name + (u'ქართული.jpg', u'ქართული.jpg'), + (u'Ქართული.jpg', u'Ქართული.jpg'), # already-capital first letters stay + (u'džep.jpg', u'Džep.jpg'), # digraph: titlecase Dž, not upper DŽ + # SpecialCasing expansions, as Commons normalizes them (API-checked) + (u'ʼnabc.jpg', u'\u02bcNabc.jpg'), + (u'file.jpg', u'File.jpg'), + (u'ևս.jpg', u'Եւս.jpg'), + # pinned by Wikimedia's $wgOverrideUcfirstCharacters + (u'ßt.jpg', u'ßt.jpg'), +]) +def test_names_are_canonical_like_mediawiki(written, canonical): + """MediaWiki file titles: File:/Image: prefix in any case, runs of + spaces and underscores collapsed, no leading/trailing underscore, + first letter upper case (Commons is first-letter case-sensitive).""" + assert normalise_name(written) == canonical + assert parse_name_list([written])[0]['name'] == canonical diff --git a/montage/tests/test_reassign.py b/montage/tests/test_reassign.py new file mode 100644 index 00000000..b3d379e6 --- /dev/null +++ b/montage/tests/test_reassign.py @@ -0,0 +1,71 @@ +# -*- coding: utf-8 -*- +"""DAO-free unit tests for the module-level task (re)assignment helpers +in montage.rdb, against an in-memory sqlite database. +""" + +from sqlalchemy import create_engine +from sqlalchemy.orm import sessionmaker + +from montage.rdb import (Base, + User, + Round, + Entry, + RoundEntry, + Vote, + reassign_rating_tasks, + ACTIVE_STATUS, + PAUSED_STATUS, + COMPLETED_STATUS, + CANCELLED_STATUS) + + +def make_session(): + engine = create_engine('sqlite://') + Base.metadata.create_all(engine) + return sessionmaker(bind=engine)() + + +def test_reassign_cancels_unfulfillable_task(): + """Regression: reassigning an active vote on an entry that every + remaining juror has already completed used to crash with a + ValueError from weighted_choice. The orphaned task is unfulfillable + (coverage is already met), so it must be cancelled instead. + """ + session = make_session() + + juror_a = User(id=1, username='JurorA') + juror_b = User(id=2, username='JurorB') + juror_c = User(id=3, username='JurorC') + + rnd = Round(name='test rating round', + vote_method='rating', + quorum=2, + status=PAUSED_STATUS, + jurors=[juror_a, juror_b, juror_c]) + entry = Entry(name='test entry.jpg') + round_entry = RoundEntry(entry=entry, round=rnd) + + vote_a = Vote(user=juror_a, round_entry=round_entry, + status=COMPLETED_STATUS, value=0.8) + vote_b = Vote(user=juror_b, round_entry=round_entry, + status=COMPLETED_STATUS, value=0.6) + vote_c = Vote(user=juror_c, round_entry=round_entry, + status=ACTIVE_STATUS) + + session.add_all([rnd, round_entry, vote_a, vote_b, vote_c]) + session.flush() + + # juror C leaves; A and B have both already rated the entry, so C's + # open task has no eligible juror and must be cancelled, not crash + res = reassign_rating_tasks(session, rnd, [juror_a, juror_b]) + session.flush() + + assert res['cancelled_task_count'] == 1 + assert vote_c.status == CANCELLED_STATUS + assert vote_c.modified_date is not None + + # completed votes are untouched and no new tasks appeared + assert vote_a.status == COMPLETED_STATUS + assert vote_b.status == COMPLETED_STATUS + active_votes = session.query(Vote).filter_by(status=ACTIVE_STATUS).all() + assert active_votes == [] diff --git a/montage/tests/test_web_basic.py b/montage/tests/test_web_basic.py index eb7bd913..fec5b342 100644 --- a/montage/tests/test_web_basic.py +++ b/montage/tests/test_web_basic.py @@ -19,7 +19,6 @@ from montage import utils from montage.log import script_log from montage.app import create_app, STATIC_PATH -from montage.utils import unicode class ClasticTestClient(Client): @@ -64,7 +63,7 @@ def fetch_url(self, url, data=None, act=None, **kw): # TODO: careful, werkzeug's pretty liberal with "data", # will take a dictionary and do weird stuff to it, turn it # into a formencoded mess. - if not isinstance(data, (bytes, unicode)): + if not isinstance(data, (bytes, str)): data = json.dumps(data) res = c.post(url, data=data, content_type=kw.get('content_type', 'application/json')) @@ -118,6 +117,20 @@ def fetch(self, role_action, url, data=None, **kw): raise return data_dict + def fetch_checked_import(self, role_action, round_id, data, **kw): + """Import into a first round the way the round form does (#510): + check the source for the round's campaign, then import with the + check token. Returns the import's response.""" + as_user = kw.get('as_user') + rnd = self.fetch('coordinator: get round', '/admin/round/%s' % round_id, + as_user=as_user)['data'] + check = self.fetch('coordinator: check import source', + '/admin/campaign/%s/import/check' % rnd['campaign']['id'], + data, as_user=as_user)['data'] + return self.fetch(role_action, '/admin/round/%s/import' % round_id, + {'import_method': data['import_method'], + 'check_token': check['token']}, **kw) + def _create_schema(db_url, echo=True): from sqlalchemy import create_engine @@ -133,6 +146,7 @@ def _create_schema(db_url, echo=True): def montage_app(tmpdir): config = utils.load_env_config(env_name='devtest') config['db_url'] = config['db_url'].replace('///', '///' + str(tmpdir) + '/') + config['import_check_path'] = str(tmpdir.join('import_checks')) db_url = config['db_url'] _create_schema(db_url=db_url) @@ -318,8 +332,8 @@ def test_home_client(base_client, api_client, mock_external_apis): data = {'import_method': 'category', 'category': 'Images_from_Wiki_Loves_Monuments_2015_in_Albania'} - resp = fetch('coordinator: import entries from a category', - '/admin/round/%s/import' % round_id, + resp = api_client.fetch_checked_import('coordinator: import entries from a category', + round_id, data, as_user='LilyOfTheWest') resp = fetch('coordinator: activate a round', @@ -333,13 +347,13 @@ def test_home_client(base_client, api_client, mock_external_apis): as_user='LilyOfTheWest') gsheet_url = 'https://docs.google.com/spreadsheets/d/1WzHFg_bhvNthRMwNmxnk010KJ8fwuyCrby29MvHUzH8/edit#gid=550467819' - resp = fetch('coordinator: import more entries from different gsheet csv into an existing round', - '/admin/round/%s/import' % round_id, + resp = api_client.fetch_checked_import('coordinator: import more entries from different gsheet csv into an existing round', + round_id, {'import_method': 'csv', 'csv_url': gsheet_url}, as_user='LilyOfTheWest') - resp = fetch('coordinator: import files selected by name', - '/admin/round/%s/import' % round_id, + resp = api_client.fetch_checked_import('coordinator: import files selected by name', + round_id, {'import_method': 'selected', 'file_names': ['Reynisfjara, Suðurland, Islandia, 2014-08-17, DD 164.JPG']}, as_user='LilyOfTheWest') @@ -376,17 +390,130 @@ def test_home_client(base_client, api_client, mock_external_apis): u'Jimbo Wales']}, as_user='LilyOfTheWest') + resp = fetch('coordinator: get round stats before quorum raise', + '/admin/round/%s' % round_id, + as_user='LilyOfTheWest') + stats = resp['data']['stats'] + entry_count = (resp['data']['total_entries'] + - stats['total_disqualified_entries']) + total_tasks_before = stats['total_tasks'] + resp = fetch('coordinator: raise quorum value', '/admin/round/%s/edit' % round_id, {'quorum': 4}, as_user='LilyOfTheWest') - resp = fetch('coordinator: try to reduce quorum (not supported)', + # regression (bug: quorum raise created zero tasks mid-round, since + # create_initial_rating_tasks only covers entries with no votes at all) + resp = fetch('coordinator: get round stats after quorum raise', + '/admin/round/%s' % round_id, + as_user='LilyOfTheWest') + stats = resp['data']['stats'] + assert stats['total_tasks'] == total_tasks_before + entry_count + assert resp['data']['quorum'] == 4 + + # combined-edit rollback: raising quorum beyond the shrunken juror + # list must 400 and leave quorum, jurors, and tasks untouched + resp = fetch('coordinator: conflicting quorum raise and juror shrink', '/admin/round/%s/edit' % round_id, - {'quorum': 1}, + {'quorum': 5, + 'new_jurors': [u'Slaporte', + u'MahmoudHashemi', + u'Jean-Frédéric']}, as_user='LilyOfTheWest', error_code=400) + resp = fetch('coordinator: confirm rollback of failed combined edit', + '/admin/round/%s' % round_id, + as_user='LilyOfTheWest') + assert resp['data']['quorum'] == 4 + assert len([j for j in resp['data']['jurors'] if j['is_active']]) == 5 + assert resp['data']['stats']['total_tasks'] == total_tasks_before + entry_count + + # combined lower-quorum + juror shrink in a single save: the new jury + # (4) is smaller than the old quorum (4 would still fit, so drop to a + # 4-juror roster with quorum 2); quorum must apply before the juror edit + resp = fetch('coordinator: lower quorum and shrink jury in one save', + '/admin/round/%s/edit' % round_id, + {'quorum': 2, + 'new_jurors': [u'Slaporte', + u'MahmoudHashemi', + u'Jean-Frédéric', + u'Jimbo Wales']}, + as_user='LilyOfTheWest') + + resp = fetch('coordinator: get round stats after combined edit', + '/admin/round/%s' % round_id, + as_user='LilyOfTheWest') + assert resp['data']['quorum'] == 2 + jd_map = {jd['username']: jd for jd in resp['data']['jurors']} + assert not jd_map['Effeietsanders']['is_active'] + assert jd_map['Effeietsanders']['stats']['total_open_tasks'] == 0 + stats = resp['data']['stats'] + assert stats['total_open_tasks'] == 2 * entry_count + assert stats['total_tasks'] == 2 * entry_count # no completed votes yet + + resp = fetch('coordinator: restore the full juror roster', + '/admin/round/%s/edit' % round_id, + data={'new_jurors': [u'Slaporte', + u'MahmoudHashemi', + u'Effeietsanders', + u'Jean-Frédéric', + u'Jimbo Wales']}, + as_user='LilyOfTheWest') + + # quorum-only decrease (used to be pinned as an unsupported 400) + resp = fetch('coordinator: reduce quorum by itself', + '/admin/round/%s/edit' % round_id, + {'quorum': 1}, + as_user='LilyOfTheWest') + + resp = fetch('coordinator: get round stats after quorum-only decrease', + '/admin/round/%s' % round_id, + as_user='LilyOfTheWest') + assert resp['data']['quorum'] == 1 + assert resp['data']['stats']['total_open_tasks'] == entry_count + + # regression (bug: a same-size edit replacing two or more jurors + # swapped only one arbitrary pair, stranding the other removed + # juror's open tasks and giving the other new juror none) + resp = fetch('coordinator: swap out two jurors at once', + '/admin/round/%s/edit' % round_id, + data={'new_jurors': [u'Slaporte', + u'MahmoudHashemi', + u'Jean-Frédéric', + u'Zack Aeon', + u'Zora Bellal']}, + as_user='LilyOfTheWest') + + resp = fetch('coordinator: check both jurors were swapped', + '/admin/round/%s' % round_id, + as_user='LilyOfTheWest') + jd_map = {jd['username']: jd for jd in resp['data']['jurors']} + for username in ('Effeietsanders', 'Jimbo Wales'): + assert not jd_map[username]['is_active'] + assert jd_map[username]['stats']['total_open_tasks'] == 0 + for username in ('Zack Aeon', 'Zora Bellal'): + assert jd_map[username]['is_active'] + assert jd_map[username]['stats']['total_open_tasks'] > 0 + + resp = fetch('coordinator: swap the original two jurors back in', + '/admin/round/%s/edit' % round_id, + data={'new_jurors': [u'Slaporte', + u'MahmoudHashemi', + u'Effeietsanders', + u'Jean-Frédéric', + u'Jimbo Wales']}, + as_user='LilyOfTheWest') + + # restore quorum 4 so every juror has a full queue for the juror + # endpoints below; at quorum 1 the reassignments can leave Jimbo + # Wales with fewer than the 3 open tasks the skip checks need + resp = fetch('coordinator: restore quorum before reactivating', + '/admin/round/%s/edit' % round_id, + {'quorum': 4}, + as_user='LilyOfTheWest') + resp = fetch('coordinator: reactivate our round', '/admin/round/%s/activate' % round_id, {'post': True}, @@ -592,6 +719,86 @@ def test_home_client(base_client, api_client, mock_external_apis): '/admin/round/%s/activate' % rnd_2_id, {'post': True}, as_user='LilyOfTheWest') + # remove_juror requires a paused round + resp = fetch('coordinator: remove_juror fails on an active round', + '/admin/round/%s/remove_juror' % rnd_2_id, + {'username': u'Effeietsanders'}, + as_user='LilyOfTheWest', + error_code=400) + + # give Jean-Frédéric some completed votes to exercise the + # keep-vs-discard behaviors below + resp = fetch('juror: JF fetches tasks before leaving', + '/juror/round/%s/tasks?count=5' % rnd_2_id, + as_user=u'Jean-Frédéric') + jf_ratings = [{'vote_id': t['id'], 'value': 0.75} + for t in resp['data']['tasks']] + assert jf_ratings + resp = fetch('juror: JF submits some ratings before leaving', + '/juror/round/%s/tasks/submit' % rnd_2_id, + {'ratings': jf_ratings}, + as_user=u'Jean-Frédéric') + + resp = fetch('coordinator: pause round 2 to remove jurors', + '/admin/round/%s/pause' % rnd_2_id, + {'post': True}, as_user='LilyOfTheWest') + + resp = fetch('coordinator: remove_juror rejects a non-juror', + '/admin/round/%s/remove_juror' % rnd_2_id, + {'username': u'Not A Juror'}, + as_user='LilyOfTheWest', + error_code=400) + + # remove a juror, keeping their completed votes (the default); the + # jury drops 4 -> 3, which still meets the quorum of 3 + resp = fetch('coordinator: remove a juror, keeping completed votes', + '/admin/round/%s/remove_juror' % rnd_2_id, + {'username': u'Effeietsanders'}, + as_user='LilyOfTheWest') + summary = resp['data'] + assert summary['removed_juror'] == u'Effeietsanders' + assert summary['cancelled_active_count'] > 0 + assert summary['discarded_completed_count'] == 0 + # every entry they were due to see is re-covered by the one + # remaining juror who has not seen it yet + assert summary['created_task_count'] == summary['cancelled_active_count'] + assert summary['uncoverable_entry_count'] == 0 + assert summary['quorum'] == 3 + + # remove another juror, discarding their completed votes; the jury + # drops 3 -> 2, so the quorum implicitly lowers to 2 + resp = fetch('coordinator: remove a juror, discarding their votes', + '/admin/round/%s/remove_juror' % rnd_2_id, + {'username': u'Jean-Frédéric', 'discard_completed': True}, + as_user='LilyOfTheWest') + summary = resp['data'] + assert summary['removed_juror'] == u'Jean-Frédéric' + assert summary['discarded_completed_count'] == len(jf_ratings) + # with 3 jurors and quorum 3, every entry had one vote per juror; + # dropping to quorum 2 leaves the remaining pair covering everything + assert summary['created_task_count'] == 0 + assert summary['uncoverable_entry_count'] == 0 + assert summary['quorum'] == 2 + + resp = fetch('coordinator: check round 2 state after removals', + '/admin/round/%s' % rnd_2_id, + as_user='LilyOfTheWest') + assert resp['data']['quorum'] == 2 + jd_map = {jd['username']: jd for jd in resp['data']['jurors']} + for username in (u'Effeietsanders', u'Jean-Frédéric'): + assert not jd_map[username]['is_active'] + assert jd_map[username]['stats']['total_open_tasks'] == 0 + + resp = fetch('coordinator: remove_juror rejects an oversized new_quorum', + '/admin/round/%s/remove_juror' % rnd_2_id, + {'username': u'Slaporte', 'new_quorum': 5}, + as_user='LilyOfTheWest', + error_code=400) + + resp = fetch('coordinator: reactivate round 2 after removals', + '/admin/round/%s/activate' % rnd_2_id, + {'post': True}, as_user='LilyOfTheWest') + submit_ratings(api_client, rnd_2_id) resp = fetch('juror: get votes stats for rating round', @@ -608,6 +815,14 @@ def test_home_client(base_client, api_client, mock_external_apis): '/admin/round/%s/preview_results' % rnd_2_id, as_user='LilyOfTheWest') + # the damped (Bayesian) mean is shown alongside the plain mean for + # rating rounds; every damped value sits between the plain extremes + assert resp['data']['damped_ratings'] + plain, damped = resp['data']['ratings'], resp['data']['damped_ratings'] + assert sum(damped.values()) == sum(plain.values()) + lo, hi = min(map(float, plain)), max(map(float, plain)) + assert all(lo <= float(v) <= hi for v in damped) + thresh_map = resp['data']['thresholds'] # TODO cur_thresh = [t for t, c in sorted(thresh_map.items()) if 0 < c <= 20][-1] @@ -712,6 +927,8 @@ def test_home_client(base_client, api_client, mock_external_apis): resp = fetch('coordinator: preview round 3 results', '/admin/round/%s/preview_results' % rnd_3_id, as_user='LilyOfTheWest') + # ranking rounds have no quorum, so no damped ratings either + assert 'damped_ratings' not in resp['data'] resp = fetch('coordinator: read round 3 reviews', '/admin/round/%s/reviews' % rnd_3_id, @@ -751,7 +968,7 @@ def test_home_client(base_client, api_client, mock_external_apis): resp = fetch('juror: check archived campaign rounds shown', '/juror', as_user='Slaporte') - assert len(resp['data']) > 1 # 4 rounds at time of writing + assert len(resp['data']) == 1 # only active rounds shown; rounds 1+2 are finalized assert glom(resp, 'data.0.campaign.is_archived') is False @@ -822,6 +1039,39 @@ def test_home_client(base_client, api_client, mock_external_apis): #resp = base_client.fetch('public: logout', '/logout') +def test_meta_shows_no_config_values(montage_app, base_client): + """/meta and /meta/json/ are public; they list the app's resources by + name, but must not show config values (clastic's default showed the + first 70 characters of repr(config)).""" + config = montage_app.resources['config'] + values = [config.get(key) for key in ('cookie_secret', 'db_url', + 'oauth_client_id', + 'oauth_client_secret')] + values = [v for v in values if isinstance(v, str) and len(v) >= 6] + assert values # the devtest config has some of these + + for url in ('/meta/', '/meta/json/'): + resp = base_client._test_client.get(url) + assert resp.status_code == 200 + body = resp.get_data(as_text=True) + assert 'config' in body + for value in values: + assert value not in body, '%s shows a config value' % url + assert "{'" not in body.replace(''', "'") + + +def test_frontend_error_log_maintainers_only(montage_app, api_client, + monkeypatch): + from montage import rdb + # every test user is a maintainer; make LilyOfTheWest a plain coordinator + monkeypatch.setattr(rdb, 'MAINTAINERS', + [m for m in rdb.MAINTAINERS if m != 'LilyOfTheWest']) + + api_client.fetch('coordinator: read frontend error log', '/logs/feel', + as_user='LilyOfTheWest', error_code=403) + api_client.fetch('maintainer: read frontend error log', '/logs/feel') + + def test_multiple_jurors(api_client, mock_external_apis): # This is copied from above. What's the best way to break up the tests into # various stages? Should I use a pytest.fixture? @@ -863,6 +1113,22 @@ def test_multiple_jurors(api_client, mock_external_apis): rnd_data, as_user='LilyOfTheWest') + round_id = resp['data']['id'] + + # a freshly-created round is paused, so jurors can be removed; + # removing the second-to-last juror works, the last is refused + resp = fetch('coordinator: remove a juror from a fresh round', + '/admin/round/%s/remove_juror' % round_id, + {'username': u'Haylad'}, + as_user='LilyOfTheWest') + assert resp['data']['removed_juror'] == u'Haylad' + + resp = fetch('coordinator: removing the last juror fails', + '/admin/round/%s/remove_juror' % round_id, + {'username': u'Slaporte'}, + as_user='LilyOfTheWest', + error_code=400) + def test_get_files_info_by_name(api_client): """GET /utils/file returns file_infos with file_id populated.""" @@ -921,9 +1187,9 @@ def test_import_entries_have_file_id(api_client, mock_external_apis): ) round_id = rnd_resp['data']['id'] - api_client.fetch( + api_client.fetch_checked_import( 'coordinator: import entries via category', - '/admin/round/%s/import' % round_id, + round_id, {'import_method': 'category', 'category': 'Images_from_Wiki_Loves_Monuments_2015_in_Albania'}, as_user='Yarl', @@ -1013,9 +1279,9 @@ def count_inserts(conn, cursor, statement, params, context, executemany): inserts[table] = inserts.get(table, 0) + 1 def do_import(): - return api_client.fetch( + return api_client.fetch_checked_import( 'coordinator: import entries via category', - '/admin/round/%s/import' % round_id, + round_id, {'import_method': 'category', 'category': 'Batch_import'}, as_user='Yarl')['data'] @@ -1042,6 +1308,99 @@ def do_import(): sorted(fi['img_name'] for fi in file_infos) +def test_round_entries_export_query_count(api_client, mock_external_apis): + """Listing and downloading a round's entries takes a fixed number of + queries, not two lazy loads (Entry, RoundSource) per round entry. + + Regression for hatnote/montage#640: per-row lazy loads made the + entries list and CSV download on large rounds outlast the gunicorn + worker timeout. + """ + import csv + import datetime + + import responses as responses_lib + from sqlalchemy import event + from sqlalchemy.engine import Engine + + from montage.rdb import IMPORT_CHUNK_SIZE + from montage.tests.conftest import (TOOLFORGE_CATEGORY_URL, + _generate_file_infos) + + n = 2 * IMPORT_CHUNK_SIZE + 50 + gen_infos = _generate_file_infos(n) + base_date = datetime.datetime(2015, 9, 6) + for i, info in enumerate(gen_infos): + info['img_name'] = 'Batch_import_%04d.jpg' % i + # _generate_file_infos overflows the seconds field past 40 files + info['img_timestamp'] = (base_date + datetime.timedelta(seconds=i) + ).strftime('%Y%m%d%H%M%S') + mock_external_apis.replace(responses_lib.POST, TOOLFORGE_CATEGORY_URL, + json={'file_infos': gen_infos, 'no_info': []}, + status=200) + + api_client.fetch('maintainer: add organizer', '/admin/add_organizer', + {'username': 'Yarl'}) + series_id = api_client.fetch('get default series', '/series')['data'][0]['id'] + campaign_id = api_client.fetch( + 'organizer: create campaign', '/admin/add_campaign', + {'name': 'export query test', + 'coordinators': ['Yarl'], + 'open_date': '2015-01-01T00:00:00', + 'close_date': '2016-01-01T00:00:00', + 'url': 'http://hatnote.com', + 'series_id': series_id}, + as_user='Yarl')['data']['id'] + round_id = api_client.fetch( + 'coordinator: create round', + '/admin/campaign/%s/add_round' % campaign_id, + {'name': 'Test round', + 'vote_method': 'yesno', + 'deadline_date': '2016-10-15T00:00:00', + 'jurors': ['Slaporte', 'MahmoudHashemi', 'Effeietsanders']}, + as_user='Yarl')['data']['id'] + api_client.fetch_checked_import( + 'coordinator: import entries via category', + round_id, + {'import_method': 'category', 'category': 'Batch_import'}, + as_user='Yarl') + + selects = {'list': 0, 'download': 0} + current = [] + + def count_selects(conn, cursor, statement, params, context, executemany): + if statement.strip().upper().startswith('SELECT'): + selects[current[0]] += 1 + + event.listen(Engine, 'before_cursor_execute', count_selects) + try: + current[:] = ['list'] + file_infos = api_client.fetch('coordinator: get round entries', + '/admin/round/%s/entries' % round_id, + as_user='Yarl')['file_infos'] + current[:] = ['download'] + resp = api_client.fetch('coordinator: download round entries', + '/admin/round/%s/entries/download' % round_id, + as_user='Yarl') + finally: + event.remove(Engine, 'before_cursor_execute', count_selects) + + print('SELECTs per request at n=%s: %r' % (n, selects)) + assert len(file_infos) == n + reader = csv.DictReader(resp.get_data(as_text=True).splitlines()) + rows = list(reader) + assert len(rows) == n + assert sorted(reader.fieldnames) == sorted(file_infos[0].keys()) + assert all(r['source_method'] == 'category' for r in rows) + # #656 adds the check's fingerprint to a checked import's params + assert all(json.loads(r['source_params'])['category'] == 'Batch_import' + for r in rows) + assert len({r['source_params'] for r in rows}) == 1 + + assert selects['list'] < 20, selects + assert selects['download'] < 20, selects + + @script_log.wrap('critical', verbose=True) def submit_ratings(client, round_id, coord_user='Yarl'): """ @@ -1175,8 +1534,8 @@ def test_vote_later_reappears(api_client, mock_external_apis): as_user='LilyOfTheWest') round_id = resp['data']['id'] - fetch('coordinator: import entries', - '/admin/round/%s/import' % round_id, + api_client.fetch_checked_import('coordinator: import entries', + round_id, {'import_method': 'category', 'category': 'Images_from_Wiki_Loves_Monuments_2015_in_Albania'}, as_user='LilyOfTheWest') @@ -1273,8 +1632,8 @@ def test_concurrent_activation_creates_no_duplicate_tasks(montage_app, api_clien as_user='LilyOfTheWest') round_id = resp['data']['id'] - fetch('coordinator: import entries', - '/admin/round/%s/import' % round_id, + api_client.fetch_checked_import('coordinator: import entries', + round_id, {'import_method': 'category', 'category': 'Images_from_Wiki_Loves_Monuments_2015_in_Albania'}, as_user='LilyOfTheWest') @@ -1313,6 +1672,103 @@ def get_round_then_other_request_activates(self, rid): ' already activated: %s tasks created' % n_tasks) +def test_import_waiting_on_concurrent_import_is_refused(montage_app, api_client, + mock_external_apis): + """ + Regression test for double imports (hatnote/montage#635): a retry + of a slow import, sent while the first was still running, added + every file a second time, because neither request locked the round + and each only saw its own snapshot of the round's entries. + + The other import is simulated by committing one more round entry + from another connection while this request takes the round lock + (on MySQL it would have been waiting on that lock). With the fix, + the import compares the round's entry count in its own snapshot + with a fresh one and refuses; without it, it imports. + """ + from sqlalchemy import create_engine, text + from montage import rdb + + fetch = api_client.fetch + + resp = fetch('get default series', '/series') + series_id = resp['data'][0]['id'] + + fetch('organizer: create campaign for concurrent import test', + '/admin/add_campaign', + {'name': 'Concurrent Import Test', + 'coordinators': [u'LilyOfTheWest', u'Slaporte', u'Yarl'], + 'open_date': '2015-09-01 17:00:00', + 'close_date': '2015-10-01 17:00:00', + 'url': 'http://hatnote.com', + 'series_id': series_id}, + as_user='Yarl') + + resp = fetch('coordinator: get admin view', '/admin', as_user='LilyOfTheWest') + campaign_id = resp['data'][-1]['id'] + + resp = fetch('coordinator: add yesno round', + '/admin/campaign/%s/add_round' % campaign_id, + {'name': 'Concurrent Import Round', + 'vote_method': 'yesno', + 'quorum': 1, + 'deadline_date': '2025-10-20T00:00:00', + 'jurors': [u'Slaporte']}, + as_user='LilyOfTheWest') + round_id = resp['data']['id'] + + import_data = {'import_method': 'category', + 'category': 'Images_from_Wiki_Loves_Monuments_2015_in_Albania'} + api_client.fetch_checked_import('coordinator: first import', round_id, + import_data, as_user='LilyOfTheWest') + + other_engine = create_engine(montage_app.resources['config']['db_url']) + + def count_round_entries(): + with other_engine.connect() as conn: + return conn.execute(text( + 'SELECT COUNT(*) FROM round_entries WHERE round_id = :rid'), + {'rid': round_id}).scalar() + + first_count = count_round_entries() + assert first_count > 0 + + orig_lock_round = rdb.CoordinatorDAO._lock_round + raced = [] + + def lock_round_while_other_import_commits(self, rid): + rnd = orig_lock_round(self, rid) + if rid == round_id and not raced: + raced.append(rnd) + # the other import committed its round entries while this + # request waited on the round lock + with other_engine.begin() as conn: + conn.execute(text( + 'INSERT INTO round_entries (entry_id, round_id)' + ' SELECT entry_id, round_id FROM round_entries' + ' WHERE round_id = :rid LIMIT 1'), {'rid': rid}) + return rnd + + with patch.object(rdb.CoordinatorDAO, '_lock_round', + lock_round_while_other_import_commits): + resp = api_client.fetch_checked_import( + 'coordinator: import while another import committed', + round_id, import_data, as_user='LilyOfTheWest', error_code=400) + assert raced + assert ('another import into this round finished' + in resp.get_data(as_text=True)) + # the refused import added nothing + assert count_round_entries() == first_count + 1 + + # without a concurrent import, a repeat import is a plain duplicate + data = api_client.fetch_checked_import( + 'coordinator: import again, no race', round_id, import_data, + as_user='LilyOfTheWest')['data'] + assert {'duplicate import': 'no new entries imported'} in data['warnings'] + assert count_round_entries() == first_count + 1 + other_engine.dispose() + + def test_activation_inserts_tasks_in_batches(montage_app, api_client, mock_external_apis): """ @@ -1357,8 +1813,8 @@ def test_activation_inserts_tasks_in_batches(montage_app, api_client, as_user='LilyOfTheWest') round_id = resp['data']['id'] - resp = fetch('coordinator: import entries', - '/admin/round/%s/import' % round_id, + resp = api_client.fetch_checked_import('coordinator: import entries', + round_id, {'import_method': 'category', 'category': 'Images_from_Wiki_Loves_Monuments_2015_in_Albania'}, as_user='LilyOfTheWest') @@ -1549,8 +2005,8 @@ def test_selected_import_with_missing_name_reports_warning(montage_app, api_clie 'jurors': ['Slaporte', 'MahmoudHashemi', 'Effeietsanders']}, as_user='Yarl')['data']['id'] - data = api_client.fetch( - 'coordinator: import selected files', '/admin/round/%s/import' % round_id, + data = api_client.fetch_checked_import( + 'coordinator: import selected files', round_id, {'import_method': 'selected', 'file_names': [SELECTED_FILE_INFO['img_name'], 'Not on Commons.jpg']}, as_user='Yarl')['data'] @@ -1563,13 +2019,13 @@ def test_selected_import_with_missing_name_reports_warning(montage_app, api_clie def test_csv_import_warnings_are_dicts(montage_app, api_client, mock_external_apis, monkeypatch): """The frontend shows one value per warning; a plain string showed only its last character. CSV warnings must be dicts like the others.""" - from montage import loaders - from montage.tests.conftest import FIXTURE_FILE_INFOS + import responses as responses_lib + from montage.tests.conftest import FIXTURE_FILE_INFOS, build_filename_csv - def fake_csv(url, source='local'): - entries = [loaders.make_entry(info) for info in FIXTURE_FILE_INFOS[:3]] - return entries, ['file "Gone.jpg" does not exist'] - monkeypatch.setattr(loaders, 'get_entries_from_csv', fake_csv) + # a plain CSV link (fetched as given, #208): 3 known names + 1 unknown + mock_external_apis.add(responses_lib.GET, 'https://example.org/files.csv', + body=build_filename_csv(FIXTURE_FILE_INFOS[:3]) + 'Gone.jpg\n', + status=200) api_client.fetch('maintainer: add organizer', '/admin/add_organizer', {'username': 'Yarl'}) series_id = api_client.fetch('get default series', '/series')['data'][0]['id'] @@ -1585,8 +2041,8 @@ def fake_csv(url, source='local'): 'jurors': ['Slaporte', 'MahmoudHashemi', 'Effeietsanders']}, as_user='Yarl')['data']['id'] - data = api_client.fetch( - 'coordinator: import csv', '/admin/round/%s/import' % round_id, + data = api_client.fetch_checked_import( + 'coordinator: import csv', round_id, {'import_method': 'csv', 'csv_url': 'https://example.org/files.csv'}, as_user='Yarl')['data'] data = _import_result(montage_app, api_client, round_id, data) diff --git a/montage/utils.py b/montage/utils.py index f6dc323b..ae84a034 100644 --- a/montage/utils.py +++ b/montage/utils.py @@ -1,15 +1,15 @@ # -*- coding: utf-8 -*- -from __future__ import absolute_import -from __future__ import print_function +from __future__ import annotations + import sys import bisect import random import getpass import os.path import datetime -from six.moves.urllib.parse import urlencode - +from urllib.parse import urlencode +from typing import Any, Generator, Sequence, TypeVar, Union import yaml import requests @@ -19,15 +19,8 @@ from boltons.timeutils import isoparse from .check_rdb import get_schema_errors -import six - -try: - unicode = unicode - basestring = basestring -except NameError: - unicode = str - basestring = str +T = TypeVar('T') CUR_PATH = os.path.dirname(os.path.abspath(__file__)) PROJ_PATH = os.path.dirname(CUR_PATH) @@ -39,14 +32,14 @@ USER_AGENT = 'montage/25.0 (https://github.com/hatnote/montage; mahmoud@hatnote.com)' -def requests_get(url, **kwargs): +def requests_get(url: str, **kwargs: Any) -> requests.Response: """Wrapper for requests.get that adds User-Agent header""" headers = kwargs.pop('headers', {}) headers.setdefault('User-Agent', USER_AGENT) return requests.get(url, headers=headers, **kwargs) -def requests_post(url, **kwargs): +def requests_post(url: str, **kwargs: Any) -> requests.Response: """Wrapper for requests.post that adds User-Agent header""" headers = kwargs.pop('headers', {}) headers.setdefault('User-Agent', USER_AGENT) @@ -73,19 +66,62 @@ class NotImplementedResponse(MontageError, BadRequest, NotImplementedError): "Raised when a feature hasn't yet been implemented" +# Import check errors (hatnote/montage#510). They carry an error_type so the +# frontend can tell them apart; subclasses of InvalidAction (400), not of +# ValueError/TypeError, which older import code caught as "try another format". +# The detail is English; reason_code / reason_params let the frontend show it +# translated (montage-round-check-error- with the params). +class _ImportProblem(InvalidAction): + error_type = None + + def __init__(self, *args, **kwargs): + kwargs.setdefault('error_type', self.error_type) + self.reason_code = kwargs.pop('reason_code', None) + self.reason_params = [str(p) for p in kwargs.pop('reason_params', ())] + super(_ImportProblem, self).__init__(*args, **kwargs) + + def to_dict(self): + ret = super(_ImportProblem, self).to_dict() + ret['reason_code'] = self.reason_code + ret['reason_params'] = self.reason_params + return ret + + +class ImportSourceInvalid(_ImportProblem): + "The import source cannot be read (bad link, no name column, ...)" + error_type = 'import_source_invalid' + + +class ImportCheckBlocked(_ImportProblem): + "The checked list has rows that block the import" + error_type = 'import_check_blocked' + + +class ImportCheckExpired(_ImportProblem): + "The check token is unknown, expired or for another campaign/source" + error_type = 'import_check_expired' + + +class ImportCheckRequired(_ImportProblem): + "An import of a first-round source was sent without a check token" + error_type = 'import_check_required' + + +class ImportEmpty(_ImportProblem): + "A new first round would get no files (#447)" + error_type = 'import_empty' + + DEFAULT_SERIES = {'name': 'Unofficial', 'description': 'For unofficial campaigns, whether for testing or just for fun!', 'url': 'TODO add docs url'} # TODO: status is always active -def to_unicode(obj): - try: - return unicode(obj) - except UnicodeDecodeError: - return unicode(obj, encoding='utf8') +def to_unicode(obj: Any) -> str: + return str(obj) -def encode_dict_to_bytes(query): +def encode_dict_to_bytes(query: Union[dict, list]) -> Generator[tuple[bytes, Any], None, None]: if hasattr(query, 'items'): query = list(query.items()) @@ -95,13 +131,13 @@ def encode_dict_to_bytes(query): yield (encode_value_to_bytes(key), encode_value_to_bytes(value)) -def encode_value_to_bytes(value): - if not isinstance(value, unicode): - return str(value) +def encode_value_to_bytes(value: Any) -> bytes: + if not isinstance(value, str): + return str(value).encode('utf8') return value.encode('utf8') -def get_mw_userid(username): +def get_mw_userid(username: str) -> int: # Look up the central/global userid based on the username api_url = 'https://commons.wikimedia.org/w/api.php?' params = {'action': 'query', @@ -119,20 +155,20 @@ def get_mw_userid(username): return user_id -def get_threshold_map(ratings_map): +def get_threshold_map(ratings_map: dict[Union[str, float], Union[str, int]]) -> dict[float, int]: thresh_counts = {} # coerce some types - ratings_map = dict([(float(k), int(v)) for k, v in ratings_map.items()]) - ratings_map[0.0] = ratings_map.get(0.0, 0) - ratings_map[1.0] = ratings_map.get(1.0, 0) - for rating in sorted(ratings_map.keys()): - total_gte = sum([v for k, v in ratings_map.items() if k >= rating]) + typed_ratings_map = dict([(float(k), int(v)) for k, v in ratings_map.items()]) + typed_ratings_map[0.0] = typed_ratings_map.get(0.0, 0) + typed_ratings_map[1.0] = typed_ratings_map.get(1.0, 0) + for rating in sorted(typed_ratings_map.keys()): + total_gte = sum([v for k, v in typed_ratings_map.items() if k >= rating]) rating_key = int(rating * 1000) / 1000.0 thresh_counts[rating_key] = total_gte return thresh_counts -def get_env_name(): +def get_env_name() -> str: env = os.environ.get('MONTAGE_ENV') if env: return env @@ -159,6 +195,7 @@ def get_env_name(): 'debug': True, 'superusers': ['Slaporte', 'MahmoudHashemi'], 'dev_local_cookie_value': '"W7XGXxmUjl4kbkE0TWaFo4Oth50=?userid=NjAyNDQ3NA==&username=IlNsYXBvcnRlIg=="', + 'import_check_path': None, '__file__': 'devtest-builtin', '__env__': 'devtest', } @@ -203,13 +240,14 @@ def _bool(key, default=False): 'api_log_path': os.environ.get('MONTAGE_API_LOG_PATH', 'montage_api.log'), 'replay_log_path': os.environ.get('MONTAGE_REPLAY_LOG_PATH'), 'feel_log_path': os.environ.get('MONTAGE_FEEL_LOG_PATH'), + 'import_check_path': os.environ.get('MONTAGE_IMPORT_CHECK_PATH'), '__env__': env_name, '__file__': 'environment', }) return config -def load_env_config(env_name=None): +def load_env_config(env_name: str | None = None) -> dict[str, Any]: if not env_name: env_name = get_env_name() elif env_name == 'devtest': @@ -232,11 +270,11 @@ def load_env_config(env_name=None): return config -def load_default_series(): +def load_default_series() -> dict[str, str]: return DEFAULT_SERIES -def check_schema(db_url, base_type, echo=False, autoexit=False): +def check_schema(db_url: str, base_type: Any, echo: bool = False, autoexit: bool = False) -> list[str]: engine = create_engine(db_url, echo=echo) session_type = sessionmaker() session_type.configure(bind=engine) @@ -256,13 +294,13 @@ def check_schema(db_url, base_type, echo=False, autoexit=False): return schema_errors -def format_date(date): +def format_date(date: datetime.datetime | str | None) -> str | None: if isinstance(date, datetime.datetime): date = date.isoformat() return date -def js_isoparse(date_str): +def js_isoparse(date_str: str) -> datetime.datetime: try: ret = isoparse(date_str) except ValueError: @@ -273,7 +311,7 @@ def js_isoparse(date_str): return ret -def json_serial(obj): +def json_serial(obj: Any) -> str | list[Any]: if isinstance(obj, (datetime.datetime, datetime.date)): serial = obj.isoformat() return serial @@ -282,7 +320,7 @@ def json_serial(obj): raise TypeError("type %s not serializable" % type(obj)) -def parse_date(date): +def parse_date(date: str | None) -> datetime.datetime | None: if date is None: return None return isoparse(date) @@ -290,26 +328,26 @@ def parse_date(date): # The following is almost ready to go to boltons -def weighted_choice(weighted_choices): +def weighted_choice(weighted_choices: Sequence[tuple[float, T]] | dict[float, T]) -> T: nsw_list, vals = process_weighted_choices(weighted_choices) if len(vals) == 1: return vals[0] return fast_weighted_choice(nsw_list, vals) -def process_weighted_choices(wcp): +def process_weighted_choices(wcp: Sequence[tuple[float, T]] | dict[float, T]) -> tuple[Sequence[float], Sequence[T]]: if not wcp: raise ValueError('expected weight-choice list or map, not %r' % wcp) if isinstance(wcp, dict): - wcp = list(wcp.items()) + wcp_list = list(wcp.items()) else: - wcp = list(wcp) - if any([p[0] < 0 for p in wcp]): + wcp_list = list(wcp) + if any([p[0] < 0 for p in wcp_list]): raise ValueError('weight cannot be less than 0') - total = float(sum([p[0] for p in wcp], 0.0)) + total = float(sum([p[0] for p in wcp_list], 0.0)) if not total: raise ValueError() - norm_pairs = [(k / total, v) for k, v in wcp] + norm_pairs = [(k / total, v) for k, v in wcp_list] norm_pairs.sort(key=lambda x: x[0], reverse=True) summed_pairs = [] @@ -322,5 +360,5 @@ def process_weighted_choices(wcp): return (summed_sorted_weights, vals) -def fast_weighted_choice(nsw, values): +def fast_weighted_choice(nsw: Sequence[float], values: Sequence[T]) -> T: return values[bisect.bisect(nsw, random.random()) - 1] diff --git a/requirements.txt b/requirements.txt index 69208d33..4e76ac7e 100644 --- a/requirements.txt +++ b/requirements.txt @@ -82,7 +82,7 @@ requests==2.32.4 # via sentry-sdk secure-cookie==0.1.0 # via clastic -sentry-sdk==2.8.0 +sentry-sdk==2.71.0 # via -r requirements.in six==1.16.0 # via diff --git a/tools/admin.py b/tools/admin.py index 42dfb8a0..0db35484 100644 --- a/tools/admin.py +++ b/tools/admin.py @@ -38,6 +38,10 @@ CoordinatorDAO, reassign_rating_tasks, lookup_user) +from montage.import_check import (run_check, raise_if_blocked, importable, + import_warnings, describe_source, + source_params, lookup_source) +from montage.loaders import make_entry from montage.utils import get_threshold_map @@ -96,6 +100,7 @@ def main(): rnd_cmd.add(pause_round, name='pause') rnd_cmd.add(advance_round, name='advance') rnd_cmd.add(edit_round_quorum, name='edit-quorum') + rnd_cmd.add(remove_juror, name='remove-juror') rnd_cmd.add(check_round_dupes, name='check-dupes') rnd_cmd.add(apply_round_ratings, name='apply-ratings') rnd_cmd.add(retask_duplicate_ratings, name='retask-dupes') @@ -218,14 +223,13 @@ def create_round(user_dao, campaign_id, advance=False, debug=False): campaign = user_dao.get_campaign(campaign_id) if not advance: - entries = coord_dao.add_entries_from_cat(rnd.id, category_name) + request = {'import_method': 'category', 'category': category_name} + entries, params = _checked_entries(user_dao, coord_dao, rnd.id, request) source = category_name - # GIST_URL = 'https://gist.githubusercontent.com/slaporte/7433943491098d770a8e9c41252e5424/raw/ca394147a841ea5f238502ffd07cbba54b9b1a6a/wlm2015_fr_500.csv' - # entries = maint_dao.add_entries_from_csv_gist(rnd, GIST_URL) - # source = GIST_URL print(('++ prepared %s entries from %r' % (len(entries), source))) - coord_dao.add_round_entries(rnd.id, entries) + coord_dao.add_round_entries(rnd.id, entries, method='category', + params=params) else: final_rnds = [r for r in campaign.rounds if r.status == 'finalized'] last_successful_rnd = final_rnds[-1] @@ -237,7 +241,8 @@ def create_round(user_dao, campaign_id, advance=False, debug=False): raise UsageError('too many entries for ranking round') source = 'round(#%s)' % last_successful_rnd.id - coord_dao.add_round_entries(rnd.id, advancing_group, source) + coord_dao.add_round_entries(rnd.id, advancing_group, method='round', + params={'round_id': last_successful_rnd.id}) print(('++ added entries from %s to round %s (%r)' % (source, rnd.id, rnd.name))) @@ -466,36 +471,95 @@ def check_round_dupes(user_dao, round_id): return +def _checked_entries(user_dao, coord_dao, round_id, request): + """Check an import source like the round form does (#510), stop on + blocking rows, and store the checked entries; returns (entries, + round source params). The check is not written to a file.""" + result = run_check(request, coord_dao.campaign.id, source=lookup_source(), + rdb_session=user_dao.rdb_session) + raise_if_blocked(result) + for warning in import_warnings(result): + print('!! %s' % (list(warning.values())[0],)) + entries = [make_entry(row['commons']) for row in importable(result)] + entries = coord_dao.add_checked_entries(round_id, entries, + describe_source(result)) + return entries, source_params(result) + + def import_gist(user_dao, round_id, url): "import round entries from a csv list" coord_dao = CoordinatorDAO.from_round(user_dao, round_id) - entries, warnings = coord_dao.add_entries_from_csv(round_id, url) + request = {'import_method': 'gistcsv', 'gist_url': url} + entries, params = _checked_entries(user_dao, coord_dao, round_id, request) stats = coord_dao.add_round_entries(round_id, entries, method='gistcsv', - params={'gist_url': url}) + params=params) print('++ added entries to round %s: %r' % (round_id, stats)) -def edit_round_quorum(maint_dao, round_id): +def edit_round_quorum(user_dao, round_id): "change the quorum of a given round, assigning and reassigning tasks as need be" - rnd = maint_dao.get_round(round_id) + coord_dao = CoordinatorDAO.from_round(user_dao, round_id) + rnd = coord_dao.get_round(round_id) old_quorum = rnd.quorum if rnd.status != 'paused': print('-- round must be paused to edit quorum, aborting') return - print(('!! new quorum cannot be lower than current quorum: %s' % old_quorum)) - print(('!! new quorum cannot be higher than the number of jurors: %s' % len(rnd.jurors))) + active_juror_count = len([rj for rj in rnd.round_jurors if rj.is_active]) + print(('-- round %s (%s) currently has quorum %s and %s active jurors' + % (rnd.id, rnd.name, old_quorum, active_juror_count))) + print('-- decreases cancel excess un-cast tasks; completed votes are always kept') + print('!! new quorum cannot be higher than the number of active jurors') new_quorum = int(input('?? New quorum: ')) - new_juror_stats = maint_dao.modify_quorum(rnd, new_quorum) + stats = coord_dao.modify_quorum(rnd.id, new_quorum) - maint_dao.rdb_session.commit() + coord_dao.rdb_session.commit() print(('++ changed quorum in round %s (%s) from %s (old quorum) to %s (new quorum)' % (rnd.id, rnd.name, old_quorum, new_quorum))) - print(('++ reassigned %s tasks, with mean load of %s tasks per juror' - % (new_juror_stats['reassigned_task_count'], new_juror_stats['task_count_mean']))) + print(('++ created %s and cancelled %s tasks, reassigned %s, with mean load' + ' of %s tasks per juror' + % (stats['created_task_count'], stats['cancelled_task_count'], + stats['reassigned_task_count'], stats['task_count_mean']))) + + return stats - return new_juror_stats + +def remove_juror(user_dao, round_id): + "remove a juror from a paused round, cancelling their open tasks" + coord_dao = CoordinatorDAO.from_round(user_dao, round_id) + rnd = coord_dao.get_round(round_id) + if rnd.status != 'paused': + print('-- round must be paused to remove a juror, aborting') + return + active_jurors = sorted([rj.user.username for rj in rnd.round_jurors + if rj.is_active]) + print(('-- round %s (%s) has quorum %s and active jurors: %s' + % (rnd.id, rnd.name, rnd.quorum, ', '.join(active_jurors)))) + username = input('?? Username to remove: ') + if not username: + print('-- username required, aborting') + return + discard = input('?? Discard their completed votes as well? [y/N]: ') + discard_completed = discard.strip().lower() in ('y', 'yes') + summary = coord_dao.remove_juror(rnd.id, username, + discard_completed=discard_completed) + + coord_dao.rdb_session.commit() + + print(('++ removed juror %s from round %s (%s)' + % (summary['removed_juror'], rnd.id, rnd.name))) + print(('++ cancelled %s open tasks, discarded %s completed votes,' + ' created %s replacement tasks (%s entries uncoverable),' + ' quorum now %s, mean juror queue %s' + % (summary['cancelled_active_count'], + summary['discarded_completed_count'], + summary['created_task_count'], + summary['uncoverable_entry_count'], + summary['quorum'], + summary['task_count_mean']))) + + return summary def advance_round(user_dao, round_id, debug): diff --git a/tools/deploy.sh b/tools/deploy.sh index c40311f4..67a2d2ca 100644 --- a/tools/deploy.sh +++ b/tools/deploy.sh @@ -43,6 +43,22 @@ echo " Ref: $REF" echo " URL: $TOOL_URL" echo "" +# ── 0b. Required settings ──────────────────────────────────────────────────── +# The app refuses to start without these (montage/app.py). Check them before +# building, so that a missing one stops the deploy instead of the restarted +# pod. See deployment.md, "Deploying new changes", step 4. +# [unverified on Toolforge] that `toolforge envvars show NAME` exits non-zero +# for a variable that is not set. The value is not printed. +REQUIRED_ENVVARS=(MONTAGE_IMPORT_CHECK_PATH) +for NAME in "${REQUIRED_ENVVARS[@]}"; do + if ! toolforge envvars show "$NAME" >/dev/null 2>&1; then + echo "!! $NAME is not set for $TOOL_NAME: the new version would not start." + echo " Set it first (deployment.md, 'Deploying new changes', step 4)." + exit 1 + fi +done +echo "==> Required settings present: ${REQUIRED_ENVVARS[*]}" + # ── 1. Get expected SHA from GitHub ────────────────────────────────────────── echo "==> Resolving ref $REF ..."