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

add advanced invoice #8

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

SashaDi13
Copy link

No description provided.

@SashaDi13 SashaDi13 force-pushed the advanced-invoice branch 3 times, most recently from 882cf4f to d7cfca0 Compare August 13, 2024 08:39
@@ -35,6 +36,7 @@ def create(amount, destination: nil, reference: nil)
@amount = convert_to_cents(amount)
@destination = destination
@reference = reference
binding.pry

Choose a reason for hiding this comment

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

забула

@@ -1,5 +1,6 @@
require "bigdecimal"
require "money"
require "pry"

Choose a reason for hiding this comment

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

не треба тут

Comment on lines 1 to 4
require_relative 'simple_invoice'
module MonopayRuby

Choose a reason for hiding this comment

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

Suggested change
require_relative 'simple_invoice'
module MonopayRuby
require_relative 'simple_invoice'
module MonopayRuby

@SashaDi13 SashaDi13 force-pushed the advanced-invoice branch 2 times, most recently from 827aeab to ae19af2 Compare August 13, 2024 08:48
CHANGELOG.md Outdated
@@ -1,3 +1,7 @@
## [1.1.0] - 2024-08-12
- add `MonopayRuby::Invoices::AdvancedInvoice`

Choose a reason for hiding this comment

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

а чому AdvancedInvoice? В чім його різниця порівняно з simple invoice? І потрібно описати детально функціонал який додається

CHANGELOG.md Outdated
@@ -1,3 +1,7 @@
## [1.1.0] - 2024-08-12
- add `MonopayRuby::Invoices::AdvancedInvoice`
- change private method `request_body` to private

Choose a reason for hiding this comment

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

тобто прайват змінився на прайват? Виглядає по опису що нічого не змінилося, опиши детально що відбулося тут

CHANGELOG.md Outdated
## [1.1.0] - 2024-08-12
- add `MonopayRuby::Invoices::AdvancedInvoice`
- change private method `request_body` to private
- modify `README.md`

Choose a reason for hiding this comment

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

яка саме модифікація відбулася?

README.md Outdated
Comment on lines 79 to 80
"https://example.com",
"https://example.com/payments/webhook"

Choose a reason for hiding this comment

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

що це за лінки? Де дока для них?

Copy link
Owner

Choose a reason for hiding this comment

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

цей код взагалі не працює

Copy link
Owner

Choose a reason for hiding this comment

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

буде помилка параметрів, бо мають бути неймед параметри

Comment on lines 9 to 10
@basketOrder = basketOrder
@otherParams = otherParams

Choose a reason for hiding this comment

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

чому тут використаний кемл кейс?

@SashaDi13 SashaDi13 force-pushed the advanced-invoice branch 2 times, most recently from c4494c9 to e04bf70 Compare August 13, 2024 13:45
README.md Outdated
Comment on lines 79 to 80
"https://example.com",
"https://example.com/payments/webhook"
Copy link
Owner

Choose a reason for hiding this comment

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

цей код взагалі не працює

README.md Outdated
Comment on lines 79 to 80
"https://example.com",
"https://example.com/payments/webhook"
Copy link
Owner

Choose a reason for hiding this comment

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

буде помилка параметрів, бо мають бути неймед параметри

Comment on lines 19 to 20
# TODO: add "ccy" and another missing params
# TODO: remove nil valued params
Copy link
Owner

Choose a reason for hiding this comment

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

resolve this todos, or remove

}
}

request_body[:merchantPaymInfo][:basketOrder] = basketOrder if basketOrder.any?
Copy link
Owner

Choose a reason for hiding this comment

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

а якщо request_body[:merchantPaymInfo] буде ніл?

}

request_body[:merchantPaymInfo][:basketOrder] = basketOrder if basketOrder.any?
request_body.merge!(otherParams).to_json
Copy link
Owner

Choose a reason for hiding this comment

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

навіщо тобі бенг метод тут?

Comment on lines 31 to 32
request_body[:merchantPaymInfo] = request_body[:merchantPaymInfo].merge!(other_merchant_paymant_info_params) # add additional merchant_payment_info params if they are present
request_body.merge!(other_params) # add additional params if they are present
Copy link
Owner

Choose a reason for hiding this comment

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

remove redundant comments, because they describe the code point in point

Comment on lines 35 to 36
if request_body[:merchantPaymInfo].has_key?(:basketOrder)
request_body[:merchantPaymInfo][:basketOrder].first[:sum] = convert_to_cents(amount)
Copy link
Owner

Choose a reason for hiding this comment

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

this is unsafe, cehck if first param present to prevent NilClass undefined method error

if request_body[:merchantPaymInfo].has_key?(:basketOrder)
request_body[:merchantPaymInfo][:basketOrder].first[:sum] = convert_to_cents(amount)
elsif request_body.has_key?(:items)
request_body[:items].first[:sum] = convert_to_cents(amount)
Copy link
Owner

Choose a reason for hiding this comment

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

this is also unsafe, so, you check for the key presence, but value might be nil

private

def sum_param_into_basket
request_body[:merchantPaymInfo][:basketOrder][:qty] * amount
Copy link
Owner

Choose a reason for hiding this comment

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

add some return if any of parameter is blank

Comment on lines 48 to 49
def sum_param_into_basket
request_body[:merchantPaymInfo][:basketOrder][:qty] * amount
Copy link
Owner

Choose a reason for hiding this comment

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

this method isn't used anywhere

@SashaDi13 SashaDi13 force-pushed the advanced-invoice branch 4 times, most recently from 222b580 to bd97df8 Compare August 27, 2024 09:07
@@ -6,3 +6,5 @@ source "https://rubygems.org"
gemspec

gem "rake", "~> 13.0"
gem 'activesupport', '~> 7.0'
Copy link
Owner

Choose a reason for hiding this comment

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

це точно потрібно?

Gemfile Outdated
@@ -6,3 +6,5 @@ source "https://rubygems.org"
gemspec

gem "rake", "~> 13.0"
gem 'activesupport', '~> 7.0'
gem 'yard'
Copy link
Owner

Choose a reason for hiding this comment

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

а це взагалі видаляй

CHANGELOG.md Outdated
- change private methods to protected
- modify `request_body` for additional params
- add protected method `default_params`
- add more details about `MonopayRuby::Invoices::AdvancedInvoice` into `README.md`
Copy link
Owner

Choose a reason for hiding this comment

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

це не потрібно

CHANGELOG.md Outdated
Comment on lines 3 to 5
- change private methods to protected
- modify `request_body` for additional params
- add protected method `default_params`
Copy link
Owner

Choose a reason for hiding this comment

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

це вже прям деталі реалізації, вони не важливі для користувача

#
# @example Create a payment with amount and additional parameters
# create(100, additional_params: { merchantPaymInfo: { destination: "Happy payment", reference: "ref123" } })

Copy link
Owner

Choose a reason for hiding this comment

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

Suggested change

@@ -1,3 +1,4 @@
require 'active_support/all'
Copy link
Owner

Choose a reason for hiding this comment

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

так а це навіщо?)

Copy link
Author

Choose a reason for hiding this comment

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

це, як і гем мені потрібний для методу blank?

Copy link
Owner

Choose a reason for hiding this comment

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

ну диви, в сімпл інвойсі воно не потрібне точно, бо не юзається, а так, то хз, я б сказав, що оке, най буде поки що

Copy link
Author

Choose a reason for hiding this comment

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

ну ок, тоді додам в едванс інвойс

end

it "returns true" do
expect(simple_invoice_instance.create(2000, other_merchant_paymant_info_params: {basketOrder: [basket_order]}, other_params: other_params)).to be_truthy
Copy link
Owner

Choose a reason for hiding this comment

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

винеси параметри в let

Comment on lines +28 to +29
@destination = @additional_params&.dig(:merchantPaymInfo, :destination)
@reference = @additional_params&.dig(:merchantPaymInfo, :reference)
Copy link
Owner

Choose a reason for hiding this comment

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

так це ріквайред параметри, вони не можуть бути відсутні, якщо ж відсутні, то рейзани якусь помилку

Copy link
Author

Choose a reason for hiding this comment

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

але ж вони по дефолту ніл, то чому ж вони ріквайред?


current_params.merge!(additional_params.except(:merchantPaymInfo))

# TODO: add sum and qty params into present additional params
Copy link
Owner

Choose a reason for hiding this comment

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

а щодо цього комента що?

Comment on lines +51 to +53
set_sum_and_qty_params(current_params&.dig(:merchantPaymInfo, :basketOrder))
set_sum_and_qty_params(current_params[:items])
Copy link
Owner

Choose a reason for hiding this comment

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

поясни будь ласка, що тут відбувається

Copy link
Author

Choose a reason for hiding this comment

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

я спробувала розписати це у коментарях

@SashaDi13 SashaDi13 force-pushed the advanced-invoice branch 2 times, most recently from b2acd0c to d86027e Compare August 28, 2024 19:53
@@ -1,3 +1,4 @@
require 'active_support/all'
Copy link
Owner

Choose a reason for hiding this comment

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

ну диви, в сімпл інвойсі воно не потрібне точно, бо не юзається, а так, то хз, я б сказав, що оке, най буде поки що

Comment on lines 49 to 50
# TODO: add and modify sum and qty params of merchantPaymInfo[basketOrder] parameters if it is present
# TODO: add and modify sum and qty params of items parameters if it is present
Copy link
Owner

Choose a reason for hiding this comment

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

для чого ці ту-ду?)

Copy link
Owner

Choose a reason for hiding this comment

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

або пофіксай, або прибери)

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.

4 participants