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

BE | Create PuzzlesController#index Endpoint #26

Merged
merged 11 commits into from
Oct 20, 2023
Merged

BE | Create PuzzlesController#index Endpoint #26

merged 11 commits into from
Oct 20, 2023

Conversation

kem247
Copy link
Contributor

@kem247 kem247 commented Oct 18, 2023

Changes:

  • added a route, controller method, and a generated test

This is related to # 3

This closes #

[] I linted my work. (RuboCop)

[] I've updated documentation & README. (if necessary)


(For Fun!) Please include a link to a meme/gif of your feelings about this branch!

Link:

Notes: The user object always validates a zipcode presence, therefore there will never be a user without a zipcode. Either we take out that validation, or frontend creates an error message when someone who is not logged in tries to enter a zipcode.

Didn;t add tests yet due to wanting confirmation on this issue.

Copy link
Contributor

@MelTravelz MelTravelz left a comment

Choose a reason for hiding this comment

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

Hi Kemi!

This is a great start with the basic logic in place. Here is what we need to do before this PR can be merged:

  1. Please pull down main into your branch on your local computer and handle any merge conflicts there.

    • This is because I added the necessary items for deploying to Heroku and CI/CD yesterday...which is why this PR did not pass the tests and cannot be merged
  2. We should schedule a time to meet so we can review what is necessary for creating an API style app. You're work here has all the basic elements but is not complete.

    • To get a head start on this, you could review my work in the PR called BE | Create Users#show Endpoint which will give you a better idea of what needs to be done.

@MelTravelz MelTravelz assigned MelTravelz and kem247 and unassigned MelTravelz Oct 18, 2023
@MelTravelz MelTravelz changed the title Api puzzles BE | Create PuzzlesController#index Endpoint Oct 18, 2023
@kem247
Copy link
Contributor Author

kem247 commented Oct 19, 2023

Hi Kemi!

This is a great start with the basic logic in place. Here is what we need to do before this PR can be merged:

  1. Please pull down main into your branch on your local computer and handle any merge conflicts there.

    • This is because I added the necessary items for deploying to Heroku and CI/CD yesterday...which is why this PR did not pass the tests and cannot be merged
  2. We should schedule a time to meet so we can review what is necessary for creating an API style app. You're work here has all the basic elements but is not complete.

    • To get a head start on this, you could review my work in the PR called BE | Create Faker Data & Users#show Endpoint for BE team to view which will give you a better idea of what needs to be done.

Hey Mel, thanks for the response. I'll take a look at your previous PRs and we can discuss later about the endpoints. It still doesn't answer my concern 'The user object always validates a zipcode presence, therefore there will never be a user without a zipcode. Either we take out that validation, or frontend creates an error message when someone who is not logged in tries to enter a zipcode'. Also, I see that there is no merge conflicts but tests are failing.

@MelTravelz
Copy link
Contributor

MelTravelz commented Oct 19, 2023

  • Yes, we'll get the GitHubt Actions tests to pass today, if you need my help I'm here all day.

  • I hear your concern about the zip_code:

    • When a user is created and saved in our database, we require a user to have a zip_code because that's how we know where their puzzles are located. So this model validation must stay and must be required.
    • And so, any one who is not logged in can still search for puzzles by any zip_code, they will just not have a button to "request/borrow" the puzzle and they will not see the name of the owner of the puzzle.
    • After the log in, a user can return to that page and again search for puzzles by their own zip code or others.

We can discuss this more at our meeting today! : )

puzzles = Puzzle.where(user_id: users.pluck(:id))
render json: PuzzleSerializer.new(puzzles)
elsif puzzles == [] || users = []
render json: { error: "Puzzles not found in this area" }, status: "404"
Copy link
Contributor

Choose a reason for hiding this comment

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

This error handling works for now to meet MVP within our time limitation. But in a refactor we'll want to get this working through the ErrorSerializer. : )


zip_code = params[:zip_code]

users = User.where(zip_code: zip_code) if zip_code.present?
Copy link
Contributor

Choose a reason for hiding this comment

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

This logic should probably be a method in the models and only called on here in the controller. But again, to meet MVP within out time limits let's just roll with it for now. Nice work.

expect(parsed_data[:data][0][:attributes][:notes]).to eq(puzzle_1.notes)
expect(parsed_data[:data][0][:attributes][:puzzle_image_url]).to eq(puzzle_1.puzzle_image_url)

# NOTE Could refactor tests to not see a puzzle_4 in the test because it's in a diff zipcode
Copy link
Contributor

Choose a reason for hiding this comment

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

Excellent note here. And I agree that there should be a test to confirm no "stray" puzzles are found in a call for a specific zip_code.


expect(parsed_error_data).to be_a(Hash)
expect(parsed_error_data.keys).to eq([:error])
expect(parsed_error_data[:error]).to eq("Puzzles not found in this area")
Copy link
Contributor

Choose a reason for hiding this comment

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

And these tests would change once we use the ErrorSerializer in a refactor.

Copy link
Contributor

@MelTravelz MelTravelz left a comment

Choose a reason for hiding this comment

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

This is great work Kemi! I'm impressed with all you've done having only worked with Ruby a little bit in your past. I made some notes for later if we have time for a refactor, but I say let's go ahead and merge this into main!

@MelTravelz MelTravelz merged commit 9b33563 into main Oct 20, 2023
1 check passed
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.

2 participants