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

backend/feat/#146 : Adding extra params to send sms #148

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

backend/feat/#146 : Adding extra params to send sms #148

wants to merge 1 commit into from

Conversation

JanhaviSoni
Copy link
Contributor

@JanhaviSoni JanhaviSoni commented Jun 21, 2023 •
edited
Loading

Type of Change

  • Bugfix
  • New feature
  • Enhancement
  • Refactoring
  • Dependency updates

Description

Adding extra params to send sms

Additional Changes

  • This PR modifies the database schema (database migration added)
  • This PR modifies dhall configs/environment variables

Motivation and Context

How did you test it?

Checklist

  • I formatted the code and addressed linter errors ./dev/format-all-files.sh
  • I reviewed submitted code
  • I added unit tests for my changes where possible
  • I added a CHANGELOG entry if applicable

This was linked to issues Jun 21, 2023
Add extra parameters for sending otp nammayatri/nammayatri#1292
Open
Adding extra params to send otp #146
Open
roman-bodavskiy
roman-bodavskiy approved these changes Jun 22, 2023
View reviewed changes
lib/mobility-core/src/Kernel/External/SMS/Interface/Types.hs Outdated
@@ -21,6 +21,8 @@ module Kernel.External.SMS.Interface.Types
where

import Data.ByteString.Lazy (fromStrict, toStrict)
-- import qualified Kernel.External.SMS.ExotelSms.Types as ExotelType
Copy link
Contributor

Choose a reason for hiding this comment

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

redundant comment

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll remove

lib/mobility-core/src/Kernel/External/SMS/Interface/Types.hs Outdated
@@ -43,9 +46,20 @@ data SmsServiceConfig = MyValueFirstConfig MyValueFirst.MyValueFirstCfg | Exotel
data SendSMSReq = SendSMSReq
{ smsBody :: Text,
phoneNumber :: Text,
sender :: Text
sender :: Text,
smsType :: Maybe SMSType,
Copy link
Contributor

Choose a reason for hiding this comment

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

why can't we make it mandatory?
then we don't need this default with empty string maybe "" toUrlPiece smsType

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is because it is an optional parameter for exotel api so if in case we don't want to pass it then also the exotel api should work

@JanhaviSoni JanhaviSoni force-pushed the backend/feat/#146/adding-extra-params-to-sendOTP branch 2 times, most recently from 0095297 to 0d21223 Compare June 26, 2023 09:40
@JanhaviSoni JanhaviSoni requested a review from hkmangla June 26, 2023 09:46
@JanhaviSoni JanhaviSoni force-pushed the backend/feat/#146/adding-extra-params-to-sendOTP branch from 0d21223 to 0c4a244 Compare June 26, 2023 10:40
hkmangla
hkmangla requested changes Jun 26, 2023
View reviewed changes
lib/mobility-core/src/Kernel/External/SMS/Interface/Types.hs Outdated
data PriorityType = Normal | High
deriving (Generic, FromJSON, ToJSON, Show, Eq)

instance ToHttpApiData SMSType where
Copy link
Contributor

Choose a reason for hiding this comment

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

why not just use toLower?

Copy link
Contributor Author

@JanhaviSoni JanhaviSoni Jun 26, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

for PriorityType done the changes

Copy link
Contributor

Choose a reason for hiding this comment

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

Why can't you do for this too?

Copy link
Contributor

@roman-bodavskiy roman-bodavskiy Jun 30, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

why not just use toLower?

toLower is not suitable because we need convert camel to snake also

Copy link
Member

Choose a reason for hiding this comment

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

Then aesonPrefix snakeCase can be used?

lib/mobility-core/src/Kernel/External/SMS/Interface/ExotelSms.hs Outdated
@@ -51,7 +51,9 @@ sendOTP exoCfg SendSMSReq {..} = do
BasicAuthData
(DT.encodeUtf8 apiKey)
(DT.encodeUtf8 apiToken)
res <- Ex.sendOTPApi exoUrl authData sid exoOtpSmsTemplate exoPhoneNumber senderName
exoSmsType = toUrlPiece IT.Transactional
Copy link
Contributor

Choose a reason for hiding this comment

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

why to call toUrlPiece here?

Copy link
Contributor Author

@JanhaviSoni JanhaviSoni Jun 26, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I have put it inside in exotel

Copy link
Contributor

Choose a reason for hiding this comment

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

But why to even use this? It should be automatically converted from instance?

@JanhaviSoni JanhaviSoni force-pushed the backend/feat/#146/adding-extra-params-to-sendOTP branch from 0c4a244 to 011e84d Compare June 26, 2023 12:03
@JanhaviSoni JanhaviSoni requested a review from hkmangla June 26, 2023 12:38
@JanhaviSoni JanhaviSoni force-pushed the backend/feat/#146/adding-extra-params-to-sendOTP branch from 011e84d to 87921f6 Compare June 27, 2023 08:10
hkmangla
hkmangla requested changes Jun 30, 2023
View reviewed changes
lib/mobility-core/src/Kernel/External/SMS/Interface/Types.hs Outdated
data PriorityType = Normal | High
deriving (Generic, FromJSON, ToJSON, Show, Eq)

instance ToHttpApiData SMSType where
Copy link
Contributor

Choose a reason for hiding this comment

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

Why can't you do for this too?

@JanhaviSoni JanhaviSoni force-pushed the backend/feat/#146/adding-extra-params-to-sendOTP branch 2 times, most recently from 302745c to a20c74c Compare June 30, 2023 11:52
@JanhaviSoni JanhaviSoni force-pushed the backend/feat/#146/adding-extra-params-to-sendOTP branch from a20c74c to d259657 Compare July 6, 2023 11:08
@JanhaviSoni JanhaviSoni force-pushed the backend/feat/#146/adding-extra-params-to-sendOTP branch from d259657 to 3aec0ef Compare July 12, 2023 09:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@roman-bodavskiy roman-bodavskiy roman-bodavskiy approved these changes

@hkmangla hkmangla hkmangla requested changes

@0utkarsh 0utkarsh 0utkarsh approved these changes

Requested changes must be addressed to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Adding extra params to send otp Add extra parameters for sending otp
4 participants
@JanhaviSoni @hkmangla @0utkarsh @roman-bodavskiy