refactor: Cache Item Reviews and other review feedback
- `get_doc` -> `get_values` and `db.sql` -> `db.delete` in Wishlist Item deletion
- cache first page of Item Reviews and burst cache on addition and deletion of reviews
- Update redisearch docs link in E Commerce Settings
- Removed unused cint import
- Broke setting attribute context into smaller functions and code cleanup
- Minor recommended items padding tweak
- Item reviews form dict now uses website item as key
- Customer reviews rendered from UI style consistency
- Stock status consistency in listing and full page
- Handle no price in variant dialog for matched item
diff --git a/erpnext/e_commerce/doctype/e_commerce_settings/e_commerce_settings.json b/erpnext/e_commerce/doctype/e_commerce_settings/e_commerce_settings.json
index abc1c4e..fc2a565 100644
--- a/erpnext/e_commerce/doctype/e_commerce_settings/e_commerce_settings.json
+++ b/erpnext/e_commerce/doctype/e_commerce_settings/e_commerce_settings.json
@@ -329,7 +329,7 @@
"fieldname": "redisearch_warning",
"fieldtype": "HTML",
"label": "Redisearch Warning",
- "options": "<p class=\"alert alert-warning\">Redisearch module not loaded. If you want to use advanced product search features, refer documentation <a class=\"alert-link\" href=\"https://frappeframework.com/docs/user/en/installation\" target=\"_blank\">here</a>.</p>"
+ "options": "<p class=\"alert alert-warning\">Redisearch is not loaded. If you want to use the advanced product search feature, refer <a class=\"alert-link\" href=\"https://docs.erpnext.com/docs/v13/user/manual/en/setting-up/articles/installing-redisearch\" target=\"_blank\">here</a>.</p>"
},
{
"default": "0",
@@ -379,7 +379,7 @@
"index_web_pages_for_search": 1,
"issingle": 1,
"links": [],
- "modified": "2021-08-24 13:40:15.294696",
+ "modified": "2021-08-24 21:10:45.669526",
"modified_by": "Administrator",
"module": "E-commerce",
"name": "E Commerce Settings",
diff --git a/erpnext/e_commerce/doctype/e_commerce_settings/e_commerce_settings.py b/erpnext/e_commerce/doctype/e_commerce_settings/e_commerce_settings.py
index 1e54146..cade54d 100644
--- a/erpnext/e_commerce/doctype/e_commerce_settings/e_commerce_settings.py
+++ b/erpnext/e_commerce/doctype/e_commerce_settings/e_commerce_settings.py
@@ -3,7 +3,7 @@
# For license information, please see license.txt
import frappe
-from frappe.utils import cint, comma_and
+from frappe.utils import comma_and
from frappe import _, msgprint
from frappe.model.document import Document
from frappe.utils import unique
diff --git a/erpnext/e_commerce/doctype/item_review/item_review.py b/erpnext/e_commerce/doctype/item_review/item_review.py
index b0d8beb..3f34c4a 100644
--- a/erpnext/e_commerce/doctype/item_review/item_review.py
+++ b/erpnext/e_commerce/doctype/item_review/item_review.py
@@ -15,44 +15,94 @@
pass
class ItemReview(Document):
- pass
+ def after_insert(self):
+ # regenerate cache on review creation
+ reviews_dict = get_queried_reviews(self.website_item)
+ set_reviews_in_cache(self.website_item, reviews_dict)
+
+ def after_delete(self):
+ # regenerate cache on review deletion
+ reviews_dict = get_queried_reviews(self.website_item)
+ set_reviews_in_cache(self.website_item, reviews_dict)
+
@frappe.whitelist()
-def get_item_reviews(web_item, start, end, data=None):
+def get_item_reviews(web_item, start=0, end=10, data=None):
+ "Get Website Item Review Data."
+ start, end = cint(start), cint(end)
+ settings = get_shopping_cart_settings()
+
+ # Get cached reviews for first page (start=0)
+ # avoid cache when page is different
+ from_cache = not bool(start)
+
if not data:
data = frappe._dict()
- settings = get_shopping_cart_settings()
-
if settings and settings.get("enable_reviews"):
- data.reviews = frappe.db.get_all("Item Review", filters={"website_item": web_item},
- fields=["*"], limit_start=cint(start), limit_page_length=cint(end))
+ reviews_cache = frappe.cache().hget("item_reviews", web_item)
+ if from_cache and reviews_cache:
+ data = reviews_cache
+ else:
+ data = get_queried_reviews(web_item, start, end, data)
+ if from_cache:
+ set_reviews_in_cache(web_item, data)
- rating_data = frappe.db.get_all("Item Review", filters={"website_item": web_item},
- fields=["avg(rating) as average, count(*) as total"])[0]
- data.average_rating = flt(rating_data.average, 1)
- data.average_whole_rating = flt(data.average_rating, 0)
+ return data
- # get % of reviews per rating
- reviews_per_rating = []
- for i in range(1,6):
- count = frappe.db.get_all("Item Review", filters={"website_item": web_item, "rating": i},
- fields=["count(*) as count"])[0].count
+def get_queried_reviews(web_item, start=0, end=10, data=None):
+ """
+ Query Website Item wise reviews and cache if needed.
+ Cache stores only first page of reviews i.e. 10 reviews maximum.
+ Returns:
+ dict: Containing reviews, average ratings, % of reviews per rating and total reviews.
+ """
+ if not data:
+ data = frappe._dict()
- percent = flt((count / rating_data.total or 1) * 100, 0) if count else 0
- reviews_per_rating.append(percent)
+ data.reviews = frappe.db.get_all(
+ "Item Review",
+ filters={"website_item": web_item},
+ fields=["*"],
+ limit_start=start,
+ limit_page_length=end
+ )
- data.reviews_per_rating = reviews_per_rating
- data.total_reviews = rating_data.total
+ rating_data = frappe.db.get_all(
+ "Item Review",
+ filters={"website_item": web_item},
+ fields=["avg(rating) as average, count(*) as total"]
+ )[0]
- return data
+ data.average_rating = flt(rating_data.average, 1)
+ data.average_whole_rating = flt(data.average_rating, 0)
+
+ # get % of reviews per rating
+ reviews_per_rating = []
+ for i in range(1,6):
+ count = frappe.db.get_all(
+ "Item Review",
+ filters={"website_item": web_item, "rating": i},
+ fields=["count(*) as count"]
+ )[0].count
+
+ percent = flt((count / rating_data.total or 1) * 100, 0) if count else 0
+ reviews_per_rating.append(percent)
+
+ data.reviews_per_rating = reviews_per_rating
+ data.total_reviews = rating_data.total
+
+ return data
+
+def set_reviews_in_cache(web_item, reviews_dict):
+ frappe.cache().hset("item_reviews", web_item, reviews_dict)
@frappe.whitelist()
def add_item_review(web_item, title, rating, comment=None):
""" Add an Item Review by a user if non-existent. """
if frappe.session.user == "Guest":
- frappe.throw(_("You are not verified to write a review yet. Please contact us for verification."),
- exc=UnverifiedReviewer)
+ # guest user should not reach here ideally in the case they do via an API, throw error
+ frappe.throw(_("You are not verified to write a review yet."), exc=UnverifiedReviewer)
if not frappe.db.exists("Item Review", {"user": frappe.session.user, "website_item": web_item}):
doc = frappe.get_doc({
@@ -88,5 +138,6 @@
elif silent:
return None
else:
- frappe.throw(_("You are not verified to write a review yet. Please contact us for verification."),
- exc=UnverifiedReviewer)
\ No newline at end of file
+ # should not reach here unless via an API
+ frappe.throw(_("You are not a verified customer yet. Please contact us to proceed."),
+ exc=UnverifiedReviewer)
diff --git a/erpnext/e_commerce/doctype/website_item/website_item.py b/erpnext/e_commerce/doctype/website_item/website_item.py
index 4216126..4f3f220 100644
--- a/erpnext/e_commerce/doctype/website_item/website_item.py
+++ b/erpnext/e_commerce/doctype/website_item/website_item.py
@@ -204,7 +204,9 @@
self.get_product_details_section(context)
if settings.enable_reviews:
- get_item_reviews(self.name, 0, 4, context)
+ reviews_data = get_item_reviews(self.name)
+ context.update(reviews_data)
+ context.reviews = context.reviews[:4]
context.wished = False
if frappe.db.exists("Wishlist Item", {"item_code": self.item_code, "parent": frappe.session.user}):
@@ -219,32 +221,38 @@
return context
def set_variant_context(self, context):
- if self.has_variants:
- context.no_cache = True
+ if not self.has_variants:
+ return
- # load variants
- # also used in set_attribute_context
- context.variants = frappe.get_all(
- "Item",
- filters={"variant_of": self.item_code, "published_in_website": 1},
- order_by="name asc")
+ context.no_cache = True
+ variant = frappe.form_dict.variant
- variant = frappe.form_dict.variant
- if not variant and context.variants:
- # the case when the item is opened for the first time from its list
- variant = context.variants[0]
+ # load variants
+ # also used in set_attribute_context
+ context.variants = frappe.get_all(
+ "Item",
+ filters={
+ "variant_of": self.item_code,
+ "published_in_website": 1
+ },
+ order_by="name asc")
- if variant:
- context.variant = frappe.get_doc("Item", variant)
+ # the case when the item is opened for the first time from its list
+ if not variant and context.variants:
+ variant = context.variants[0]
- for fieldname in ("website_image", "website_image_alt", "web_long_description", "description",
- "website_specifications"):
- if context.variant.get(fieldname):
- value = context.variant.get(fieldname)
- if isinstance(value, list):
- value = [d.as_dict() for d in value]
+ if variant:
+ context.variant = frappe.get_doc("Item", variant)
+ fields = ("website_image", "website_image_alt", "web_long_description", "description",
+ "website_specifications")
- context[fieldname] = value
+ for fieldname in fields:
+ if context.variant.get(fieldname):
+ value = context.variant.get(fieldname)
+ if isinstance(value, list):
+ value = [d.as_dict() for d in value]
+
+ context[fieldname] = value
if self.slideshow:
if context.variant and context.variant.slideshow:
@@ -253,48 +261,57 @@
context.update(get_slideshow(self))
def set_attribute_context(self, context):
- if self.has_variants:
- attribute_values_available = {}
- context.attribute_values = {}
- context.selected_attributes = {}
+ if not self.has_variants:
+ return
- # load attributes
- for v in context.variants:
- v.attributes = frappe.get_all("Item Variant Attribute",
- fields=["attribute", "attribute_value"],
- filters={"parent": v.name})
- # make a map for easier access in templates
- v.attribute_map = frappe._dict({})
- for attr in v.attributes:
- v.attribute_map[attr.attribute] = attr.attribute_value
+ attribute_values_available = {}
+ context.attribute_values = {}
+ context.selected_attributes = {}
- for attr in v.attributes:
- values = attribute_values_available.setdefault(attr.attribute, [])
- if attr.attribute_value not in values:
- values.append(attr.attribute_value)
+ # load attributes
+ self.set_selected_attributes(context.variants, context, attribute_values_available)
- if v.name == context.variant.name:
- context.selected_attributes[attr.attribute] = attr.attribute_value
+ # filter attributes, order based on attribute table
+ item = frappe.get_cached_doc("Item", self.item_code)
+ self.set_attribute_values(item.attributes, context, attribute_values_available)
- # filter attributes, order based on attribute table
- item = frappe.get_cached_doc("Item", self.item_code)
- for attr in item.attributes:
- values = context.attribute_values.setdefault(attr.attribute, [])
+ context.variant_info = json.dumps(context.variants)
- if cint(frappe.db.get_value("Item Attribute", attr.attribute, "numeric_values")):
- for val in sorted(attribute_values_available.get(attr.attribute, []), key=flt):
- values.append(val)
+ def set_selected_attributes(self, variants, context, attribute_values_available):
+ for variant in variants:
+ variant.attributes = frappe.get_all(
+ "Item Variant Attribute",
+ filters={"parent": variant.name},
+ fields=["attribute", "attribute_value as value"])
- else:
- # get list of values defined (for sequence)
- for attr_value in frappe.db.get_all("Item Attribute Value",
- fields=["attribute_value"],
- filters={"parent": attr.attribute}, order_by="idx asc"):
+ # make an attribute-value map for easier access in templates
+ variant.attribute_map = frappe._dict(
+ { attr.attribute : attr.value for attr in variant.attributes}
+ )
- if attr_value.attribute_value in attribute_values_available.get(attr.attribute, []):
- values.append(attr_value.attribute_value)
+ for attr in variant.attributes:
+ values = attribute_values_available.setdefault(attr.attribute, [])
+ if attr.value not in values:
+ values.append(attr.value)
- context.variant_info = json.dumps(context.variants)
+ if variant.name == context.variant.name:
+ context.selected_attributes[attr.attribute] = attr.value
+
+ def set_attribute_values(self, attributes, context, attribute_values_available):
+ for attr in attributes:
+ values = context.attribute_values.setdefault(attr.attribute, [])
+
+ if cint(frappe.db.get_value("Item Attribute", attr.attribute, "numeric_values")):
+ for val in sorted(attribute_values_available.get(attr.attribute, []), key=flt):
+ values.append(val)
+ else:
+ # get list of values defined (for sequence)
+ for attr_value in frappe.db.get_all("Item Attribute Value",
+ fields=["attribute_value"],
+ filters={"parent": attr.attribute}, order_by="idx asc"):
+
+ if attr_value.attribute_value in attribute_values_available.get(attr.attribute, []):
+ values.append(attr_value.attribute_value)
def set_disabled_attributes(self, context):
"""Disable selection options of attribute combinations that do not result in a variant"""
diff --git a/erpnext/e_commerce/doctype/wishlist/wishlist.py b/erpnext/e_commerce/doctype/wishlist/wishlist.py
index 40b5ad9..276ecae 100644
--- a/erpnext/e_commerce/doctype/wishlist/wishlist.py
+++ b/erpnext/e_commerce/doctype/wishlist/wishlist.py
@@ -50,16 +50,19 @@
@frappe.whitelist()
def remove_from_wishlist(item_code):
if frappe.db.exists("Wishlist Item", {"item_code": item_code, "parent": frappe.session.user}):
- frappe.db.sql("""
- DELETE
- FROM `tabWishlist Item`
- WHERE
- item_code=%(item_code)s
- and parent='%(user)s'
- """ % {"item_code": frappe.db.escape(item_code), "user": frappe.session.user})
-
+ frappe.db.delete(
+ "Wishlist Item",
+ {
+ "item_code": item_code,
+ "parent": frappe.session.user
+ }
+ )
frappe.db.commit()
- wishlist = frappe.get_doc("Wishlist", frappe.session.user)
+ wishlist_items = frappe.db.get_values(
+ "Wishlist Item",
+ filters={"parent": frappe.session.user}
+ )
+
if hasattr(frappe.local, "cookie_manager"):
- frappe.local.cookie_manager.set_cookie("wish_count", str(len(wishlist.items)))
\ No newline at end of file
+ frappe.local.cookie_manager.set_cookie("wish_count", str(len(wishlist_items)))
\ No newline at end of file
diff --git a/erpnext/e_commerce/product_data_engine/query.py b/erpnext/e_commerce/product_data_engine/query.py
index 5c67272..0ac9090 100644
--- a/erpnext/e_commerce/product_data_engine/query.py
+++ b/erpnext/e_commerce/product_data_engine/query.py
@@ -2,9 +2,10 @@
# License: GNU General Public License v3. See license.txt
import frappe
+from frappe.utils import flt
from erpnext.e_commerce.shopping_cart.product_info import get_product_info_for_website
from erpnext.e_commerce.doctype.item_review.item_review import get_customer
-from frappe.utils import flt
+from erpnext.utilities.product import get_non_stock_item_status
from erpnext.e_commerce.shopping_cart.product_info import get_product_info_for_website
@@ -235,12 +236,22 @@
def get_stock_availability(self, item):
"""Modify item object and add stock details."""
item.in_stock = False
+ warehouse = item.get("website_warehouse")
+ is_stock_item = frappe.get_cached_value("Item", item.item_code, "is_stock_item")
- if item.get("website_warehouse"):
- stock_qty = frappe.utils.flt(
- frappe.db.get_value("Bin", {"item_code": item.item_code, "warehouse": item.get("website_warehouse")},
- "actual_qty"))
- item.in_stock = bool(stock_qty)
+ if not is_stock_item:
+ if warehouse:
+ # product bundle case
+ item.in_stock = get_non_stock_item_status(item.item_code, "website_warehouse")
+ else:
+ item.in_stock = True
+ elif warehouse:
+ # stock item and has warehouse
+ actual_qty = frappe.db.get_value(
+ "Bin",
+ {"item_code": item.item_code,"warehouse": item.get("website_warehouse")},
+ "actual_qty")
+ item.in_stock = bool(flt(actual_qty))
def get_cart_items(self):
customer = get_customer(silent=True)
diff --git a/erpnext/e_commerce/shopping_cart/product_info.py b/erpnext/e_commerce/shopping_cart/product_info.py
index c66abba..3ebf6d2 100644
--- a/erpnext/e_commerce/shopping_cart/product_info.py
+++ b/erpnext/e_commerce/shopping_cart/product_info.py
@@ -24,7 +24,7 @@
selling_price_list = cart_quotation.get("selling_price_list") if cart_quotation else _set_price_list(cart_settings, None)
- price = []
+ price = {}
if cart_settings.show_price:
is_guest = frappe.session.user == "Guest"
# Show Price if logged in.
diff --git a/erpnext/public/scss/shopping_cart.scss b/erpnext/public/scss/shopping_cart.scss
index 7753378..8648cdb 100644
--- a/erpnext/public/scss/shopping_cart.scss
+++ b/erpnext/public/scss/shopping_cart.scss
@@ -188,13 +188,6 @@
font-weight: 600;
}
- .out-of-stock {
- font-weight: 500;
- font-size: 14px;
- line-height: 20px;
- color: #F47A7A;
- }
-
.item-card {
padding: var(--padding-sm);
min-width: 300px;
@@ -450,6 +443,10 @@
.r-item-image {
width: 40%;
+ .product-image {
+ padding: 2px 15px;
+ }
+
.no-image-r-item {
display: flex; justify-content: center;
background-color: var(--gray-200);
@@ -464,7 +461,6 @@
.r-item-info {
font-size: 14px;
- padding-left: 8px;
padding-right: 0;
width: 60%;
@@ -1322,6 +1318,13 @@
font-weight: 500;
}
+.out-of-stock {
+ font-weight: 500;
+ font-size: 14px;
+ line-height: 20px;
+ color: #F47A7A;
+}
+
.mt-minus-2 {
margin-top: -2rem;
}
diff --git a/erpnext/templates/generators/item/item_add_to_cart.html b/erpnext/templates/generators/item/item_add_to_cart.html
index d52168e..399240b 100644
--- a/erpnext/templates/generators/item/item_add_to_cart.html
+++ b/erpnext/templates/generators/item/item_add_to_cart.html
@@ -35,8 +35,8 @@
{% if cart_settings.show_stock_availability %}
<div class="mt-2">
{% if product_info.in_stock == 0 %}
- <span class="text-danger no-stock">
- {{ _('Not in stock') }}
+ <span class="no-stock out-of-stock">
+ {{ _('Out of stock') }}
</span>
{% elif product_info.in_stock == 1 %}
<span class="in-green has-stock">
diff --git a/erpnext/templates/generators/item/item_configure.js b/erpnext/templates/generators/item/item_configure.js
index 3220226..10cd416 100644
--- a/erpnext/templates/generators/item/item_configure.js
+++ b/erpnext/templates/generators/item/item_configure.js
@@ -214,7 +214,7 @@
? `<div class="alert alert-success d-flex justify-content-between align-items-center" role="alert">
<div><div>
${one_item}
- ${product_info && product_info.price
+ ${product_info && product_info.price && !$.isEmptyObject()
? '(' + product_info.price.formatted_price_sales_uom + ')'
: ''
}
diff --git a/erpnext/templates/generators/item/item_reviews.html b/erpnext/templates/generators/item/item_reviews.html
index 508dbce..c62c6f7 100644
--- a/erpnext/templates/generators/item/item_reviews.html
+++ b/erpnext/templates/generators/item/item_reviews.html
@@ -29,7 +29,7 @@
{% if total_reviews > 4 %}
<div class="mt-6 mb-6"style="color: var(--primary);">
- <a href="/customer_reviews?item_code={{ doc.item_code }}">{{ _("View all reviews") }}</a>
+ <a href="/customer_reviews?web_item={{ doc.name }}">{{ _("View all reviews") }}</a>
</div>
{% endif %}
diff --git a/erpnext/templates/pages/customer_reviews.js b/erpnext/templates/pages/customer_reviews.js
index 9be12c7..e13ded6 100644
--- a/erpnext/templates/pages/customer_reviews.js
+++ b/erpnext/templates/pages/customer_reviews.js
@@ -96,23 +96,25 @@
reviews.forEach((review) => {
$content.append(`
<div class="mb-3 review">
- <div style="display: flex;">
+ <div class="d-flex">
+ <p class="mr-4 user-review-title">
+ <span>${__(review.review_title)}</span>
+ </p>
<div class="rating">
${me.get_review_stars(review.rating)}
</div>
- <p class="ml-4 user-review-title">
- <span>${__(review.review_title)}</span>
- </p>
</div>
- <div class="review-signature">
- <span class="reviewer">${__(review.customer)}</span>
- <span>${__(review.published_on)}</span>
- </div>
- <div class="product-description mb-4 mt-4">
+
+ <div class="product-description mb-4">
<p>
${__(review.comment)}
</p>
</div>
+ <div class="review-signature mb-2">
+ <span class="reviewer">${__(review.customer)}</span>
+ <span class="indicator grey" style="--text-on-gray: var(--gray-300);"></span>
+ <span class="reviewer">${__(review.published_on)}</span>
+ </div>
</div>
`);
});
@@ -122,9 +124,11 @@
let stars = ``;
for (let i = 1; i < 6; i++) {
let fill_class = i <= rating ? 'star-click' : '';
- stars += `<svg class="icon icon-md ${fill_class}">
+ stars += `
+ <svg class="icon icon-sm ${fill_class}">
<use href="#icon-star"></use>
- </svg>`;
+ </svg>
+ `;
}
return stars;
}
diff --git a/erpnext/templates/pages/customer_reviews.py b/erpnext/templates/pages/customer_reviews.py
index 696ecea..02c03d3 100644
--- a/erpnext/templates/pages/customer_reviews.py
+++ b/erpnext/templates/pages/customer_reviews.py
@@ -10,10 +10,10 @@
context.full_page = True
context.reviews = None
- if frappe.form_dict and frappe.form_dict.get("item_code"):
- context.item_code = frappe.form_dict.get("item_code")
- context.web_item = frappe.db.get_value("Website Item", {"item_code": context.item_code}, "name")
+ if frappe.form_dict and frappe.form_dict.get("web_item"):
+ context.web_item = frappe.form_dict.get("web_item")
context.user_is_customer = check_if_user_is_customer()
context.enable_reviews = get_shopping_cart_settings().enable_reviews
if context.enable_reviews:
- get_item_reviews(context.web_item, 0, 10, context)
+ reviews_data = get_item_reviews(context.web_item)
+ context.update(reviews_data)
diff --git a/erpnext/utilities/product.py b/erpnext/utilities/product.py
index cbb430f..6b514b6 100644
--- a/erpnext/utilities/product.py
+++ b/erpnext/utilities/product.py
@@ -139,7 +139,7 @@
# if item is a product bundle, check if its bundle items are in stock
if frappe.db.exists("Product Bundle", item_code):
items = frappe.get_doc("Product Bundle", item_code).get_all_children()
- bundle_warehouse = frappe.db.get_value('Item', item_code, item_warehouse_field)
- return all([get_web_item_qty_in_stock(d.item_code, item_warehouse_field, bundle_warehouse).in_stock for d in items])
+ bundle_warehouse = frappe.db.get_value("Website Item", {"item_code": item_code}, item_warehouse_field)
+ return all(get_web_item_qty_in_stock(d.item_code, item_warehouse_field, bundle_warehouse).in_stock for d in items)
else:
return 1