list for currency selection in general settings #329

jyotigautam · 2018-11-28T12:26:14Z

Adding a dropdown on create general settings form to choose a currency from a given list of currencies.

Motivation and Context

Since we need a common currency globally set for a store, hence the currency is fetched from the store settings at every place where currency is used.

Describe your changes

Added a list of currencies to choose from in general settings form.
Used the currency set in general config everywhere as default currency.

Checklist

I have read CONTRIBUTING.md.
My code follows the style guidelines of this project.
I have commented my code, particularly in hard-to-understand areas.
My code does not generate any (new) credo and compile-time warnings.
I have updated the documentation wherever necessary.
I have added tests to cover my changes.

Dear Gatekeeper,

Please make sure that the commits that will be merged or rebased into the base
branch are formatted according to our standards.

The only additional requirement is that the the PR number must appear in the
commit title in brackets: (#XXX)

Changes: Adding a dropdown on create general settings form to choose a currency from a given list of currencies.

codecov · 2018-11-28T12:31:01Z

Codecov Report

Merging #329 into develop will increase coverage by 0.02%.
The diff coverage is 71.42%.

@@             Coverage Diff             @@
##           develop     #329      +/-   ##
===========================================
+ Coverage    67.06%   67.08%   +0.02%     
===========================================
  Files          134      134              
  Lines         1497     1498       +1     
===========================================
+ Hits          1004     1005       +1     
  Misses         493      493

Impacted Files	Coverage Δ
.../lib/core/tools/mailer/order_confirmation_email.ex	`93.75% <ø> (ø)`	⬆️
apps/snitch_core/lib/core/tools/money.ex	`100% <100%> (ø)`	⬆️
apps/snitch_core/lib/core/domain/package.ex	`83.33% <100%> (+0.98%)`	⬆️
...snitch_core/lib/core/domain/shipping_calculator.ex	`100% <100%> (ø)`	⬆️
..._core/lib/core/data/model/general_configuration.ex	`10.71% <100%> (+6.86%)`	⬆️
apps/snitch_core/lib/core/domain/order/order.ex	`42.55% <20%> (ø)`	⬆️

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 42ca81f...5c6adc6. Read the comment docs.

pkrawat1 · 2018-11-29T05:20:10Z

apps/admin_app/lib/admin_app_web/templates/general_settings/_form.html.eex

@@ -22,7 +22,15 @@
                <%= input f, :hosted_payment_url, nil, is_horizontal: true, description: "Payment URL for the store."  %>
            </div>
            <div class="form-group row ">
-                <%= input f, :currency, nil, is_horizontal: true, description: "Default currency for the store."  %>
+                <label  class="col-sm-3 col-form-label">


indentation is not correct

pkrawat1 · 2018-11-29T05:34:07Z

apps/snitch_core/lib/core/data/model/general_configuration.ex

+    general_config = Repo.all(GC) |> List.first()
+    if general_config != nil, do: general_config.currency, else: "USD"
+  end
+


def fetch_currency do case Repo.one(GC) nil -> "USD" gc -> gc.currency end end

This is looking better.

This is more succinct way.

ashish173 · 2018-11-29T05:34:45Z

apps/snitch_core/lib/core/domain/order/order.ex

@@ -38,7 +38,7 @@ defmodule Snitch.Domain.Order do
  """
  @spec payments_total(Order.t(), String.t()) :: Money.t()
  def payments_total(order, payment_state) do
-    {:ok, currency} = Defaults.fetch(:currency)
+    currency = GCModel.fetch_currency()


I would really prefer to have {:ok, currency} or {:error, msg} rather than returning just as currency. this way matching becomes easier. @SagarKarwande thoughts?

We can use just the return value since we will be using a Repo.one query, which either gives a value or nil.

Yes returning tuples with success and error will help.

@ashish173 As per the definition of GeneralConfiguration.fetch_currency() we would never be able to get the error case.

That's okay @SagarKarwande for now. Since we always start/provision a store with a currency. I think now I agree with what @pkrawat1 is saying.

But there would be no {:error, msg} scenario, it will always return a string.

ashish173 · 2018-11-29T05:38:25Z

apps/admin_app/lib/admin_app_web/views/general_settings_view.ex

@@ -2,7 +2,13 @@ defmodule AdminAppWeb.GeneralSettingsView do
  use AdminAppWeb, :view
  alias Snitch.Data.Model.ProductBrand

+  @currencies ["USD", "INR", "GDP", "EUR"]


This should go in the core rather than in the admin application IMO. @pkrawat1 @SagarKarwande ?

Yes, valid currencies should be a part of core

May I know the idea behind this?

Any application would always depend on core and can easily access the valid currencies.

ashish173 · 2018-11-29T05:39:05Z

apps/admin_app/lib/admin_app_web/views/layout_view.ex

@@ -36,7 +37,8 @@ defmodule AdminAppWeb.LayoutView do
  end

  def check_general_settings() do
-    Repo.all(GeneralConfiguration) |> List.first()
+    count = Repo.aggregate(from(g in "snitch_general_configurations"), :count, :id)
+    if count == 1, do: true, else: false


This method is too complex to identify the presence of GC or not, please simplify this.

def check_general_settings do case Repo.one(GC) nil -> false _ -> true end end

pkrawat1 · 2018-11-29T05:39:29Z

apps/admin_app/lib/admin_app_web/views/general_settings_view.ex

@@ -2,7 +2,13 @@ defmodule AdminAppWeb.GeneralSettingsView do
  use AdminAppWeb, :view
  alias Snitch.Data.Model.ProductBrand

+  @currencies ["USD", "INR", "GDP", "EUR"]


I was thinking if any other app in the umbrella would be needing the list of currencies. If it does then it will be required to move to some other place. So how about keeping it at a place in app that can expose this to all other apps.

I think core should contain list of valid currencies

ashish173 · 2018-11-29T05:40:24Z

apps/snitch_core/lib/core/data/model/general_configuration.ex

@@ -10,6 +10,12 @@ defmodule Snitch.Data.Model.GeneralConfiguration do
  alias Snitch.Tools.Helper.ImageUploader
  alias Ecto.Multi

+  @spec fetch_currency() :: String.t()
+  def fetch_currency do
+    general_config = Repo.all(GC) |> List.first()


Why Repo.all() when we can use Repo.one()? That way you also won't have to do List.first()

yes, then I have to handle the error raised if multiple results of Repo.one(GC) could be returned.

Repo.one only returns just one result. In any case my assumption is that we can only have one general setting with the way how the code is written.

ashish173 · 2018-11-29T05:58:10Z

@jyotigautam move the currency list to the core application. This is an important change.

SagarKarwande · 2018-11-29T06:15:40Z

@ashish173 @pkrawat1
Initially during store setup General Setting is not configured until user does it.
If any product is created before setting General Settings and later on setting up general setting with different currency would break the Money calculations. Any thought about this?

Should the currency change be allowed after General Setting is created?

SagarKarwande · 2018-11-29T06:23:46Z

apps/admin_app/lib/admin_app_web/templates/product/form.html.eex

@@ -51,7 +51,7 @@
      <div class="col-sm-9 input-group">
        <%= text_input :selling_price, :amount, value: get_amount(f.data.selling_price), class: "form-control", name: "product[selling_price][amount]" %>
        <div class="input-group-append">
-          <%= select :selling_price, :currency, get_currency(),value: get_currency_value(f.data.selling_price), class: "form-control", name: "product[selling_price][currency]" %>


@jyotigautam
As we will be using currency from GeneralSetting. Does showing currency selection to user makes sense?

Currency for product prices can be set in changeset.

Here the currency selected only shows one value i.e, from the general settings or "usd" if no general setting is set. So, the user can't choose from this list.

@jyotigautam @SagarKarwande I would say we remove the select altogether and just show a label of the currency. Just like it is done with etsy store. Thoughts?

Yes, there is no point of showing it as select input when we have just one option.

ashish173 · 2018-11-29T06:37:46Z

@SagarKarwande we are provisioning the general settings with a currency "USD" for now. So all the products which get created happen with the currency.

Currency change is another issue in itself we will revisit it at a later phase. #330

ashish173 · 2018-11-29T09:24:08Z

apps/snitch_core/lib/core/data/model/general_configuration.ex

+
+  @spec get_currency_list() :: List.t()
+  def get_currency_list do
+    ["USD", "INR", "GDP", "EUR"]


@jyotigautam let's make this array a module attribute, I see this as a constant what do you think?

all changes done.

Changes: Adding a dropdown on create general settings form to choose a currency from a given list of currencies.

Since we need a common currency globally set for a store, hence the currency is fetched from the store settings at every place where currency is used. Changes:- Added a list of currencies to choose from in general settings form. Used the currency set in general config everywhere as default currency.

jyotigautam added the PR state: WIP label Nov 28, 2018

Add list of currencies(#329)

765ee82

Changes: Adding a dropdown on create general settings form to choose a currency from a given list of currencies.

jyotigautam force-pushed the show_currency_list branch from 2cc163e to 765ee82 Compare November 28, 2018 12:28

jyotigautam requested a review from SagarKarwande November 28, 2018 13:15

jyotigautam added PR state: review and merge and removed PR state: WIP labels Nov 28, 2018

jyotigautam requested a review from ashish173 November 28, 2018 13:16

jyotigautam force-pushed the show_currency_list branch from 501b3d3 to 518bfd8 Compare November 28, 2018 13:59

adding seed for general_configuration

023f454

jyotigautam force-pushed the show_currency_list branch from 518bfd8 to 023f454 Compare November 29, 2018 05:08

pkrawat1 reviewed Nov 29, 2018

View reviewed changes

ashish173 reviewed Nov 29, 2018

View reviewed changes

pkrawat1 reviewed Nov 29, 2018

View reviewed changes

ashish173 reviewed Nov 29, 2018

View reviewed changes

SagarKarwande previously requested changes Nov 29, 2018

View reviewed changes

jyotigautam added 3 commits November 29, 2018 13:24

code fixes

f82d70e

chnaging alias name for general congiguration as gc

e844e0b

set currency field as readonly textinput

43ce448

ashish173 reviewed Nov 29, 2018

View reviewed changes

use currency list as module attribute since it's constant

56d0669

Merge branch 'develop' into show_currency_list

5c6adc6

ashish173 approved these changes Nov 29, 2018

View reviewed changes

ashish173 merged commit 74a82a9 into develop Nov 29, 2018

ashish173 deleted the show_currency_list branch November 29, 2018 15:00

jyotigautam added a commit that referenced this pull request Nov 29, 2018

Add list of currencies(#329)

5bc170a

Changes: Adding a dropdown on create general settings form to choose a currency from a given list of currencies.

jyotigautam added a commit that referenced this pull request Nov 29, 2018

Add list of currencies(#329)

c808453

Changes: Adding a dropdown on create general settings form to choose a currency from a given list of currencies.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

list for currency selection in general settings #329

list for currency selection in general settings #329

jyotigautam commented Nov 28, 2018

codecov bot commented Nov 28, 2018 •

edited

Loading

pkrawat1 Nov 29, 2018

pkrawat1 Nov 29, 2018

ashish173 Nov 29, 2018

jyotigautam Nov 29, 2018

ashish173 Nov 29, 2018

pkrawat1 Nov 29, 2018

SagarKarwande Nov 29, 2018

ashish173 Nov 29, 2018

jyotigautam Nov 29, 2018

ashish173 Nov 29, 2018

SagarKarwande Nov 29, 2018

jyotigautam Nov 29, 2018

SagarKarwande Nov 29, 2018

ashish173 Nov 29, 2018

pkrawat1 Nov 29, 2018

jyotigautam Nov 29, 2018

pkrawat1 Nov 29, 2018

SagarKarwande Nov 29, 2018

ashish173 Nov 29, 2018

jyotigautam Nov 29, 2018 •

edited

Loading

ashish173 Nov 29, 2018

ashish173 commented Nov 29, 2018

SagarKarwande commented Nov 29, 2018

SagarKarwande Nov 29, 2018

jyotigautam Nov 29, 2018

ashish173 Nov 29, 2018

jyotigautam Nov 29, 2018

ashish173 commented Nov 29, 2018 •

edited

Loading

ashish173 Nov 29, 2018

list for currency selection in general settings #329

list for currency selection in general settings #329

Conversation

jyotigautam commented Nov 28, 2018

Motivation and Context

Describe your changes

Checklist

codecov bot commented Nov 28, 2018 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jyotigautam Nov 29, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ashish173 commented Nov 29, 2018

SagarKarwande commented Nov 29, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ashish173 commented Nov 29, 2018 • edited Loading

Choose a reason for hiding this comment

codecov bot commented Nov 28, 2018 •

edited

Loading

jyotigautam Nov 29, 2018 •

edited

Loading

ashish173 commented Nov 29, 2018 •

edited

Loading