Skip to content

Add reminders for publishing reports to meeting authors#8757

Merged
andreslucena merged 11 commits intodecidim:developfrom
i-need-another-coffee:feature/add_close_meeting_reminder
Mar 29, 2022
Merged

Add reminders for publishing reports to meeting authors#8757
andreslucena merged 11 commits intodecidim:developfrom
i-need-another-coffee:feature/add_close_meeting_reminder

Conversation

@roxanaopr
Copy link
Copy Markdown
Contributor

@roxanaopr roxanaopr commented Jan 27, 2022 •
edited
Loading

🎩 What? Why?

As an admin/moderator I want to be able to set up a notification to be sent to meeting organizers if their meeting passed and they didn’t published an meeting report so that the quality of the meetings on the platform is improved.

It is necessary to have the meeting reports filled in, therefore, email reminders will be sent to meeting author, as an initial notification and a reminder notification.

As a meeting author, I want to receive a notification if my meeting passed and I didn't publish a report so that I can remember to close it and attach the meeting report. Also, I want to receive another reminder at a given time if I still didn't have the chance to close the passed meeting.

Reminders interval times: 3.days, 7.days

📌 Related Issues

Meta proposal: https://meta.decidim.org/processes/roadmap/f/122/proposals/16852
Initial PR: #8551

Testing

Describe the best way to test or validate your PR.

📋 Checklist

🚨 Please review the guidelines for contributing to this repository.

  • ❓ CONSIDER adding a unit test if your PR resolves an issue.
  • ✔️ DO check open PR's to avoid duplicates.
  • ✔️ DO keep pull requests small so they can be easily reviewed.
  • ✔️ DO build locally before pushing.
  • ✔️ DO make sure tests pass.
  • ✔️ DO make sure any new changes are documented in docs/.
  • ✔️ DO add and modify seeds if necessary.
  • ✔️ DO add CHANGELOG upgrade notes if required.
  • ✔️ DO add to GraphQL API if there are new public fields.
  • ✔️ DO add link to MetaDecidim if it's a new feature.
  • ❌AVOID breaking the continuous integration build.
  • ❌AVOID making significant changes to the overall architecture.

📷 Screenshots

Screenshot from 2022-01-28 15-28-00
Screenshot from 2022-01-28 15-31-27

♥️ Thank you!

alecslupu
alecslupu reviewed Jan 28, 2022
View reviewed changes
@roxanaopr roxanaopr force-pushed the feature/add_close_meeting_reminder branch from ee5b7b4 to 8dac392 Compare January 28, 2022 11:18
@roxanaopr roxanaopr marked this pull request as ready for review January 31, 2022 07:40
@roxanaopr roxanaopr force-pushed the feature/add_close_meeting_reminder branch from a083ada to 1394504 Compare February 1, 2022 07:55
@andreslucena andreslucena mentioned this pull request Feb 1, 2022
Closed
12 tasks
alecslupu
alecslupu reviewed Feb 4, 2022
View reviewed changes
alecslupu
alecslupu suggested changes Feb 4, 2022
View reviewed changes
Copy link
Copy Markdown
Contributor

@alecslupu alecslupu left a comment

Choose a reason for hiding this comment

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

Please change the Virtus.model to Decidim::AttributeObject::Model

@andreslucena
Copy link
Copy Markdown
Member

andreslucena commented Feb 16, 2022

First, the classic substitution of "events/meetings" 😉
event report → meeting report
event organizer → meeting author

As an admin/moderator I want to be able to set up a notification to be sent to meeting organizers if their meeting passed and they didn’t published an event report so that the quality of the meetings on the platform is improved.

Reading my comment again, I think I wasn't clear on what I had in mind, but just to be clear:

  • All the meetings authors will receive this by default. The "happy path" would be that normally a participant would create only a couple of meetings.
  • The admins will (potentially) receive lots of these kinds of notifications - for instance in some processes in Barcelona they could have dozens of notifications, so they have the possibility to disable.

For what I've seen in the code, this is how it works (as it's enabled by default and only admins can disable it), but just to confirm that we're all in the same page.

andreslucena
andreslucena requested changes Feb 16, 2022
View reviewed changes
Copy link
Copy Markdown
Member

@andreslucena andreslucena left a comment

Choose a reason for hiding this comment

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

Some suggestions of things to change, can you give it an eye @roxanaopr 🙏🏽 ?

@roxanaopr
Copy link
Copy Markdown
Contributor Author

roxanaopr commented Feb 17, 2022

  • All the meetings authors will receive this by default. The "happy path" would be that normally a participant would create only a couple of meetings.
  • The admins will (potentially) receive lots of these kinds of notifications - for instance in some processes in Barcelona they could have dozens of notifications, so they have the possibility to disable.

For what I've seen in the code, this is how it works (as it's enabled by default and only admins can disable it), but just to confirm that we're all in the same page.

Indeed, the option to disable/enable is only for moderators. By default, the option is enabled.

@roxanaopr roxanaopr force-pushed the feature/add_close_meeting_reminder branch 2 times, most recently from e4a0545 to a21c652 Compare February 17, 2022 11:16
@roxanaopr roxanaopr force-pushed the feature/add_close_meeting_reminder branch from a21c652 to b3ac6e4 Compare February 17, 2022 12:53
alecslupu
alecslupu previously approved these changes Feb 17, 2022
View reviewed changes
Copy link
Copy Markdown
Contributor

@alecslupu alecslupu left a comment

Choose a reason for hiding this comment

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

Looks good for me

@alecslupu
Copy link
Copy Markdown
Contributor

alecslupu commented Feb 18, 2022

@andreslucena this can be reviewed.

@roxanaopr roxanaopr force-pushed the feature/add_close_meeting_reminder branch from f3b4bb7 to f6af6dd Compare February 24, 2022 17:09
@andreslucena
Copy link
Copy Markdown
Member

andreslucena commented Mar 22, 2022

@roxanaopr I don't know where's this comment, I found it in my email, but I don't see it in GitHub UI

@andreslucena I've started working on the refactoring part (rename component_notification_settings to notification_settings and include all the other user settings: email_on_notification, email_on_moderations, newsletter_notifications, notifications_from_followed, notifications_from_own_activity, allow_public_contact). The problem is that lots of files are impacted (including spec files, schema migrations, data migrations) and I think that this PR will look very messy at the end. My suggestion is to do this refactoring in a separate PR, to keep thinks more clear.

I agree, just with having the field as a starting point is more than enough for now!

@roxanaopr
Copy link
Copy Markdown
Contributor Author

roxanaopr commented Mar 22, 2022 •
edited
Loading

@roxanaopr I don't know where's this comment, I found it in my email, but I don't see it in GitHub UI

@andreslucena I realized that this was the conclusion of the bellow discussion and I deleted the comment. Hopefully the PR it's ok now.

@roxanaopr
Copy link
Copy Markdown
Contributor Author

roxanaopr commented Mar 24, 2022

@ahukkanen Can you please take a look after all the changes done?

ahukkanen
ahukkanen requested changes Mar 24, 2022
View reviewed changes
Copy link
Copy Markdown
Contributor

@ahukkanen ahukkanen left a comment

Choose a reason for hiding this comment

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

I noticed one issue after the column name / notification settings registry method change.

Also, I added few refactoring ideas, as I think those empty classes are not necessary if you are not using them for anything.

@roxanaopr roxanaopr force-pushed the feature/add_close_meeting_reminder branch from feb6f7b to 6f3cbcd Compare March 25, 2022 12:50
ahukkanen
ahukkanen reviewed Mar 25, 2022
View reviewed changes
@roxanaopr roxanaopr force-pushed the feature/add_close_meeting_reminder branch from 6f3cbcd to db97e3d Compare March 25, 2022 13:48
@ahukkanen ahukkanen requested a review from andreslucena March 25, 2022 13:53
andreslucena
andreslucena approved these changes Mar 28, 2022
View reviewed changes
Copy link
Copy Markdown
Member

@andreslucena andreslucena left a comment

Choose a reason for hiding this comment

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

👍🏽

@andreslucena andreslucena merged commit 0fb2edd into decidim:develop Mar 29, 2022
entantoencuanto added a commit to PopulateTools/decidim that referenced this pull request Apr 5, 2022
@entantoencuanto
Merge branch 'chore/meetings_optimization' into pwa-staging
6d6adcf 
* chore/meetings_optimization:
  Add touch:true to Follow association with followable
  Fragment cache meetings partial in meetings index
  Bump minimist and node-forge (decidim#9131)
  Bump puma from 5.6.2 to 5.6.4 (decidim#9132)
  Add base URI to meta image URLs (decidim#9125)
  Make Decidim fully configurable via ENV vars part II (decidim#8990)
  Allow assembly admins to manage components in child assemblies (decidim#8955)
  Export calendar improvements (decidim#9035)
  Add reminders for publishing reports to meeting authors (decidim#8757)
  VAPID key generator availabe in core (decidim#9107)
  Dont add external link container inside editor (decidim#9095)
@andreslucena andreslucena mentioned this pull request Apr 26, 2022
Closed
16 tasks
@alecslupu alecslupu added this to the 0.27.0 milestone Jul 14, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@andreslucena andreslucena andreslucena approved these changes

@ahukkanen ahukkanen ahukkanen approved these changes

+1 more reviewer

@alecslupu alecslupu alecslupu left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

module: meetings

Projects

None yet

Milestone

0.27.0

Development

Successfully merging this pull request may close these issues.

4 participants

@roxanaopr @andreslucena @alecslupu @ahukkanen