Meetings merge minutes and close actions by entantoencuanto · Pull Request #7968 · decidim/decidim

entantoencuanto · 2021-05-07T19:03:15Z

🎩 What? Why?

Migrates the minutes model attributes to meetings.
Removes the minutes management from admin panel and moves the management of the new meeting attributes related with minutes to the close action
Changes the front to display correct minutes attributes
Copies existing minutes descriptions into closing reports when previous closing report was blank
Closes automatically meetings if minutes data is present
If a meeting was closed but there were no minutes attributes the migration sets closing_visible to true to display closing values as before.
Extends closing_visible behavior to hide all closing attributes
Changes API response to send minutes attributes in the meeting request response
Removes minutes model and related API type. The migration to remove minutes table also changes the Decidim::ActionLog instances to avoid errors (see 55107b2 for more details)

📌 Related Issues

Link your PR to an issue

Fixes Merge actions of "Minutes" and "Close" in meetings #7285

Testing

Describe the best way to test or validate your PR.

📋 Checklist

🚨 Please review the guidelines for contributing to this repository.

📷 Screenshots

Please add screenshots of the changes you're proposing

♥️ Thank you!

leio10 · 2021-05-24T11:30:37Z

@entantoencuanto Hi! Is this PR ready to be reviewed?

…update tests

…eting

The migration also changes existing action_log entries (and associated version) associated with minutes to point to the meeting instance instead of a minutes instance. This avoids errors once the table and model are deleted

This migration: * Changes renames minutes_visible with closing_visible which is going to be extended to all closing attributes. * Removes minutes_description column copies it before into closing_report if blank. * Closes meetings when minutes attributes exist if not closet yet. * If a meeting is closed but there are no minutes attributes the migration sets closing_visible to true to display closing values as before.

… removal * Extends closing_visible behavior to hide all closing attributes * Change admin form to manage close and minute attributes in only one section * Changes API response to remove minutes_description reference and hide all close attributes when closing_visible sets it * Updates some tests

entantoencuanto · 2021-05-26T15:03:11Z

@entantoencuanto Hi! Is this PR ready to be reviewed?

Oh, yes, I've resolved some conflicts rebasing, but I think it's ready. Sorry for the delay in responding

entantoencuanto · 2021-05-26T15:05:58Z

I've seen a small change commented in the issue, I'll add a commit reordering meeting show view

leio10

Great job, especially with the documentation and the data migration! I've added a few comments, let's address them and we can merge this PR.

leio10 · 2021-05-27T14:36:22Z

decidim-meetings/lib/decidim/api/meeting_type.rb

+      field :video_url, GraphQL::Types::String, "URL for the video of the session, if any", null: true
+      field :audio_url, GraphQL::Types::String, "URL for the audio of the session, if any", null: true
+      # probably useful in the future, when handling user permissions
+      # field :closing_visible, !types.Boolean, "Whether this minutes is public or not", property: :visible


Do we need to keep this commented code? I'd prefer to remove it and add it in the future when needed. 😬

leio10 · 2021-05-27T15:36:44Z

decidim-meetings/db/migrate/20210506180226_merge_meetings_minutes_into_meetings_table.rb

+
+private
+
+def minutes_class


There is any reason to create this class dynamically? It would be possible to define a static class instead of using a method?

No, there is no special reason to define class dynamically, I'll use a static class instead

leio10 · 2021-05-27T15:48:19Z

decidim-meetings/db/migrate/20210506180226_merge_meetings_minutes_into_meetings_table.rb

+  end
+
+  def down
+    Decidim::Meetings::Meeting.find_each do |meeting|


I'm trying to reduce the errors during data migrations, so I recommend you not to load model instances in that context. In this discussion you can find a clear list of alternatives to fix it.

Also, I encourage you to add feedback if you find any error or possible improvement in the proposed solutions.

leio10 · 2021-05-27T15:53:18Z

decidim-meetings/db/migrate/20210506180226_merge_meetings_minutes_into_meetings_table.rb

+      "TmpMinutes"
+    end
+
+    belongs_to :meeting, foreign_key: "decidim_meeting_id", class_name: "Decidim::Meetings::Meeting"


We can apply the data migration rules also for the class_name attribute.

leio10 · 2021-05-27T15:53:34Z

decidim-meetings/db/migrate/20210512100333_drop_decidim_meetings_minutes_table.rb

+
+class DropDecidimMeetingsMinutesTable < ActiveRecord::Migration[6.0]
+  def up
+    Decidim::ActionLog.where(resource_type: "Decidim::Meetings::Minutes").each do |action_log|


We can apply the data migration rules here. Maybe you need to add the Traceable concern to the new ActionLog class.

In this case I have not needed the Traceable concern to update the action_log entries

leio10 · 2021-05-27T15:54:38Z

decidim-meetings/db/migrate/20210512100333_drop_decidim_meetings_minutes_table.rb

+      t.timestamps
+    end
+
+    Decidim::Meetings::Meeting.find_each do |meeting|


We can apply the data migration rules here.

leio10 · 2021-05-27T15:55:27Z

decidim-meetings/db/migrate/20210512100333_drop_decidim_meetings_minutes_table.rb

+    meeting.minutes_description.blank? || meeting.minutes_description.is_a?(Hash) && meeting.minutes_description.values.all?(&:blank?)
+end
+
+def minutes_class


Same questions as before, is there any reason to define this class dinamically?

leio10 · 2021-05-27T15:58:51Z

...im-meetings/db/migrate/20210518133236_merge_minutes_with_closing_report_in_meetings_table.rb

+
+class MergeMinutesWithClosingReportInMeetingsTable < ActiveRecord::Migration[6.0]
+  def up
+    Decidim::Meetings::Meeting.find_each do |meeting|


We can apply the data migration rules here. In fact, I think that if you define a new class for Meeting you could use the attributes and call meeting.save! as the last statement for the loop and remove the calls to update_attribute.

leio10 · 2021-05-28T07:48:13Z

CHANGELOG.md

+* If there was minutes data and the meeting was not closed, the meeting is closed and the minutes `description` value is copied to the meeting `closing_report`, the `video_url` and `audio_url` minutes attributes values are copied to the respective meeting attributes and the minutes `visible` attribute value is copied to the meeting `closing_visible` attribute.
+* If there was minutes data and the meeting was closed, the meeting remains closed and the meeting `closing_report` value remains if present. Elsewere the minutes `description` value is copied to the meeting `closing_report`. the `video_url` and `audio_url` minutes attributes values are copied to the respective meeting attributes and the minutes `visible` attribute value is copied to the meeting `closing_visible` attribute. In this case the visibility of closing report may change to false if there was a minutes with `visible` set to false.
+
+Please, note that if there was previously minutes_description and closing_report data for a meeting, after applying the changes of this release, the minutes_description data will be lost.


Can you format the variables in this paragraph?

…ions * develop: Fix broken test on meetings after merging PR without rebase (#8076) Show participants list in meetings (#7933) Security feature external link warning (#7397)

entantoencuanto · 2021-05-28T17:19:37Z

I think it's ready, @leio10

* develop: (59 commits) Update supported versions in docs (#8079) Meetings merge minutes and close actions (#7968) Meeting calendars providers (#7944) Fix broken test on meetings after merging PR without rebase (#8076) Show participants list in meetings (#7933) Security feature external link warning (#7397) Add missing tests for scope types admin page (#8053) Use symbols for polymorphic route arguments (#8052) Mockup design for Participation statistics tables in Votings (#7879) Fix boolean fields for .reported? and .hidden? which is nil if no report exists (#7990) Fix redirects broken by Terms and Conditions redirect (#8036) Amend CSS overwritting (#8007) New Crowdin updates (#8048) Fix undetected broken tests because of missing dependencies (#8050) Validate results by Monitoring Committee Members (#7899) Electoral certificate validation by Monitoring Committee Members (#7871) Publish and unpublish a meeting (#7893) New Crowdin updates (#8005) Polling station closure attach the physical electoral closure certificate (#7929) Fix attachment title migration generating possibly invalid values (#8020) ...

entantoencuanto force-pushed the 7285-meetings-merge-minutes-and-close-actions branch 2 times, most recently from 4d4d1aa to 6546013 Compare May 7, 2021 21:22

entantoencuanto marked this pull request as ready for review May 12, 2021 19:11

entantoencuanto force-pushed the 7285-meetings-merge-minutes-and-close-actions branch 2 times, most recently from 04471b0 to a61aabd Compare May 19, 2021 09:28

entantoencuanto added 20 commits May 26, 2021 16:46

Add migration to merge minutes into meetings table

7041d8d

Manage minutes attributes from admin panel in close action

1839734

Remove minutes management from admin panel

9297b56

Change tests of admin close meetings action to include minutes

1f02bdc

Change meeting show to display minutes from meeting close action and …

3713b41

…update tests

Update API response for meetings

80efb0e

Remove validation to allow empty attending organizations closing a me…

3ca7190

…eting

Hide attending organizations section on meetings show when blank

8d62b2f

Add minutes attributes to admin log meeting presenter

aeae024

Add migration to delete decidim_meetings_minutes table

810d6b7

The migration also changes existing action_log entries (and associated version) associated with minutes to point to the meeting instance instead of a minutes instance. This avoids errors once the table and model are deleted

Remove Decidim::Meetings::Minutes model and API related type

fa8f88d

Remove no longer required test

ff22446

Update CHANGELOG.md

0ff5f4c

Fix changelog

afa31e8

Remove unnecessary span on meetings index actions icons

0a854ab

Update changelog

0eb58bc

Replace report term with minutes

8237e2e

Update test

719aac4

entantoencuanto force-pushed the 7285-meetings-merge-minutes-and-close-actions branch from 964fa37 to 719aac4 Compare May 26, 2021 14:55

leio10 added the in-review label May 26, 2021

Move minutes related information below report

7fa0f1f

leio10 added module: meetings type: change PRs that implement a change for an existing feature labels May 27, 2021

leio10 suggested changes May 28, 2021

View reviewed changes

entantoencuanto added 4 commits May 28, 2021 14:09

Remove comment

2b9f6ee

Fix data migrations

ef850f5

Fix format in Changelog

85d5cb0

Merge branch 'develop' into 7285-meetings-merge-minutes-and-close-act…

c419ea8

…ions * develop: Fix broken test on meetings after merging PR without rebase (#8076) Show participants list in meetings (#7933) Security feature external link warning (#7397)

leio10 approved these changes May 28, 2021

View reviewed changes

leio10 merged commit 9725ec5 into develop May 28, 2021

leio10 deleted the 7285-meetings-merge-minutes-and-close-actions branch May 28, 2021 18:39

carolromero mentioned this pull request Sep 6, 2022

Missing minutes from meetings #9781

Open

Uh oh!

Conversation

entantoencuanto commented May 7, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🎩 What? Why?

📌 Related Issues

Testing

📋 Checklist

📷 Screenshots

Uh oh!

leio10 commented May 24, 2021

Uh oh!

entantoencuanto commented May 26, 2021

Uh oh!

entantoencuanto commented May 26, 2021

Uh oh!

leio10 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

entantoencuanto commented May 28, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

entantoencuanto commented May 7, 2021 •

edited

Loading