I want to maximize the chance that vendor reviews makes it into a future release as well as create less work for myself later by getting the communities input and guidance early on. I created a generic interface based on product reviews that I applied to the vendor reviews. This required that I create a vendorReviews table akin to the product reviews table and add the corresponding reviews and rating fields to the vendors table.
Currently only VendorReviews makes full use of the interfaces and the ProductReviews code is left largely untouched.
Ultimately I foresee creating a reviews base class to handle common review functionality.
Should I go ahead and make the common base class and in the process change the product reviews code feature now? Or should I wait until the vendorReviews code is released and proven before I create the common reviews base class that would involve editing the product reviews code for release in a subsequent version.
both ways are fully functional.
Vote for this feature below please.
vote here
Vendor Reviews
- 79
- 1091
- 5/5/2010
- Spain
I've looked at the pull request's changes and I have some suggestions:
- I don't see the need of having different Review implementations for each reviewable entity. ProductReview, VendorReview, ManufacturerReview they can all be the same object and stored in the same table. Even if a different class is desired, I would use inheritance and store them all in the same table anyway, as they all have the same information.
- I would remove the naviation property from Review to the reviewable entity (the Vendor property in VendorReview). Naviation from review to vendor wouldn't be a good practice. As it should never be used, it's best if it's not there.
- As composition is better than inheritance, I wonder if it would be better to put the IReviewable properties in a separate entity instead of directly on the Vendor, Product... These entities are already very big and over time they get more and more properties added. To me, it would be better if some things were moved out of there with one to one associations to other tables. This way if someone doesn't use all these features, the entities and db stay very light.
- Use honeypot instead of captcha
That said, the current solution looks good as it mainly follows nop standards, but I think these would be improvements.
- I don't see the need of having different Review implementations for each reviewable entity. ProductReview, VendorReview, ManufacturerReview they can all be the same object and stored in the same table. Even if a different class is desired, I would use inheritance and store them all in the same table anyway, as they all have the same information.
- I would remove the naviation property from Review to the reviewable entity (the Vendor property in VendorReview). Naviation from review to vendor wouldn't be a good practice. As it should never be used, it's best if it's not there.
- As composition is better than inheritance, I wonder if it would be better to put the IReviewable properties in a separate entity instead of directly on the Vendor, Product... These entities are already very big and over time they get more and more properties added. To me, it would be better if some things were moved out of there with one to one associations to other tables. This way if someone doesn't use all these features, the entities and db stay very light.
- Use honeypot instead of captcha
That said, the current solution looks good as it mainly follows nop standards, but I think these would be improvements.
- 8
- 100
- 3/13/2015
- United States
fcastells wrote:
I think your suggestions are definitely best practices and that we should ultimately head in that direction. With this being my first nop contribution I wanted to maintain as light a footprint as possible and did not want to risk breaking the product reviews logic by changing it, while still being able to copy most of the tried and true product review logic into the vendor reviews logic. Before making the changes above and adding some additional supporting features I would like to request that this feature gets merged as is so that I know the feature is important enough to the community to spend additional time on.
I've looked at the pull request's changes and I have some suggestions:
- I don't see the need of having different Review implementations for each reviewable entity. ProductReview, VendorReview, ManufacturerReview they can all be the same object and stored in the same table. Even if a different class is desired, I would use inheritance and store them all in the same table anyway, as they all have the same information.
- I would remove the naviation property from Review to the reviewable entity (the Vendor property in VendorReview). Naviation from review to vendor wouldn't be a good practice. As it should never be used, it's best if it's not there.
- As composition is better than inheritance, I wonder if it would be better to put the IReviewable properties in a separate entity instead of directly on the Vendor, Product... These entities are already very big and over time they get more and more properties added. To me, it would be better if some things were moved out of there with one to one associations to other tables. This way if someone doesn't use all these features, the entities and db stay very light.
- Use honeypot instead of captcha
That said, the current solution looks good as it mainly follows nop standards, but I think these would be improvements.
- I don't see the need of having different Review implementations for each reviewable entity. ProductReview, VendorReview, ManufacturerReview they can all be the same object and stored in the same table. Even if a different class is desired, I would use inheritance and store them all in the same table anyway, as they all have the same information.
- I would remove the naviation property from Review to the reviewable entity (the Vendor property in VendorReview). Naviation from review to vendor wouldn't be a good practice. As it should never be used, it's best if it's not there.
- As composition is better than inheritance, I wonder if it would be better to put the IReviewable properties in a separate entity instead of directly on the Vendor, Product... These entities are already very big and over time they get more and more properties added. To me, it would be better if some things were moved out of there with one to one associations to other tables. This way if someone doesn't use all these features, the entities and db stay very light.
- Use honeypot instead of captcha
That said, the current solution looks good as it mainly follows nop standards, but I think these would be improvements.
I think your suggestions are definitely best practices and that we should ultimately head in that direction. With this being my first nop contribution I wanted to maintain as light a footprint as possible and did not want to risk breaking the product reviews logic by changing it, while still being able to copy most of the tried and true product review logic into the vendor reviews logic. Before making the changes above and adding some additional supporting features I would like to request that this feature gets merged as is so that I know the feature is important enough to the community to spend additional time on.
- 79
- 1091
- 5/5/2010
- Spain
BinaryBrains wrote:
I makes sense to me. I hope they accept the pull request!
Thanks
I think your suggestions are definitely best practices and that we should ultimately head in that direction. With this being my first nop contribution I wanted to maintain as light a footprint as possible and did not want to risk breaking the product reviews logic by changing it, while still being able to copy most of the tried and true product review logic into the vendor reviews logic. Before making the changes above and adding some additional supporting features I would like to request that this feature gets merged as is so that I know the feature is important enough to the community to spend additional time on.
I makes sense to me. I hope they accept the pull request!
Thanks
- 1
- 5
- 3/17/2015
- India
Hi , In frontend registration page I have added another field name as ' Registration Type' that contains radio buttons 'Buyer' and Vendor' . This new field is added via Configuration --> Settings --> Customer Settings
By default when any person registers into the page he is allotted with role 'Registered' . But I want that if user is selecting Vendor as his 'Registration type' , then by he must automatically get Vendor role assigned.
His is it possible to get it done in Nopcommerce 3.50????
By default when any person registers into the page he is allotted with role 'Registered' . But I want that if user is selecting Vendor as his 'Registration type' , then by he must automatically get Vendor role assigned.
His is it possible to get it done in Nopcommerce 3.50????
- 8
- 100
- 3/13/2015
- United States
keshavsone wrote:
What you are trying to do would require custom code and cannot be done via settings. If you are interested in pursuing this you could start a new topic in the development forum since this doesn't really fall under vendor reviews.
Hi , In frontend registration page I have added another field name as ' Registration Type' that contains radio buttons 'Buyer' and Vendor' . This new field is added via Configuration --> Settings --> Customer Settings
By default when any person registers into the page he is allotted with role 'Registered' . But I want that if user is selecting Vendor as his 'Registration type' , then by he must automatically get Vendor role assigned.
His is it possible to get it done in Nopcommerce 3.50????
By default when any person registers into the page he is allotted with role 'Registered' . But I want that if user is selecting Vendor as his 'Registration type' , then by he must automatically get Vendor role assigned.
His is it possible to get it done in Nopcommerce 3.50????
What you are trying to do would require custom code and cannot be done via settings. If you are interested in pursuing this you could start a new topic in the development forum since this doesn't really fall under vendor reviews.