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

Fix replacing references of presenter_id #16771

Merged

Conversation

oliverguenther
Copy link
Member

@oliverguenther oliverguenther force-pushed the fix/replace-meeting-agenda-item-presenter branch from 4d94fb3 to 1f4d3a3 Compare September 22, 2024 19:09
@oliverguenther
Copy link
Member Author

The failing spec is unrelated

Copy link
Member

@cbliard cbliard left a comment

Choose a reason for hiding this comment

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

Nice. I tested it and it works.

Then I added a few commits instead of adding review comments to

  • fix rubocop warning
  • remove leftover shared example in the original file
  • make test (hopefully) easier to read by removing the need to specify created_at and updated_at attributes, and removing author_id: principal.id attribute as it would have been overwritten anyway.
  • add a test for Journal::MeetingAgendaItemJournal#author_id

If these changes are ok for you, feel free to merge it.

@cbliard
Copy link
Member

cbliard commented Sep 23, 2024

And we could also rewrite the shared example to replace this output:

    with WorkPackage
      behaves like rewritten record
        for work_package
          when author_id is set to the replaced user
            replaces its value
          when author_id is set to a different user
            keeps its value
      behaves like rewritten record
        for work_package
          when assigned_to_id is set to the replaced user
            replaces its value
          when assigned_to_id is set to a different user
            keeps its value
      behaves like rewritten record
        for work_package
          when responsible_id is set to the replaced user
            replaces its value
          when responsible_id is set to a different user
            keeps its value
    with CostQuery
      behaves like rewritten record
        for cost_query
          when user_id is set to the replaced user
            replaces its value
          when user_id is set to a different user
            keeps its value
    with Notification actor
      behaves like rewritten record
        for notification
          when actor_id is set to the replaced user
            replaces its value
          when actor_id is set to a different user
            keeps its value

with this output

    with WorkPackage
      replaces user references found in #author_id
      replaces user references found in #assigned_to_id
      replaces user references found in #responsible_id
    with CostQuery
      replaces user references found in #user_id
    with Notification
      replaces user references found in #actor_id

But I refrained from doing it because it may push things a little too far.

@oliverguenther
Copy link
Member Author

Thanks @cbliard , I like the change. If you have it prepared already, could you submit this to dev? I'll go ahead and merge this so we can get 14.5.1 out

@oliverguenther oliverguenther merged commit c3e04c8 into release/14.5 Sep 23, 2024
10 checks passed
@oliverguenther oliverguenther deleted the fix/replace-meeting-agenda-item-presenter branch September 23, 2024 17:44
@cbliard
Copy link
Member

cbliard commented Sep 24, 2024

Sure, will do 👍

@cbliard
Copy link
Member

cbliard commented Sep 24, 2024

I give up on it. The code needed to get it looking like what I posted is a bit too convoluted. I'll keep it in a local branch in case I have an idea about how to make it work.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

2 participants