# Winston's Xfers Best Practice Notes

### ActiveRecord

#### Don't Use ActiveRecord Callbacks if possible

https://engineering.gusto.com/the-rails-callbacks-best-practices-used-at-gusto/

#### Use ActiveRecord Query Rather than Raw SQL if possible

Raw SQL is the way to go for complex query though
https://stackoverflow.com/questions/9715184/is-it-a-good-practice-to-directly-write-sql-query-in-rails

### Job

#### Don't put long data into a job
Our logging logs what is putted into a job. Keep the data there small
You cannot put like Something.perform_now(Array.new(100))

#### Don't use delayed job. it is deprecated

#### Catch Unexpected error
Sidekiq Automatically retries your job in case of an unexpected error. to be more save about the behaviors on unexpected error, catch it and do something/ throw slack notif

https://github.com/mperham/sidekiq/wiki/Error-Handling

### API:
https://docs.google.com/presentation/d/1_K0LlU6kRUI58i1LLR84ul4k__EZtUSDzz65D8KJwQg/edit#slide=id.p

#### Use plural, not singular
/api/v3/withdrawals, not /api/v3/withdrawal

#### Use object, not verb
/api/v3/withdrawals, not /api/v3/withdraws

#### Don't Throw Unexpected Error if you know the reason.
For 500 error, we will log the error data in api_query_logs but
only showing `unexpected error` to customer. So, as much as possible, don't throw 500 error.

#### Provide filtering, sorting, field selection and paging for collections
For List of withdrawals, list of charges, ...

#### Provide idempotency key for post request
Customer need to put a key that sent to us, that when is using same key, we will throw an error. function is to handle
double transactions

#### We also need to give our internal ID as receipt that the request is received.
at each request, we still need to reply customer with our ID as confirmation we have properly recorded the txn.
If customer file a complain, ideally they should use this ID.
This ID should :
1. Not Our database's ID, customer can know how many users/ transaction we have
2. Have identifier, rather than return `xasnkxaks`, we should return something like `user_xasnkxaks`

#### ApiQueryLog is deleted every 3 months
if your API have data that you want to write for logging permanently, at the API you write
AppLogger.ops.info "`controller name` received posting with params of #{params}"

### External Integration

#### Handle Timeout Differently
Don't treat them as failed
Don't treat them as successful.
If you don't want to handle autoretry logic initially, just throw slack error and manually solve first

#### Don't do retry infinitely
If we retry infinitely on a transaction, our partner somehow fucks up the idempotency key,
the potential loss is infinity. We must put a maximum retry

#### Follow the structuring like our existing Client (SampoernaClient)
Main Details:
have Request folder consists of requests ideally for each URL -> make sure the data type is receivable by the
other party, whether it is `query` or `json`, or `xml`
have Response.rb
have FakeResponse.rb
have client.rb -> should also handle the mocking in sandbox and staging
have `something`_api_query_logs to log down request
client_spec.rb should actually hit their sandbox API once and recorded in the VCR

### Database Design

#### String VS Text
Understand when to use string and when to use text/
Text have limit 65535 but cannot have index
String have limit 191 but can have index

#### Zero Downtime Migration
Be very aware of this when altering a table
https://www.notion.so/xfers/Zero-Downtime-Migration-ea7cab3218334438acf154485e50616b

#### tracking changes
use `has_paper_trail only: [:your_column]` if you want to track data change in your table.
It will be shown in the `versions` table
(Your table should be small/not many columns and not changing super often to use this table)

#### state machine
For state machine, if your number of state is limited and state progression is clear, use aasm_state.
if not, use string enum if possible (please understand its difference with number enum) and
String enum:
Ruby:
enum event_name: {
option_1: "option_1",
option_2: "option_2",
}
Database:
option_1
option_2

Number Enum:
enum kyc_status: %i[
option_1
option_2
]

Database:
0
1

(In number enum 0 will map to option_1 and 1 map to option_2, which will be hard to read for the SQL reader)

enum event_name: {
## User Related
connect_token_disabled: "connect_token_disabled",
user_verification_status_updated: "user_verification_status_updated",

### spec
#### use before(:each) and after(:each) mostly
Most of the spec you write will use the each rather than the (:all), or (:suite)

#### The name of unit test should match the path in app
Don't do:
app file in spec/api/v3/contractual_models/users_controller_spec.rb
spec file in spec/api/v3/users_controller_spec.rb

Should do:
app file in spec/api/v3/contractual_models/users_controller_spec.rb
spec file in spec/api/v3/contractual_models/users_controller_spec.rb

#### Freeze the time if necessary
Your spec may be stuffs that fails on certain time, do something like `travel_to(Time.zone.now)`

#### Use factory if possible
If already defined, do create(:user) rathen than User.create!(...)

#### Stub External Call. external hit to external party should be stubbed in spec
stub_request()....

#### Use described_class
in your unit test ,rather than using MyRequest.call(...), use described_class.call(...)

#### don't put logics/ ifs here
don't do something like:
if a == 1
expect(b).to eq "..."
else
expect(c).to eq "..."
end

#### don't expect something to raise specific error
cannot do:
expect(test.perform_now).to raise_error(InsufficientBalanceError)
should do:
expect(test.perform_now).to raise_error

#### If possible, don't use allow function to .. in integration test.
The below should not be written in integration test (can do in unit test)
allow(Sobatku::Client).to receive(:debit_account).and_return(fake_response)

### Other
#### Don't use recursion
It is hard to mentally read it

#### Don't write commented code
https://kentcdodds.com/blog/please-dont-commit-commented-out-code

#### Don't use eval
eval(...) should not be used, have some SQL injection issue

#### Folder your files to separate from other country
In Ideal world, we want to make our files become something like:
- Core
- Indonesia
- ID merchant
- ID merchant 2
- Singapore
- SG merchant
If possible, don't put Indonesia specific or Singapore Specific logic to Core

#### use the time function properly (I think not standardized yet across whole codebase)

Good Examples:
Time.use_current_zone{ 4.hours.ago }.iso8601
Time.in_time_zone(ConfigManager.current_time_zone)

Bad examples:
Time.use_current_zone { Date.tomorrow + 2.hour }

You often goes into trap of time behaving differently in
a. CircleCI server
b. Real Prod Server ID
c. Real Prod Server SG
d. your local