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.

Sign up for GitHub

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

feat: /delete command for clear messages | resolve #25 #31

Merged
merged 2 commits into from
Nov 6, 2024
Merged

feat: /delete command for clear messages | resolve #25 #31

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
028f6e4
refactor: 💡 elixir Enum.reduce_while
ThaddeusJiang Nov 6, 2024
5d9c21e
feat: 🎸 /delete for clear messages
ThaddeusJiang Nov 6, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
52 changes: 46 additions & 6 deletions lib/save_it/bot.ex
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,14 +25,17 @@ defmodule SaveIt.Bot do
setup_commands: true

command("start")

command("search", description: "Search photos")
command("similar", description: "Find similar photos")
command("about", description: "About the bot")
command("delete", description: "Delete message")

command("login", description: "Login")
command("code", description: "Get code for login")
command("folder", description: "Update Google Drive folder ID")

command("about", description: "Know more about this bot")

middleware(ExGram.Middleware.IgnoreUsername)

def bot(), do: @bot
Expand All @@ -43,11 +46,11 @@ defmodule SaveIt.Bot do

def handle({:command, :about, _msg}, context) do
answer(context, """
This bot is created by @ThaddeusJiang, feel free to contact me if you have any questions.
SaveIt can download images and videos, just give me a link.

GitHub: https://github.com/ThaddeusJiang
Blog: https://thaddeusjiang.com
X: https://x.com/thaddeusjiang
Created by @ThaddeusJiang, powered by Cobalt, Typesense, and Elixir.Access

Give a star ⭐ if you like it, https://github.com/ThaddeusJiang/save_it
""")
end

Expand Down Expand Up @@ -137,7 +140,25 @@ defmodule SaveIt.Bot do
end

def handle({:command, :similar, %{chat: chat, photo: nil}}, _context) do
send_message(chat.id, "Upload a photo to find similar photos.")
send_message(chat.id, "Upload a photo with /similar for finding similar photos.")
end

def handle({:command, :delete, %{chat: chat, reply_to_message: nil}}, _ctx) do
send_message(chat.id, "reply a message with /delete command.")
end

def handle(
{:command, :delete,
%{chat: chat, message_id: message_id, from: from, reply_to_message: reply_to_message}},
_ctx
) do
{:ok, %{id: bot_id, username: bot_username}} = ExGram.get_me()

if Enum.member?([bot_id, from.id], reply_to_message.from.id) do
handle_delete_command(chat.id, message_id, reply_to_message)
else
send_message(chat.id, "Only delete messages from @#{bot_username} and yourself.")
end
end

# caption: nil -> find same photos
Expand Down Expand Up @@ -467,4 +488,23 @@ defmodule SaveIt.Bot do
""")
end
end

defp handle_delete_command(chat_id, message_id, reply_to_message) do
case reply_to_message do
%{photo: nil} ->
delete_message(chat_id, reply_to_message.message_id)

%{photo: photo} ->
photo
|> Enum.map(& &1.file_id)
|> PhotoService.delete_photos()

delete_message(chat_id, reply_to_message.message_id)

_ ->
send_message(chat_id, "reply a message with /delete command.")
end

delete_message(chat_id, message_id)
end
Comment on lines +492 to +509
Copy link

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Enhance error handling and message deletion order.

The current implementation has several areas for improvement:

  1. Photo deletion could fail silently
  2. Message deletion order might cause confusion if one deletion fails
  3. No logging for failures
   defp handle_delete_command(chat_id, message_id, reply_to_message) do
+    # Delete command message first to provide immediate feedback
+    with {:ok, _} <- delete_message(chat_id, message_id) do
       case reply_to_message do
         %{photo: nil} ->
-          delete_message(chat_id, reply_to_message.message_id)
+          case delete_message(chat_id, reply_to_message.message_id) do
+            {:ok, _} -> :ok
+            {:error, reason} ->
+              Logger.error("Failed to delete message: #{inspect(reason)}")
+              send_message(chat_id, "Failed to delete the message. Please try again.")
+          end

         %{photo: photo} ->
-          photo
-          |> Enum.map(& &1.file_id)
-          |> PhotoService.delete_photos()
-
-          delete_message(chat_id, reply_to_message.message_id)
+          file_ids = Enum.map(photo, & &1.file_id)
+          case PhotoService.delete_photos(file_ids) do
+            {:ok, _} ->
+              case delete_message(chat_id, reply_to_message.message_id) do
+                {:ok, _} -> :ok
+                {:error, reason} ->
+                  Logger.error("Failed to delete message: #{inspect(reason)}")
+                  send_message(chat_id, "Photos deleted but failed to delete the message.")
+              end
+            {:error, reason} ->
+              Logger.error("Failed to delete photos: #{inspect(reason)}")
+              send_message(chat_id, "Failed to delete photos. Please try again.")
+          end

         _ ->
           send_message(chat_id, "reply a message with /delete command.")
       end
-
-      delete_message(chat_id, message_id)
+    else
+      {:error, reason} ->
+        Logger.error("Failed to delete command message: #{inspect(reason)}")
+        send_message(chat_id, "Failed to process delete command. Please try again.")
+    end
   end

Committable suggestion skipped: line range outside the PR's diff.

end
6 changes: 3 additions & 3 deletions lib/save_it/migration/typesense.ex
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,11 +24,11 @@ defmodule SaveIt.Migration.Typesense do
Typesense.handle_response!(res)
end

def delete_collection!(collection_name) do
def delete_collection(collection_name) do
req = build_request("/collections/#{collection_name}")
res = Req.delete!(req)
res = Req.delete(req)

Typesense.handle_response!(res)
Typesense.handle_response(res)
Comment on lines +27 to +31
Copy link

Choose a reason for hiding this comment

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

💡 Codebase verification

Error handling mismatch detected in caller

The drop_photos/0 function in lib/save_it/migration/typesense/photo.ex calls delete_collection/1 but doesn't handle its error cases, while being called by reset!/0 which uses the bang convention (indicating it should raise on errors). This creates an inconsistent error handling pattern.

  • drop_photos/0 should be renamed to drop_photos!/0 and handle the response from delete_collection/1 by raising on error
  • Alternatively, remove the bang from reset!/0 and propagate the error handling up the chain

Example fix for the first approach:

def drop_photos!() do
  case Typesense.delete_collection(@collection_name) do
    {:ok, response} -> response
    {:error, reason} -> raise "Failed to drop photos collection: #{inspect(reason)}"
  end
end
🔗 Analysis chain

LGTM! Consistent error handling approach.

The change from bang (!) to non-bang methods is consistent throughout the function, making error handling more graceful.

Let's verify the impact on callers of this function:

Consider adding @doc and @spec annotations to document the possible error cases and return values, for example:

@doc """
Deletes a Typesense collection.
Returns {:ok, response} on success or {:error, reason} on failure.
"""
@spec delete_collection(String.t()) :: {:ok, map()} | {:error, any()}
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Find all callers of delete_collection to ensure they handle the new error cases
rg -A 3 "delete_collection" --type elixir

Length of output: 553

Script:

#!/bin/bash
# Let's check the implementation of reset! in photo.ex to understand how it handles the response
rg -B 2 -A 5 "reset!" lib/save_it/migration/typesense/photo.ex

Length of output: 178

Script:

#!/bin/bash
# Let's also check if there are any other files that might be using similar patterns
ast-grep --pattern 'def reset!() do
  $$$
end'

Length of output: 371

Script:

#!/bin/bash
# Let's see the full content of photo.ex to understand the context
cat lib/save_it/migration/typesense/photo.ex

Length of output: 2012

end

defp get_env() do
Expand Down
9 changes: 5 additions & 4 deletions lib/save_it/migration/typesense/photo.ex
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,8 @@ defmodule SaveIt.Migration.Typesense.Photo do

Typesense.update_collection!(@collection_name, %{
"fields" => [
%{"name" => "file_id", "type" => "string", "optional" => true}
%{"name" => "file_id", "type" => "string", "optional" => true},
%{"name" => "url", "drop" => true}
]
})
end
Expand Down Expand Up @@ -66,12 +67,12 @@ defmodule SaveIt.Migration.Typesense.Photo do
Logger.info("migrated #{count} photos")
end

def drop_photos!() do
Typesense.delete_collection!(@collection_name)
def drop_photos() do
Typesense.delete_collection(@collection_name)
end

def reset!() do
drop_photos!()
drop_photos()
create_photos_20241024!()
migrate_photos_20241029!()
Comment on lines +75 to 77
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue

Consider making reset operations atomic.

The reset! function combines multiple operations (drop_photos, create_photos_20241024!, migrate_photos_20241029!) without handling partial failures. If any step fails after drop_photos, the system could be left in an inconsistent state.

Consider implementing a more robust reset operation:

   def reset!() do
-    drop_photos()
-    create_photos_20241024!()
-    migrate_photos_20241029!()
+    with :ok <- drop_photos(),
+         :ok <- create_photos_20241024!(),
+         :ok <- migrate_photos_20241029!() do
+      :ok
+    else
+      error ->
+        Logger.error("Reset failed: #{inspect(error)}")
+        error
+    end
   end

Committable suggestion skipped: line range outside the PR's diff.

end
Expand Down
8 changes: 8 additions & 0 deletions lib/save_it/photo_service.ex
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,14 @@ defmodule SaveIt.PhotoService do
end
end

def delete_photo(file_id) do
Typesense.delete_document_by_query("photos", "file_id:[#{file_id}]")
end

def delete_photos(file_ids) do
Typesense.delete_document_by_query("photos", "file_id:[#{Enum.join(file_ids, ",")}]")
end
Comment on lines +43 to +45
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue

Improve bulk deletion implementation

The bulk deletion function needs several improvements for robustness and safety:

  1. Add proper error handling and validation
  2. Consider chunking for large lists
  3. Validate each file_id in the list
-  def delete_photos(file_ids) do
+  def delete_photos!(file_ids) when is_list(file_ids) do
+    unless Enum.all?(file_ids, &is_binary/1) do
+      raise ArgumentError, "All file_ids must be strings"
+    end
+
+    if length(file_ids) > 100 do
+      Logger.warn("Attempting to delete #{length(file_ids)} photos at once")
+    end
+
+    result = Typesense.delete_document_by_query(
+      "photos",
+      "file_id:[#{Enum.join(file_ids, ",")}]"
+    )
+
+    case result do
+      {:ok, response} -> response
+      {:error, error} -> raise "Failed to delete photos: #{inspect(error)}"
+    end
   end

Consider implementing batch processing for large lists to avoid potential timeouts or performance issues:

def delete_photos_in_batches!(file_ids, batch_size \\ 100) do
  file_ids
  |> Enum.chunk_every(batch_size)
  |> Enum.map(&delete_photos!/1)
end


def get_photo(photo_id) do
Typesense.get_document("photos", photo_id)
end
Expand Down
12 changes: 11 additions & 1 deletion lib/small_sdk/typesense.ex
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,13 +63,23 @@ defmodule SmallSdk.Typesense do
handle_response(res)
end

def delete_document!(collection_name, document_id) do
def delete_document(collection_name, document_id) do
req = build_request("/collections/#{collection_name}/documents/#{document_id}")
res = Req.delete(req)

handle_response(res)
end

# docs: https://typesense.org/docs/27.1/api/documents.html#delete-documents
def delete_document_by_query(collection_name, filter_by, opts \\ []) do
batch_size = Keyword.get(opts, :batch_size, 100)

req = build_request("/collections/#{collection_name}/documents")

Req.delete(req, params: %{"filter_by" => filter_by, "batch_size" => batch_size})
|> handle_response()
end
Comment on lines +73 to +81
Copy link

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Add validation and improve error handling for batch deletions.

While the implementation follows the Typesense API, there are several areas for improvement:

  1. No validation of the filter_by parameter
  2. No handling of partial deletion failures
  3. No progress tracking for large deletions

Consider these improvements:

 # docs: https://typesense.org/docs/27.1/api/documents.html#delete-documents
+@doc """
+Deletes documents matching the given filter query.
+
+## Parameters
+  - collection_name: The name of the collection
+  - filter_by: Filter query in Typesense syntax (e.g., "user_id:=1234")
+  - opts: Keyword list of options
+    - batch_size: Number of documents to delete per batch (default: 100)
+
+## Examples
+    delete_document_by_query("photos", "user_id:=1234")
+    delete_document_by_query("photos", "created_at:<1234567890", batch_size: 50)
+"""
 def delete_document_by_query(collection_name, filter_by, opts \\ []) do
+  unless is_binary(filter_by) and filter_by != "" do
+    raise ArgumentError, "filter_by must be a non-empty string"
+  end
+
   batch_size = Keyword.get(opts, :batch_size, 100)
 
   req = build_request("/collections/#{collection_name}/documents")
-
-  Req.delete(req, params: %{"filter_by" => filter_by, "batch_size" => batch_size})
-  |> handle_response()
+  case Req.delete(req, params: %{"filter_by" => filter_by, "batch_size" => batch_size}) do
+    {:ok, %{status: status} = response} when status in 200..299 ->
+      response
+      |> handle_response()
+      |> Map.get("num_deleted", 0)
+    error ->
+      Logger.error("Batch deletion failed: #{inspect(error)}")
+      handle_response(error)
+  end
 end

For large collections, consider adding progress tracking:

# Add this helper function
defp log_deletion_progress(deleted, total) do
  Logger.info("Deleted #{deleted} out of #{total} documents (#{round(deleted/total * 100)}%)")
end

For very large collections, consider implementing this as a background job with progress tracking and the ability to resume on failure.


def create_search_key() do
{url, _} = get_env()
req = build_request("/keys")
Expand Down
8 changes: 4 additions & 4 deletions lib/small_sdk/web_downloader.ex
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,10 +1,7 @@
defmodule SmallSdk.WebDownloader do
require Logger

# FIXME:TODAY return {:ok, file_name, file_content} | {:error, reason}
def download_files(urls) do
Logger.info("download_files started, urls: #{inspect(urls)}")

res =
urls
|> Enum.map(&download_file/1)
Expand All @@ -13,7 +10,10 @@ defmodule SmallSdk.WebDownloader do
{:error, reason}, _ -> {:halt, {:error, reason}}
end)

{:ok, res}
case res do
{:error, reason} -> {:error, reason}
files -> {:ok, files}
end
end

# TODO: have to handle Stream data
Expand Down
1 change: 1 addition & 0 deletions priv/typesense/2024-10-29_photos_url_to_file_id.ex
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1 +1,2 @@
# mix run priv/typesense/2024-10-29_photos_url_to_file_id.ex
SaveIt.Migration.Typesense.Photo.migrate_photos_20241029!()
Loading