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

TAN-2538 Cleanup old initiatives code (Part 2) #9819

Open
wants to merge 60 commits into
base: master
Choose a base branch
from

Conversation

jinjagit
Copy link
Contributor

@jinjagit jinjagit commented Dec 17, 2024

Removes

  • controllers
  • routes
  • serializers
  • policies
  • side_fx services
  • notifications
  • campaigns
  • permissions
  • seeds
  • some analytics code
  • InitiativesFinder
  • related specs

Does NOT remove

  • models, except the notification & campaign models are removed.
    There are some small changes to models, but nothing that will change db data)
  • posts polymorphysm, as I noticed Sebi already has a PR open for this. TAN-2538 Cleanup old initiatives code (Part 1) #8864
  • factories (that are still used by models)

Changes

  • fixes some tests

Adds

  • one or two new tests, where I felt there were obvious gaps

Notes

  • public_api - controller, route + spec removed in this PR - only had 16 hits of initiatives endpoint in last 4 weeks (Cloudwatch). All UK, and James thinks:
    "... they are likely to be Havant using Power BI and I actually think PBI will be just throw a couple of errors in the background but nothing that will cause data issues for them as they were not using initiatives."

See also TAN-2538 Cleanup old initiatives code #8864

Changelog

Technical

  • [TAN-2538] BE remove initiatives code (Part 1)

Copy link

@jinjagit jinjagit marked this pull request as draft December 17, 2024 11:44
@jinjagit jinjagit self-assigned this Dec 17, 2024
@cl-dev-bot
Copy link
Collaborator

cl-dev-bot commented Dec 17, 2024

Messages
📖 Changelog provided 🎉
📖 Notion issue: TAN-2538
📖

Run the e2e tests

📖 Check translation progress

Generated by 🚫 dangerJS against a40997f

@jinjagit jinjagit marked this pull request as ready for review December 23, 2024 15:10
Copy link
Contributor

@sebastienhoorens sebastienhoorens left a comment

Choose a reason for hiding this comment

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

I'm keeping TODO's as review comments here. I propose making one big PR with all changes and rename this one to Part 1. Each part should go through a review process, and their PRs would then be closed as the big combined PR would get merged instead.

@@ -24,7 +24,7 @@ class Permission < ApplicationRecord
PERMITTED_BIES = %w[everyone everyone_confirmed_email users admins_moderators verified].freeze
ACTIONS = {
# NOTE: Order of actions in each array is used when using :order_by_action
nil => %w[visiting following posting_initiative commenting_initiative reacting_initiative attending_event],
Copy link
Contributor

Choose a reason for hiding this comment

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

We will also need to delete existing permissions on existing platforms.

@@ -6,31 +6,21 @@ class NotificationService
Notifications::CommentDeletedByAdmin,
Notifications::CommentMarkedAsSpam,
Notifications::CommentOnIdeaYouFollow,
Notifications::CommentOnInitiativeYouFollow,
Copy link
Contributor

Choose a reason for hiding this comment

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

We will also need to delete existing notifications on existing platforms.

.order(published_at: :desc)
.group('union_posts.id, union_posts.published_at') # Remove union_post duplicates
.select('union_posts.id')
paged_posts = paginate paged_posts

# Get the comments, grouped by the corresponding posts
# page.
# Get the comments, grouped by the corresponding posts page.
Copy link
Contributor

Choose a reason for hiding this comment

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

TODO: post_id and post_type -> idea or input

factory :threshold_reached_for_admin, parent: :notification, class: 'Notifications::ThresholdReachedForAdmin' do
association :post, factory: :initiative
association :post, factory: :idea
Copy link
Contributor

Choose a reason for hiding this comment

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

TODO: On Notification model: Post -> Idea, PostStatus -> IdeaStatus

cosponsor2 = create(:user)
create(:invitation_to_cosponsor_initiative, cosponsors_initiative: cosponsors_initiative)
initiative.update!(cosponsor_ids: [cosponsor2.id])
# it 'removes cosponsors_initiative even when an associated notifcation exists' do
Copy link
Contributor

Choose a reason for hiding this comment

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

Can be deleted

@sebastienhoorens sebastienhoorens changed the title [TAN-2538] BE remove initiatives code (Part 1) TAN-2538 Cleanup old initiatives code (Part 2) Dec 23, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants