Merge pull request #4 from seantis/fix-agenda-item-not-being-monton-a…

…scending

Ensure `AgendaItem.position` values are strictly increasing.

setup.cfg

@@ -87,6 +87,7 @@ fanstatic.libraries =
     privatim:css = privatim.static:css_library
 
 console_scripts =
+    print_ag = privatim.cli.print_agenda_items:main
     print_trees = privatim.cli.print_trees:main
     data_retention = privatim.cli.apply_data_retention_policy:hard_delete
     add_user = privatim.cli.user:add_user

src/privatim/__init__.py

@@ -29,6 +29,8 @@
 
 
 from typing import Any, TYPE_CHECKING, Iterable
+
+from privatim.utils import fix_agenda_item_positions
 from subscribers import register_subscribers
 
 if TYPE_CHECKING:
@@ -502,4 +504,6 @@ def upgrade(context: 'UpgradeContext'):  # type: ignore[no-untyped-def]
 
     context.drop_column('consultations', 'updated')
 
+    fix_agenda_item_positions(context)
+
     context.commit()

src/privatim/cli/print_agenda_items.py

@@ -0,0 +1,53 @@
+import click
+from pyramid.paster import bootstrap
+from pyramid.paster import get_appsettings
+from sqlalchemy import select
+from privatim.models import Meeting
+from privatim.orm import get_engine, Base
+
+
+from typing import TYPE_CHECKING
+if TYPE_CHECKING:
+    from privatim.models import AgendaItem
+
+
+DEFAULT_MEETING_ID = '359d71d2-2162-4110-b021-a0239f7a3236'
+
+
+@click.command()
+@click.argument('config_uri')
+@click.option(
+    '--meeting-id', default=DEFAULT_MEETING_ID, help='Meeting ID to analyze'
+)
+def main(config_uri: str, meeting_id: str) -> None:
+    """Analyze agenda items positions for a specific meeting."""
+    env = bootstrap(config_uri)
+    settings = get_appsettings(config_uri)
+    engine = get_engine(settings)
+    Base.metadata.create_all(engine)
+
+    with env['request'].tm:
+        session = env['request'].dbsession
+
+        meeting = session.execute(
+            select(Meeting).where(Meeting.id == meeting_id)
+        ).scalar_one_or_none()
+
+        if not meeting:
+            print(f'Meeting with ID {meeting_id} not found')
+            return
+        if not meeting.agenda_items:
+            print('\nNo agenda items found for this meeting')
+            return
+
+        print_agenda_items_positions(meeting.agenda_items)
+
+
+def print_agenda_items_positions(items: list['AgendaItem']) -> None:
+    """Print position and title of each agenda item."""
+    for item in sorted(items, key=lambda x: x.position):
+        print(f'{item.position}. {item.title}')
+
+
+if __name__ == '__main__':
+    main()

src/privatim/models/meeting.py

@@ -78,6 +78,7 @@ def create(
             meeting=meeting,
             position=new_position,
         )
+
         return new_agenda_item
 
     id: Mapped[UUIDStrPK]
@@ -97,7 +98,7 @@ def create(
     meeting: Mapped['Meeting'] = relationship(
         'Meeting',
         back_populates='agenda_items',
-        order_by='AgendaItem.position'
+
     )
 
     @classmethod
@@ -171,9 +172,9 @@ def __init__(
     time: Mapped[DateTimeWithTz] = mapped_column(nullable=False)
 
     attendance_records: Mapped[list[MeetingUserAttendance]] = relationship(
-        "MeetingUserAttendance",
-        back_populates="meeting",
-        cascade="all, delete-orphan",
+        'MeetingUserAttendance',
+        back_populates='meeting',
+        cascade='all, delete-orphan',
     )
 
     @property
@@ -199,10 +200,14 @@ def attendees(self) -> list[User]:
     agenda_items: Mapped[list[AgendaItem]] = relationship(
         AgendaItem,
         back_populates='meeting',
-        order_by="AgendaItem.position",
-        cascade="all, delete-orphan"
+        cascade='all, delete-orphan',
+        order_by='AgendaItem.position'
     )
 
+    @property
+    def sorted_agenda_items(self) -> list[AgendaItem]:
+        return sorted(self.agenda_items, key=lambda x: x.position)
+
     created: Mapped[datetime] = mapped_column(default=utcnow)
     updated: Mapped[datetime] = mapped_column(default=utcnow, onupdate=utcnow)
 

src/privatim/models/utils.py

@@ -5,8 +5,11 @@
 
 
 from typing import IO, TYPE_CHECKING, Iterator, Any
+
+
 if TYPE_CHECKING:
     from _typeshed import SupportsRead
+    from privatim.models import AgendaItem
     from docx.blkcntnr import BlockItemContainer
 
 
@@ -79,3 +82,14 @@ def recursively_iter_block_items(
             for row in item.rows:
                 for cell in row.cells:
                     yield from recursively_iter_block_items(cell)
+
+
+def normalize_agenda_item_positions(items: list['AgendaItem']) -> None:
+    """
+    Normalize positions to ensure they are sequential without duplicates.
+    Items are sorted by their current position first, and title as secondary
+    sort to ensure consistent ordering when positions are duplicated.
+    """
+    sorted_items = sorted(items, key=lambda x: (x.position, x.title))
+    for i, item in enumerate(sorted_items):
+        item.position = i

src/privatim/utils.py

@@ -17,6 +17,7 @@
 
 from typing import Any, TYPE_CHECKING, overload, TypeVar, Callable, Sequence
 if TYPE_CHECKING:
+    from privatim import UpgradeContext
     from collections.abc import Mapping
     from privatim.types import FileDict, LaxFileDict
     from typing import Iterable
@@ -198,9 +199,9 @@ def maybe_escape(value: str | None) -> str:
 def strip_p_tags(text: str) -> str:
     """Remove <p> tags and strip whitespace from the given text.
 
-    Typically used to display HTML within <ul> and <li> tags,
-    ensuring elements remain on the same horizontal line. It's not the most
-    elegant solution, but less likely to break in the future.
+    Typically used to display HTML within <ul> and <li> tags, where <p> tags
+    would break the layout. It's not the most elegant solution, but less likely
+    to break in the future than other approaches.
     """
     _text = text.replace('<p>', '').replace('</p>', '')
     return _text.strip()
@@ -375,3 +376,22 @@ def get_guest_users(
     return [
         user for user in all_users if str(user.id) not in excluded_ids
     ]
+
+
+def fix_agenda_item_positions(context: 'UpgradeContext') -> None:
+    """Fix agenda item positions to be strictly increasing within each meeting.
+    """
+
+    session = context.session
+
+    # Get all meetings that have agenda items
+    stmt = select(Meeting).join(Meeting.agenda_items)
+    meetings = session.execute(stmt).scalars().unique().all()
+
+    for meeting in meetings:
+        # Get agenda items ordered by current position
+        items = meeting.agenda_items
+
+        # Reassign positions sequentially
+        for new_position, item in enumerate(items):
+            item.position = new_position

src/privatim/views/__init__.py

@@ -37,7 +37,7 @@
 from privatim.views.mtan import mtan_view, mtan_setup_view
 from privatim.views.meetings import (add_meeting_view,
                                      export_meeting_as_pdf_view,
-                                     sortable_agenda_items_view)
+                                     move_agenda_item)
 from privatim.views.meetings import delete_meeting_view
 from privatim.views.meetings import edit_meeting_view
 from privatim.views.meetings import meeting_view
@@ -512,13 +512,13 @@ def includeme(config: 'Configurator') -> None:
         factory=meeting_factory
     )
     config.add_view(
-        sortable_agenda_items_view,
+        move_agenda_item,
         route_name='sortable_agenda_items',
         request_method='POST',
         xhr=False
     )
     config.add_view(
-        sortable_agenda_items_view,
+        move_agenda_item,
         route_name='sortable_agenda_items',
         renderer='json',
         request_method='POST',

src/privatim/views/activities.py

@@ -77,7 +77,7 @@ def activity_to_dict(activity: Any) -> 'ActivityDict':
                 if is_creation
                 else _('Consultation Updated')
             ),
-            'route_url': obj_type.lower(),
+            'route_url': 'consultation',
             'id': activity.id,
             'icon_class': _get_icon_class(obj_type),
             'content': _get_activity_content(activity),

src/privatim/views/meetings.py

@@ -434,10 +434,7 @@ def delete_meeting_view(
     )
 
 
-def sortable_agenda_items_view(
-        context: Meeting, request: 'IRequest'
-) -> 'RenderData':
-
+def move_agenda_item(context: Meeting, request: 'IRequest') -> 'RenderData':
     try:
         subject_id = int(request.matchdict['subject_id'])
         direction = request.matchdict['direction']
@@ -446,34 +443,42 @@ def sortable_agenda_items_view(
         raise HTTPBadRequest('Request parameters are missing or invalid') \
             from e
 
-    agenda_items = context.agenda_items
+    if direction not in ['above', 'below']:
+        raise HTTPMethodNotAllowed('Invalid direction')
+
+    # Get all agenda items sorted by position
+    items = context.agenda_items
+
+    # Find the items we're working with
     subject_item = next(
-        (item for item in agenda_items if item.position == subject_id), None
+        (item for item in items if item.position == subject_id), None
     )
     target_item = next(
-        (item for item in agenda_items if item.position == target_id), None
+        (item for item in items if item.position == target_id), None
     )
 
-    if subject_item is None or target_item is None:
+    if not subject_item or not target_item:
         raise HTTPMethodNotAllowed('Invalid subject or target id')
 
-    if direction not in ['above', 'below']:
-        raise HTTPMethodNotAllowed('Invalid direction')
-
-    new_position = target_item.position
-    if direction == 'below':
-        new_position += 1
+    old_pos = subject_item.position
+    new_pos = (
+        target_item.position if direction == 'above'
+        else target_item.position + 1
+    )
 
-    for item in agenda_items:
-        match direction:
-            case 'above' if (new_position
-                             <= item.position < subject_item.position):
-                item.position += 1
-            case 'below' if (subject_item.position
-                             < item.position <= new_position):
+    # Adjust positions of items between old and new positions
+    for item in items:
+        if old_pos < new_pos:
+            # Moving down
+            if old_pos < item.position <= new_pos - 1:
                 item.position -= 1
+        else:
+            # Moving up
+            if new_pos <= item.position < old_pos:
+                item.position += 1
 
-    subject_item.position = new_position
+    # Set the subject item's new position
+    subject_item.position = new_pos - 1 if old_pos < new_pos else new_pos
 
     return {
         'status': 'success',

tests/shared/utils.py

@@ -164,3 +164,18 @@ def get_pre_filled_content_on_searchable_field(page, field_id):
     form_fields = page.form.fields
     attendees_options = form_fields[field_id][0].__dict__['options']
     return [entry[2] for entry in attendees_options if entry[1]]
+
+
+def verify_sequential_positions(items: list[AgendaItem]) -> None:
+    """Verify that positions are sequential and have no duplicates."""
+    positions = [item.position for item in items]
+    # Check if positions are sequential from 0 to len(items)-1
+    expected = set(range(len(items)))
+    actual = set(positions)
+    assert (
+        expected == actual
+    ), f'Positions {positions} are not sequential 0-based integers'
+    # Check for duplicates
+    assert len(positions) == len(
+        set(positions)
+    ), f'Duplicate positions found in {positions}'