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.

Sign up for GitHub

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

Jump to bottom

Add settings API #2728

Merged
merged 9 commits into from
Aug 31, 2018
Merged

Add settings API #2728

merged 9 commits into from
Aug 31, 2018

Conversation

artursmet
Copy link
Contributor

@artursmet artursmet commented Aug 28, 2018 •
edited
Loading

I want to merge this change because it resolves #2705

Pull Request Checklist

  1. Privileged views and APIs are guarded by proper permission checks.
  2. All visible strings are translated with proper context.
  3. All data-formatting is locale-aware (dates, numbers, and so on).
  4. Database queries are optimized and the number of queries is constant.
  5. The changes are tested.
  6. The code is documented (docstrings, project documentation).

@codecov
Copy link

codecov bot commented Aug 28, 2018 •
edited
Loading

Codecov Report

Merging #2728 into master will decrease coverage by 0.08%.
The diff coverage is 87.82%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2728      +/-   ##
==========================================
- Coverage   89.32%   89.24%   -0.09%     
==========================================
  Files         206      207       +1     
  Lines       10362    10449      +87     
  Branches     1001     1010       +9     
==========================================
+ Hits         9256     9325      +69     
- Misses        795      805      +10     
- Partials      311      319       +8
Impacted Files Coverage Δ
saleor/graphql/core/types/common.py 84.61% <100%> (+0.61%) ⬆️
saleor/graphql/core/mutations.py 94.53% <100%> (ø) ⬆️
saleor/graphql/api.py 98.03% <100%> (+0.03%) ⬆️
saleor/core/weight.py 88.05% <100%> (+0.36%) ⬆️
saleor/graphql/shop/mutations.py 84.05% <84.05%> (ø)
saleor/graphql/shop/types.py 95.12% <87.5%> (ø)
saleor/graphql/core/types/money.py 90.69% <0%> (-9.31%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 84490be...2e94a37. Read the comment docs.

@artursmet artursmet force-pushed the feature/settings-api branch 2 times, most recently from 55c2789 to 8804492 Compare August 29, 2018 08:59
maarcingebala
maarcingebala reviewed Aug 29, 2018
View reviewed changes
saleor/graphql/site/mutations.py Outdated
domain = graphene.String(description='Domain name for shop')
header_text = graphene.String(description='Header text')
description = graphene.String(description='SEO description')
top_menu = graphene.ID(
Copy link
Member

Choose a reason for hiding this comment

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

Assigning storefront menus is covered in PR #2721 - I've added dedicated mutations for that.

saleor/graphql/site/mutations.py Outdated
description='Display prices with tax in store')
track_inventory_by_default = graphene.Boolean(
description='Enable inventory tracking')
homepage_collection = graphene.ID(
Copy link
Member

Choose a reason for hiding this comment

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

Maybe this should be done in a dedicated mutation?

Copy link
Member

Choose a reason for hiding this comment

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

Maybe as a rule of thumb, we should only put here simple settings such as booleans or strings, while having dedicated mutations for operations that manipulate objects or relationships?

@artursmet artursmet requested a review from patrys August 30, 2018 12:02
maarcingebala
maarcingebala reviewed Aug 31, 2018
View reviewed changes
saleor/graphql/shop/mutations.py Outdated
default_weight_unit = WeightUnitsEnum(description='Default weight unit')


class ShopNavigationInput(graphene.InputObjectType):
Copy link
Member

Choose a reason for hiding this comment

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

This seems unused.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch!

saleor/graphql/shop/mutations.py Outdated

class ShopDomainUpdate(BaseMutation):
class Arguments:
domain = graphene.String(description='Domain name for shop')
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't we also have an argument to update site.name in this mutation?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

site name is not used anywhere I think

Copy link
Member

Choose a reason for hiding this comment

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

It is used as the storefront's HTML title.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Right. I've added support for changing site name.

@maarcingebala maarcingebala merged commit af4a385 into master Aug 31, 2018
@maarcingebala maarcingebala deleted the feature/settings-api branch August 31, 2018 09:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@patrys patrys patrys approved these changes

@maarcingebala maarcingebala maarcingebala approved these changes

Assignees

@maarcingebala maarcingebala

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add site settings mutations
3 participants
@artursmet @patrys @maarcingebala