medusa-project / book-tracker

Medusa Book Tracker
0 stars 0 forks source link

SQS message sent to queue when book record changed #46

Closed gaurijo closed 10 months ago

gaurijo commented 10 months ago

This PR is for issue 7. This was tested in development mode similarly to how the service check automation was tested in development.

Summary of Changes:

Screenshots below show how the sqs message is triggered after a service check runs:

Screenshot 2024-01-02 at 2 14 29 PM Screenshot 2024-01-02 at 2 57 44 PM Screenshot 2024-01-02 at 2 08 25 PM

Note: I had written a test for the sending of the sqs message, which did pass locally, but threw an error in GitHub Actions. I'm working on figuring out how exactly to resolve it (at first I thought it was because the edited credentials ymlhadn't been staged for commit, but that didn't change the error), so I commented the test out for now:

book-tracker-test_1  | Error:
book-tracker-test_1  | RecordSourceTest#test_sends_a_message_sqs:
book-tracker-test_1  | Aws::Errors::MissingCredentialsError: unable to sign request without credentials set
book-tracker-test_1  |     test/models/record_source_test.rb:11:in `block in <class:RecordSourceTest>'
book-tracker-test_1  | 
book-tracker-test_1  | bin/rails test test/models/record_source_test.rb:6
adolski commented 10 months ago

This is a good first stab. What we want to do, though, instead of sending a summary message at the end of the import, is send one message per imported book.

I can see how this might have been awkward to do based on how the code was designed, so I've pushed a commit to develop that should make it easier. It redesigns Book.params_from_marcxml_record() as Book.from_marcxml_record() to return a Book instance instead of a hash. That way, the argument that is passed to RecordSource.upsert() is an array of Book objects, rather than an array of hashes containing book properties.

So the changes needed could be something like this:

In Book:

##
# @return [Hash]
#
def as_message()
  # I guess we don't know exactly what we want the contents
  # of the message to be yet, but for starters, we could just
  # have this wrap as_json().
end

##
# @param message [Hash] Hash returned from {as_message}.
#
def send_message(message)
  # your message-sending code goes here
end

In RecordSource:

def upsert(batch)
  Rails.logger.debug("RecordSource.upsert(): upserting #{batch.length} records")
  Book.bulk_upsert(batch)
  # new code to send messages
  batch.each do |book|
    message = book.as_message
    book.send_message(message)
  end
ensure
  batch.clear
end

Does this make sense?

gaurijo commented 10 months ago

Ohhh, I didn't realize we wanted the message sent after each record updated and not a summary of updated changes. What you laid out makes sense, thank you. I'll let you know if I run into any blockers.

gaurijo commented 10 months ago

I just pushed up some more commits based on your suggested changes. I ran an import in development (which ran super slow) to trigger the Book model upsertmethod. Here is how the SQS message looks like now with these changes:

Screenshot 2024-01-03 at 2 01 50 PM Screenshot 2024-01-03 at 2 06 43 PM

I included a test inside book_test.rb for the message sending action, and while it does pass locally, GitHub Actions keeps throwing an error due to a missing aws credential. I commented the test out while I figure out why it's doing that.

gaurijo commented 10 months ago

I made the suggested changes. Here is how a SQS message should look now when a record is changed:

Screenshot 2024-01-04 at 2 39 39 PM
gaurijo commented 10 months ago

Ok, thanks. I updated the send_message() method to now call on the configuration for both region and queue_name. I added those to the template.yml and also edited the demo credentials with the same, assuming that was necessary. I left the prod credentials alone for now.

gaurijo commented 10 months ago

So I tried implementing some mocking to see if I could test the send_messagemethod, but unfortunately the test keeps failing in GitHub Actions. I'm no longer getting a missing aws credentials error, but now the error is more vague (a missing '[]' for nil class error). If you notice anything off the bat in my code that needs to be fixed I can quickly implement it, but if it seems like it's not worth the hassle of getting this particular test to work, I'll just remove the latest commits and we can merge without this test.

gaurijo commented 10 months ago

Final update - I realized I never updated the ci.yml config with the credentials for the sqs queue. Now all the tests are passing in Github Actions, so this should be ready for merging. Let me know if there is anything else that should be looked at here.

adolski commented 10 months ago

@gaurijo Have you edited the demo Book Tracker's terraform script to create the demo SQS queue, and does the queue name have the word "demo" in it somewhere? We will need to have separate queues for demo and production.

gaurijo commented 10 months ago

@gaurijo Have you edited the demo Book Tracker's terraform script to create the demo SQS queue, and does the queue name have the word "demo" in it somewhere? We will need to have separate queues for demo and production.

Yes, that's been created here. Once this is merged I was going to work on setting up the production queue.