Fullstack challenge - Jed Stanton #4

JStant95 · 2024-05-20T14:36:40Z

My Process

Started with challenge two:

I initially cleaned up the index action, removing any n+1s, string injection and using activerecord methods for sort/offset/limit etc rather than ruby.

Having set that up I moved on to creating the index view. Being mindful of time I just did all the styling inline but if I had more time I would've moved them into a scss file.

I then moved on to pagination for reviews for each product. I then created two new partials one which will render the reviews for each product and another which some very basic pagination (list all the pages of the reviews which is calculated by the total number of reviews for each product which i pass in the data instance variable). I added some javascript to send an AJAX request to a new route + action I created, it'll pass the page the user has clicked and product id. The action will then rerender the reviews partial for that particular product. Improvements I would make to this if I had more time would be to improve the UI/UX of pagination partial, currently just a list of all the page numbers rather than what was specific on the design.

I did not have time to add pagination to the product pages, however if I did I probably would've used the kaminari gem.

I did not have time to add tests here but if I did I would test the following.

rspec test for the index action that would data object contained the correct products with the correct associated reviews
cucumber test that would test the reviews pagination, that after selecting a different page the reviews display were adjusted correctly

Screen.Recording.2024-05-20.at.15.37.53.mov

Challenge 1:

Added new route for reviews and then a very basic form to create a new review as I did not have a enough time to style it to match design. As it was a standalone page rather than a button on each product on the index page for the product dropdown rather than listing all product which would have made the dropdown v slow I added a search field for the user to type in to populate the dropdown, once the user had typed at least 5 letters this sent an ajax request to a new route that I had added which then sent the results of the query back to the javascript where I populated the dropdown. I also changed the create action to async call the worker i created to make the reviews plus added validations to the review model to ensure valid reviews were being created.

Again here I unfortunately did not the time to added tests, however had I done:

rspec test for the create worker to ensure it created a review when called
rspec test to the model to test validations
rspec test to the search action on product controller to ensure it returned correct data for different params
happy path end-to-end cuke test for creating a review

Screen.Recording.2024-05-20.at.15.38.40.mov

I also would've tidied up my commits to make them as atomic as possible if I had more time

Initially users weren't able to view any products unless shop_id param was present. In this commit I've also optimised some of the existing queries as well as removed string injection in sql queries.

In order to allow a user to create a new review, I made the following changes: - Add new route to create a review - Add create review work so we can do this asynchronously - Added a new form to create a review - As part of the new review form, I added a new field to allow the user to select a product however in order to improve performance the dropdown only populates when a user types text into search field

o0khoiclub0o · 2024-05-22T14:03:20Z

Are you actually able to run the app without upgrading any dependencies (Ruby, gems, JS packages)? Or did you upgrade some of them but didn't commit?

o0khoiclub0o · 2024-05-22T14:18:59Z

Thanks for the demo videos, they are very helpful!

o0khoiclub0o · 2024-05-22T14:22:03Z

tasks/fullStack/app/views/reviews/new.slim

+  .field
+    = form.label :shop_id, 'Shop'
+    = form.collection_select :shop_id, @shops, :id, :name, prompt: 'Select a Shop'


shop_id field is redundant.

o0khoiclub0o · 2024-05-22T14:25:20Z

tasks/fullStack/app/javascript/packs/application.js

+  const searchInput = document.createElement('input');
+  searchInput.type = 'text';
+  searchInput.placeholder = 'Search for a product';
+  searchInput.id = 'product-search';
+  productSelect.parentNode.insertBefore(searchInput, productSelect);
+
+  let timeout = null;
+
+  searchInput.addEventListener('input', function () {
+    clearTimeout(timeout);
+    timeout = setTimeout(function () {
+      const query = searchInput.value.trim();
+      if (query.length > 4) {
+        fetch(`/products/search?q=${encodeURIComponent(query)}`)
+          .then(response => response.json())
+          .then(data => {
+            productSelect.innerHTML = '<option value="">Select a Product</option>';
+            data.items.forEach(product => {
+              const option = document.createElement('option');
+              option.value = product.id;
+              option.textContent = product.title;
+              productSelect.appendChild(option);
+            });
+          })
+          .catch(error => console.error('Error fetching products:', error));
+      } else {
+        productSelect.innerHTML = '<option value="">Select a Product</option>';
+      }
+    }, 300);
+  });


I'm impressed!

o0khoiclub0o · 2024-05-22T14:35:34Z

tasks/fullStack/app/controllers/reviews_controller.rb

+    products.each do |product|
+      product_reviews_page_data = params[:product_reviews_page_data] || {}
+      page = product_reviews_page_data[product.id.to_s]&.to_i || 1
+      reviews = product.reviews.order(created_at: :desc).limit(REVIEWS_PER_PAGE).offset((page.to_i - 1) * REVIEWS_PER_PAGE)


Why do you sort product reviews by created_at DESC but sort products by created ASC?

I thought it made sense to show the most recently created review first but with the products I thought most recently added gets tagged on to the last page made sense. However thinking about it now they should probably both be DESC

No problem here. I'm just curious about your thoughts.

o0khoiclub0o · 2024-05-22T14:38:31Z

tasks/fullStack/app/controllers/reviews_controller.rb

+    products_query = products_query.where(shop_id: params[:shop_id]) if params[:shop_id].present?
+    products_query = products_query.order(created_at: :asc)
+
+    products = products_query.includes(:reviews).limit(params[:per_page]).offset(offset)


Why do you need .includes(:reviews)?

would there not be a N+1 without it on line 22?

reviews = product.reviews.order(created_at: :desc).limit(REVIEWS_PER_PAGE).offset((page.to_i - 1) * REVIEWS_PER_PAGE)

What is the difference in generated SQL queries between with and without includes(:reviews)?

o0khoiclub0o · 2024-05-22T14:44:48Z

tasks/fullStack/app/controllers/reviews_controller.rb

@@ -41,4 +65,7 @@ def tags_with_default(params)
    default_tags.concat(params[:tags].split(',')).uniq
  end

+  def review_params
+    params.require(:review).permit(:rating, :body, :reviewer_name, :product_id, :shop_id, :tags)


review_params[:tags] is not used anywhere

o0khoiclub0o · 2024-05-22T14:46:46Z

tasks/fullStack/app/controllers/reviews_controller.rb

+    products.each do |product|
+      product_reviews_page_data = params[:product_reviews_page_data] || {}
+      page = product_reviews_page_data[product.id.to_s]&.to_i || 1
+      reviews = product.reviews.order(created_at: :desc).limit(REVIEWS_PER_PAGE).offset((page.to_i - 1) * REVIEWS_PER_PAGE)


Nit: page.to_i is redundant

JStant95 · 2024-05-22T14:49:57Z

Are you actually able to run the app without upgrading any dependencies (Ruby, gems, JS packages)? Or did you upgrade some of them but didn't commit?

did end up having to upgrade a few gems and js packages but didn't end up committing them

o0khoiclub0o · 2024-05-22T14:55:07Z

tasks/fullStack/app/javascript/packs/application.js

+  })
+    .then(response => response.text())
+    .then(data => {
+      eval(data);


This AJAX way is not recommended but it works, so still fine to me.

o0khoiclub0o · 2024-05-22T15:03:59Z

Are you actually able to run the app without upgrading any dependencies (Ruby, gems, JS packages)? Or did you upgrade some of them but didn't commit?

did end up having to upgrade a few gems and js packages but didn't end up committing them

Thanks as I'm not distracted by a big PR.

Jed Stanton added 7 commits May 20, 2024 14:14

Optimize product query and add support for fetching without shop_id

160335b

Initially users weren't able to view any products unless shop_id param was present. In this commit I've also optimised some of the existing queries as well as removed string injection in sql queries.

Display products and corresponding reviews on review index page

3c5ceac

Use Unicode star characters to display review rating

722c507

Display how long ago the review was created next to rating

ad36ca5

Display reviewers name above review

e71feb8

Add pagination to reviews

d8398a0

notABluesSinger added the Full Stack label May 20, 2024

o0khoiclub0o reviewed May 22, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fullstack challenge - Jed Stanton #4

Fullstack challenge - Jed Stanton #4

JStant95 commented May 20, 2024 •

edited

Loading

o0khoiclub0o commented May 22, 2024

o0khoiclub0o commented May 22, 2024

o0khoiclub0o May 22, 2024

o0khoiclub0o May 22, 2024 •

edited

Loading

o0khoiclub0o May 22, 2024

JStant95 May 22, 2024

o0khoiclub0o May 22, 2024

o0khoiclub0o May 22, 2024

JStant95 May 22, 2024

o0khoiclub0o May 22, 2024

o0khoiclub0o May 22, 2024

o0khoiclub0o May 22, 2024

JStant95 commented May 22, 2024

o0khoiclub0o May 22, 2024

o0khoiclub0o commented May 22, 2024

Fullstack challenge - Jed Stanton #4

Are you sure you want to change the base?

Fullstack challenge - Jed Stanton #4

Conversation

JStant95 commented May 20, 2024 • edited Loading

o0khoiclub0o commented May 22, 2024

o0khoiclub0o commented May 22, 2024

Choose a reason for hiding this comment

o0khoiclub0o May 22, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JStant95 commented May 22, 2024

Choose a reason for hiding this comment

o0khoiclub0o commented May 22, 2024

JStant95 commented May 20, 2024 •

edited

Loading

o0khoiclub0o May 22, 2024 •

edited

Loading