From a5e0549ac93ffe9dff17849061e840a5ee00d4b4 Mon Sep 17 00:00:00 2001 From: Ayush Shukla Date: Thu, 16 Apr 2026 04:38:16 +0530 Subject: [PATCH 01/46] fix(dao): prevent silent discarding of SQLAlchemy filters in get_campaign_rounds (#399) --- montage/rdb.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/montage/rdb.py b/montage/rdb.py index b7052ff6..c32f0773 100644 --- a/montage/rdb.py +++ b/montage/rdb.py @@ -1138,8 +1138,8 @@ def from_round(cls, user_dao, round_id): def get_campaign_rounds(self, campaign, with_cancelled=False): q = self.query(Round).filter_by(campaign=campaign) if not with_cancelled: - q.filter(Round.status != CANCELLED_STATUS) - q.order_by(Round.create_date) + q = q.filter(Round.status != CANCELLED_STATUS) + q = q.order_by(Round.create_date) return q.all() def get_active_jurors(self, round_id): From af95219b0fb2a439e4a8a2d9b7236e8e7ef02cab Mon Sep 17 00:00:00 2001 From: Ayush Shukla Date: Tue, 21 Apr 2026 18:25:40 +0530 Subject: [PATCH 02/46] test(dao): add isolated unit tests for campaign round filtering and ordering (#399) --- montage/tests/test_dao_unit.py | 93 ++++++++++++++++++++++++++++++++++ 1 file changed, 93 insertions(+) create mode 100644 montage/tests/test_dao_unit.py 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) From 35d88d60b52d8841d62a4c1db313b13497045589 Mon Sep 17 00:00:00 2001 From: Ayush Shukla Date: Thu, 30 Apr 2026 21:01:03 +0530 Subject: [PATCH 03/46] refactor: modernize utils.py with type hints and remove legacy Py2 code --- montage/utils.py | 82 +++++++++++++++++++++--------------------------- 1 file changed, 36 insertions(+), 46 deletions(-) diff --git a/montage/utils.py b/montage/utils.py index 99433060..a329dfe9 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) @@ -78,14 +71,11 @@ class NotImplementedResponse(MontageError, BadRequest, NotImplementedError): '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 +85,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 +109,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: username = getpass.getuser() return USER_ENV_MAP.get(username, DEFAULT_ENV_NAME) @@ -153,7 +143,7 @@ def get_env_name(): } -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': @@ -173,11 +163,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) @@ -197,13 +187,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: @@ -214,7 +204,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 @@ -223,7 +213,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) @@ -231,26 +221,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 = [] @@ -263,5 +253,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] From 42c154ba491edc72916691f97354a325bbec0c0b Mon Sep 17 00:00:00 2001 From: Lodewijk <32471480+lgelauff@users.noreply.github.com> Date: Sun, 5 Apr 2026 16:57:17 +0100 Subject: [PATCH 04/46] Docs update, gitignore, and fix static route for local dev MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - README: add GitHub Issues link, mark Phabricator as archived, add pointer to dev.md - dev.md: fix Node.js version (v16→v18), rdb.py line number (113→119), and project tree (filenames, removed non-existent config/ dir) - .gitignore: add .claude/ and tmp/ - app.py: fix /a/ StaticFileRoute to point to static/index.html (not static/a/index.html which no longer exists) Co-Authored-By: Claude Sonnet 4.6 --- README.md | 9 ++++++++- dev.md | 5 ++--- montage/app.py | 2 +- 3 files changed, 11 insertions(+), 5 deletions(-) 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/dev.md b/dev.md index cec7c84a..06468e0c 100644 --- a/dev.md +++ b/dev.md @@ -174,8 +174,8 @@ npm run build ## Project structure ```bash -├── DEV.md -├── Dockerfile +├── dev.md +├── dockerfile ├── LICENSE ├── MANIFEST.in ├── Makefile @@ -222,7 +222,6 @@ npm run build │   ├── templates │   ├── tests │   └── utils.py -├── report.html ├── requirements-dev.txt ├── requirements.in ├── requirements.txt diff --git a/montage/app.py b/montage/app.py index 593f7521..d08caf15 100644 --- a/montage/app.py +++ b/montage/app.py @@ -185,7 +185,7 @@ def get_engine(): root_mws.append(SentryMiddleware()) root_app = Application([StaticFileRoute('/', STATIC_PATH + '/index.html'), - StaticFileRoute('/a/', STATIC_PATH + '/a/index.html'), + StaticFileRoute('/a/', STATIC_PATH + '/index.html'), ('/', static_app), ('/', ui_app), ('/v1/', api_app), From e989c139d4c9ad3e9c90c04bde1a931ca5afd501 Mon Sep 17 00:00:00 2001 From: Lodewijk <32471480+lgelauff@users.noreply.github.com> Date: Sun, 5 Apr 2026 16:58:53 +0100 Subject: [PATCH 05/46] dev.md: document frontend build step required before make start Co-Authored-By: Claude Sonnet 4.6 --- dev.md | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/dev.md b/dev.md index 06468e0c..73b7cc76 100644 --- a/dev.md +++ b/dev.md @@ -105,6 +105,10 @@ 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 From 4336c09a1b4e6184eaf36870c7ed27ad5c38376f Mon Sep 17 00:00:00 2001 From: Lodewijk <32471480+lgelauff@users.noreply.github.com> Date: Sun, 5 Apr 2026 17:59:54 +0100 Subject: [PATCH 06/46] dev.md: correct backend ready signal (Debugger PIN, not Serving on...) Co-Authored-By: Claude Sonnet 4.6 --- dev.md | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/dev.md b/dev.md index 73b7cc76..0e7f215d 100644 --- a/dev.md +++ b/dev.md @@ -113,7 +113,9 @@ npm run toolforge:build ```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. From 4d08ec71562b84e2e36e573faac97b7cfbb49bde Mon Sep 17 00:00:00 2001 From: Lodewijk <32471480+lgelauff@users.noreply.github.com> Date: Sun, 5 Apr 2026 18:12:44 +0100 Subject: [PATCH 07/46] Document debug mode, add debug option to config.default, remove dead static files - dev.md: document debug:true and userid options for bypassing OAuth locally - config.default.yaml: add debug:false as an optional documented field - Remove montage/static/a/index.html and static/dist/ (obsolete pre-Vue frontend files) Co-Authored-By: Claude Sonnet 4.6 --- config.default.yaml | 3 + dev.md | 2 + montage/static/a/index.html | 30 -------- montage/static/dist/images/logo_white_fat.svg | 71 ------------------- 4 files changed, 5 insertions(+), 101 deletions(-) delete mode 100644 montage/static/a/index.html delete mode 100644 montage/static/dist/images/logo_white_fat.svg diff --git a/config.default.yaml b/config.default.yaml index 649da534..0b58d145 100644 --- a/config.default.yaml +++ b/config.default.yaml @@ -6,6 +6,9 @@ db_url: "sqlite:///tmp_montage.db" cookie_secret: ReplaceThisWithSomethingSomewhatSecret superuser: Slaporte +# Optional: set to true to bypass OAuth and log in automatically as a maintainer (dev only) +debug: false + dev_local_cookie_value: "contact maintainers for details" dev_remote_cookie_value: "contact maintainers for details" oauth_secret_token: "see note below" diff --git a/dev.md b/dev.md index 0e7f215d..0b380165 100644 --- a/dev.md +++ b/dev.md @@ -95,6 +95,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 and be logged in automatically as a maintainer. This is the easiest way to get started locally without setting up OAuth. +* (Optional) Set `userid: ` to be auto-logged in as yourself instead of the default maintainer account. Your numeric user 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. diff --git a/montage/static/a/index.html b/montage/static/a/index.html deleted file mode 100644 index 1940cd71..00000000 --- a/montage/static/a/index.html +++ /dev/null @@ -1,30 +0,0 @@ - - - - Montage Admin - - - - - - - - - - - - - - - - - \ 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 - - - - - - - - - - From e4174ad43ab60ef6bca6521e303e831ba9fe8956 Mon Sep 17 00:00:00 2001 From: Lodewijk <32471480+lgelauff@users.noreply.github.com> Date: Tue, 7 Apr 2026 17:01:19 +0100 Subject: [PATCH 08/46] Make debug login configurable via debug_userid/debug_username config keys Add debug_userid and debug_username config options so developers can set which account they are auto-logged in as when debug: true, instead of always defaulting to Slaporte. Document the /complete_login workaround for bypassing OAuth locally. Co-Authored-By: Claude Sonnet 4.6 --- config.default.yaml | 5 ++++- dev.md | 4 ++-- montage/public_endpoints.py | 4 ++-- 3 files changed, 8 insertions(+), 5 deletions(-) diff --git a/config.default.yaml b/config.default.yaml index 0b58d145..05690a83 100644 --- a/config.default.yaml +++ b/config.default.yaml @@ -6,8 +6,11 @@ db_url: "sqlite:///tmp_montage.db" cookie_secret: ReplaceThisWithSomethingSomewhatSecret superuser: Slaporte -# Optional: set to true to bypass OAuth and log in automatically as a maintainer (dev only) +# 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 Slaporte / 6024474) +# debug_username: Slaporte +# debug_userid: 6024474 dev_local_cookie_value: "contact maintainers for details" dev_remote_cookie_value: "contact maintainers for details" diff --git a/dev.md b/dev.md index 0b380165..4b5dca90 100644 --- a/dev.md +++ b/dev.md @@ -95,8 +95,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 and be logged in automatically as a maintainer. This is the easiest way to get started locally without setting up OAuth. -* (Optional) Set `userid: ` to be auto-logged in as yourself instead of the default maintainer account. Your numeric user ID can be found at https://en.wikipedia.org/wiki/Special:CentralAuth/YOUR_USERNAME. +* (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. diff --git a/montage/public_endpoints.py b/montage/public_endpoints.py index fdfcc2da..0244b749 100644 --- a/montage/public_endpoints.py +++ b/montage/public_endpoints.py @@ -154,8 +154,8 @@ def logout(request, cookie, root_path): def complete_login(request, consumer_token, cookie, rdb_session, root_path, api_log, config): # TODO: Remove or standardize the DEBUG option if config.get('debug'): - identity = {'sub': 6024474, - 'username': 'Slaporte'} + identity = {'sub': config.get('debug_userid', 6024474), + 'username': config.get('debug_username', 'Slaporte')} else: handshaker = Handshaker(WIKI_OAUTH_URL, consumer_token) From 575f93a3154c5dbce7ca9bf41a84e49ad8cc2a04 Mon Sep 17 00:00:00 2001 From: Lodewijk <32471480+lgelauff@users.noreply.github.com> Date: Fri, 10 Apr 2026 13:12:44 +0200 Subject: [PATCH 09/46] Update deployment.md and dev.md with beta/prod observations - deployment.md: correct db_url format (mysql+pymysql://), document log paths for both production and beta instances separately - dev.md: remove non-existent config/ directory from project tree, fix Dockerfile -> dockerfile reference in Docker files section Co-Authored-By: Claude Sonnet 4.6 --- dev.md | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/dev.md b/dev.md index 4b5dca90..733574d2 100644 --- a/dev.md +++ b/dev.md @@ -188,10 +188,6 @@ npm run build ├── MANIFEST.in ├── Makefile ├── PROJECT_LOG.md -├── config -│   ├── beta-uwsgi.ini -│   ├── dev-uwsgi.ini -│   └── prod-uwsgi.ini ├── config.default.yaml ├── deployment.md ├── design.md @@ -293,5 +289,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 From d69126910919f3494ba42524ac629e21078ae8f1 Mon Sep 17 00:00:00 2001 From: Lodewijk <32471480+lgelauff@users.noreply.github.com> Date: Thu, 21 May 2026 20:19:31 +0200 Subject: [PATCH 10/46] fix: remove unused router import in RoundEdit.vue Co-Authored-By: Claude Sonnet 4.6 --- frontend/src/components/Round/RoundEdit.vue | 3 --- 1 file changed, 3 deletions(-) diff --git a/frontend/src/components/Round/RoundEdit.vue b/frontend/src/components/Round/RoundEdit.vue index 138a4a8c..e5dd75f9 100644 --- a/frontend/src/components/Round/RoundEdit.vue +++ b/frontend/src/components/Round/RoundEdit.vue @@ -100,9 +100,6 @@ import Delete from 'vue-material-design-icons/Delete.vue' import alertService from '@/services/alertService' import adminService from '@/services/adminService' import dialogService from '@/services/dialogService' -import { useRouter } from 'vue-router' - -const router = useRouter() const { t: $t } = useI18n() const props = defineProps({ round: Object, From 5a214cec7beca801900b11270a2c2290b7a4c250 Mon Sep 17 00:00:00 2001 From: Lodewijk <32471480+lgelauff@users.noreply.github.com> Date: Thu, 21 May 2026 20:36:20 +0200 Subject: [PATCH 11/46] fix: hide finalized/cancelled rounds from juror active view The juror round listing was filtering on campaign status only, so finalized and cancelled rounds inside active campaigns still appeared. Jurors clicking them got a confusing 400 "round not active" error. Now only active and paused rounds are shown in the active view. Suspected cause: e3069dd (PR #350, merged 2025-10-23) added a finalize_round endpoint that can finalize individual rounds without advancing, creating a state the juror listing never filtered for. Co-Authored-By: Claude Sonnet 4.6 --- montage/rdb.py | 3 +++ montage/tests/test_web_basic.py | 2 +- 2 files changed, 4 insertions(+), 1 deletion(-) diff --git a/montage/rdb.py b/montage/rdb.py index b7052ff6..ab0c369c 100644 --- a/montage/rdb.py +++ b/montage/rdb.py @@ -2707,6 +2707,9 @@ def get_all_rounds_task_counts(self, only_active=False): if only_active: users_rounds_query = users_rounds_query.where(campaigns_t.c.status == 'active') users_rounds_query = users_rounds_query.where(campaigns_t.c.is_archived == False) + users_rounds_query = users_rounds_query.where( + rounds_t.c.status.in_([ACTIVE_STATUS, PAUSED_STATUS]) + ) users_rounds_query = users_rounds_query.group_by( rounds_t.c.id, diff --git a/montage/tests/test_web_basic.py b/montage/tests/test_web_basic.py index 8050ea15..9ad856e5 100644 --- a/montage/tests/test_web_basic.py +++ b/montage/tests/test_web_basic.py @@ -750,7 +750,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/paused rounds shown; rounds 1+2 are finalized assert glom(resp, 'data.0.campaign.is_archived') is False From a8dac03fdf0bbda75dd0b7853e2b7174f5e58cf7 Mon Sep 17 00:00:00 2001 From: Lodewijk <32471480+lgelauff@users.noreply.github.com> Date: Thu, 21 May 2026 21:11:35 +0200 Subject: [PATCH 12/46] fix: exclude paused rounds from juror index view Paused rounds were still appearing in the juror round list because the only_active filter allowed both ACTIVE_STATUS and PAUSED_STATUS. However confirm_active() only permits ACTIVE_STATUS, so clicking a paused round always produced a 400 "round not active" error. Fix restricts the juror index to strictly active rounds only. Suspected introduced by e3069dd (hatnote/montage#350). Co-Authored-By: Claude Sonnet 4.6 --- montage/rdb.py | 2 +- montage/tests/test_web_basic.py | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/montage/rdb.py b/montage/rdb.py index ab0c369c..91cb6011 100644 --- a/montage/rdb.py +++ b/montage/rdb.py @@ -2708,7 +2708,7 @@ def get_all_rounds_task_counts(self, only_active=False): users_rounds_query = users_rounds_query.where(campaigns_t.c.status == 'active') users_rounds_query = users_rounds_query.where(campaigns_t.c.is_archived == False) users_rounds_query = users_rounds_query.where( - rounds_t.c.status.in_([ACTIVE_STATUS, PAUSED_STATUS]) + rounds_t.c.status == ACTIVE_STATUS ) users_rounds_query = users_rounds_query.group_by( diff --git a/montage/tests/test_web_basic.py b/montage/tests/test_web_basic.py index 9ad856e5..d6c4fd16 100644 --- a/montage/tests/test_web_basic.py +++ b/montage/tests/test_web_basic.py @@ -750,7 +750,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 # only active/paused rounds shown; rounds 1+2 are finalized + assert len(resp['data']) == 1 # only active rounds shown; rounds 1+2 are finalized assert glom(resp, 'data.0.campaign.is_archived') is False From 69d690327439cd114fbddc0ca789ecb0934896d0 Mon Sep 17 00:00:00 2001 From: Lodewijk <32471480+lgelauff@users.noreply.github.com> Date: Thu, 21 May 2026 21:47:37 +0200 Subject: [PATCH 13/46] fix: pass only_active parameter through in juror get_index get_index() accepted an only_active parameter but hard-coded True when calling get_all_rounds_task_counts, so get_all_campaigns() (the archive endpoint at /juror/campaigns/all) was always returning an empty list instead of finalized rounds. Co-Authored-By: Claude Sonnet 4.6 --- montage/juror_endpoints.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/montage/juror_endpoints.py b/montage/juror_endpoints.py index 0f6241cd..14382826 100644 --- a/montage/juror_endpoints.py +++ b/montage/juror_endpoints.py @@ -83,7 +83,7 @@ def get_index(user_dao, only_active=True): Summary: Get juror-level index of active campaigns and rounds. """ juror_dao = JurorDAO(user_dao) - counts = juror_dao.get_all_rounds_task_counts(only_active=True) + counts = juror_dao.get_all_rounds_task_counts(only_active=only_active) stats = [make_juror_round_details(rnd, rnd_stats, ballot) for rnd, rnd_stats, ballot in counts] return stats From 2d27134f120cfc2d4f0ee9bc52d2216048078c68 Mon Sep 17 00:00:00 2001 From: Sanjai Siddharthan Date: Sat, 27 Jun 2026 14:57:52 +0530 Subject: [PATCH 14/46] chore(disable_edit): Disabled Edit Round button when round is active and added entries in i18n --- frontend/src/components/Round/RoundView.vue | 16 +++++++++++----- frontend/src/i18n/en.json | 1 + frontend/src/i18n/qqq.json | 1 + 3 files changed, 13 insertions(+), 5 deletions(-) diff --git a/frontend/src/components/Round/RoundView.vue b/frontend/src/components/Round/RoundView.vue index c8828199..c0a7a302 100644 --- a/frontend/src/components/Round/RoundView.vue +++ b/frontend/src/components/Round/RoundView.vue @@ -34,11 +34,17 @@

- - - - {{ isRoundEditing ? $t('montage-btn-cancel') : $t('montage-round-edit') }} - + + + + + {{ isRoundEditing ? $t('montage-btn-cancel') : $t('montage-round-edit') }} + +
diff --git a/frontend/src/i18n/en.json b/frontend/src/i18n/en.json index 2ffdc5da..b5d80fc1 100644 --- a/frontend/src/i18n/en.json +++ b/frontend/src/i18n/en.json @@ -79,6 +79,7 @@ "montage-round-file-setting": "Round file settings", "montage-round-delete": "Delete round", "montage-round-edit": "Edit round", + "montage-round-edit-active-warning": "Please pause the round to make edits.", "montage-round-save": "Save Round", "montage-round-delete-confirm": "Are you sure you want to delete this round?", "montage-round-allowed-filetypes": "Allowed filetypes", diff --git a/frontend/src/i18n/qqq.json b/frontend/src/i18n/qqq.json index 5e641420..4d23cc83 100644 --- a/frontend/src/i18n/qqq.json +++ b/frontend/src/i18n/qqq.json @@ -82,6 +82,7 @@ "montage-round-file-setting": "Label for the file settings in a round.", "montage-round-delete": "Label for the action to delete a round.", "montage-round-edit": "Label for the action to edit a round.", + "montage-round-edit-active-warning": "Tooltip shown on the disabled edit button explaining that an active round must be paused before it can be edited.", "montage-round-save": "Label for the action to save changes to a round.", "montage-round-delete-confirm": "Confirmation message for deleting a round.", "montage-round-allowed-filetypes": "Label for the allowed file types setting in a round.", From 0c7e33db9c848ac56ad34b305baa591ccec64583 Mon Sep 17 00:00:00 2001 From: Mahmoud Hashemi Date: Wed, 19 Aug 2026 11:04:22 -0700 Subject: [PATCH 15/46] Support quorum decrease, coupled juror removal, damped-mean display Rounds could dead-end when a juror dropped out mid-round: modify_jurors refused to shrink the jury below quorum, and modify_quorum rejected any decrease (NotImplementedResponse, unchanged since 2017). The blocker was "whose completed vote do you discard?"; the answer is "none": results are a per-entry mean over completed votes, rounds close when no active votes remain, and unequal per-entry vote counts are already a supported state. Lowering quorum therefore only cancels excess not-yet-cast tasks, treating quorum as a coverage floor (incomplete block design posture), never discarding completed votes. New capabilities: - modify_quorum now supports decreases (cancel excess active tasks, then rebalance without reassign_all, so jurors keep what they hold) and its increase path actually creates tasks mid-round (see bug 1 below). - New module-level helpers: create_rating_tasks_for_coverage (top every non-DQ entry up to a target of non-cancelled votes, fewest-open-tasks juror first) and cancel_excess_rating_tasks (cancel above-target active votes from the most-loaded jurors first; max-min entry coverage is preserved by construction). - CoordinatorDAO.remove_juror + POST /admin/round//remove_juror: remove one juror from a paused round, cancelling their open tasks; their completed votes are kept unless discard_completed is set (values retained on the rows for audit; scoring only reads completed votes). Quorum implicitly drops to the remaining jury size when needed (or to an explicit new_quorum); affected entries are re-covered by remaining jurors who have not seen them. - edit_round applies quorum before jurors, so one RoundEdit.vue save can lower the quorum and shrink the jury together; error responses roll back both (covered by a rollback test). No frontend change needed: the quorum input has no client-side lower bound. - get_round_damped_rating_map: IMDb-style damped (Bayesian) mean with prior weight m = quorum, shown as damped_ratings beside the plain ratings in preview_results, to spot low-coverage entries whose score is unstable. Advancement still uses the plain mean. - tools/admin.py: round edit-quorum rewritten (it called nonexistent MaintainerDAO methods; now uses CoordinatorDAO.from_round like its siblings, prints created/cancelled counts) and new round remove-juror command. Latent bugs fixed in the 2016-17 assignment plumbing (5 and 6 were demonstrated red-first; 1 is pinned by a task-count regression test): 1. modify_quorum increases silently created zero tasks mid-round (create_initial_rating_tasks only covers vote-less entries). 2. reassign_ranking_tasks returned None when juror sets matched; callers subscript the result. 3. create_initial_rating_tasks' quorum ceiling counted inactive jurors. 4. get_rating_advancing_group averaged votes of any status; now filters completed like get_round_average_rating_map (this also guarantees discarded votes stay out of advancement). 5. modify_jurors' same-size swap path assumed exactly one added and one removed juror; replacing two-plus jurors at once stranded a removed juror's tasks. Multi-swaps now fall through to reassign_tasks. 6. reassign_rating_tasks crashed (ValueError in weighted_choice) when an active vote had no eligible juror; such orphaned tasks are now cancelled (coverage is already met) and reported via a new cancelled_task_count key. Also: Round.to_details_dict juror_details is no longer a 1-tuple (trailing comma). Tests: red-first regressions for bugs 1/5/6 (test_reassign.py is a new DAO-free unit test against in-memory sqlite), quorum decrease and combined-edit lifecycle coverage, remove_juror keep/discard flows, error paths (active round, unknown juror, last juror, oversized new_quorum, rollback), and damped-ratings presence/bounds. Full suite green (15 passed, 1 xfailed). CLI smoke-tested against a scratch DB. Design references: incomplete block designs (Niu & Rosenberger, arXiv:1806.00034), PeerReview4All max-min fairness (JMLR 22(163)), IMDb weighted rating / Bayesian average, and slaporte's 2017 Montage work-allocation gist (its "preserve" default is adopted; its discard question is dissolved by keep-everything). Workstream-Session-Id: 01a01955-97bb-7000-82f4-f3188a675ce2 --- montage/admin_endpoints.py | 32 +++ montage/rdb.py | 414 ++++++++++++++++++++++++++++---- montage/tests/test_reassign.py | 71 ++++++ montage/tests/test_web_basic.py | 215 ++++++++++++++++- tools/admin.py | 62 ++++- 5 files changed, 734 insertions(+), 60 deletions(-) create mode 100644 montage/tests/test_reassign.py diff --git a/montage/admin_endpoints.py b/montage/admin_endpoints.py index 6b918fba..decec64b 100644 --- a/montage/admin_endpoints.py +++ b/montage/admin_endpoints.py @@ -62,6 +62,7 @@ def get_admin_routes(): POST('/admin/round//finalize', finalize_round), GET('/admin/round/', get_round), POST('/admin/round//edit', edit_round), + POST('/admin/round//remove_juror', remove_juror), POST('/admin/round//cancel', cancel_round), GET('/admin/round//preview_results', get_round_results_preview), @@ -523,6 +524,36 @@ def edit_round(user_dao, round_id, request_dict): return {'data': new_val_map} +def remove_juror(user_dao, round_id, request_dict): + """ + Summary: Remove a juror from a paused round, cancelling their open + tasks. Their completed votes are kept unless + discard_completed is set. For yesno/rating rounds the + quorum drops to new_quorum (or to the remaining jury size + when that is smaller than the current quorum). + + Request model: + username: + type: string + discard_completed: + type: boolean + new_quorum: + type: int64 + + Response model: RemoveJurorSummary + """ + coord_dao = CoordinatorDAO.from_round(user_dao, round_id) + username = request_dict.get('username') + if not username or not isinstance(username, str): + raise InvalidAction('expected a username') + discard_completed = bool(request_dict.get('discard_completed', False)) + new_quorum = request_dict.get('new_quorum') + summary = coord_dao.remove_juror(round_id, username, + discard_completed=discard_completed, + new_quorum=new_quorum) + return {'data': summary} + + def cancel_round(user_dao, round_id): coord_dao = CoordinatorDAO.from_round(user_dao, round_id) rnd = coord_dao.cancel_round(round_id) @@ -543,6 +574,7 @@ def get_round_results_preview(user_dao, round_id): if rnd.vote_method in ('yesno', 'rating'): data['ratings'] = coord_dao.get_round_average_rating_map(round_id) + data['damped_ratings'] = coord_dao.get_round_damped_rating_map(round_id) try: data['thresholds'] = get_threshold_map(data['ratings']) except: diff --git a/montage/rdb.py b/montage/rdb.py index 2bfa56b9..75d38f72 100644 --- a/montage/rdb.py +++ b/montage/rdb.py @@ -43,7 +43,7 @@ to_unicode, get_mw_userid, weighted_choice, - PermissionDenied, InvalidAction, NotImplementedResponse, + PermissionDenied, InvalidAction, DoesNotExist, get_env_name, load_default_series, @@ -455,7 +455,7 @@ def to_details_dict(self): ret['quorum'] = self.quorum ret['total_round_entries'] = len(self.round_entries) ret['stats'] = self.get_count_map() - ret['juror_details'] = [rj.to_details_dict() for rj in self.round_jurors], + ret['juror_details'] = [rj.to_details_dict() for rj in self.round_jurors] return ret def confirm_active(self): @@ -1285,6 +1285,16 @@ def edit_round(self, round_id, round_dict): # if round_dict ...: # raise InvalidAction('unable to modify round attributes: %r' # % request_dict.keys()) + # quorum applies before jurors, so a single save can lower the + # quorum and shrink the jury together (the juror-edit guard + # checks the new roster against the already-updated quorum) + new_quorum = round_dict.get('quorum') + if new_quorum and new_quorum != rnd.quorum: + if rnd.status != PAUSED_STATUS: + raise InvalidAction('round must be paused to edit quorum') + else: + new_juror_stats = self.modify_quorum(round_id, new_quorum) + new_val_map['quorum'] = new_quorum new_juror_names = round_dict.get('new_jurors') cur_jurors = self.get_active_jurors(round_id) cur_juror_names = [u.username for u in cur_jurors] @@ -1296,13 +1306,6 @@ def edit_round(self, round_id, round_dict): raise InvalidAction('new_jurors must be a list of strings') new_juror_stats = self.modify_jurors(round_id, new_juror_names) new_val_map['new_jurors'] = new_juror_names - new_quorum = round_dict.get('quorum') - if new_quorum and new_quorum != rnd.quorum: - if rnd.status != PAUSED_STATUS: - raise InvalidAction('round must be paused to edit quorum') - else: - new_juror_stats = self.modify_quorum(round_id, new_quorum) - new_val_map['quorum'] = new_quorum show_stats = round_dict.get('show_stats') if show_stats is not None: rnd.show_stats = show_stats @@ -1888,6 +1891,7 @@ def get_rating_advancing_group(self, round_id, threshold=None): .options(joinedload('entry'))\ .filter_by(dq_user_id=None, round_id=round_id)\ .join(Vote)\ + .filter(Vote.status == COMPLETED_STATUS)\ .group_by(Vote.round_entry_id)\ .having(avg >= threshold)\ .all() @@ -1909,6 +1913,36 @@ def get_round_average_rating_map(self, round_id): return dict(rating_ctr) + def get_round_damped_rating_map(self, round_id): + """Same shape as get_round_average_rating_map, but each entry's + average is damped toward the round's overall mean C with prior + weight m = quorum (a Bayesian average): + + damped = (n * avg + m * C) / (n + m) + + Entries whose plain average rests on few completed votes get + pulled toward C, so coordinators can spot scores that are still + unstable (e.g. after a quorum decrease or juror removal). + """ + rnd = self.get_round(round_id) + results = self.query(func.avg(Vote.value), func.count(Vote.id))\ + .join(RoundEntry, RoundEntry.id == Vote.round_entry_id)\ + .filter(RoundEntry.round_id == round_id, + Vote.status == COMPLETED_STATUS)\ + .group_by(Vote.round_entry_id)\ + .all() + if not results: + return {} + + total_count = sum([count for _, count in results]) + overall_mean = sum([avg * count for avg, count in results]) / total_count + m = rnd.quorum or 1 + + damped_ctr = Counter([round((count * avg + m * overall_mean) + / (count + m), 3) + for avg, count in results]) + return dict(damped_ctr) + def get_round_ranking_list(self, round_id, notation=None): res = (self.query(Vote) .join(RoundEntry, RoundEntry.id == Vote.round_entry_id) @@ -2067,9 +2101,10 @@ def modify_jurors(self, round_id, new_jurors, force_balance=False): if rnd.vote_method == 'ranking': rnd.quorum = len(new_jurors) elif rnd.quorum > len(new_jurors): - raise InvalidAction('expected at least %s jurors to make quorum' - ' (%s) for round #%s' - % (len(new_jurors), rnd.quorum, rnd.id)) + raise InvalidAction('round #%s has quorum %s, which requires at' + ' least %s jurors; lower the quorum first (or' + ' in the same edit), or use the remove_juror' + ' endpoint' % (rnd.id, rnd.quorum, rnd.quorum)) new_juror_names = sorted([nj.username for nj in new_jurors]) old_jurors = self.get_active_jurors(rnd.id) old_juror_names = sorted([oj.username for oj in old_jurors]) @@ -2077,10 +2112,11 @@ def modify_jurors(self, round_id, new_jurors, force_balance=False): if new_juror_names == old_juror_names: raise InvalidAction('new jurors must differ from current jurors') - if len(new_jurors) == len(old_jurors) and not force_balance: - added_juror = list(set(new_jurors) - set(old_jurors))[0] - removed_juror = list(set(old_jurors) - set(new_jurors))[0] - res = swap_tasks(self.rdb_session, rnd, added_juror, removed_juror) + added = list(set(new_jurors) - set(old_jurors)) + removed = list(set(old_jurors) - set(new_jurors)) + if (len(added) == 1 and len(removed) == 1 + and len(new_jurors) == len(old_jurors) and not force_balance): + res = swap_tasks(self.rdb_session, rnd, added[0], removed[0]) else: res = reassign_tasks(self.rdb_session, rnd, new_jurors) @@ -2102,47 +2138,184 @@ def modify_jurors(self, round_id, new_jurors, force_balance=False): self.log_action('modify_jurors', round=rnd, message=msg) return res - def modify_quorum(self, round_id, new_quorum, strategy=None): - # This only supports increasing the quorum. Decreaseing the - # quorum would require handling some completed tasks (eg, - # whose vote do you discard? Randomly choose?) - + def remove_juror(self, round_id, username, + discard_completed=False, new_quorum=None): + """Remove a single juror from a paused round, cancelling their + not-yet-cast tasks. Their completed votes are kept unless + *discard_completed* is set (discarded votes flip to cancelled, + values retained on the rows for audit, and affected entries are + re-covered by the remaining jurors where possible). + + For yesno/rating rounds the quorum drops to *new_quorum* when + given, else to the remaining jury size when that is smaller + than the current quorum. + """ rnd = self.get_round(round_id) + if rnd.status != PAUSED_STATUS: + raise InvalidAction('round must be paused to remove a juror') + + removed_rj = None + for rj in rnd.round_jurors: + if rj.user.username == username and rj.is_active: + removed_rj = rj + break + if removed_rj is None: + raise InvalidAction('user %r is not an active juror of round #%s' + % (username, rnd.id)) + removed_user = removed_rj.user + + remaining = [rj.user for rj in rnd.round_jurors + if rj.is_active and rj.user.id != removed_user.id] + if not remaining: + raise InvalidAction('cannot remove the last juror') + session = self.rdb_session + now = datetime.datetime.now(datetime.timezone.utc).replace(tzinfo=None) old_quorum = rnd.quorum - if not new_quorum: - raise InvalidAction('must specify new quorum') + # discard first, so the excess-task cancellation below sees the + # true per-entry coverage and does not cancel tasks it would + # then have to recreate + discarded_completed_count = 0 + if discard_completed: + completed_votes = (session.query(Vote) + .filter_by(status=COMPLETED_STATUS, + user=removed_user) + .join(RoundEntry) + .filter_by(round=rnd) + .all()) + # values stay on the rows for audit; scoring only reads + # completed votes, so cancelled rows drop out of results + for vote in completed_votes: + vote.status = CANCELLED_STATUS + vote.modified_date = now + discarded_completed_count = len(completed_votes) - if new_quorum <= old_quorum: - raise NotImplementedResponse('currently we do not support quorum ' - 'decreases. current quorum is %r, got %r' - % (old_quorum, new_quorum)) + if rnd.vote_method == 'ranking': + if new_quorum is not None: + raise InvalidAction('no quorum for a ranking round') + cancelled_active_count = (session.query(Vote) + .filter_by(status=ACTIVE_STATUS, + user=removed_user) + .join(RoundEntry) + .filter_by(round=rnd) + .count()) + res = self.modify_jurors(round_id, + [u.username for u in remaining]) + created_task_count, uncoverable_entry_count = 0, 0 + else: + if new_quorum is not None: + if not 1 <= new_quorum <= len(remaining): + raise InvalidAction('new quorum must be between 1 and' + ' the number of remaining jurors' + ' (%s), got %r' + % (len(remaining), new_quorum)) + target_quorum = new_quorum + else: + target_quorum = min(rnd.quorum, len(remaining)) - jurors = self.get_active_jurors(rnd.id) - session = self.rdb_session + active_votes = (session.query(Vote) + .filter_by(status=ACTIVE_STATUS, + user=removed_user) + .join(RoundEntry) + .filter_by(round=rnd) + .all()) + for vote in active_votes: + vote.status = CANCELLED_STATUS + vote.modified_date = now + cancelled_active_count = len(active_votes) + + removed_rj.is_active = 0 + + if target_quorum < rnd.quorum: + cancel_excess_rating_tasks(session, rnd, target_quorum) + rnd.quorum = target_quorum + + # re-cover entries the cancellations/discards pushed below + # quorum, where remaining jurors who never saw them exist; + # unavoidable shortfalls are counted, not fatal (the mean + # stays defined for any completed count) + created_votes, uncoverable_entry_count = \ + create_rating_tasks_for_coverage( + session, rnd, target_per_entry=rnd.quorum) + created_task_count = len(created_votes) + res = reassign_rating_tasks(session, rnd, remaining) + + msg = ('%s removed juror %s from round #%s (discard_completed=%r,' + ' quorum %r -> %r): cancelled %s open tasks, discarded %s' + ' completed votes, created %s replacement tasks' + % (self.user.username, username, rnd.id, discard_completed, + old_quorum, rnd.quorum, cancelled_active_count, + discarded_completed_count, created_task_count)) + self.log_action('remove_juror', round=rnd, message=msg) + + return {'removed_juror': username, + 'cancelled_active_count': cancelled_active_count, + 'discarded_completed_count': discarded_completed_count, + 'created_task_count': created_task_count, + 'uncoverable_entry_count': uncoverable_entry_count, + 'quorum': rnd.quorum, + 'task_count_mean': res['task_count_mean']} - rnd_stats = rnd.to_details_dict() - if not rnd_stats['stats']['total_tasks']: - create_initial_tasks(session, rnd) + def modify_quorum(self, round_id, new_quorum, strategy=None): + """Change the quorum of a yesno/rating round mid-round. - rnd.quorum = new_quorum + Increases create just enough new tasks to bring every entry up + to the new quorum. Decreases cancel excess not-yet-cast tasks; + completed votes are never discarded (quorum is a coverage + floor, so entries may keep more completed votes than quorum). + """ + rnd = self.get_round(round_id) + old_quorum = rnd.quorum if rnd.vote_method == 'ranking': raise InvalidAction('no quorum for a ranking round') - elif rnd.vote_method in ('yesno', 'rating'): - new_tpe = new_quorum - old_quorum - # I'm pretty sure this will fairly distribute tasks - created_tasks = create_initial_rating_tasks(session, rnd, - tasks_per_entry=new_tpe) + if rnd.vote_method not in ('yesno', 'rating'): + raise ValueError('invalid vote method: %r' % rnd.vote_method) + if not new_quorum or new_quorum < 1: + raise InvalidAction('must specify a positive quorum') + if new_quorum == old_quorum: + raise InvalidAction('new quorum matches current quorum (%r)' + % old_quorum) + + jurors = self.get_active_jurors(rnd.id) + if new_quorum > len(jurors): + raise InvalidAction('quorum cannot be greater than the number' + ' of jurors') + + session = self.rdb_session + + if not rnd._get_task_count(): + create_initial_tasks(session, rnd) + + created_task_count, cancelled_task_count = 0, 0 + if new_quorum > old_quorum: + created_votes, _ = create_rating_tasks_for_coverage( + session, rnd, target_per_entry=new_quorum) + created_task_count = len(created_votes) + rnd.quorum = new_quorum ret = reassign_rating_tasks(session, rnd, jurors, - strategy=strategy, reassign_all=True) + strategy=strategy, + reassign_all=True) else: - raise ValueError('invalid vote method: %r' % rnd.vote_method) + cancel_stats = cancel_excess_rating_tasks( + session, rnd, target_per_entry=new_quorum) + cancelled_task_count = cancel_stats['cancelled_task_count'] + rnd.quorum = new_quorum + # no reassign_all: jurors keep the tasks they hold, only + # over-full queues shed tasks (less juror churn) + ret = reassign_rating_tasks(session, rnd, jurors, + strategy=strategy) - msg = ('%s changed round #%s quorum (%r -> %r), reassigned %s tasks' - ' (average juror task queue is now at %s)' + ret['created_task_count'] = created_task_count + ret['cancelled_task_count'] = (cancelled_task_count + + ret.get('cancelled_task_count', 0)) + + msg = ('%s changed round #%s quorum (%r -> %r), created %s and' + ' cancelled %s tasks, reassigned %s (average juror task' + ' queue is now at %s)' % (self.user.username, rnd.id, old_quorum, new_quorum, + ret['created_task_count'], ret['cancelled_task_count'], ret['reassigned_task_count'], ret['task_count_mean'])) self.log_action('modify_quorum', round=rnd, message=msg) @@ -3009,12 +3182,13 @@ def create_initial_rating_tasks(rdb_session, rnd, tasks_per_entry=None): if not tasks_per_entry: tasks_per_entry = rnd.quorum - if tasks_per_entry > len(rnd.round_jurors): - raise InvalidAction('quorum cannot be greater than the number of jurors') - jurors = [rj.user for rj in rnd.round_jurors if rj.is_active] if not jurors: raise InvalidAction('expected round with active jurors') + + if tasks_per_entry > len(jurors): + raise InvalidAction('quorum cannot be greater than the number of jurors') + random.shuffle(jurors) rdb_type = rdb_session.bind.dialect.name @@ -3058,6 +3232,135 @@ def create_initial_rating_tasks(rdb_session, rnd, tasks_per_entry=None): return ret +def create_rating_tasks_for_coverage(session, rnd, target_per_entry): + """Bring every non-disqualified entry of *rnd* up to + *target_per_entry* total non-cancelled (active or completed) votes. + + Unlike create_initial_rating_tasks, this covers entries that + already have votes, so it works mid-round (quorum increases, + re-covering entries after a juror's completed votes are + discarded). + + Each new task goes to the eligible active juror with the fewest + open tasks in the round (ties broken by user id, for determinism). + A juror is eligible for an entry if they have no active or + completed vote on it; a cancelled vote does not disqualify. + + Returns ``(created_votes, shortfall_entry_count)``, the latter + counting entries with fewer eligible jurors than needed tasks + (they get as many as possible; the entry mean stays defined for + any completed count). + """ + jurors = [rj.user for rj in rnd.round_jurors if rj.is_active] + if not jurors: + raise InvalidAction('expected round with active jurors') + + round_entries = (session.query(RoundEntry) + .filter(RoundEntry.round_id == rnd.id, + RoundEntry.dq_user_id == None) + .all()) + + votes = (session.query(Vote) + .join(RoundEntry, RoundEntry.id == Vote.round_entry_id) + .filter(RoundEntry.round_id == rnd.id, + Vote.status.in_((ACTIVE_STATUS, + COMPLETED_STATUS))) + .all()) + + have_map = defaultdict(list) # round_entry_id -> [Vote, ...] + open_count_map = dict([(j.id, 0) for j in jurors]) + for vote in votes: + have_map[vote.round_entry_id].append(vote) + if vote.status == ACTIVE_STATUS and vote.user_id in open_count_map: + open_count_map[vote.user_id] += 1 + + juror_map = dict([(j.id, j) for j in jurors]) + + created_votes = [] + shortfall_entry_count = 0 + for round_entry in round_entries: + cur_votes = have_map[round_entry.id] + needed = target_per_entry - len(cur_votes) + if needed <= 0: + continue + seen_ids = set([v.user_id for v in cur_votes]) + elig_ids = [j_id for j_id in juror_map if j_id not in seen_ids] + if len(elig_ids) < needed: + shortfall_entry_count += 1 + for _ in range(min(needed, len(elig_ids))): + j_id = min(elig_ids, key=lambda j: (open_count_map[j], j)) + elig_ids.remove(j_id) + vote = Vote(user=juror_map[j_id], round_entry=round_entry, + status=ACTIVE_STATUS) + session.add(vote) + created_votes.append(vote) + open_count_map[j_id] += 1 + + return created_votes, shortfall_entry_count + + +def cancel_excess_rating_tasks(session, rnd, target_per_entry): + """Cancel not-yet-cast (active) votes in excess of + *target_per_entry* per entry. Completed votes are never touched: + an entry with more completed votes than the target simply keeps + them all (quorum is a coverage floor, not an exact count). + + Excess tasks are cancelled from the jurors with the most open + tasks in the round first (running counts; ties broken by vote id, + newest first), evening out juror queues while entry coverage stays + at or above the target by construction. + + Returns a dict with ``cancelled_task_count`` and + ``entries_below_target`` (informational: entries whose total + non-cancelled votes are under the target, only possible via prior + discards, never via this function). + """ + now = datetime.datetime.now(datetime.timezone.utc).replace(tzinfo=None) + + round_entries = (session.query(RoundEntry) + .filter(RoundEntry.round_id == rnd.id, + RoundEntry.dq_user_id == None) + .all()) + + votes = (session.query(Vote) + .join(RoundEntry, RoundEntry.id == Vote.round_entry_id) + .filter(RoundEntry.round_id == rnd.id, + RoundEntry.dq_user_id == None, + Vote.status.in_((ACTIVE_STATUS, + COMPLETED_STATUS))) + .all()) + + entry_vote_map = defaultdict(list) + open_count_map = defaultdict(int) + for vote in votes: + entry_vote_map[vote.round_entry_id].append(vote) + if vote.status == ACTIVE_STATUS: + open_count_map[vote.user_id] += 1 + + cancelled_task_count = 0 + entries_below_target = 0 + for round_entry in round_entries: + re_votes = entry_vote_map[round_entry.id] + if len(re_votes) < target_per_entry: + entries_below_target += 1 + continue + completed_count = len([v for v in re_votes + if v.status == COMPLETED_STATUS]) + active = [v for v in re_votes if v.status == ACTIVE_STATUS] + keep_active = max(0, target_per_entry - completed_count) + for _ in range(len(active) - keep_active): + vote = max(active, + key=lambda v: (open_count_map[v.user_id], v.id)) + active.remove(vote) + vote.status = CANCELLED_STATUS + vote.modified_date = now + open_count_map[vote.user_id] -= 1 + cancelled_task_count += 1 + + return {'cancelled_task_count': cancelled_task_count, + 'entries_below_target': entries_below_target} + + def reassign_tasks(session, rnd, new_jurors, strategy=None): if rnd.vote_method in ('yesno', 'rating'): ret = reassign_rating_tasks(session, rnd, new_jurors, @@ -3106,7 +3409,7 @@ def reassign_ranking_tasks(session, rnd, new_jurors, strategy=None): old_juror_id_set = set([j.id for j in old_jurors]) new_juror_id_set = set([j.id for j in new_jurors]) if new_juror_id_set == old_juror_id_set: - return + return {'reassigned_task_count': 0, 'task_count_mean': -1} removed_jurors = [j for j in old_jurors if j.id not in new_juror_id_set] added_jurors = [j for j in new_jurors if j.id not in old_juror_id_set] @@ -3218,16 +3521,29 @@ def choose_eligible(eligible_users): return weighted_choice(wcp) + now = datetime.datetime.now(datetime.timezone.utc).replace(tzinfo=None) + cancelled_votes = [] + while reassg_queue: vote = reassg_queue.pop() - vote.user = choose_eligible(elig_map[vote.round_entry]) - elig_map[vote.round_entry].remove(vote.user) + eligible = elig_map[vote.round_entry] + if not eligible: + # every juror in new_jurors has already completed this + # entry: the orphaned task is unfulfillable and the entry's + # coverage is already met, so cancel it + vote.status = CANCELLED_STATUS + vote.modified_date = now + cancelled_votes.append(vote) + continue + vote.user = choose_eligible(eligible) + eligible.remove(vote.user) target_work_map[vote.user].append(vote) vote_count_map = dict([(u, len(t)) for u, t in target_work_map.items()]) return {'incomplete_task_count': len(incomp_votes), - 'reassigned_task_count': len(reassg_votes), + 'reassigned_task_count': len(reassg_votes) - len(cancelled_votes), + 'cancelled_task_count': len(cancelled_votes), 'task_count_map': vote_count_map, 'task_count_mean': mean(list(vote_count_map.values()))} 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 44605851..8b6ac44a 100644 --- a/montage/tests/test_web_basic.py +++ b/montage/tests/test_web_basic.py @@ -376,17 +376,122 @@ 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') + resp = fetch('coordinator: reactivate our round', '/admin/round/%s/activate' % round_id, {'post': True}, @@ -592,6 +697,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 +793,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 +905,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, @@ -863,6 +1058,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.""" diff --git a/tools/admin.py b/tools/admin.py index 42dfb8a0..5a99da0a 100644 --- a/tools/admin.py +++ b/tools/admin.py @@ -96,6 +96,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') @@ -476,26 +477,69 @@ def import_gist(user_dao, round_id, url): 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): From e72ae5895f5c5292c0079c108c065824a26e0888 Mon Sep 17 00:00:00 2001 From: Mahmoud Hashemi Date: Thu, 20 Aug 2026 01:29:13 -0700 Subject: [PATCH 16/46] Fix stale quorum/jurors in RoundEdit found while screenshotting Discovered live testing the quorum-decrease UI: the round-edit form was unusable for its own core cases, independent of the quorum-decrease feature itself. - Round.to_info_dict() never included quorum (only to_details_dict() did), but GET /admin/campaign/ -- the endpoint that populates the round list RoundEdit.vue reads its initial `round` prop from -- uses to_info_dict(). So the quorum field always opened blank. Worse, saving with a blank field sends quorum: '' to PATCH /round//edit, which edit_round's `if new_quorum and new_quorum != rnd.quorum` silently treats as "no change" (200 OK, nothing applied, no error toast) -- indistinguishable from the backend just not working. - RoundEdit.vue seeded its juror chips from the unfiltered jurors list (every juror ever added to the round, active or not -- intentional at the to_info_dict() layer, since the finalized-campaign report's all_jurors credit list at rdb.py depends on seeing removed jurors too). The edit form needs only the active roster; each juror already carries is_active, so filtered on the frontend instead of narrowing the shared serializer. - saveRound() never reloaded the page on success, unlike its own sibling deleteRound() (and RoundInfo's activate/pause). Round-level panels refresh independently via their own fetch, but the `round` prop RoundEdit reads from is fetched once at the parent and was otherwise never refreshed for the rest of the SPA session -- so even a successful save would show reverted-looking quorum/jurors on the next Edit open. Fix: add quorum to Round.to_info_dict() (drop the now-redundant reassignment in to_details_dict()); filter jurors to is_active before seeding RoundEdit's form; reload after a successful save. Verified live against the seeded scratch round from the earlier screenshot session: quorum now pre-fills with the true current value, a removed juror's chip and the new quorum both persist correctly on reopening Edit with no page refresh, matching the exact remove-juror-then-lower-quorum flow the quorum-decrease feature exists to support. Workstream-Session-Id: 01a01955-97bb-7000-82f4-f3188a675ce2 --- frontend/src/components/Round/RoundEdit.vue | 3 ++- montage/rdb.py | 2 +- 2 files changed, 3 insertions(+), 2 deletions(-) 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/montage/rdb.py b/montage/rdb.py index 75d38f72..6df07efd 100644 --- a/montage/rdb.py +++ b/montage/rdb.py @@ -445,6 +445,7 @@ def to_info_dict(self): 'status': self.status, 'config': self.config, 'show_stats': self.show_stats, + 'quorum': self.quorum, 'round_sources': []} return ret @@ -452,7 +453,6 @@ def to_details_dict(self): ret = self.to_info_dict() ret['is_closable'] = self.check_closability() ret['campaign'] = self.campaign.to_info_dict() - ret['quorum'] = self.quorum ret['total_round_entries'] = len(self.round_entries) ret['stats'] = self.get_count_map() ret['juror_details'] = [rj.to_details_dict() for rj in self.round_jurors] From e358a264c75e76c6b04d7478cc770c8430c0b864 Mon Sep 17 00:00:00 2001 From: lgelauff <32471480+lgelauff@users.noreply.github.com> Date: Sun, 4 Oct 2026 10:31:16 +0000 Subject: [PATCH 17/46] fix(meta): show resource names, not config values; logs/feel for maintainers only /meta and /meta/json/ are public. clastic's resource section shows the first 70 characters of repr() of each app resource, which for 'config' showed the start of the config dict to anyone. The resource section now lists each resource by name and type only (ResourceNamesPeripheral); the rest of /meta (start time, routes, host, process) is unchanged, so deploy.sh's smoke test keeps working. GET /v1/logs/feel (the front-end error log tail: usernames and error payloads) had no permission check; it now requires a maintainer, like /logs/api and /logs/api_exc. Co-Authored-By: Claude Opus 5.5 --- montage/app.py | 23 ++++++++++++++++++++++- montage/meta_endpoints.py | 6 ++++-- montage/tests/test_web_basic.py | 33 +++++++++++++++++++++++++++++++++ 3 files changed, 59 insertions(+), 3 deletions(-) diff --git a/montage/app.py b/montage/app.py index 3219be11..ff76d148 100644 --- a/montage/app.py +++ b/montage/app.py @@ -5,6 +5,7 @@ import logging from clastic import Application, StaticFileRoute, MetaApplication +from clastic.meta import DEFAULT_PERIPHERALS, ResourcePeripheral from clastic.static import StaticApplication from clastic.middleware import HTTPCacheMiddleware @@ -46,6 +47,26 @@ 'templates') +class ResourceNamesPeripheral(ResourcePeripheral): + """/meta's resource section with names and types only. clastic's + default shows the first 70 characters of each resource's repr(), + which for 'config' showed config values to anyone.""" + + def get_context(self, _application): + resources = [] + for key, val in _application.resources.items(): + desc = type(val).__name__ + if isinstance(val, (dict, list, tuple, set)): + desc += ' (%s items)' % len(val) + resources.append({'key': key, 'value': desc}) + return {'resources': resources} + + +META_PERIPHERALS = [ResourceNamesPeripheral() + if isinstance(peri, ResourcePeripheral) else peri + for peri in DEFAULT_PERIPHERALS] + + def set_mysql_session_charset_and_collation(connection, branch): # https://dev.mysql.com/doc/refman/8.0/en/set-names.html connection.execute("SET NAMES 'utf8mb4' COLLATE 'utf8mb4_unicode_ci'") @@ -197,7 +218,7 @@ def get_engine(): ('/', static_app), ('/', ui_app), ('/v1/', api_app), - ('/meta', MetaApplication())], + ('/meta', MetaApplication(base_peripherals=META_PERIPHERALS))], resources={'config': config}, middlewares=root_mws) diff --git a/montage/meta_endpoints.py b/montage/meta_endpoints.py index 73880f98..7d510274 100644 --- a/montage/meta_endpoints.py +++ b/montage/meta_endpoints.py @@ -127,8 +127,10 @@ def post_frontend_error_log(user, config, request_dict): return -def get_frontend_error_log(config, request_dict): - # TODO +def get_frontend_error_log(config, user, request_dict): + # usernames and error payloads: maintainers only, like the other logs + if not user or not user.is_maintainer: + raise Forbidden() request_dict = request_dict or dict() count = int(request_dict.get('count', DEFAULT_LINE_COUNT)) feel_path = config.get('feel_log_path', None) diff --git a/montage/tests/test_web_basic.py b/montage/tests/test_web_basic.py index eb7bd913..11e4597b 100644 --- a/montage/tests/test_web_basic.py +++ b/montage/tests/test_web_basic.py @@ -822,6 +822,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? From 56fdf604a9bb1b6386ec049b43b338ad9ecffbe5 Mon Sep 17 00:00:00 2001 From: lgelauff <32471480+lgelauff@users.noreply.github.com> Date: Mon, 5 Oct 2026 19:21:54 +0000 Subject: [PATCH 18/46] wip(#510): import errors, labs lookup by file_id, source fetch/parse - utils: ImportSourceInvalid/CheckBlocked/CheckExpired/CheckRequired (400 with error_type); config key import_check_path (MONTAGE_IMPORT_CHECK_PATH) - labs: one query text for lookups by name and by file_id; get_files_info_by_ids - loaders: fetch_source_text, parse_source_rows (filename > img_name, optional file_id, BOM/case-insensitive headers), parse_name_list, lookups; old full-CSV/name-list loaders removed (#509, #517); only gist links are rewritten (#208) Refs hatnote/montage#510, #509, #208 Co-Authored-By: Claude Opus 5.5 --- montage/labs.py | 43 ++++- montage/loaders.py | 349 ++++++++++++++++++++-------------- montage/tests/test_loaders.py | 327 +++++++++++++++++++++---------- montage/utils.py | 33 ++++ 4 files changed, 500 insertions(+), 252 deletions(-) diff --git a/montage/labs.py b/montage/labs.py index dd6e6e18..c7223b91 100644 --- a/montage/labs.py +++ b/montage/labs.py @@ -150,10 +150,11 @@ def get_files_by_name(file_names): return _files_by_name(fetchall, file_names) -def _files_by_name(fetchall, file_names): - if not file_names: - return [] - query = ''' +def _files_query(column, count): + """The file details query for `count` values of file.; one + query text for lookups by name and by file_id.""" + assert column in ('file_name', 'file_id') + return ''' SELECT DISTINCT {cols} FROM commonswiki_p.file AS file JOIN commonswiki_p.filerevision AS fr ON fr.fr_id = file.file_latest @@ -161,14 +162,28 @@ def _files_by_name(fetchall, file_names): 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}) + WHERE file.{column} IN ({values}) AND file.file_deleted = 0 '''.format(cols=', '.join(FILE_COLS), earliest_rev=_EARLIEST_REVISION_SUBQUERY, - names=', '.join(['%s'] * len(file_names))) + column=column, + values=', '.join(['%s'] * count)) + + +def _files_by_name(fetchall, file_names): + if not file_names: + return [] + query = _files_query('file_name', len(file_names)) return fetchall(query, tuple(file_names)) +def _files_by_id(fetchall, file_ids): + if not file_ids: + return [] + query = _files_query('file_id', len(file_ids)) + return fetchall(query, tuple(file_ids)) + + def get_files_info_by_names(file_names): """{underscored name: file info} for the names that exist, looked up in chunks instead of one query per file, all on one connection.""" @@ -184,6 +199,22 @@ def get_files_info_by_names(file_names): return ret +def get_files_info_by_ids(file_ids): + """{file_id: file info} for the file_ids of live files (Commons' + file.file_id), in chunks on one connection, like + get_files_info_by_names (hatnote/montage#510).""" + ids = sorted(set(int(i) for i in file_ids)) + ret = {} + if not ids: + return ret + with commonswiki_connection(COMMONS_DB_HOST) as fetchall: + for i in range(0, len(ids), FILE_LOOKUP_CHUNK_SIZE): + chunk = ids[i:i + FILE_LOOKUP_CHUNK_SIZE] + for rec in _files_by_id(fetchall, chunk): + ret[int(rec['file_id'])] = rec + return ret + + def get_files(category_name): # Two steps because category membership and file data now live on # different database clusters, which cannot be joined in SQL. diff --git a/montage/loaders.py b/montage/loaders.py index 8a53b561..bdcfcf8d 100644 --- a/montage/loaders.py +++ b/montage/loaders.py @@ -2,29 +2,25 @@ from __future__ import absolute_import import datetime -from io import BytesIO, StringIO +from io import StringIO +import csv import json import re +import requests from boltons.iterutils import chunked_iter -from unicodecsv import DictReader import montage.rdb # TODO: circular import -from .labs import get_files, get_files_info_by_names -from .utils import unicode, requests_get, requests_post +from .labs import get_files, get_files_info_by_names, get_files_info_by_ids +from .utils import (ImportSourceInvalid, to_unicode, requests_get, + requests_post) REMOTE_UTILS_URL = 'https://montage.toolforge.org/v1/utils/' GSHEET_URL = 'https://docs.google.com/spreadsheets/d/%s/gviz/tq?tqx=out:csv' -CSV_FULL_COLS = ['img_name', - 'img_major_mime', - 'img_minor_mime', - 'img_width', - 'img_height', - 'img_user', - 'img_user_text', - 'img_timestamp'] +# seconds; a source link is fetched within the 30 s request limit +SOURCE_FETCH_TIMEOUT = 15 def wpts2dt(timestamp): @@ -68,8 +64,8 @@ def make_entry(edict): 'archive_name': edict['oi_archive_name']} raw_entry['upload_date'] = wpts2dt(edict['img_timestamp']) raw_entry['resolution'] = width * height - # file_id is only available from wikireplica imports (labs.py), not CSV. - # CSV-imported entries intentionally get file_id=NULL. + # file_id comes from Commons (labs.py); every checked import has one, + # except dev's remote lookups where the utils endpoint returns none. if edict.get('file_id') is not None: raw_entry['file_id'] = edict['file_id'] if edict.get('flags'): @@ -77,165 +73,222 @@ def make_entry(edict): return montage.rdb.Entry(**raw_entry) -def load_full_csv(csv_file_obj, source='remote'): - # TODO: streaming this for big CSVs is an unnecessary headache +# --------------------------------------------------------------------------- +# Import sources (hatnote/montage#510): every first-round source is fetched +# and parsed into rows here, then looked up on Commons and classified by +# import_check.py. Commons supplies all metadata; a CSV only gives names and, +# optionally, Commons file_ids (#509). +# --------------------------------------------------------------------------- - ret = [] - warnings = [] +NAME_COLUMNS = ('filename', 'img_name') # in order of preference +FILE_ID_COLUMN = 'file_id' - dr = DictReader(csv_file_obj) +# a header-less list's first line is a file name, not a header, when it ends +# in a file extension (header words never do) +_FILE_EXT_RE = re.compile(r'\.[A-Za-z0-9]{2,5}$') +_FILE_ID_RE = re.compile(r'^[0-9]+$') - if 'filename' in dr.fieldnames: - return load_partial_csv(dr, source=source) +# Cells starting with one of these get a leading ' in Montage's CSV +# downloads, so spreadsheet programs do not run them as formulas; reading +# a CSV removes that ' again, so a download can be uploaded unchanged. +FORMULA_START_CHARS = "=+-@'" - for key in CSV_FULL_COLS: - if key not in dr.fieldnames: - raise ValueError('missing required column "%s" in csv file' % key) - for edict in dr: - try: - entry = make_entry(edict) - except TypeError as e: - warnings.append((edict, e)) - else: - ret.append(entry) +def guard_cell(value): + if value and value[0] in FORMULA_START_CHARS: + return "'" + value + return value - return ret, warnings +def unguard_cell(value): + if len(value) > 1 and value[0] == "'" and value[1] in FORMULA_START_CHARS: + return value[1:] + return value -def load_partial_csv(dr, source='remote'): - ret = [] - warnings = [] - file_names = [r['filename'] for r in dr] - if not file_names or isinstance(file_names[0], unicode): - file_names_obj = StringIO('\n'.join(file_names)) - else: - file_names_obj = BytesIO(b'\n'.join(file_names)) - return load_name_list(file_names_obj, source=source) +def normalise_name(name): + """A file name as Montage and the replica store it: no File: prefix, + underscores for spaces.""" + name = unguard_cell((name or '').strip()) + if name.startswith('File:'): + name = name[5:].strip() + return name.replace(' ', '_') -def load_name_list(file_obj, source='local'): - """ Just the file names, and we'll look up the rest""" +def parse_file_id(cell): + """(file_id, malformed) for a file_id cell: a plain whole number above + 0, or empty. 1.49673E+08, 149,673,020 or 12.0 are malformed.""" + value = unguard_cell((cell or '').strip().strip(u'\xa0').strip()) + if not value: + return None, False + if _FILE_ID_RE.match(value) and int(value) > 0: + return int(value), False + return None, True - ret = [] - warnings = [] - rl = file_obj.readlines() +def _source_row(row_number, name_cell, file_id_cell=''): + file_id, malformed = parse_file_id(file_id_cell) + return {'row': row_number, + 'name_as_written': name_cell or '', + 'file_id_as_written': file_id_cell or '', + 'name': normalise_name(name_cell), + 'file_id': file_id, + 'malformed_file_id': malformed} - # clean up the filenames - for i, filename in enumerate(rl): - if isinstance(filename, bytes): - filename = filename.decode('utf8') - filename = filename.strip() - if filename.startswith('File:'): - filename = filename[5:] - rl[i] = filename - edicts = [] +def _is_blank(row): + return not row['name'] and not row['file_id_as_written'].strip() - if source == 'remote': - edicts, warnings = get_by_filename_remote(rl) - else: - edicts, warnings = _lookup_file_names(rl) - for edict in edicts: - try: - entry = make_entry(edict) - except TypeError as e: - warnings.append((edict, e)) - else: - ret.append(entry) +def _header_key(cell): + return cell.replace(u'\ufeff', '').strip().lower() - return ret, warnings -def get_entries_from_csv(raw_url, source='local'): - if 'google.com' in raw_url: - return get_entries_from_gsheet(raw_url, source) - return get_entries_from_gist(raw_url, source) +def _looks_like_file_name(line): + value = line.strip().strip('"').strip() + if value.startswith('File:'): + value = value[5:] + return bool(_FILE_EXT_RE.search(value)) -def get_entries_from_gist(raw_url, source='local'): - if 'githubusercontent' not in raw_url: - raw_url = raw_url.replace('gist.github.com', - 'gist.githubusercontent.com') + '/raw' - resp = requests_get(raw_url) +def _line_name(line): + """A header-less list's line: one CSV cell (a quoted name, as Google + Sheets exports), or else the whole line, so commas in names survive.""" try: - ret, warnings = load_full_csv(BytesIO(resp.content)) - except ValueError as e: - # not a full csv - ret, warnings = load_name_list(BytesIO(resp.content), source=source) - - return ret, warnings - - -def get_entries_from_gsheet(raw_url, source='local'): - #TODO: add support for sheet tabs - doc_id = parse_doc_id(raw_url) - url = GSHEET_URL % doc_id - resp = requests_get(url) - - if not 'text/csv' in resp.headers['content-type']: - raise ValueError('cannot load Google Sheet "%s" (is link sharing on?)' % raw_url) - + cells = next(csv.reader([line])) + except (StopIteration, csv.Error): + cells = [] + if len(cells) == 1: + return cells[0] + return line.strip().strip('"') + + +def parse_source_rows(text): + """(rows, columns) of a CSV or a header-less name list. + + The name column is `filename`, else `img_name`; `file_id` is optional; + headers are matched case-insensitively, ignoring a BOM. Other columns + are ignored (Commons supplies the metadata) and listed in + columns['ignored']. Row numbers are spreadsheet rows (header = 1) or, + for a header-less list, line numbers; blank rows are skipped. + """ + columns = {'name': None, 'file_id': None, 'ignored': []} try: - ret, warnings = load_full_csv(BytesIO(resp.content), source=source) - except ValueError: + records = list(csv.reader(StringIO(text))) + except csv.Error as e: + raise ImportSourceInvalid('cannot read the CSV: %s' % (e,)) + if not records: + return [], columns + header = [_header_key(c) for c in records[0]] + name_col = next((c for c in NAME_COLUMNS if c in header), None) + rows = [] + if name_col is not None: + name_idx = header.index(name_col) + id_idx = header.index(FILE_ID_COLUMN) if FILE_ID_COLUMN in header else None + columns['name'] = name_col + columns['file_id'] = FILE_ID_COLUMN if id_idx is not None else None + columns['ignored'] = [c.replace(u'\ufeff', '').strip() + for i, c in enumerate(records[0]) + if i not in (name_idx, id_idx) and c.strip()] + for i, record in enumerate(records[1:]): + name = record[name_idx] if name_idx < len(record) else '' + file_id = (record[id_idx] if id_idx is not None + and id_idx < len(record) else '') + rows.append(_source_row(i + 2, name, file_id)) + else: + lines = text.splitlines() + if len(records[0]) > 1 and not _looks_like_file_name(lines[0]): + raise ImportSourceInvalid( + 'no filename or img_name column: the first row has %s columns' + ' but none is called "filename" or "img_name"' + % len(records[0])) + for i, line in enumerate(lines): + rows.append(_source_row(i + 1, _line_name(line))) + return [r for r in rows if not _is_blank(r)], columns + + +def parse_name_list(file_names): + """Rows of a file list (import method `selected`): a list of names or + one name per line; blank entries are skipped but keep their number.""" + if file_names is None: + raise ImportSourceInvalid('no file names given') + if isinstance(file_names, (str, bytes)): + file_names = to_unicode(file_names).splitlines() + rows = [_source_row(i + 1, to_unicode(name or '')) + for i, name in enumerate(file_names)] + return [r for r in rows if not _is_blank(r)] + + +def source_url(raw_url): + """(fetch url, is a Google Sheet) for a gist / CSV / Sheet link.""" + if 'google.com' in raw_url: try: - ret, warnings = load_partial_csv(resp) # TODO: load_partial_csv expects a dictreader, did this ever work? - except (ValueError, TypeError): - resp_content = resp.content.decode('utf8') - file_names = [fn.strip('\"') for fn in resp_content.split('\n')] - file_names_obj = StringIO('\n'.join(file_names)) - ret, warnings = load_name_list(file_names_obj, source=source) + doc_id = parse_doc_id(raw_url) + except ValueError as e: + raise ImportSourceInvalid(str(e)) + return GSHEET_URL % doc_id, True + if 'gist.github.com' in raw_url and 'githubusercontent' not in raw_url: + # a gist's page -> its raw text; other links are fetched as given + # (#208: '/raw' used to be appended to every link) + raw_url = raw_url.replace('gist.github.com', + 'gist.githubusercontent.com') + '/raw' + return raw_url, False - return ret, warnings +def fetch_source_text(raw_url): + """The text of a gist / CSV link or a Google Sheet's CSV export. No + Commons lookup happens here.""" + raw_url = (raw_url or '').strip() + if not raw_url: + raise ImportSourceInvalid('no link given') + url, is_sheet = source_url(raw_url) + try: + resp = requests_get(url, timeout=SOURCE_FETCH_TIMEOUT) + except requests.RequestException as e: + raise ImportSourceInvalid('cannot load "%s" (%s)' + % (raw_url, e.__class__.__name__)) + if resp.status_code != 200: + raise ImportSourceInvalid('cannot load "%s" (HTTP status %s)' + % (raw_url, resp.status_code)) + if is_sheet and 'text/csv' not in resp.headers.get('content-type', ''): + raise ImportSourceInvalid('cannot load Google Sheet "%s" (is link sharing on?)' + % raw_url) + try: + return resp.content.decode('utf-8-sig') + except UnicodeDecodeError: + raise ImportSourceInvalid('"%s" is not UTF-8 text' % raw_url) -def _lookup_file_names(filenames): - """File infos in input order, plus a warning per name that was not found. - Looks the names up in batches: one query per file made large file-list - imports outlast the request timeout. - """ - found = get_files_info_by_names(filenames) - files, warnings = [], [] - for filename in filenames: - file_info = found.get(filename.replace(' ', '_')) - if file_info is not None: - files.append(file_info) - else: - warnings.append( - 'file "%s" does not exist, please check that its name is spelled correctly, ' - 'that it has not been renamed or removed' % (filename,) - ) - return files, warnings - - -def load_by_filename(filenames, source='local'): - entries = [] - warnings = [] +def lookup_by_names(names, source='local'): + """{normalised name: Commons file info} for the names that exist.""" + names = sorted(set(n for n in names if n)) + if not names: + return {} if source == 'remote': - files, warnings = get_by_filename_remote(filenames) - else: - files, warnings = _lookup_file_names(filenames) - for edict in files: - entry = make_entry(edict) - entries.append(entry) - return entries, warnings - - -def load_category(category_name, source='local'): - ret = [] + file_infos, _ = get_by_filename_remote(names) + wanted = set(names) + found = {} + for rec in file_infos: + name = normalise_name(rec['img_name']) + if name in wanted: # infos for names not asked for are ignored + found[name] = rec + return found + return get_files_info_by_names(names) + + +def lookup_by_ids(file_ids): + """{file_id: Commons file info} for the file_ids of live files; on the + wikireplica only (the public utils endpoint has no lookup by id).""" + return get_files_info_by_ids(file_ids) + + +def category_records(category_name, source='local'): + """Commons file infos of a category's files.""" + if not (category_name or '').strip(): + raise ImportSourceInvalid('no category given') if source == 'remote': - files = get_from_category_remote(category_name) - else: - files = get_files(category_name) - for edict in files: - entry = make_entry(edict) - ret.append(entry) - - return ret + return get_from_category_remote(category_name) + return get_files(category_name) def get_from_category_remote(category_name): @@ -247,9 +300,13 @@ def get_from_category_remote(category_name): def get_from_remote(url, params): headers = {'Content-Type': 'application/json', 'Accept': 'application/json'} data = json.dumps(params) - response = requests_post(url, data=data, headers=headers) - resp_json = response.json() - file_infos = resp_json['file_infos'] + try: + response = requests_post(url, data=data, headers=headers) + resp_json = response.json() + file_infos = resp_json['file_infos'] + except (requests.RequestException, ValueError, KeyError, TypeError) as e: + raise ImportSourceInvalid('Commons lookup through %s failed (%s)' + % (url, e.__class__.__name__)) no_infos = resp_json.get('no_info') return file_infos, no_infos diff --git a/montage/tests/test_loaders.py b/montage/tests/test_loaders.py index 98e6aa96..6f97e4e8 100644 --- a/montage/tests/test_loaders.py +++ b/montage/tests/test_loaders.py @@ -9,7 +9,10 @@ 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, + parse_file_id, normalise_name, fetch_source_text, + guard_cell, unguard_cell) +from montage.utils import ImportSourceInvalid from .conftest import ( FIXTURE_FILE_INFOS, @@ -20,66 +23,24 @@ ) 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,118 @@ 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 img_name column' in exc.value.detail + + +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'] + + +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) + + +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 +334,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 == [] + assert sorted(found) == names -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)) - - 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 +391,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 +472,95 @@ 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 + + +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) diff --git a/montage/utils.py b/montage/utils.py index f6dc323b..2db9997f 100644 --- a/montage/utils.py +++ b/montage/utils.py @@ -73,6 +73,37 @@ 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". +class _ImportError(InvalidAction): + error_type = None + + def __init__(self, *args, **kwargs): + kwargs.setdefault('error_type', self.error_type) + super(_ImportError, self).__init__(*args, **kwargs) + + +class ImportSourceInvalid(_ImportError): + "The import source cannot be read (bad link, no name column, ...)" + error_type = 'import_source_invalid' + + +class ImportCheckBlocked(_ImportError): + "The checked list has rows that block the import" + error_type = 'import_check_blocked' + + +class ImportCheckExpired(_ImportError): + "The check token is unknown, expired or for another campaign/source" + error_type = 'import_check_expired' + + +class ImportCheckRequired(_ImportError): + "An import of a first-round source was sent without a check token" + error_type = 'import_check_required' + + 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 @@ -159,6 +190,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,6 +235,7 @@ 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', }) From 399c5280d19ec59a5bf177e20cb9591ab3af4939 Mon Sep 17 00:00:00 2001 From: lgelauff <32471480+lgelauff@users.noreply.github.com> Date: Mon, 5 Oct 2026 19:25:35 +0000 Subject: [PATCH 19/46] wip(#510): import_check module (classify, same-name keys, check files) Refs hatnote/montage#510, #645 Co-Authored-By: Claude Opus 5.5 --- montage/import_check.py | 524 +++++++++++++++++++++++++++++ montage/tests/test_import_check.py | 416 +++++++++++++++++++++++ 2 files changed, 940 insertions(+) create mode 100644 montage/import_check.py create mode 100644 montage/tests/test_import_check.py diff --git a/montage/import_check.py b/montage/import_check.py new file mode 100644 index 00000000..31b1f774 --- /dev/null +++ b/montage/import_check.py @@ -0,0 +1,524 @@ +# -*- coding: utf-8 -*- +"""Checked imports (hatnote/montage#510). + +Every first-round import source (CSV / gist / Google Sheet link, file +list, category) is first *checked*: fetched, looked up on Commons (by +file_id where a row has one, else by name) and classified row by row. The +result is kept as a JSON file under a random token; the import then reads +exactly that list, without fetching the source or asking Commons again. + +Row statuses: + + ok found on Commons, imported + renamed found by file_id under another name: imported under + Commons' current name (information) + duplicate the same Commons file as an earlier row: imported once + unknown_name no file_id and the name is not on Commons: left out, + warning + unknown_file_id the file_id is not on Commons: blocks + malformed_file_id the file_id is not a plain whole number: blocks + same_name a different Commons file whose name the database treats + as equal to another's (utf8mb4_unicode_ci): blocks a + list import (CSV, gist, Sheet, file list); in a category + the first file is kept and the others left out + +The check file holds the campaign id, the source, each row as written and +Commons' data per file (including Commons uploader names); no Montage user. +""" +from __future__ import absolute_import + +import os +import re +import csv +import json +import secrets +import datetime +import tempfile +import unicodedata +from io import StringIO +from collections import Counter + +from sqlalchemy import text + +from .loaders import (fetch_source_text, parse_source_rows, parse_name_list, + lookup_by_names, lookup_by_ids, category_records, + guard_cell) +from .utils import (PROJ_PATH, get_env_name, ImportSourceInvalid, + ImportCheckBlocked, ImportCheckExpired) + +FORMAT = 1 + +CHECKED_METHODS = ('csv', 'gistcsv', 'category', 'selected') +LIST_METHODS = ('csv', 'gistcsv', 'selected') # same-name pairs block these + +BLOCKING_STATUSES = ('unknown_file_id', 'malformed_file_id', 'same_name') +IMPORTED_STATUSES = ('ok', 'renamed') +ALL_STATUSES = ('ok', 'renamed', 'duplicate', 'unknown_name', + 'unknown_file_id', 'malformed_file_id', 'same_name') + +CHECK_MAX_AGE = datetime.timedelta(days=7) +ISSUES_SHOWN = 1000 # non-ok rows in the check response; the file has all +BLOCKING_ROWS_SHOWN = 10 # rows named in a "blocked" error +WARNING_ROWS_SHOWN = 50 # rows named per import warning + +# The same-name check (one switch). Two different files whose names the +# database compares as equal cannot both be stored where entries.name has a +# unique index (#645), and are easily confused by everyone else. +SAME_NAME_CHECK = True +SAME_NAME_COLLATION = 'utf8mb4_unicode_ci' # entries.name's collation +SAME_NAME_CHUNK_SIZE = 500 + +ENV_NAME = get_env_name() +DEFAULT_CHECK_DIR = os.path.join(PROJ_PATH, 'tmp', 'import_checks') +# deployed instances: the folder must be shared by all pods (and the import +# worker), so it has to be configured +CHECK_DIR_REQUIRED_ENVS = ('beta', 'prod', 'devlabs') + +_TOKEN_RE = re.compile(r'^[A-Za-z0-9_-]{32}$') + + +def lookup_source(): + """Where Commons is looked up: the wikireplica, except in local dev, + which asks production's public utils endpoint (as rdb.py does).""" + return 'remote' if ENV_NAME == 'dev' else 'local' + + +def _utcnow(): + return datetime.datetime.now(datetime.timezone.utc).replace(microsecond=0) + + +def _iso(dt): + return dt.strftime('%Y-%m-%dT%H:%M:%SZ') + + +def _parse_iso(value): + dt = datetime.datetime.strptime(value, '%Y-%m-%dT%H:%M:%SZ') + return dt.replace(tzinfo=datetime.timezone.utc) + + +# --------------------------------------------------------------------------- +# The check +# --------------------------------------------------------------------------- + +def run_check(request_dict, campaign_id, source='local', rdb_session=None, + now=None): + """Fetch, look up and classify an import source; returns the check + result (a JSON-serialisable dict). Writes nothing.""" + request_dict = request_dict or {} + import_method = request_dict.get('import_method') + if import_method not in CHECKED_METHODS: + raise ImportSourceInvalid('cannot check import method %r' % (import_method,)) + columns = {'name': None, 'file_id': None, 'ignored': []} + if import_method in ('csv', 'gistcsv'): + if import_method == 'gistcsv': + url = request_dict.get('gist_url') or request_dict.get('csv_url') + else: + url = request_dict.get('csv_url') + rows, columns = parse_source_rows(fetch_source_text(url)) + source_info = {'csv_url': url} + rows = _classify_list(rows, source) + elif import_method == 'selected': + rows = parse_name_list(request_dict.get('file_names')) + source_info = {'file_names': [r['name_as_written'] for r in rows]} + rows = _classify_list(rows, source) + else: + category = request_dict.get('category') + records = category_records(category, source) + source_info = {'category': category} + rows = [_row({'row': i + 1, 'name_as_written': rec['img_name'], + 'file_id_as_written': ''}, 'ok', rec=rec) + for i, rec in enumerate(records)] + _mark_duplicates(rows) + if SAME_NAME_CHECK: + _mark_same_names(rows, import_method, rdb_session) + now = now or _utcnow() + return {'format': FORMAT, + 'checked_at': _iso(now), + 'campaign_id': campaign_id, + 'import_method': import_method, + 'source': source_info, + 'lookup': source, + 'columns': columns, + 'rows': rows} + + +def _row(src, status, rec=None, reason='', file_id=None): + commons_name = rec['img_name'] if rec else None + if rec and rec.get('file_id') is not None: + file_id = int(rec['file_id']) + return {'row': src['row'], + 'name_as_written': src['name_as_written'], + 'file_id_as_written': src['file_id_as_written'], + 'commons_name': commons_name, + 'file_id': file_id, + 'status': status, + 'reason': reason, + 'group': None, + 'commons': rec} + + +def _classify_list(rows, source): + """Statuses of a CSV / file list's rows, looked up on Commons.""" + ids = [r['file_id'] for r in rows if r['file_id'] is not None] + by_id = lookup_by_ids(ids) if ids and source != 'remote' else {} + # dev (remote): the public utils endpoint has no lookup by file_id, so + # rows with a file_id are looked up by name and their ids compared + names = [r['name'] for r in rows + if r['name'] and not r['malformed_file_id'] + and (r['file_id'] is None or source == 'remote')] + by_name = lookup_by_names(names, source) + + ret = [] + for r in rows: + if r['malformed_file_id']: + ret.append(_row(r, 'malformed_file_id', + reason='file_id "%s" is not a plain whole number' + % r['file_id_as_written'].strip())) + elif r['file_id'] is not None and source == 'remote': + rec = by_name.get(r['name']) + if rec and rec.get('file_id') is not None and int(rec['file_id']) == r['file_id']: + ret.append(_row(r, 'ok', rec=rec)) + else: + ret.append(_row(r, 'unknown_file_id', file_id=r['file_id'], + reason='dev: compared by name only; file_id %s' + ' not confirmed' % r['file_id'])) + elif r['file_id'] is not None: + rec = by_id.get(r['file_id']) + if rec is None: + ret.append(_row(r, 'unknown_file_id', file_id=r['file_id'], + reason='file_id %s is not on Commons (deleted,' + ' or a wrong number)' % r['file_id'])) + elif not r['name']: + ret.append(_row(r, 'ok', rec=rec, + reason='no name given; Commons name used')) + elif rec['img_name'] == r['name']: + ret.append(_row(r, 'ok', rec=rec)) + else: + ret.append(_row(r, 'renamed', rec=rec, + reason=u'renamed on Commons: %s → %s' + % (r['name'], rec['img_name']))) + else: + rec = by_name.get(r['name']) + if rec is not None: + ret.append(_row(r, 'ok', rec=rec)) + else: + ret.append(_row(r, 'unknown_name', + reason='"%s" is not on Commons: check the' + ' spelling (renamed or deleted?)' % r['name'])) + return ret + + +def _file_key(row): + if row['file_id'] is not None: + return ('file_id', row['file_id']) + return ('name', row['commons_name']) + + +def _mark_duplicates(rows): + """The same Commons file listed twice (same name, or an old name and + its file_id): imported once, from its first row.""" + first_row = {} + for r in rows: + if r['status'] not in IMPORTED_STATUSES: + continue + key = _file_key(r) + if key in first_row: + r['status'] = 'duplicate' + r['reason'] = 'the same file as row %s' % first_row[key] + r['commons'] = None + else: + first_row[key] = r['row'] + + +def _mark_same_names(rows, import_method, rdb_session): + candidates = [r for r in rows if r['status'] in IMPORTED_STATUSES] + keys = same_name_keys([r['commons_name'] for r in candidates], rdb_session) + groups = {} + for r in candidates: + groups.setdefault(keys[r['commons_name']], []).append(r) + group_id = 0 + for members in groups.values(): + if len(members) < 2: + continue + group_id += 1 + if import_method in LIST_METHODS: + for r in members: + others = [m for m in members if m is not r] + r['status'] = 'same_name' + r['group'] = group_id + r['reason'] = ('the database treats this name as equal to: %s' + % ', '.join('row %s %s' % (m['row'], m['commons_name']) + for m in others)) + r['commons'] = None + else: + # category: keep the first in binary name order, as before + members.sort(key=lambda m: m['commons_name'].encode('utf8')) + kept = members[0] + kept['group'] = group_id + for r in members[1:]: + r['status'] = 'same_name' + r['group'] = group_id + r['reason'] = ('left out: the database treats this name as' + ' equal to %s (kept)' % kept['commons_name']) + r['commons'] = None + + +def same_name_keys(names, rdb_session=None): + """{name: comparison key}: two names with equal keys are the same name + for Montage's database (entries.name, utf8mb4_unicode_ci). + + On MariaDB/MySQL the database computes the keys itself with + WEIGHT_STRING, so they are exactly its comparison (no table is read). + Anywhere else (SQLite in tests and local dev) a Python approximation is + used, for tests only: it folds case and drops accents, so е/ё and й/и + compare equal as in utf8mb4_unicode_ci, but ß/ss, æ/ae and other + expansions do not.""" + names = sorted(set(names)) + if not names: + return {} + if rdb_session is not None and _dialect_name(rdb_session) == 'mysql': + return _database_keys(rdb_session, names) + return {name: _test_only_approximate_key(name) for name in names} + + +def _dialect_name(rdb_session): + try: + return rdb_session.get_bind().dialect.name + except Exception: + return None + + +def _database_keys(rdb_session, names): + ret = {} + for i in range(0, len(names), SAME_NAME_CHUNK_SIZE): + chunk = names[i:i + SAME_NAME_CHUNK_SIZE] + cols = ', '.join('WEIGHT_STRING(CONVERT(:n%d USING utf8mb4) COLLATE %s) AS w%d' + % (j, SAME_NAME_COLLATION, j) for j in range(len(chunk))) + params = {'n%d' % j: name for j, name in enumerate(chunk)} + row = rdb_session.execute(text('SELECT ' + cols), params).fetchone() + for j, name in enumerate(chunk): + ret[name] = bytes(row[j]) + return ret + + +def _test_only_approximate_key(name): + """TEST-ONLY stand-in for the database's utf8mb4_unicode_ci comparison + (SQLite has none). Never used against MariaDB.""" + decomposed = unicodedata.normalize('NFKD', name) + stripped = u''.join(c for c in decomposed if not unicodedata.combining(c)) + return stripped.lower() + + +# --------------------------------------------------------------------------- +# Reading a check result +# --------------------------------------------------------------------------- + +def counts(result): + found = Counter(r['status'] for r in result['rows']) + return {status: found.get(status, 0) for status in ALL_STATUSES} + + +def blocking_rows(result): + """Rows that block the import, recomputed from the rows.""" + if result['import_method'] not in LIST_METHODS: + return [] + return [r for r in result['rows'] if r['status'] in BLOCKING_STATUSES] + + +def importable(result): + return [r for r in result['rows'] + if r['status'] in IMPORTED_STATUSES and r['commons'] is not None] + + +def raise_if_blocked(result): + blocked = blocking_rows(result) + if not blocked: + return + per_status = Counter(r['status'] for r in blocked) + detail = '%s rows block this import (%s). Fix them in the source and check again.' % ( + len(blocked), ', '.join('%s %s' % (n, s.replace('_', ' ')) + for s, n in sorted(per_status.items()))) + lines = ['row %s: %s' % (r['row'], r['reason']) for r in blocked[:BLOCKING_ROWS_SHOWN]] + if len(blocked) > BLOCKING_ROWS_SHOWN: + lines.append('... and %s more' % (len(blocked) - BLOCKING_ROWS_SHOWN)) + raise ImportCheckBlocked(detail + '\n' + '\n'.join(lines)) + + +def _listed(lines): + shown = [u'- ' + line for line in lines[:WARNING_ROWS_SHOWN]] + if len(lines) > WARNING_ROWS_SHOWN: + shown.append(u'- ... and %s more' % (len(lines) - WARNING_ROWS_SHOWN)) + return u'\n'.join(shown) + + +def import_warnings(result): + """Warnings for the import response: a list of one-key dicts, like + the other import warnings.""" + ret = [] + rows = result['rows'] + unknown = [r['reason'] for r in rows if r['status'] == 'unknown_name'] + if unknown: + ret.append({'import issues': u'unable to load %s files:\n%s' + % (len(unknown), _listed(unknown))}) + renamed = [r['reason'] for r in rows if r['status'] == 'renamed'] + if renamed: + ret.append({'renamed': u'%s files are imported under their current' + u' name on Commons:\n%s' % (len(renamed), _listed(renamed))}) + left_out = [u'%s (%s)' % (r['commons_name'], r['reason']) for r in rows + if r['status'] == 'same_name'] + if left_out: + ret.append({'same name': u'%s files left out:\n%s' + % (len(left_out), _listed(left_out))}) + return ret + + +def source_params(result): + """round_sources.params for the import, from the check (not from the + import request).""" + return dict(result['source']) + + +def describe_source(result): + src = result['source'] + if 'category' in src: + return 'category (%s)' % src['category'] + if 'csv_url' in src: + return 'csv (%r)' % src['csv_url'] + return 'file list (%s names)' % len(src.get('file_names', [])) + + +def summarize(result, token): + """The check endpoint's response.""" + issues = [{k: v for k, v in r.items() if k != 'commons'} + for r in result['rows'] if r['status'] != 'ok'] + groups = {} + for r in result['rows']: + if r['group'] is not None: + groups.setdefault(r['group'], []).append( + {'row': r['row'], 'name_as_written': r['name_as_written'], + 'commons_name': r['commons_name'], 'file_id': r['file_id'], + 'status': r['status']}) + checked_at = _parse_iso(result['checked_at']) + return {'token': token, + 'checked_at': result['checked_at'], + 'expires_at': _iso(checked_at + CHECK_MAX_AGE), + 'import_method': result['import_method'], + 'source': result['source'], + 'columns': result['columns'], + 'counts': counts(result), + 'blocking': bool(blocking_rows(result)), + 'total_rows': len(result['rows']), + 'importable_count': len(importable(result)), + 'issues': issues[:ISSUES_SHOWN], + 'issues_total': len(issues), + 'issues_truncated': len(issues) > ISSUES_SHOWN, + 'same_name_groups': [groups[k] for k in sorted(groups)]} + + +def upload_csv(result): + """The download: a ready-to-use upload file with the columns filename + (Commons' current name) and file_id, one row per file the import would + take. Uploading it again passes the check unchanged. Cells that a + spreadsheet would run as a formula get a leading ' (removed again when + Montage reads the file).""" + out = StringIO() + writer = csv.writer(out, lineterminator='\n') + writer.writerow(['filename', 'file_id']) + for r in importable(result): + writer.writerow([guard_cell(r['commons_name']), + '' if r['file_id'] is None else r['file_id']]) + return out.getvalue().encode('utf8') + + +# --------------------------------------------------------------------------- +# Check files +# --------------------------------------------------------------------------- + +def check_dir(config): + return ((config or {}).get('import_check_path') + or os.environ.get('MONTAGE_IMPORT_CHECK_PATH') + or DEFAULT_CHECK_DIR) + + +def check_dir_problem(config, env_name): + """Why the app must not start, or None.""" + configured = ((config or {}).get('import_check_path') + or os.environ.get('MONTAGE_IMPORT_CHECK_PATH')) + if env_name in CHECK_DIR_REQUIRED_ENVS and not configured: + return ('MONTAGE_IMPORT_CHECK_PATH is not set: env %r needs a folder for' + ' import checks that all pods share, e.g.' + ' /data/project//import_checks' % env_name) + return None + + +def _cleanup(folder, now): + cutoff = (now - CHECK_MAX_AGE).timestamp() + for name in os.listdir(folder): + if not (name.endswith('.json') or name.startswith('.tmp-')): + continue + path = os.path.join(folder, name) + try: + if os.path.getmtime(path) < cutoff: + os.remove(path) + except FileNotFoundError: # another process cleaned up first + pass + + +def save(result, config, now=None): + """Write the check result to a new check file; returns its token. + Deletes check files older than CHECK_MAX_AGE first.""" + folder = check_dir(config) + os.makedirs(folder, exist_ok=True) + os.chmod(folder, 0o700) + _cleanup(folder, now or _utcnow()) + token = secrets.token_urlsafe(24) + fd, tmp_path = tempfile.mkstemp(dir=folder, prefix='.tmp-', suffix='.json') + try: + os.fchmod(fd, 0o600) + with os.fdopen(fd, 'w', encoding='utf-8') as f: + json.dump(result, f, separators=(',', ':'), ensure_ascii=False, + default=str) + f.flush() + os.fsync(f.fileno()) + os.replace(tmp_path, os.path.join(folder, token + '.json')) + except BaseException: + try: + os.remove(tmp_path) + except OSError: + pass + raise + return token + + +def _method_family(import_method): + return 'csv' if import_method in ('csv', 'gistcsv') else import_method + + +def load(token, config, campaign_id, import_method=None, now=None): + """The check result for a token, if it exists, is younger than + CHECK_MAX_AGE and was made for this campaign (and import method).""" + expired = 'check not found or expired, please check the source again' + if not isinstance(token, str) or not _TOKEN_RE.match(token): + raise ImportCheckExpired(expired) + folder = os.path.realpath(check_dir(config)) + path = os.path.realpath(os.path.join(folder, token + '.json')) + if not path.startswith(folder + os.sep): + raise ImportCheckExpired(expired) + try: + with open(path, encoding='utf-8') as f: + result = json.load(f) + checked_at = _parse_iso(result['checked_at']) + except (OSError, ValueError, KeyError, TypeError): + raise ImportCheckExpired(expired) + if result.get('format') != FORMAT: + raise ImportCheckExpired(expired) + if (now or _utcnow()) - checked_at > CHECK_MAX_AGE: + raise ImportCheckExpired('check expired (older than 7 days), please' + ' check the source again') + if result.get('campaign_id') != campaign_id: + raise ImportCheckExpired('this check was made for another campaign,' + ' please check the source again') + if import_method and _method_family(result.get('import_method')) != _method_family(import_method): + raise ImportCheckExpired('this check was made for another import' + ' source, please check the source again') + return result diff --git a/montage/tests/test_import_check.py b/montage/tests/test_import_check.py new file mode 100644 index 00000000..2b52a7d1 --- /dev/null +++ b/montage/tests/test_import_check.py @@ -0,0 +1,416 @@ +# -*- 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): + self.by_name = {f['img_name']: f for f in files} + self.by_id = {f['file_id']: f for f in files} + 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 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): + fake.__init__(files) + 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) + 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' + 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_ok(monkeypatch, commons): + commons([info('A.jpg', 1)]) + result = csv_check(monkeypatch, 'filename,file_id\n,1\n') + assert statuses(result) == [(2, 'ok')] + assert result['rows'][0]['reason'] == 'no name given; Commons name used' + + +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 rows block this import (1 unknown file id)') + assert 'row 2: file_id 77 is not on Commons' 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 this 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 [b'k' for _ in range(self.n)] + + 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)) + return Result(len(params)) + + monkeypatch.setattr(import_check, 'SAME_NAME_CHUNK_SIZE', 2) + keys = same_name_keys(['c.jpg', 'a.jpg', 'b.jpg'], Session()) + assert set(keys) == {'a.jpg', 'b.jpg', 'c.jpg'} + assert [len(p) for _, p in sent] == [2, 1] + 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 + + +# --------------------------------------------------------------------------- +# 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) + + +# --------------------------------------------------------------------------- +# 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_old_files(config, tmpdir): + keep = save(_result(), config) + stale = 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)) + other = os.path.join(folder, 'notes.txt') + open(other, 'w').close() + os.utime(other, (week_ago, week_ago)) + newest = save(_result(), config) + assert sorted(os.listdir(folder)) == sorted([keep + '.json', newest + '.json', 'notes.txt']) + + +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_on_deployed_instances(monkeypatch): + monkeypatch.delenv('MONTAGE_IMPORT_CHECK_PATH', raising=False) + for env in ('beta', 'prod', 'devlabs'): + assert 'MONTAGE_IMPORT_CHECK_PATH' in import_check.check_dir_problem({}, env) + assert import_check.check_dir_problem({'import_check_path': '/x'}, env) is None + assert import_check.check_dir_problem({}, 'dev') is None + monkeypatch.setenv('MONTAGE_IMPORT_CHECK_PATH', '/y') + assert import_check.check_dir_problem({}, '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')] From 0befcf1bbdddea549c286daace1a5bd89de1fa95 Mon Sep 17 00:00:00 2001 From: lgelauff <32471480+lgelauff@users.noreply.github.com> Date: Mon, 5 Oct 2026 19:28:46 +0000 Subject: [PATCH 20/46] wip(#510): check/download endpoints, import only by check token - POST /admin/campaign//import/check, GET .../import/check//download - import_entries requires check_token for csv/gistcsv/category/selected and imports exactly the checked list; blocked check -> 400 before writes - add_checked_entries replaces the three DAO loaders; file_id lookup in every campaign (FILE_ID_LOOKUP_CUTOFF removed) - app refuses to start on beta/prod/devlabs without MONTAGE_IMPORT_CHECK_PATH - no 'all disqualified' warning for an empty import (#208) - tests import through check + token Refs hatnote/montage#510, #208 Co-Authored-By: Claude Opus 5.5 --- montage/admin_endpoints.py | 113 ++++++++++++++++++--------- montage/app.py | 7 ++ montage/rdb.py | 77 ++++-------------- montage/tests/test_import_entries.py | 28 ++++--- montage/tests/test_web_basic.py | 67 ++++++++++------ tools/admin.py | 36 ++++++--- 6 files changed, 181 insertions(+), 147 deletions(-) diff --git a/montage/admin_endpoints.py b/montage/admin_endpoints.py index a8faabfc..2595c018 100644 --- a/montage/admin_endpoints.py +++ b/montage/admin_endpoints.py @@ -11,8 +11,11 @@ from .utils import (format_date, get_threshold_map, InvalidAction, + ImportCheckRequired, NotImplementedResponse, js_isoparse) +from . import import_check +from .loaders import make_entry from .rdb import (FINALIZED_STATUS, CoordinatorDAO, @@ -56,6 +59,9 @@ def get_admin_routes(): POST('/admin/campaign//publish', publish_report), POST('/admin/campaign//unpublish', unpublish_report), GET('/admin/campaign//audit', get_campaign_log), + POST('/admin/campaign//import/check', check_import), + GET('/admin/campaign//import/check//download', + download_import_check), POST('/admin/round//import', import_entries), POST('/admin/round//activate', activate_round), POST('/admin/round//pause', pause_round), @@ -304,21 +310,60 @@ def get_campaign_log(user_dao, campaign_id, request_dict): return {'data': ret} -def import_entries(user_dao, round_id, request_dict): +def check_import(user_dao, campaign_id, request_dict, config): """ - Summary: Load entries into a round via one of four import methods + Summary: Check a first-round import source before importing it (#510) + + Fetches the source, looks every row up on Commons (by file_id where a + row has one, else by name) and classifies the rows. Writes the result + as a check file and returns a summary with its token; the import then + takes exactly that list (import_entries with check_token). Writes + nothing to the database. + + Request model: as import_entries: import_method (csv, gistcsv, + category, selected) and csv_url / gist_url / category / file_names. + + Response model name: + - data: token, checked_at, expires_at, counts (per status), blocking, + total_rows, importable_count, columns, issues (rows that are not + ok, at most 1000), issues_total, issues_truncated, + same_name_groups + """ + coord_dao = CoordinatorDAO.from_campaign(user_dao, campaign_id) + result = import_check.run_check(request_dict, coord_dao.campaign.id, + source=import_check.lookup_source(), + rdb_session=user_dao.rdb_session) + token = import_check.save(result, config) + return {'data': import_check.summarize(result, token)} + + +def download_import_check(user_dao, campaign_id, token, config): + """The checked list as a ready-to-use upload file (filename, file_id).""" + coord_dao = CoordinatorDAO.from_campaign(user_dao, campaign_id) + result = import_check.load(token, config, coord_dao.campaign.id) + output_name = 'montage_import-%s-%s.csv' % (campaign_id, + result['checked_at'][:10]) + resp = Response(import_check.upload_csv(result), mimetype='text/csv') + resp.mimetype_params['charset'] = 'utf-8' + resp.headers['Content-Disposition'] = 'attachment; filename=%s' % (output_name,) + return resp + + +def import_entries(user_dao, round_id, request_dict, config): + """ + Summary: Load entries into a round via one of five import methods Request model: - round_id (in path) - import_method: - - gistcsv + - csv / gistcsv (a CSV, gist or Google Sheet link) - category + - selected (a list of file names) - round - - selected - - gist_url (if import_method=gistcsv) - - category (if import_method=category) - - threshold (if import_method=round) - - file_names (if import_method=selected) + - check_token: from /admin/campaign//import/check; required for + every method except round (#510). The import takes exactly the + checked list; the source is not fetched again. + - threshold, previous_round_id (if import_method=round) Response model name: - data: @@ -329,6 +374,9 @@ def import_entries(user_dao, round_id, request_dict): - status: success or failure - errors: description of the failure (if any) - warnings: possible problems to alert the user + - import issues (files not on Commons, left out) + - renamed (imported under their current Commons name) + - same name (category files left out, #645) - empty import (no entries) - duplicate import (no new entries) - all disqualified @@ -339,40 +387,28 @@ def import_entries(user_dao, round_id, request_dict): # loader warnings import_warnings = list() - if import_method == 'csv' or import_method == 'gistcsv': - if import_method == 'gistcsv': - csv_url = request_dict['gist_url'] - else: - csv_url = request_dict['csv_url'] - - entries, warnings = coord_dao.add_entries_from_csv(round_id, - csv_url) - params = {'csv_url': csv_url} - if warnings: - msg = u'unable to load {} files ({!r})'.format(len(warnings), warnings) - # a dict like the other warnings: the frontend shows one value - # per warning, and showed only the last character of a string - import_warnings.append({'import issues': msg}) - elif import_method == CATEGORY_METHOD: - cat_name = request_dict['category'] - entries = coord_dao.add_entries_from_cat(round_id, cat_name) - params = {'category': cat_name} - elif import_method == ROUND_METHOD: + if import_method == ROUND_METHOD: threshold = request_dict['threshold'] prev_round_id = request_dict['previous_round_id'] entries = coord_dao.get_rating_advancing_group(prev_round_id, threshold) params = {'threshold': threshold, 'round_id': prev_round_id} - elif import_method == SELECTED_METHOD: - file_names = request_dict['file_names'] - entries, warnings = coord_dao.add_entries_by_name(round_id, file_names) - if warnings: - formatted_warnings = u'\n'.join([ - u'- {}'.format(warning) for warning in warnings - ]) - msg = u'unable to load {} files:\n{}'.format(len(warnings), formatted_warnings) - import_warnings.append({'import issues': msg}) - params = {'file_names': file_names} + elif import_method in import_check.CHECKED_METHODS: + token = request_dict.get('check_token') + if not token: + raise ImportCheckRequired( + 'check the import source first: POST it to' + ' /admin/campaign//import/check, then import' + ' with the check_token it returns') + result = import_check.load(token, config, coord_dao.campaign.id, + import_method) + import_check.raise_if_blocked(result) # before anything is written + entries = [make_entry(row['commons']) + for row in import_check.importable(result)] + entries = coord_dao.add_checked_entries( + round_id, entries, import_check.describe_source(result)) + params = dict(import_check.source_params(result), check_token=token) + import_warnings.extend(import_check.import_warnings(result)) else: raise NotImplementedResponse() @@ -391,7 +427,8 @@ def import_entries(user_dao, round_id, request_dict): # automatically disqualify entries based on round config auto_dq = autodisqualify(user_dao, round_id, request_dict={}) new_entry_stats['disqualified'] = auto_dq['data'] - if len(new_entry_stats['disqualified']) >= len(entries): + # (an empty import is not "all disqualified", #208) + if entries and len(new_entry_stats['disqualified']) >= len(entries): new_entry_stats['warnings'].append({'all disqualified': 'all entries disqualified by round settings'}) diff --git a/montage/app.py b/montage/app.py index 3219be11..affb95e7 100644 --- a/montage/app.py +++ b/montage/app.py @@ -25,6 +25,7 @@ SQLProfilerMiddleware) from .rdb import Base, bootstrap_maintainers, ensure_series from .utils import get_env_name, load_env_config +from . import import_check from .check_rdb import get_schema_errors, ping_connection from .meta_endpoints import META_API_ROUTES, META_UI_ROUTES @@ -67,6 +68,12 @@ def create_app(env_name='prod', config=None): config = load_env_config(env_name=env_name) print('== loaded config file: %s' % (config['__file__'],)) + # import check files (#510) must be on storage all pods share + check_dir_problem = import_check.check_dir_problem(config, env_name) + if check_dir_problem: + raise ValueError(check_dir_problem) + print('== import check folder: %s' % (import_check.check_dir(config),)) + engine = create_engine(config.get('db_url', DEFAULT_DB_URL), pool_recycle=60) session_type = sessionmaker() session_type.configure(bind=engine) diff --git a/montage/rdb.py b/montage/rdb.py index f0894377..be4004d8 100644 --- a/montage/rdb.py +++ b/montage/rdb.py @@ -654,12 +654,6 @@ def _entry_insert_row(entry): return {key: getattr(entry, key) for key in _ENTRY_INSERT_COLS} -# Imports have stored Commons' file_id since 2026-05-31: on production the -# last category-imported entry without one dates from 2026-05-30 13:21, the -# first with one from 2026-05-31 20:52. A file new to Montage after this date -# therefore has its file_id stored (hatnote/montage#513). -FILE_ID_LOOKUP_CUTOFF = datetime.datetime(2026, 6, 1) - # Batch size for lookups by file_id: below 200 on purpose. MariaDB's # eq_range_index_dive_limit is 200 by default (ToolsDB too): with fewer # values in an IN (...) list the optimizer counts the matching rows in the @@ -675,21 +669,17 @@ def _entry_insert_row(entry): FILE_ID_LOOKUP_CHUNK_SIZE = 199 -def _use_file_id_lookup(rnd, entries): +def _use_file_id_lookup(entries): """Whether add_entries looks entries up by file_id instead of by name - (#513): when the campaign opened after FILE_ID_LOOKUP_CUTOFF and every - entry has a file_id (wikireplica imports; CSV imports have none). - Production has no index on entries.name, so a lookup by name reads the - whole table; by file_id it uses ix_entries_file_id, in batches of - FILE_ID_LOOKUP_CHUNK_SIZE. + (#513): when every entry has a file_id, which every checked import + gives (#510), in every campaign. Production has no index on + entries.name, so a lookup by name reads the whole table; by file_id it + uses ix_entries_file_id, in batches of FILE_ID_LOOKUP_CHUNK_SIZE. Files not found by file_id are looked up by name only where entries.name is indexed (see _add_entries_by_file_id); elsewhere a file imported - before the cutoff, without a file_id, gets a second entries row. - Campaigns opening after the cutoff rarely contain such files.""" - open_date = rnd.campaign.open_date if rnd.campaign else None - if open_date is None or open_date < FILE_ID_LOOKUP_CUTOFF: - return False + before 2026-05-31, stored without a file_id, gets a second entries row + (accepted until the backfill, #513).""" return bool(entries) and all(e.file_id is not None for e in entries) @@ -1658,58 +1648,19 @@ def activate_round(self, round_id): return - def add_entries_from_cat(self, round_id, cat_name): + def add_checked_entries(self, round_id, entries, source_description): + """Store the entries of a checked import (import_check.py, #510); + returns the stored Entry rows, in the order given.""" rnd = self.user_dao.get_round(round_id) - if ENV_NAME == 'dev': - source = 'remote' - else: - source = 'local' - entries = loaders.load_category(cat_name, source=source) entries, new_entry_count = self.add_entries(rnd, entries) - msg = ('%s loaded %s entries from category (%s), %s new entries added' - % (self.user.username, len(entries), cat_name, new_entry_count)) + msg = ('%s loaded %s entries from %s, %s new entries added' + % (self.user.username, len(entries), source_description, + new_entry_count)) self.log_action('add_entries', message=msg, round=rnd) return entries - def add_entries_by_name(self, round_id, file_names): - rnd = self.user_dao.get_round(round_id) - if ENV_NAME == 'dev': - source = 'remote' - else: - source = 'local' - entries, warnings = loaders.load_by_filename(file_names, source=source) - entries, new_entry_count = self.add_entries(rnd, entries) - - msg = ('%s loaded %s entries from filenames, %s new entries added' - % (self.user.username, len(entries), new_entry_count)) - self.log_action('add_entries', message=msg, round=rnd) - - return entries, warnings - - def add_entries_from_csv(self, round_id, csv_url): - # NOTE: this no longer creates RoundEntries, use - # add_round_entries to do this. - rnd = self.user_dao.get_round(round_id) - if ENV_NAME == 'dev': - source = 'remote' - else: - source = 'local' - try: - entries, warnings = loaders.get_entries_from_csv(csv_url, - source=source) - except ValueError: - raise InvalidAction('unable to load csv "%s"' % csv_url) - - entries, new_entry_count = self.add_entries(rnd, entries) - - msg = ('%s loaded %s entries from csv (%r), %s new entries added' - % (self.user.username, len(entries), csv_url, new_entry_count)) - self.log_action('add_entries', message=msg, round=rnd) - - return entries, warnings - def get_round_sources(self, round_id, import_method): round_sources = (self.query(RoundSource) .filter_by(round_id=round_id, @@ -1750,7 +1701,7 @@ def add_entries(self, rnd, entries): deduped.append(e) entries = deduped - if _use_file_id_lookup(rnd, entries): + if _use_file_id_lookup(entries): return self._add_entries_by_file_id(entries) entry_chunks = chunked(entries, IMPORT_CHUNK_SIZE) diff --git a/montage/tests/test_import_entries.py b/montage/tests/test_import_entries.py index d9a4f22f..12a173bc 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); stored, see test_check_token_is_stored... + row['source_params'].pop('check_token', 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) +AFTER_CUTOFF = datetime.datetime(2026, 6, 1) + datetime.timedelta(days=30) def with_file_ids(infos): @@ -538,12 +541,15 @@ def test_file_id_lookup_after_cutoff(montage_app, coord_client, 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 diff --git a/montage/tests/test_web_basic.py b/montage/tests/test_web_basic.py index eb7bd913..1a8516bb 100644 --- a/montage/tests/test_web_basic.py +++ b/montage/tests/test_web_basic.py @@ -118,6 +118,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 +147,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 +333,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 +348,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') @@ -921,9 +936,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 +1028,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'] @@ -1175,8 +1190,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 +1288,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') @@ -1357,8 +1372,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 +1564,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 +1578,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 +1600,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/tools/admin.py b/tools/admin.py index 42dfb8a0..9a9824a6 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 @@ -218,14 +222,12 @@ 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) - 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 + request = {'import_method': 'category', 'category': category_name} + entries, params = _checked_entries(user_dao, coord_dao, rnd.id, request) print(('++ prepared %s entries from %r' % - (len(entries), source))) - coord_dao.add_round_entries(rnd.id, entries) + (len(entries), category_name))) + 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] @@ -466,13 +468,29 @@ 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)) From fbc1255646a976190a8183ef5fc0bf11c6799281 Mon Sep 17 00:00:00 2001 From: lgelauff <32471480+lgelauff@users.noreply.github.com> Date: Mon, 5 Oct 2026 19:31:19 +0000 Subject: [PATCH 21/46] wip(#510): endpoint tests (check, download, token import, retry, expiry) Co-Authored-By: Claude Opus 5.5 --- montage/tests/test_import_check.py | 245 +++++++++++++++++++++++++++++ 1 file changed, 245 insertions(+) diff --git a/montage/tests/test_import_check.py b/montage/tests/test_import_check.py index 2b52a7d1..3a38693a 100644 --- a/montage/tests/test_import_check.py +++ b/montage/tests/test_import_check.py @@ -414,3 +414,248 @@ def test_header_less_list_with_comma_names(monkeypatch, commons): 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_check_token_is_stored_in_the_round_source(montage_app, coord_client, + local_commons, monkeypatch): + local_commons([info('A.jpg', 1)]) + _source(monkeypatch, 'A.jpg\n') + round_id = new_round(coord_client, 'param') + token = _check(coord_client, _campaign_of(coord_client, round_id))['data']['token'] + _import(coord_client, round_id, token) + params = db_query(montage_app, 'SELECT params FROM round_sources WHERE round_id = :r', + r=round_id) + assert json.loads(params[0]['params']) == { + 'csv_url': 'https://example.org/list.csv', 'check_token': token} + + +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: file_id 9999 is not on Commons' 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 test_import_after_seven_days_is_refused(montage_app, coord_client, + local_commons, monkeypatch, tmpdir): + local_commons([info('A.jpg', 1)]) + _source(monkeypatch, 'A.jpg\n') + round_id = new_round(coord_client, 'old check') + token = _check(coord_client, _campaign_of(coord_client, round_id))['data']['token'] + later = datetime.datetime.now(datetime.timezone.utc) + datetime.timedelta(days=8) + monkeypatch.setattr(import_check, '_utcnow', lambda: later) + resp = _import(coord_client, round_id, token, error_code=400) + assert 'import_check_expired' in _body(resp) and '7 days' in _body(resp) + + +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_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) From 8046d37a05555de6483170fefc1f35d9ddde6e21 Mon Sep 17 00:00:00 2001 From: lgelauff <32471480+lgelauff@users.noreply.github.com> Date: Mon, 5 Oct 2026 19:34:09 +0000 Subject: [PATCH 22/46] wip(#510): round form checks the source; round created once - Check button + ImportCheckResult panel (text only), download link - Save only after a non-blocking check; source change clears the check; stale check responses ignored; source locked while checking - the round is created once; a failed import is retried into it, with the round settings locked - i18n (en + qqq) for the new texts and the import warning dialog - Cypress: check before Add Round (not run) Refs hatnote/montage#510 Co-Authored-By: Claude Opus 5.5 --- frontend/cypress/e2e/campaign.cy.js | 31 ++- .../components/Round/ImportCheckResult.vue | 157 ++++++++++++ frontend/src/components/Round/RoundNew.vue | 224 ++++++++++++++---- frontend/src/i18n/en.json | 33 ++- frontend/src/i18n/qqq.json | 33 ++- frontend/src/services/adminService.js | 7 +- 6 files changed, 438 insertions(+), 47 deletions(-) create mode 100644 frontend/src/components/Round/ImportCheckResult.vue diff --git a/frontend/cypress/e2e/campaign.cy.js b/frontend/cypress/e2e/campaign.cy.js index bbfcf20e..69c346bb 100644 --- a/frontend/cypress/e2e/campaign.cy.js +++ b/frontend/cypress/e2e/campaign.cy.js @@ -84,7 +84,36 @@ describe('Campaign Details Page', () => { .find('li') .first() .click(); - cy.get('.button-group button').contains('Add Round').click().click(); + // #510: the source is checked before the round can be saved + cy.intercept('POST', '/v1/admin/campaign/*/import/check', { + body: { + status: 'success', + data: { + token: 'cypress-check-token-000000000000', + checked_at: '2025-08-01T00:00:00Z', + expires_at: '2025-08-08T00:00:00Z', + 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') + cy.get('.form-container').contains('label', 'File List').click() + cy.get('.form-container textarea').first().type('Example.jpg') + cy.get('.button-group button').contains('Add Round').should('be.disabled') + cy.get('[data-testid="check-source-button"]').click() + cy.wait('@checkImport') + cy.get('[data-testid="import-check-result"]').should('be.visible') + cy.get('.button-group button').contains('Add Round').click(); cy.log(' Round created successfully'); }) diff --git a/frontend/src/components/Round/ImportCheckResult.vue b/frontend/src/components/Round/ImportCheckResult.vue new file mode 100644 index 00000000..ff2f646e --- /dev/null +++ b/frontend/src/components/Round/ImportCheckResult.vue @@ -0,0 +1,157 @@ + + + + + diff --git a/frontend/src/components/Round/RoundNew.vue b/frontend/src/components/Round/RoundNew.vue index b3e4d129..a5fcd581 100644 --- a/frontend/src/components/Round/RoundNew.vue +++ b/frontend/src/components/Round/RoundNew.vue @@ -33,14 +33,18 @@ - + {{ $t('montage-round-no-category') }} - + - + - +
+ + {{ + isChecking + ? $t('montage-round-check-running') + : $t('montage-round-check-source') + }} + +

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

+ +
+ - + {{ $t('montage-description-round-stats') }}

 - + - +

{{ $t('montage-round-file-setting') }}

- + - +
diff --git a/frontend/src/i18n/en.json b/frontend/src/i18n/en.json index 0da79d79..a8ae6a1e 100644 --- a/frontend/src/i18n/en.json +++ b/frontend/src/i18n/en.json @@ -214,6 +214,9 @@ "montage-round-check-status-unknown-name": "Not on Commons", "montage-round-check-status-unknown-file-id": "File ID not on Commons", "montage-round-check-status-malformed-file-id": "Not a valid file ID", + "montage-round-check-status-page-id": "Page ID, not a file ID", + "montage-round-check-status-revision-id": "Revision ID, not a file ID", + "montage-round-check-status-ambiguous-id": "File ID that is also a page or revision ID", "montage-round-check-status-same-name": "Same name as another file inside this import", "montage-round-check-same-name-title": "Different files with names the database treats as the same", "montage-round-check-same-name-list": "Rename one of them on Commons or remove one row from the source, then check again.", diff --git a/frontend/src/i18n/qqq.json b/frontend/src/i18n/qqq.json index e12086c1..0d010009 100644 --- a/frontend/src/i18n/qqq.json +++ b/frontend/src/i18n/qqq.json @@ -214,6 +214,9 @@ "montage-round-check-status-unknown-name": "Import check status: no file with this name exists on Commons; the row is left out.", "montage-round-check-status-unknown-file-id": "Import check status: no file with this file ID exists on Commons; this blocks the import.", "montage-round-check-status-malformed-file-id": "Import check status: the file ID is not a plain whole number (for example 1.49673E+08); this blocks the import.", + "montage-round-check-status-page-id": "Import check status: a number in the list is the page ID of a file page on Commons (shown under Page information), not the file ID Montage needs; this blocks the import.", + "montage-round-check-status-revision-id": "Import check status: a number in the list is a revision ID (one edit of a file page on Commons), not the file ID Montage needs; this blocks the import.", + "montage-round-check-status-ambiguous-id": "Import check status: a number in the list is the file ID of one file but also the page ID or a revision ID of another file page, so it is unclear which file is meant; this blocks the import.", "montage-round-check-status-same-name": "Import check status: a different file in the same import (not a file already in Montage) has a name that the database treats as the same (for example differing only in case, accents or е/ё).", "montage-round-check-same-name-title": "Heading above pairs of file names that the database cannot tell apart.", "montage-round-check-same-name-list": "Advice for same-name pairs in a CSV or file list (they block the import).", diff --git a/montage/docs/api.md b/montage/docs/api.md index bf0b1d38..7988fd28 100644 --- a/montage/docs/api.md +++ b/montage/docs/api.md @@ -263,7 +263,7 @@ its downloads stay at most 7 days. Writes nothing to the database. ### Response - `data`: `token`, `checked_at`, `expires_at`, `import_method`, `source`, `columns`, `counts` (per status: `ok`, `renamed`, `duplicate`, `unknown_name`, `unknown_file_id`, - `malformed_file_id`, `same_name`), `blocking`, `total_rows`, `importable_count`, `issues` (rows + `malformed_file_id`, `page_id`, `revision_id`, `ambiguous_id`, `same_name`), `blocking`, `total_rows`, `importable_count`, `issues` (rows that are not ok, at most 1000), `issues_total`, `issues_truncated`, `same_name_groups` ### Errors diff --git a/montage/import_check.py b/montage/import_check.py index 37fa7593..ad333bfb 100644 --- a/montage/import_check.py +++ b/montage/import_check.py @@ -17,6 +17,11 @@ warning unknown_file_id the file_id is not on Commons: blocks malformed_file_id the file_id is not a plain whole number: blocks + page_id a bare number (no name) that is a File: page's page ID, + not a file_id: blocks + revision_id a bare number that is a revision ID of a File: page: blocks + ambiguous_id a bare number that is one file's file_id and another + File: page's page or revision ID: blocks same_name a different Commons file whose name the database treats as equal to another's (utf8mb4_unicode_ci): blocks a list import (CSV, gist, Sheet, file list); in a category @@ -43,7 +48,8 @@ from .loaders import (fetch_source_text, parse_source_rows, parse_name_list, pasted_csv_text, - lookup_by_names, lookup_by_ids, category_records, + lookup_by_names, lookup_by_ids, lookup_other_ids, + category_records, guard_cell) from .utils import (PROJ_PATH, get_env_name, ImportSourceInvalid, ImportCheckBlocked, ImportCheckExpired) @@ -53,10 +59,12 @@ CHECKED_METHODS = ('csv', 'gistcsv', 'category', 'selected') LIST_METHODS = ('csv', 'gistcsv', 'selected') # same-name pairs block these -BLOCKING_STATUSES = ('unknown_file_id', 'malformed_file_id', 'same_name') +BLOCKING_STATUSES = ('unknown_file_id', 'malformed_file_id', 'page_id', + 'revision_id', 'ambiguous_id', 'same_name') IMPORTED_STATUSES = ('ok', 'renamed') ALL_STATUSES = ('ok', 'renamed', 'duplicate', 'unknown_name', - 'unknown_file_id', 'malformed_file_id', 'same_name') + 'unknown_file_id', 'malformed_file_id', 'page_id', 'revision_id', + 'ambiguous_id', 'same_name') # A check can be imported for an hour: files are renamed and deleted on # Commons all the time, and the check and the save happen in one sitting. @@ -185,10 +193,18 @@ def _classify_list(rows, source): if r['name'] and not r['malformed_file_id'] and (r['file_id'] is None or source == 'remote')] by_name = lookup_by_names(names, source) + # a bare number (no name) may be a page ID or revision ID someone found + # on Commons; nothing confirms it is a file_id + bare = [r['file_id'] for r in rows if r['file_id'] is not None and not r['name']] + others = lookup_other_ids(bare) if bare and source != 'remote' else {} ret = [] for r in rows: - if r['malformed_file_id']: + other = others.get(r['file_id']) if not r['name'] else None + rec = by_id.get(r['file_id']) if other else None + if other and (rec is None or rec['img_name'] != other['name']): + ret.append(_other_id_row(r, other, rec)) + elif r['malformed_file_id']: ret.append(_row(r, 'malformed_file_id', reason='file_id "%s" is not a plain whole number' % r['file_id_as_written'].strip())) @@ -226,6 +242,27 @@ def _classify_list(rows, source): return ret +_ID_KINDS = {'page_id': 'the page ID', 'revision_id': 'a revision ID'} + + +def _other_id_row(r, other, rec): + """A bare number that is a File: page's page ID or revision ID.""" + num, page = r['file_id'], 'File:' + other['name'] + if rec is not None: + return _row(r, 'ambiguous_id', file_id=num, + reason='%s is the file_id of %s but also %s of %s; write' + ' the file name instead, so it is clear which file is meant' + % (num, rec['img_name'], _ID_KINDS[other['kind']], page)) + if other['kind'] == 'revision_id': + reason = ('%s is a revision ID (one edit of the page %s), not a file_id;' + ' Montage imports files, so write the file name instead: %s' + % (num, page, other['name'])) + else: + reason = ('%s is the page ID of %s, not a file_id; write the file name' + ' instead: %s' % (num, page, other['name'])) + return _row(r, other['kind'], file_id=num, reason=reason) + + def _file_key(row): if row['file_id'] is not None: return ('file_id', row['file_id']) diff --git a/montage/labs.py b/montage/labs.py index c7223b91..2a51304e 100644 --- a/montage/labs.py +++ b/montage/labs.py @@ -215,6 +215,32 @@ def get_files_info_by_ids(file_ids): return ret +def get_file_pages_by_other_ids(numbers): + """{number: {'kind': 'page_id' | 'revision_id', 'name': file name}} for + the numbers that are the page ID, or a revision ID, of a File: page. + Those are the IDs people find on Commons and may take for a file_id + (hatnote/montage#510). A page ID wins over a revision ID.""" + nums = sorted(set(int(n) for n in numbers)) + ret = {} + if not nums: + return ret + with commonswiki_connection(COMMONS_DB_HOST) as fetchall: + for i in range(0, len(nums), FILE_LOOKUP_CHUNK_SIZE): + chunk = tuple(nums[i:i + FILE_LOOKUP_CHUNK_SIZE]) + marks = ', '.join(['%s'] * len(chunk)) + revs = fetchall( + 'SELECT rev_id AS num, page_title AS name FROM revision' + ' JOIN page ON rev_page = page_id' + ' WHERE page_namespace = 6 AND rev_id IN (%s)' % marks, chunk) + pages = fetchall( + 'SELECT page_id AS num, page_title AS name FROM page' + ' WHERE page_namespace = 6 AND page_id IN (%s)' % marks, chunk) + for kind, rows in (('revision_id', revs), ('page_id', pages)): + for rec in rows: + ret[int(rec['num'])] = {'kind': kind, 'name': rec['name']} + return ret + + def get_files(category_name): # Two steps because category membership and file data now live on # different database clusters, which cannot be joined in SQL. diff --git a/montage/loaders.py b/montage/loaders.py index afc65189..bc6438f8 100644 --- a/montage/loaders.py +++ b/montage/loaders.py @@ -14,7 +14,8 @@ from boltons.iterutils import chunked_iter import montage.rdb # TODO: circular import -from .labs import get_files, get_files_info_by_names, get_files_info_by_ids +from .labs import (get_files, get_files_info_by_names, get_files_info_by_ids, + get_file_pages_by_other_ids) from .utils import (ImportSourceInvalid, requests_get, requests_post) @@ -412,6 +413,12 @@ def lookup_by_ids(file_ids): return get_files_info_by_ids(file_ids) +def lookup_other_ids(numbers): + """{number: {kind, name}} for numbers that are a File: page's page ID or + revision ID rather than a file_id; on the wikireplica only.""" + return get_file_pages_by_other_ids(numbers) + + def category_records(category_name, source='local'): """Commons file infos of a category's files.""" if category_name is not None and not isinstance(category_name, str): diff --git a/montage/tests/test_import_check.py b/montage/tests/test_import_check.py index 7dc23a4f..93f2d7ce 100644 --- a/montage/tests/test_import_check.py +++ b/montage/tests/test_import_check.py @@ -39,9 +39,10 @@ def info(name, file_id, **kw): class FakeCommons(object): """Stands in for the wikireplica lookups (local mode).""" - def __init__(self, files): + 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'): @@ -52,6 +53,10 @@ 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')) @@ -61,12 +66,13 @@ def category(self, name, source='local'): def commons(monkeypatch): fake = FakeCommons([]) - def install(files): - fake.__init__(files) + 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 @@ -400,6 +406,38 @@ def test_file_list_of_bare_file_ids(commons): (4, 'unknown_file_id')] assert result['rows'][0]['commons_name'] == VOLOCHEK + +def test_bare_page_and_revision_ids_are_refused_with_a_reason(commons): + """People find page IDs and revision IDs on Commons, not file_ids. A + bare number that is one of those blocks, saying what it is and which + file it points to, instead of 'not on Commons' or, worse, importing + another file whose file_id happens to be that number.""" + commons([info('A.jpg', 1), info('B.jpg', 2)], + other_ids={1: {'kind': 'page_id', 'name': 'A.jpg'}, # same file: fine + 2: {'kind': 'page_id', 'name': 'Other.jpg'}, # ambiguous + 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, 'ok'), (2, 'ambiguous_id'), (3, 'revision_id'), + (4, 'page_id'), (5, 'unknown_file_id'), (6, 'ok')] + 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 'D.jpg' in reasons[4] + assert 'B.jpg' in reasons[2] and 'Other.jpg' in reasons[2] + assert [r['row'] for r in blocking_rows(result)] == [2, 3, 4, 5] + + +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) + # --------------------------------------------------------------------------- # Check files # --------------------------------------------------------------------------- diff --git a/montage/tests/test_loaders.py b/montage/tests/test_loaders.py index 29cadbae..0c5acd51 100644 --- a/montage/tests/test_loaders.py +++ b/montage/tests/test_loaders.py @@ -582,6 +582,20 @@ def test_get_files_info_by_ids_live(): 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 From aaf5d3bb8a4902f365ca6e1f79b611fea940461b Mon Sep 17 00:00:00 2001 From: lgelauff <32471480+lgelauff@users.noreply.github.com> Date: Thu, 8 Oct 2026 09:33:47 +0000 Subject: [PATCH 34/46] fix(#510): import check UI review: translated reasons, clearer texts, focus, phones MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit From a three-viewpoint UI review (coordinator, consistency, screen reader + phone), 2026-10-08. Translated reasons: every checked row and every source error now carries reason_code + reason_params; the form shows montage-round-check-reason-* / montage-round-check-error-* and falls back to the English server text (marked lang="en") only without a translation. The dialog after saving is built from the check's rows, translated, instead of server English. Texts (one glossary: problems, file ID, "check the source again", "can be imported", Commons, group): - a check with only warnings now says the round can be saved and that rows are left out or changed; - the hints next to Save no longer point to rows "below" (they are above); Save after a blocking check no longer says "check first"; - same-name: "names Montage cannot tell apart", explains why (case, accents, е/ё), "groups" instead of "pairs", no advice to rename; - labels match the help text (file names or file IDs; link to the sheet or file list); expiry says nothing was saved and why; - the partial-download link is short, the steps follow as text; - blocked-save detail: no raw status codes, singular "1 row". Screen reader and phone: - focus moves to the check result or the save error after a check, an expired check or a refused save (Save loses focus while disabled); the result panel is a named region instead of being announced twice; the live region is outside the aria-busy container; - Save is disabled while a check runs; - buttons and the button row wrap instead of cutting labels off; the problems table scrolls sideways on phones (no inner vertical scroll); row numbers are row headers; file names in for right-to-left names; dialog text wraps long names. Tests: reason codes per row and on source errors; wording updates. hatnote/montage#510 Co-Authored-By: Claude Opus 5.5 --- .../components/Round/ImportCheckResult.vue | 95 ++++++++-- frontend/src/components/Round/RoundNew.vue | 175 ++++++++++++------ .../src/components/Round/importCheckText.js | 35 ++++ frontend/src/i18n/en.json | 73 ++++++-- frontend/src/i18n/qqq.json | 53 +++++- frontend/src/services/dialogService.js | 6 +- montage/docs/api.md | 6 +- montage/import_check.py | 90 +++++---- montage/loaders.py | 62 ++++--- montage/tests/test_import_check.py | 41 +++- montage/tests/test_loaders.py | 3 +- montage/utils.py | 10 + 12 files changed, 476 insertions(+), 173 deletions(-) create mode 100644 frontend/src/components/Round/importCheckText.js diff --git a/frontend/src/components/Round/ImportCheckResult.vue b/frontend/src/components/Round/ImportCheckResult.vue index f7f42434..607b46dd 100644 --- a/frontend/src/components/Round/ImportCheckResult.vue +++ b/frontend/src/components/Round/ImportCheckResult.vue @@ -1,11 +1,29 @@