[DNCR-41] Add manual payment option #35

megawebmaster · 2016-10-31T19:16:00Z

No description provided.

tlegutko

At first I thought that this bigger payment modal from this task could be also used in attendee-list, but on the second thought it's nice to have two ways to do it - it's going to be easier to get feedback from client.

Important: action button doesn't work if you click on the middle of it, where the + sign is - unfortunately that's where I think it's most intuitive to click - it took me several clicks untill I figured out where to click on it to make it work.

GUI-wise, I love how it + becomes X when list on action button is expanded. I'd love transparent round fa-buttons as options, but that's definitely not the time to spend on polishing it - current version is great for now :).

I'd love cancel button on attendee-details view, it seems natural to view details of one attendee and then go back to others. There is some PR with @droodev task for reception cancel button waiting for him, I'll merge it tomorrow.

I didn't go through and understand all the code yet, so I'll continue tomorrow.

Edit:
I merged @droodev task, but it doesn't cover attendee-details. If you could add similar close button on left it'd be awesome.

tlegutko · 2016-11-01T22:45:51Z

app/Http/Controllers/PaymentMethodsController.php

+  public function index()
+  {
+    // TODO: Replace with proper company fetching
+    /** @var Builder $query */


oh, I didn't know that php convention for type-hinting, interesting :)

tlegutko · 2016-11-03T08:30:05Z

app/Http/Controllers/CoursesController.php

@@ -11,14 +11,6 @@

 class CoursesController extends Controller
 {
-  /**


why is this no longer needed? Is this line in our api.php equivalent (with auth instead of api ofc) ?
Route::group(['middleware' => 'auth'], function(){

Yes, it's thanks to this group we can omitwhole middleware. Moreover - being in the api.php routes file adds automatically api middleware ;)

tlegutko · 2016-11-03T08:34:34Z

app/Http/Controllers/Courses/AttendeesController.php

+
+use App\Http\Controllers\Controller;
+
+class AttendeesController extends Controller


Glad you did separate namespace, it was getting big.

I get that it's in another namespace, but I still wonder if the name isn't confusing with App\Http\Controllers\AttendeesController. But I see the idea behind this convention, so maybe it'll work.

And CoursesController should fit in this namespace too :).

Why? This is natural to add course-related things to Courses namespace, but it's not wise to add the Course controller here as well. As an example take the REST API conventions ;)

tlegutko · 2016-11-03T08:47:12Z

app/Models/CompanyPaymentType.php

+
+  public function company()
+  {
+    return $this->hasOne(Company::class, 'id', 'company_id');


Isn't 'many-to-many' modelled using $this->belongsToMany on both sides, in this case in both Company and PaymentType ? https://laravel.com/docs/5.3/eloquent-relationships#many-to-many

I mean I guess it'll work and we get relationship table more explicitly than by using the pivot attribute, but it's different from docs and we'll soon need some another many-to-many relationships (attendees_presence, course_has_instructor, attendee_has_course), so I'd like to understand difference here.

I think sticking to docs would be easier, but I might be missing why this approach is better

You are missing the idea. This CompanyPaymentType is assignment of the selected payment type to selected company. We also have deposit value that is used to define how much money you have to take to let someone enter which may vary between companies.

This special many-to-many might work for simple relations, but using this we got simply more power and control over it.

tlegutko · 2016-11-03T09:07:21Z

frontend/src/app/attendee/add-payment/add-payment.component.ts

+  ngOnInit() {
+    this.paymentService.list().subscribe(
+      (methods) => this.methods = methods, () => {
+        this.notifications.error('Błąd', 'Nie udało się pobrać metod płatności');


~~this notification is already fired in PaymentMethodsResolve, I think one's enough~~

Edit: Oh wait, it's a modal and from what I see it doesn't have distinct routing path, so it doesn't use resolve at all. But on the other hand it's url indicates it should be a child of course-details/:course-id which already has this data, but it's not, because they attendee-details don't share anything view-wise with course-details.. now I remember that discussion from Marek's PR.

I think they should share title in general, but for now let's leave it, too much changes. it could theoretically be just passed as parameter from attendee-details page, but this way it's fetched only when needed, ok.

I've searched for a way to replace selected router-outlet with the views (we are using it with UI-Router in AngularJS), but I couldn't find it.

tlegutko · 2016-11-03T11:01:26Z

frontend/src/app/attendee/add-payment/add-payment.component.html

@@ -0,0 +1,25 @@
+<div class="modal-header">


It seems very similar to payment-confirmation.component, but I guess you're right, it'd be more hassle than gain to reuse it.

This is similar BUT very different. PaymentConfirmation component is just for confirming that the payment you have selected is correct, while this one is to select payment type.

…ront-end. Moved attendee-details title line-up and action button to the top in tablet view.

tlegutko · 2016-11-05T22:11:48Z

I did commit with some changes, please pull / look at it before continuing!

tlegutko · 2016-11-05T22:13:23Z

database/migrations/2016_10_30_093750_create_company_payment_type_table.php

+use Illuminate\Database\Schema\Blueprint;
+use Illuminate\Support\Facades\Schema;
+
+class CreateCompanyPaymentTypeTable extends Migration


I think now we don't need this migration when this relation is created with belongsToMany().

We need it as models are not parsed during migration and we need real table to exist. Checked it.

tlegutko · 2016-11-05T22:26:25Z

app/Http/Controllers/Courses/PaymentController.php

+
+    $values = $request->only(['payment_type_id', 'attendee_id']);
+    $values['course_id'] = $id;
+    $values['amount'] = $course->getAttribute('price');


As discussed on Slack, I think it's incorrect. I propose to create additional field on relation's table (pivot), maybe amount?

And I realized we should be able to store information about paying deposit itself (and allow this use-case in front-end). I think it should be a separate task. I'm not sure how to model it, especially that I think the use-case with deposits is that attendee's unnotified absence means some amount is subtracted from it. But for now I think modelling it as extra payment (and returning deposit as negative payment?) should be good enough.

If you agree, can you create such task / describe how you think we should do it? I think in the scope of this task just fixing this line above would be perfect.

TBH it's more complex than you think and I propose to do this in separate task. I think you might be too much into our single client as well in this example. I also think that with other systems like Benefit you are right that payment is really only a part of whole price.

megawebmaster added 3 commits October 31, 2016 20:13

[DNCR-41] Create backend for manual payment

418e1ad

[DNCR-41] Add support for payment in reception modal

92783f5

[DNCR-41] Add new payment modal to reception

b4799a9

megawebmaster force-pushed the DNCR-41 branch from 1735155 to b4799a9 Compare October 31, 2016 19:49

tlegutko force-pushed the DNCR-116 branch 2 times, most recently from e7c7ed8 to f6c3fff Compare November 1, 2016 21:49

tlegutko requested changes Nov 1, 2016

View reviewed changes

tlegutko requested changes Nov 3, 2016

View reviewed changes

megawebmaster added 3 commits November 5, 2016 16:16

Merge branch 'DNCR-108' into DNCR-116

978ac59

Merge branch 'DNCR-116' into DNCR-41

8ccfbfd

[DNCR-41] Improve attendee-details view and payment types relations

63acf05

megawebmaster force-pushed the DNCR-116 branch 2 times, most recently from 3ca8bcf to 3d4490d Compare November 5, 2016 16:44

Merge branch 'DNCR-116' into DNCR-41

15a713e

tlegutko changed the base branch from DNCR-116 to develop November 5, 2016 19:52

tlegutko added 2 commits November 5, 2016 20:53

Merge branch 'develop' into DNCR-41

ee87475

[DNCR-41] Added amount field to Payment. Renamed payment service on f…

f6a6319

…ront-end. Moved attendee-details title line-up and action button to the top in tablet view.

tlegutko requested changes Nov 5, 2016

View reviewed changes

Merge branch 'develop' into DNCR-41

fd94155

tlegutko approved these changes Nov 6, 2016

View reviewed changes

tlegutko merged commit b7b4c09 into develop Nov 6, 2016

tlegutko deleted the DNCR-41 branch November 6, 2016 09:39

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[DNCR-41] Add manual payment option #35

[DNCR-41] Add manual payment option #35

megawebmaster commented Oct 31, 2016

tlegutko left a comment •

edited

Loading

tlegutko Nov 1, 2016

tlegutko Nov 3, 2016

megawebmaster Nov 5, 2016

tlegutko Nov 3, 2016

megawebmaster Nov 5, 2016

tlegutko Nov 3, 2016

megawebmaster Nov 5, 2016

tlegutko Nov 3, 2016

megawebmaster Nov 5, 2016

tlegutko Nov 3, 2016

megawebmaster Nov 5, 2016

tlegutko commented Nov 5, 2016

tlegutko Nov 5, 2016

megawebmaster Nov 6, 2016

tlegutko Nov 5, 2016

megawebmaster Nov 6, 2016


		use App\Http\Controllers\Controller;

		class AttendeesController extends Controller

[DNCR-41] Add manual payment option #35

[DNCR-41] Add manual payment option #35

Conversation

megawebmaster commented Oct 31, 2016

tlegutko left a comment • edited Loading

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

tlegutko commented Nov 5, 2016

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

tlegutko left a comment •

edited

Loading