Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Motion detail refactoring #4024

Merged
merged 51 commits into from
Sep 16, 2024

Conversation

bastianjoel
Copy link
Member

@bastianjoel bastianjoel commented Aug 14, 2024

resolves #4023
resolves #3605
resolves #4041
resolves #2719
resolves #1664 - prevent unnecessary model store updates from data by merged subscriptions
resolves #4052
resolves #2946

Other changes

  • Motions should load around 20% faster with this.
  • View models got a new property viewModelUpdateTimestamp
  • Fix tiptap not updating on form change via patch
  • User gets notified if motion changed while editing
  • Minor cleanup

following additional issues will be addressed here too:

TODO (not a full list of changes):

  • Use os-motion-comment in os-motion-personal-note
  • Change Detection/Odd accessor replacement
    • MotionManageTitleComponent
    • MotionManagePollsComponent
    • MotionManageMotionMeetingUsersComponent
    • MotionHighlightFormComponent
    • MotionExtensionFieldComponent
    • MotionDetailDiffSummaryComponent
    • MotionAddPollComponent
  • Cleanup BaseMotionDetailChildComponent
    • Remove editMode logic
    • Remove MotionServiceCollector
    • Improve change detection marking
    • Remove change reco/amendment calculation or make it static
    • Remove MotionDetailViewService accessors
    • Maybe get rid of this completely
  • Move AmendmentCreateWizard into MotionFormModule
  • Simplify AmendmentCreateWizard by using the motion base form
  • Simplify control flow by utilizing @else/@else if
  • Review @for track parameters
  • Improve motion and meeting switching (Fix missing loading bar for a motion #2719)
  • Remove (parts of) MotionDetailViewService
  • Amendment not changing to diff on reload

@bastianjoel bastianjoel self-assigned this Aug 14, 2024
@bastianjoel bastianjoel changed the title WIP: Split motion detail component Motion detail refactoring Aug 14, 2024
@bastianjoel bastianjoel force-pushed the 4023-motion-detail-refactoring branch from e1fbe78 to 4754a16 Compare August 15, 2024 13:39
@bastianjoel bastianjoel force-pushed the 4023-motion-detail-refactoring branch from 4754a16 to 3724a94 Compare August 15, 2024 13:41
Change detection for amendments and crs not working yet
@bastianjoel bastianjoel force-pushed the 4023-motion-detail-refactoring branch from 385b402 to 1795554 Compare August 16, 2024 09:30
@bastianjoel bastianjoel force-pushed the 4023-motion-detail-refactoring branch 3 times, most recently from 45dceaf to 1424705 Compare August 19, 2024 12:23
@bastianjoel bastianjoel force-pushed the 4023-motion-detail-refactoring branch from 1424705 to 4eca6ab Compare August 19, 2024 13:00
@luisa-beerboom luisa-beerboom removed their assignment Aug 28, 2024
@MSoeb MSoeb removed their assignment Aug 30, 2024
@MSoeb MSoeb removed their request for review August 30, 2024 12:49
@Elblinator Elblinator self-assigned this Sep 9, 2024
@Elblinator Elblinator self-requested a review September 9, 2024 14:23
Copy link
Member

@Elblinator Elblinator left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • If a motion was never forwarded (but can be forwarded then the forward button should be visible. Currently the button is initially not there, and if you press in the meta-data then the button appears
  • Edit whole motion text amendment parent not loaded #4041 following the reproduction is giving the following result:
    motion before reload:
    Screenshot_20240910_134538
    after reload:
    Screenshot_20240910_134457
  • The meta-data Category, Tags and Motion block should be hidden if there is no category/tag/motion block
  • If a motion title has a change recommendation then the indicator is always shown. The title should only have the indicator if the original version is shown.

2nd and 3rd are moved to the commend below
4th is a non-issue after the explanation

@bastianjoel
Copy link
Member Author

Title indicator is displayed on diff and original version which I think is okay, as the title change does not get displayed directly but instead in the motion content. Also that behavior is the same as before the indicator was broken.

Empty category, tags and block areas do get hidden for users which cannot edit those areas. Cannot replicate.

Amendment form also works for me.

@Elblinator
Copy link
Member

Elblinator commented Sep 11, 2024

  • if a workflow was deleted then a reload is needed to show the correct workflow list (workflow -> create wf -> delete wf -> still displayed -> reload -> gone -> fixed by Reduce data sent by shared worker #4066
  • unclear description from my part with the not hidden category/tags/motion block
    If you are someone who is allowed to manage the meta data then you can 'try' to change these options even if there ist no category/tag/motion block to select:
    Screenshot_20240911_083007
    before this PR this empty mat-menu was hidden if empty
  • Edit whole motion text amendment parent not loaded #4041 still results in the empty screen shown in the screenshot above (tested on a productive instance, in a new meeting, following the instructions from the issue)
  • If a participant can manage the meta data but not the whole motion, and the forwarding is possible then the following Error is thrown if the motion-list or the motion-detail view is opened:
Error: You are not allowed to perform presenter get_forwarding_meetings Missing permission: motion.can_manage 
```</s> -> needs it's own issue will be created in the future
- </s>If a new tab with a motion-detail view was opened and then left alone the following Error is thrown at some point in the console and no new motions are loaded and old motions are not openable

Failed to connect an existing shared worker because the type or credentials given on the SharedWorker constructor doesn't match the existing shared worker's type or credentials.

@bastianjoel
Copy link
Member Author

Still cannot reproduce #4041. Tried this now in Firefox, Chromium with and without shared worker active. Everything worked.

@bastianjoel
Copy link
Member Author

If a new tab with a motion-detail view was opened and then left alone the following Error is thrown at some point in the console and no new motions are loaded and old motions are not openable

I cannot reproduce this and also I don't think this in related to this PR.

@bastianjoel
Copy link
Member Author

If a participant can manage the meta data but not the whole motion, and the forwarding is possible then the following Error is thrown if the motion-list or the motion-detail view is opened:

This in happening on current main too. Please open a new issue. I won't fix this here.

@bastianjoel
Copy link
Member Author

if a workflow was deleted then a reload is needed to show the correct workflow list (workflow -> create wf -> delete wf -> still displayed -> reload -> gone

Will be fixed with #4066. Same issue happens when deleting other models.

@Elblinator
Copy link
Member

Elblinator commented Sep 11, 2024

  • If the default title is used for amendment creation then the following error is thrown if amendment is saved: Error: data must contain ['title'] properties

payload:

  "action": "motion.create",
  "data": [
    {
      "meeting_id": 376,
      "lead_motion_id": 29753,
      "text": "<p>Lorem ipsum</p>",
      "attachment_ids": [],
      "agenda_create": false,
      "agenda_type": "internal"
    }
  ]
}
  • while creating a motion: sometimes the save button stays disabled even if a title and text are added. If the save button stays disabled indefinitely then only manually adding a submitter enables the save button. The save button sometimes is not disabled at all and sometimes enables itself after a while
  • If the group permission "Can support motion" is activated/deactivated then a reload is needed to show/hide the support button -> repo unclear, does not need fixing now
  • If the group permission "Can manage meta data" is activated then a reload is needed to show meta information -> repo unclear, does not need fixing now
  • This could very likely be unrelated to this PR: opening navigating around in the motion-list and the motions, opening new tabs etc. I get the Error page: 'You are not supposed to be here... Authorization Error' rather often
  • while creating a new motion the default workflow is not displayed (the workflow is displayed after assigning it manually)

@bastianjoel
Copy link
Member Author

If the group permission "Can support motion" is activated/deactivated then a reload is needed to show/hide the support button

Cannot reproduce

If the group permission "Can manage meta data" is activated then a reload is needed to show meta information

Also works for me

@Elblinator
Copy link
Member

If a motion block/tag/catgory is deleted and no item is left then the motion detail needs a reload in order to hide the options
I supposed this is also fixed by #4066, @bastianjoel?

@bastianjoel
Copy link
Member Author

If a motion block/tag/catgory is deleted and no item is left then the motion detail needs a reload in order to hide the options
I supposed this is also fixed by #4066, @bastianjoel?

Yes

@Elblinator Elblinator enabled auto-merge (squash) September 16, 2024 08:42
@Elblinator Elblinator merged commit 56e5cf3 into OpenSlides:main Sep 16, 2024
2 checks passed
@bastianjoel bastianjoel deleted the 4023-motion-detail-refactoring branch September 16, 2024 09:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment