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

Payments #59

Merged
merged 4 commits into from May 13, 2022
Merged

Payments #59

merged 4 commits into from May 13, 2022

Conversation

hei-ryan
Copy link
Collaborator

No description provided.

doc/api.yml Outdated Show resolved Hide resolved
doc/api.yml Outdated Show resolved Hide resolved
src/test/java/school/hei/haapi/integration/PaymentIT.java Outdated Show resolved Hide resolved
@hei-teacher hei-teacher marked this pull request as draft March 15, 2022 10:33
@hei-teacher hei-teacher marked this pull request as ready for review March 15, 2022 10:34
@hei-teacher hei-teacher marked this pull request as draft March 15, 2022 10:34
@hei-ryan hei-ryan marked this pull request as ready for review May 12, 2022 13:18
@hei-ryan hei-ryan force-pushed the payments branch 5 times, most recently from c1bd68b to b8dc673 Compare May 12, 2022 14:56
@sonarcloud
Copy link

sonarcloud bot commented May 12, 2022

SonarCloud Quality Gate failed.    Quality Gate failed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 2 Code Smells

71.4% 71.4% Coverage
0.0% 0.0% Duplication

@hei-ryan hei-ryan requested a review from hei-teacher May 12, 2022 15:15
parameters:
- name: status
in: query
description: "Filter fees by status. Available status are PAID, UNPAID and LATE"
Copy link
Member

Choose a reason for hiding this comment

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

Do not enumerate statuses here as they may change. Just say: See the PaymentStatus object for its value.

operationId: getFees
responses:
'200':
description: List of all student fees
Copy link
Member

Choose a reason for hiding this comment

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

of filtered student fees

/students/{student_id}/fees/{fee_id}/payments:
get:
tags:
- Paying
summary: Get all student payments
summary: Get all student payments for one specific fee ordered by creation datetime desc
Copy link
Member

Choose a reason for hiding this comment

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

of as specific fee. Resulting list is ordered by...

in: query
schema:
$ref: '#/components/schemas/PageSize'
operationId: getStudentFeePayments
Copy link
Member

Choose a reason for hiding this comment

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

getStudentPayments

responses:
'200':
description: Created student payments
description: Created student payments,
Copy link
Member

Choose a reason for hiding this comment

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

rm ,

insert into "payment"
(id, fee_id, type, comment,amount, creation_datetime)
values
('payment1_id','fee1_id', 'CASH', 'Comment', 2000, '2022-11-08T08:25:24.00Z'),
Copy link
Member

Choose a reason for hiding this comment

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

Use a richer set of data. Use all types. And sometimes set comment to null.

Also, add the MOBILE_MONEY type, which requires comment to be set when used (check is done on the Java side, not Postgres).

FIX type also requires comment to be set (to be checkd on Java side)

@@ -88,10 +105,48 @@ void student_pages_are_ordered_by_reference() throws ApiException {
assertTrue(isBefore(page2.get(0).getRef(), page2.get(2).getRef()));
}

@Test
void fees_page_are_ordered_by_due_datetime_desc() throws ApiException {
Copy link
Member

Choose a reason for hiding this comment

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

pages

}

@Test
void payments_page_are_ordered_by_due_datetime_desc() throws ApiException {
Copy link
Member

Choose a reason for hiding this comment

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

pages

PayingApi api = new PayingApi(student1Client);
int pageSize = 2;

List<Fee> page1 = api.getStudentFees(STUDENT1_ID, 1, pageSize);
Copy link
Member

Choose a reason for hiding this comment

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

Can you check whether we put ordered indexes (of the approriate orderding) on fee.due_datetime (DESC) and payment.creation_datetime (DESC or ASC, I don't remember)? https://www.postgresql.org/docs/current/indexes-ordering.html

When we now that we will ALWAYS (or very often) return a list ordered by some attribute, we add such ordered indexes. Warning: if the index order is DIFFERENT from the request submitted, then we may have dramatic result!

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Actually, we didn't index yet the fee.due_datetime(DESC) and payment.creation_datetime(DESC). For our use case, the most recent (descending) order is the most efficient, and I don't think there is a case where we should change this order.

So I think we can put ordered (DESC) indexes for these fields.

ApiClient manager1Client = anApiClient(MANAGER1_TOKEN);
PayingApi api = new PayingApi(manager1Client);
CreatePayment toCreate1 = creatablePayment1().amount(null);
CreatePayment toCreate2 = creatablePayment1().amount(-1);
Copy link
Member

Choose a reason for hiding this comment

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

Unless type is FIX! But just create a ticket for addressing FIX types for the moment

Copy link
Member

Choose a reason for hiding this comment

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

My above comment is bad. Indeed, amount can NEVER be negative. The FIX type is used to cancel fees that we don't want pay (eg: fees that we created falsly).

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

For sure, but this feature is not still implemented so I've created a ticket anyway : #60

@hei-teacher hei-teacher requested review from hei-teacher and removed request for hei-teacher May 13, 2022 09:24
@hei-teacher hei-teacher merged commit 8a15311 into dev May 13, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants