Generated with the help of an LLM.
Problem/Motivation
Access checks on Mapillary content in organizations (OrganizationAccess) work one entity at a time:
organizations()loads the relationships of the entity. Group caches them per entity for the request, so a page of 50 rows still loads them 50 times.governingOrganizations()runs a group query on every call. For a sequence, a map feature or a detection,OrganizationGovernance::mapillaryOrganizationIds()also runs one query per path through its images.- The update and delete checks of
AccessHooks::entityAccess()call the view check first, so a row with edit and delete operations repeats the governing lookup. - On a cache miss, the notices on the organization page and the "Mismatched content" tab access-check every candidate image.
The listing queries are filtered in the database; the cost is in the per-row checks and grows with the rows of a page.
Left over from #3633409: Clean up the deferred findings of Mapillary organizations on Group and #3634056: Improve screen reader, keyboard focus, reduced motion and narrow screen support in the map picker and the split viewer:
CHANGELOG.mdhas no entries for #3633409: Clean up the deferred findings of Mapillary organizations on Group.AGENTS.mdstill describes the organization filter as a numeric group ID text field and says that hooks checkmoduleExists('group'); both changed there.README.mddoes not say that the page of a public organization is open to non-members.- #3634056: Improve screen reader, keyboard focus, reduced motion and narrow screen support in the map picker and the split viewer was merged without the cluster item: with reduced motion, the picker's
clusterGroup()still uses the animations of markercluster.Drupal.mapillary.prefersReducedMotion()is injs/mapillary-viewer.js(mapillary/viewer), and the picker librarymapillary_leaflet/pickerdoes not depend on it. - The focus goes back to a marker after a redraw, but not to a cluster: a zoom that splits or merges a cluster replaces its element.
- For the
.mapillary-split-marker__conerule inmapillary-split-viewer.css, stylelint with core's configuration wants theradial-gradient()value wrapped and the Prettier CLI wants it on one line.
Steps to reproduce
- Enable
mapillary_organization, create a members-only organization with a Mapillary organization ID and import 50 images of that Mapillary organization. - Log the database queries and open the image listing as a member with the edit permission: the number of queries grows with the number of rows.
- Turn on "Reduce motion" in the operating system, open the map picker and zoom out until markers cluster: the clusters still animate.
- Give a cluster the keyboard focus and zoom the map until it splits: the focus leaves the map.
Proposed resolution
Access (mapillary_organization):
organizations()andgoverningOrganizations()keep their answer per entity for the request until a tag it depends on is invalidated, asorganizationIdsByViewAccess()does.- A
hook_entity_load()implementation preloads the organizations of the Mapillary images, sequences, map features and detections being loaded: one relationship query per entity type, and for sequences, map features and detections one query per governance path. - The members-only organizations by Mapillary organization ID are read once per request.
- Who may see and edit what does not change; the existing access tests stay as they are. A kernel test counts the queries of view, update and delete checks on 1 and on 20 entities of each type and asserts that the count does not grow with the entities.
Map picker and split viewer:
Drupal.mapillary.prefersReducedMotion()moves to a small library of the base module,mapillary/motion, whichmapillary/viewerandmapillary_leaflet/pickerdepend on. With reduced motion, the picker's cluster groups are created withanimate: false.- When a redraw replaces the cluster that has the focus, the focus goes to the marker or cluster nearest to its position, or to the map when there is none.
- The cone rule is written the way stylelint wants it, since Drupal CI runs stylelint.
- Tests:
MapillaryPickerTestchecks the cluster focus after a zoom and, withmatchMediastubbed, that the cluster groups are created without animation.
Documentation:
CHANGELOG.md: the changes of #3633409: Clean up the deferred findings of Mapillary organizations on Group that site builders and users see, and the entries of this issue, under[Unreleased].AGENTS.md: the two outdated lines, the preloading and themapillary/motionlibrary.README.md: the page of a public organization is open to non-members; the map picker follows reduced motion.
Upgrade path: no schema or configuration changes. An empty post update function rebuilds the container for the new hook implementation.
Remaining tasks
- Implementation and tests.
- Merge request.
- Review.
User interface changes
- With reduced motion, map picker clusters do not animate.
- The keyboard focus stays on the map when a focused cluster splits or merges.
API changes
- New library
mapillary/motion.Drupal.mapillary.prefersReducedMotion()keeps its name;mapillary/viewerdepends on the new library, so code that uses the helper throughmapillary/viewerkeeps working.
Data model changes
None.
Issue fork mapillary-3634264
Show commands
Start within a Git clone of the project using the version control instructions.
Or, if you do not have SSH keys set up on git.drupalcode.org:
Comments
Comment #11
orkutmuratyilmaz