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): add setAccountProfileCurrency mutation #4094

Merged

Conversation

kieckhafer
Copy link
Member

@kieckhafer kieckhafer commented Mar 29, 2018

Resolves #3922
Impact: minor
Type: feature

Issue

Add a setAccountProfileCurrency mutation to allow GraphQL to update and return currency data for an account

Solution

  • Create mutation file for setAccountProfileCurrency
  • Update accounts/setProfileCurrency meteor method to take in a userId, and to return an account
  • Update existing calls to accounts/setProfileCurrency to add userId

Breaking changes

None. If a userId is not provided, the code will default to the current user, which is what happens at this time.

Testing

  1. Start up the app
  2. Navigate to localhost:3000/graphiql
  3. Run the following, with accountId being the base64 encoding of "reaction/account:{accountId}"
mutation {
  setAccountProfileCurrency(input: {
    accountId: "reaction/account:{accountId}"
    currencyCode: "USD"
    clientMutationId: "clientMutationId13579"
  }) {
    clientMutationId
    account {
      createdAt
      currency {
        code
        _id
        symbol
        rate
        format
      }
    }
  }
}

@kieckhafer kieckhafer changed the base branch from master to release-1.11.0 March 29, 2018 22:48
@kieckhafer kieckhafer changed the title [WIP] (feat): add setAccountProfileCurrency mutation (feat): add setAccountProfileCurrency mutation Mar 30, 2018
@kieckhafer kieckhafer requested a review from ticean March 30, 2018 17:32
aldeed
aldeed previously requested changes Apr 2, 2018
* @summary Sets users profile currency
*/
export function setProfileCurrency(currencyName) {
export function setProfileCurrency(accountId, currencyName) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd rather see accountId added as second arg, which is then backwards compatible and you don't have to modify the places that call this.

This also needs to check to make sure the current user has permission to update accountId


// retrive all currencies from the Shops collection and xform them
function getXformedCurrenciesByShop() {
const shopCurrencies = Reaction.getShopCurrencies();
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we'll want to merge my changes that give access to Mongo outside of Meteor first, and then change this to use the collections from context. We need to avoid importing Reaction

@aldeed aldeed removed the request for review from ticean April 2, 2018 15:59
@aldeed aldeed self-assigned this Apr 5, 2018
@aldeed aldeed added this to the Bierstadt milestone Apr 5, 2018
@aldeed aldeed requested a review from willopez April 9, 2018 21:08
@aldeed aldeed dismissed their stale review April 9, 2018 21:09

made suggested changes

@aldeed
Copy link
Contributor

aldeed commented Apr 9, 2018

@willopez Do you mind reviewing this one? Ignore the snyk failures because they're fixed in my other PR

@willopez
Copy link
Member

willopez commented Apr 9, 2018

@aldeed I'll take look.

@willopez
Copy link
Member

NOTE: There is a known issue with an account's rate that will be addressed in a future PR.

Copy link
Member

@willopez willopez left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@aldeed aldeed merged commit c667d46 into release-1.11.0 Apr 10, 2018
@aldeed aldeed deleted the feat-3922-kieckhafer-setAccountProfileCurrencyMutation branch April 10, 2018 18:15
@spencern spencern mentioned this pull request Apr 19, 2018
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.

3 participants