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

Issues/issue 113 #150

Merged
merged 4 commits into from
Aug 14, 2018

Conversation

ketangarala
Copy link
Contributor

To Show Selection Data of Product into WishList new property called "ProductShortDescription" Added in WishListItem Class.

Copy link
Member

@WillStrohl WillStrohl left a comment

Choose a reason for hiding this comment

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

@ketangarala Please review the comments in the code and respond.

@@ -112,7 +112,7 @@ private List<SavedItemViewModel> PrepItems(List<WishListItem> items)
foreach (var item in items)
{
var p = HccApp.CatalogServices.Products.FindWithCache(item.ProductId);

item.ProductShortDescription = p.Options.CartDescription(item.SelectionData.OptionSelectionList);
Copy link
Member

Choose a reason for hiding this comment

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

@ketangarala I don't see any defensive coding here for when the product doesn't have any choices/variants selected. Shouldn't you be checking for that, and only executing this line of code when there is relevant selection data?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@hismightiness We checked CartDescription at that time, There is condition to check if option is null then return string.empty. That was the reason we did not put condition p.Hasoption(). I also agree that we should add this condition so that in future if any changes will come in to the core method CartDescription we are fine with this area. I will put condition and generate new pull request.

Copy link
Member

@WillStrohl WillStrohl left a comment

Choose a reason for hiding this comment

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

The update doesn't appear to resolve the issue I reported. :)

@@ -112,7 +112,10 @@ private List<SavedItemViewModel> PrepItems(List<WishListItem> items)
foreach (var item in items)
{
var p = HccApp.CatalogServices.Products.FindWithCache(item.ProductId);
item.ProductShortDescription = p.Options.CartDescription(item.SelectionData.OptionSelectionList);
if (p.HasOptions())
Copy link
Member

Choose a reason for hiding this comment

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

This will always be true if the product has options, regardless to whether any have been selected prior to adding it to the cart or wish list. You'll need to evaluate item.SelectionData.OptionSelectionList instead.

Copy link
Member

Choose a reason for hiding this comment

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

@ketangarala This is still an issue.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As per your Suggestion item.SelectionData.OptionSelectionList is replaced in condition to check nullable and code is Committed , so please check last committed change.

@WillStrohl WillStrohl merged commit 5482db2 into HotcakesCommerce:development Aug 14, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants