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

feat: set country in config, login to nvidia when starting #162

Merged
merged 23 commits into from Sep 22, 2020

Conversation

fuckingrobot
Copy link
Contributor

@fuckingrobot fuckingrobot commented Sep 21, 2020

Description

Fixes #161. Refactors Nvidia to hide more of the logic. Adds support for a bunch of countries to Nvidia (with add to cart!) and support for 3090 (despite no DR IDSs).

Testing

Tested on usa and belgium. Use .env as README shows.

image

@geman220
Copy link
Contributor

This can relatively easily be replicated across stores so that we don't end up with 10 copies of every store for each locale. I'm going to hold off on trying to implement this on other sites until this is tested and merged, but I like where your head is at on this.

@hmellor
Copy link

hmellor commented Sep 21, 2020

It'd be good to add a config based method for choosing location like in #163 (I closed the PR because it was very similar to this one and I like this one better)

@fuckingrobot
Copy link
Contributor Author

@hmellor was working on it. See most recent commit.

@fuckingrobot fuckingrobot changed the title [feat] country based settings for Nvidia [feat] country based settings for Nvidia, basic 3090 support Sep 21, 2020
@fuckingrobot fuckingrobot marked this pull request as ready for review September 21, 2020 16:52
@hmellor
Copy link

hmellor commented Sep 21, 2020

Could you add GB from #161? Thanks

kirbdee
kirbdee previously approved these changes Sep 21, 2020
Copy link

@kirbdee kirbdee left a comment

Choose a reason for hiding this comment

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

locales look good to me 👍

src/store/model/nvidia.ts Outdated Show resolved Hide resolved
kirbdee
kirbdee previously approved these changes Sep 21, 2020
@fuckingrobot fuckingrobot changed the title [feat] country based settings for Nvidia, basic 3090 support [feat] set country in config, login before stock Sep 21, 2020
@fuckingrobot
Copy link
Contributor Author

fuckingrobot commented Sep 21, 2020

@jef ready for review and merge when you have a moment. Other stores with multiple regions should refactor to use COUNTRY once this is merged.

@fuckingrobot fuckingrobot changed the title [feat] set country in config, login before stock feat: set country in config, login before stock Sep 21, 2020
@@ -19,6 +19,7 @@ SHOW_ONLY_SERIES="3080"
SLACK_CHANNEL="SlackChannelName"
SLACK_TOKEN="slack-token"
STORES="bestbuy,bandh,nvidia"
COUNTRY="usa"
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we plan to allow multiple countries? e.g: I am in Canada, and would like to scan both US and CA stores

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, this is the first step in localization. We don't want to localize every store all at once in case there are issues. Nvidia is the first and proof of concept, assuming all goes well we can work on implementing localization across stores.

Copy link

@kirbdee kirbdee Sep 21, 2020

Choose a reason for hiding this comment

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

Work around would be to just run two instances with different configs, but also depends on the stores. IE FE's from nvidia would be the same stock

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not a priority for me, but yes, this is the first step to getting there (while keeping the code clean-ish/manageable).

@fuckingrobot fuckingrobot changed the title feat: set country in config, login before stock feat: set country in config, login to nvidia when starting Sep 21, 2020
@geman220 geman220 linked an issue Sep 21, 2020 that may be closed by this pull request
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.

List of additional 3080 Product IDs for Nvidia Add EU sites
8 participants