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

Implement Google merchant generator #6

Merged
merged 1 commit into from
Nov 27, 2020
Merged

Conversation

nirebu
Copy link
Contributor

@nirebu nirebu commented Sep 18, 2020

Ref #4

This PR implements a generator for a Google Merchant XML feed. This rendered useful adding some global configuration attributes based on the Store information.

lib/solidus_feeds.rb Outdated Show resolved Hide resolved
@elia elia requested a review from aldesantis October 2, 2020 16:00
@elia elia force-pushed the g-merchant-generator branch 2 times, most recently from 4ca6bf2 to e7ea0d5 Compare October 2, 2020 16:17
@elia elia marked this pull request as ready for review October 2, 2020 17:41
lib/solidus_feeds/generators/google_merchant.rb Outdated Show resolved Hide resolved
lib/solidus_feeds/generators/google_merchant.rb Outdated Show resolved Hide resolved
lib/solidus_feeds/generators/google_merchant.rb Outdated Show resolved Hide resolved
lib/solidus_feeds/generators/google_merchant.rb Outdated Show resolved Hide resolved
Copy link
Member

@elia elia left a comment

Choose a reason for hiding this comment

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

@aldesantis the idea behind not caring too much about making the google_merchant product feed extendability is that letting individual stores inherit from it is not ideal, especially since this is a concrete class, and I'm sure will face significant changes before we're done. That's why all the methods you mentioned are private.

We'd rather prefer it to be copied verbatim and modified, and given how short it is it should be pretty straightforward.

I'm not against a helper class that can collect useful code for building a google merchant feed generator, but would be better to source it from at least a couple of concrete classes.

@elia elia force-pushed the g-merchant-generator branch 3 times, most recently from 9fdd51e to e61799b Compare October 9, 2020 13:15
@nirebu nirebu requested a review from aldesantis October 9, 2020 13:15
Comment on lines 24 to 27
xml.title 'store title'
xml.link 'store link'
xml.description 'store description'
xml.language 'store lang'
Copy link
Member

Choose a reason for hiding this comment

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

I think we should try to provide some sensible defaults here. Ideally, I'd like each feed to be usable at a basic level without the need for customization.

For example, can we pass these as options as we do in solidus_product_feed?

Let me know if you disagree for some reason.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In the latest commit I should have addressed these concerns 😃

@nirebu nirebu linked an issue Oct 30, 2020 that may be closed by this pull request
@nirebu nirebu force-pushed the g-merchant-generator branch 2 times, most recently from 8c51458 to a6e932a Compare November 6, 2020 10:45
@nirebu nirebu self-assigned this Nov 6, 2020
@nirebu nirebu force-pushed the g-merchant-generator branch 6 times, most recently from 145f7e2 to 4f2ce1f Compare November 6, 2020 15:15
As specified in https://support.google.com/merchants/answer/7052112.

This also adds global configuration attributes to be used in the store
feed.

Co-Authored-By: Flavio Auciello <[email protected]>
Co-Authored-By: Elia Schito <[email protected]>
@nirebu nirebu dismissed aldesantis’s stale review November 27, 2020 16:26

Changes were addressed

@nirebu nirebu merged commit 605ac70 into master Nov 27, 2020
@nirebu nirebu deleted the g-merchant-generator branch November 27, 2020 16:44
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.

Implement a Google Merchant generator
3 participants