CircuitVerse icon indicating copy to clipboard operation
CircuitVerse copied to clipboard

Fix notifications when the projects or users are deleted

Open tachyons opened this issue 1 year ago • 19 comments

When a project is forked, a notification is generated. But if that project is deleted, error is displayed since the notification unable to find the notification.

Solution:

Use null object pattern

Ref:

  • https://dev.to/daviducolo/exploring-the-null-object-pattern-in-ruby-k5l
  • https://thoughtbot.com/blog/rails-refactoring-example-introduce-null-object
  def message
-    user = params[:user]
-    project = params[:project]
+    user = params[:user] || DeletedUser.new
+    project = params[:project] || DeletedUser.new
    t("users.notifications.fork_notification", user: user.name, project: project.name)
  end

tachyons avatar Apr 20 '24 16:04 tachyons

Hi @tachyons I am interested in work on this issue.

Malavi1 avatar Apr 21 '24 04:04 Malavi1

HI @tachyons , If I am not wrong, the notification is triggered from the below piece of code

def fork(user)
    forked_project = dup
    forked_project.build_project_datum.data = project_datum&.data
    forked_project.circuit_preview.attach(circuit_preview.blob)
    forked_project.image_preview = image_preview
    forked_project.update!(
      view: 1, author_id: user.id, forked_project_id: id, name: name
    )
    @project = Project.find(id)
    if @project.author != user # rubocop:disable Style/IfUnlessModifier
      ForkNotification.with(user: user, project: @project).deliver_later(@project.author)
    end
    forked_project
  end

Since, find is used, the exception will be raised, ActiveRecord::RecordNotFound for deleted project and the notification will not be triggered. To fix this issue, should we have to handle the above case also, or am I missing something?

Harry-kp avatar Apr 21 '24 05:04 Harry-kp

@Harry-kp Issue happens when the project or user id deleted after the notification record is created. So that code snippet is fine.

tachyons avatar Apr 21 '24 07:04 tachyons

@tachyons Understood. Let me know if it would be okay for me to work on this issue as it has been already assigned.

Harry-kp avatar Apr 21 '24 08:04 Harry-kp

You can pair with Malavi

On Sun, 21 Apr, 2024, 2:13 pm Harshit Chaudhary, @.***> wrote:

@tachyons https://github.com/tachyons Understood. Let me know if it would be okay for me to work on this issue as it has been already assigned.

— Reply to this email directly, view it on GitHub https://github.com/CircuitVerse/CircuitVerse/issues/4904#issuecomment-2067962915, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAXYAEGJRPOAIAHSY5AL2DTY6N34LAVCNFSM6AAAAABGQTRBNOVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDANRXHE3DEOJRGU . You are receiving this because you were mentioned.Message ID: @.***>

tachyons avatar Apr 21 '24 10:04 tachyons

can you give me steps to reproduce this issue?

adarsh7raj avatar Apr 22 '24 17:04 adarsh7raj

I also want to work on this issue

Asrani-Aman avatar Apr 22 '24 17:04 Asrani-Aman

@Asrani-Aman Malavi and Harry are already assigned

subhamkumarr avatar Apr 22 '24 18:04 subhamkumarr

hey, i just fixed the issue please review

Ayushd785 avatar Jun 11 '24 09:06 Ayushd785