[DNCR-27] Course creation #25

tlegutko · 2016-10-02T16:03:17Z

Things not yet working / known issues:

css detail - I couldn't make 'save' button in disabled state, not that important.
css detail - when there are multiple errors in backend form validation, it'll look sloppy, but it's unlikely, so not that important
css detail - I couldn't make a placeholder for instructor and location options, I tried hidden and disabled and selected, but couldn't figure it out in reasonable time
I didn't add proper datepicker, but created task for it: https://trello.com/c/hdWq1x8D
I added task for course-edition https://trello.com/c/iyCC4DRn/104-mened-er-edycja-kursu
Instructors should have many-to-many relation with courses, but @szimin18 did a lot of changes in instructors area in his task, so I created separate task for it: https://trello.com/c/Lov1fALb
Mobile view looks bad. I can create separate task for fixing it - I didn't want to spend even more time on tihs task (and block several other tasks) and I think it's not our top top priority now. But maybe some different columns decisions could make a quick fix?

…cripts dartisan.sh and prepenv.sh.

…component. Removed start_date parameter from front-end request.

megawebmaster · 2016-10-07T07:08:58Z

app/Http/Controllers/CoursesController.php

+    // TODO remove mock company_id above and inject real one once DNCR-92 is merged
+    $course = Course::create($courseData);
+
+    $start_date_from_request = ['start_date' => (new DateTime($request->start_time))->format('Y-m-d')];


Either camelCase or snake_case and I think we stick to the camelCase.

megawebmaster · 2016-10-07T09:19:21Z

app/Http/Controllers/CoursesController.php

-    return response()->json(Course::create($request->all()));
+    $courseData = $request->only('name', 'price', 'classes_count', 'description', 'seats_count') + ['company_id' => 1];
+    // TODO remove mock company_id above and inject real one once DNCR-92 is merged
+    $course = Course::create($courseData);


What about transaction for this? https://laravel.com/docs/5.2/database#database-transactions

megawebmaster · 2016-10-07T09:24:02Z

config/auth.php

@@ -100,7 +100,7 @@
            'provider' => 'users',
            'email' => 'auth.emails.password',
            'table' => 'password_resets',
-            'expire' => 60,
+            'expire' => 120,


Why did you prolong token validation? We need to add token refreshing in our frontend app, that's all ;)

megawebmaster · 2016-10-07T09:25:22Z

frontend/src/app/_commons/form-field/form-field.component.ts

@@ -7,7 +7,7 @@ import { Component, Input } from '@angular/core';
      <div class="form-group" [class.has-danger]="errors">
        <ng-content></ng-content>
        <div class="form-control-feedback" *ngIf="errors">
-          <span *ngFor="let error of errors">{{ error }}</span>
+          <span *ngFor="let error of errors">{{ error }} </span>


This probably needs to be converted into a list, but as agreed - we don't have to cover this right now.

megawebmaster · 2016-10-07T09:28:04Z

frontend/src/app/course/course.model.ts

@@ -21,3 +24,62 @@ export class CourseTime {
 export class CourseEvent {
  date: string;
 }
+
+export class CreateCourseRequest {


I'm not sure, but I think this is equivalent to Course model, isn't it? The Course should have required fields for submodels (like CourseTime). Why did you add this class?

At the moment of creating this there was no Course model and I thought it'd be easier to create flat request structure and make it nested later when adding multiple courseTimes (https://trello.com/c/JtGMgJvu). It's up to you - should I change it now or leave it for mentioned task?

OK, leave it for now, let's just proceed with enhancements.

megawebmaster · 2016-10-07T10:13:40Z

frontend/src/app/manager/courses/manager-calendar/manager-calendar.component.ts

-    return events;
+  public ngOnInit(): void {
+    this.service.calendarEvents().subscribe((events) => this.events = events);
+    this.service.courseUpdates.subscribe(


IMO this should be called courseCreated instead of courseUpdates - because in case of update you need to get whole list and re-render everything. In case of creation - just add the events as you do here.

megawebmaster · 2016-10-07T10:15:07Z

frontend/src/app/manager/courses/manager-calendar/manager-calendar.component.ts

@@ -43,6 +44,7 @@ export class ManagerCalendarComponent {

  onDayClick(e: CalendarDayClick) {
    console.log('clicked on day in manager with date: ' + e.date.format());
-    this.router.navigate(['/manager/courses']);
+    this.service.broadcastCalendarDateClick(e.date);


As I said earlier - this should be replaced with calendar event (I mean @Output() here) so the component above properly handles the click. This will allow us to easier control what is done where and how it works in general (see how React is built for example - with explicit control and data flows).

Hm, the thing is now it's only a click that redirects to page / updates the date input. Do you mean to create and insert event on calendar on click (as google calendar does on click) and broadcast it instead of date only? I think it'd look good, but maybe let's do a separate task for it? There are some quirks as to how to handle title, connect drag & drop with create-course form etc.

As said on Slack - I wanted it to be more tree-like passing data, but it seems impossible to do. Leave it as it is now.

megawebmaster · 2016-10-07T10:15:59Z

frontend/src/app/manager/courses/manager-calendar/manager-calendar.component.ts

@@ -43,6 +44,7 @@ export class ManagerCalendarComponent {

  onDayClick(e: CalendarDayClick) {
    console.log('clicked on day in manager with date: ' + e.date.format());
-    this.router.navigate(['/manager/courses']);
+    this.service.broadcastCalendarDateClick(e.date);
+    this.router.navigate(['/manager/courses/create-course']);


This navigation should also be moved to the upper component as the calendar should display data only, not redirect.

I don't yet feel that it's wrong, but I'll trust you with handling it outside of manager-calendar.

On the other hand - how about redirection in CoursesService? I'm having this problem in DNCR-103, where there is course details (& edit) page and Save button is separated from CourseEditComponent (CreateCourseComponent in this task) with <router-outlet> and Service seems like the best place to handle both request and redirection, so the same action (creation and update) is handled in one place.

I may have put it in a convoluted way, bottom line is - do you think it's a good idea to handle route changes in service?

I don't. I think it should be like this: this.service.update(course).subscribe((course) => this.router.navigate(...));. Why? Because the service is responsible for updating the entity only (SRP) and the component is responsible for proper redirection in case of success.

Well I started migrating code that navigates router from manager-calendar to ManagerCoursesComponent (it's parent) and I stopped, because it seems pointless. Think about it. We have CalendarComponent which wraps calendar from ngPrime. So it only displays data and outputs changes with our interfaces, okay. Then we have ManagerCalendarComponent and ReceptionCalendarComponent which add little customization (default view, ability to edit) and are components surrounding calendar, they subscribe to course service to fetch & update data.
And I see three approaches:

they handle actions on clicks, whether it's notification to courses.service or route change, as it is now.

they broadcast all changes and pass event handling to parent component, but then ManagerCoursesComponent and Reception have to duplicate all event handling interfaces and ManagerCalendar and ReceptionCalendar are additional layer of wrappers that doesn't do much.

handle some of the events and leave some handling to upper component (so in this commented line - should ManagerCalendar tell service to broadcast click and then emit change so it's parent changes route? where's the line of responsibility? now that sounds like breaking SRP to me.

I think you're expecting something between 2 and 3, I don't see it clearly. I think leaving 1 is the simplest approach.

EDIT: And think about encapsulating calendar interface - I think it'd be good for ManagerCalendar and ReceptionCalendar to be the last ones to have to know about calendar details. And then all the calendar-handling-logic is in one place and we don't have additional layer of abstraction that does almost nothing. The 'dumb displaying of data and propagating events' is already done in CalendarComponent..

I won't agree with you on that as you seem to miss the point. How can calendar know what to do with selected course? It's up to the container to decide and this is why ManagerCalendarComponent became responsible for multiple purposes: showing data (with fetching them!) and navigating to details page (what if we wanted it to behave differently when used in different place?). The reason to abstract it is to make sure that we can reason about the program easily and this means we can open ManagerCoursesComponent to know what we want to know.

This is course clicked event rather than date clicked event so we are abstracting things and pushing "hey, somebody clicked on course 1" to the parent.

BTW. I really would like to hear me as I worked on large project in AngularJS and I know this simple things will make it far easier to update and maintain in the future.

megawebmaster · 2016-10-07T10:19:49Z

frontend/src/app/manager/manager.module.ts

    ],
    imports: [
      BrowserModule, FormsModule, RouterModule.forChild(routes), CommonsModule, AttendeeModule, CourseModule
+    ],
+    providers: [
+      CourseResolve, InstructorsService, LocationsService


Any reason to add CourseResolve here?

megawebmaster · 2016-10-07T10:23:34Z

public/index.css

@@ -0,0 +1,15 @@
+app > span.loader {


This file was in .git-ignore and was removed on purpose. Please remove it again.

… frontend. Todo csses and removal of errors.

megawebmaster · 2016-10-08T06:39:36Z

app/Http/Controllers/CoursesController.php

+    {
+      $course = Course::create($courseData);
+      $courseTime = $course->times()->create($courseTimeData);
+      $courseTime->course()->associate($course);


Is this line needed? I thought when you do $course->times()->create() it properly associates it back to itself.

megawebmaster · 2016-10-08T06:42:53Z

app/Http/Controllers/CoursesController.php

+      {
+        $date = clone($start_date);
+        $event = new CourseEvent(['date' => $date->add(new DateInterval("P{$week}W"))]);
+        $event->courseTime()->associate($courseTime);


Instead of these 2 lines you could do $courseTimes->events()->create(...); - we have transaction so it does not hurt us anymore.

megawebmaster · 2016-10-08T06:43:46Z

app/Http/Controllers/CoursesController.php

+      return $course;
+    });
+
+    $course->load('times.events');


Why is this needed? We have added everything as we should so it has all the data it needs, right?

We need it because this single returned course is added to calendar in front-end, so we need it's times and events so CalendarItems can be parsed from it.

But as you see the $course already contains all events and times you need, don't you?

megawebmaster · 2016-10-08T06:49:40Z

frontend/src/app/course/course.model.ts

@@ -21,3 +24,62 @@ export class CourseTime {
 export class CourseEvent {
  date: string;
 }
+
+export class CreateCourseRequest {


OK, leave it for now, let's just proceed with enhancements.

megawebmaster · 2016-10-08T06:50:56Z

frontend/src/app/course/courses.service.ts

  constructor(private http: AuthHttp) {
+    this.recentlyClickedTimeSource.next(CreateCourseTime.defaultTime()); // default for page refresh


Well, as this is a prototype simply remove this line. We won't ever need default time and when we click to create term - we just get the time we want, that's all ;)

megawebmaster · 2016-10-08T07:27:37Z

frontend/src/app/manager/courses/create/course-times/course-times.component.html

+
+<p class="alert alert-danger" role="alert" *ngIf="error">{{ error }}</p>
+<form-field [errors]="errors.startTime" class="col-xs-12 row moved-errors">
+  <label for="startTime" class="col-xs-3 col-xl-2 col-form-label test">Rozpoczęcie</label>


Explained on Slack - if you need help here please ask ;)

megawebmaster · 2016-10-08T07:28:29Z

frontend/src/app/manager/courses/create/course-times/course-times.component.ts

+
+  public ngOnInit() {
+    this.model.repeatWeeksCount = 1;
+    this.coursesService.recentlyClickedTime.subscribe(


I have strong feeling it does not belong here, but this should be updated when multiple terms are being worked on.

megawebmaster · 2016-10-08T07:30:16Z

frontend/src/app/manager/courses/manager-calendar/manager-calendar.component.ts

@@ -43,6 +44,7 @@ export class ManagerCalendarComponent {

  onDayClick(e: CalendarDayClick) {
    console.log('clicked on day in manager with date: ' + e.date.format());
-    this.router.navigate(['/manager/courses']);
+    this.service.broadcastCalendarDateClick(e.date);


As said on Slack - I wanted it to be more tree-like passing data, but it seems impossible to do. Leave it as it is now.

megawebmaster · 2016-10-08T07:32:04Z

frontend/src/app/manager/courses/manager-calendar/manager-calendar.component.ts

@@ -43,6 +44,7 @@ export class ManagerCalendarComponent {

  onDayClick(e: CalendarDayClick) {
    console.log('clicked on day in manager with date: ' + e.date.format());
-    this.router.navigate(['/manager/courses']);
+    this.service.broadcastCalendarDateClick(e.date);
+    this.router.navigate(['/manager/courses/create-course']);


I don't. I think it should be like this: this.service.update(course).subscribe((course) => this.router.navigate(...));. Why? Because the service is responsible for updating the entity only (SRP) and the component is responsible for proper redirection in case of success.

megawebmaster · 2016-10-08T07:32:47Z

frontend/src/app/manager/manager.module.ts

@@ -68,14 +77,22 @@ export const routes = [
      ManagerCoursesSingleComponent,
      ManagerCoursesDetailsComponent,
      ManagerCoursesAttendeesComponent,
+


Empty line here? :)

…Improved observables code style in CoursesService. Removed toast error handling and added date check.

tlegutko · 2016-10-08T15:49:19Z

frontend/src/app/manager/courses/create/course-properties/course-properties.component.html

+    <option *ngFor="let i of instructors" [value]="i.id">{{i.name + ' ' + i.surname}}</option>
+  </select>
+</labelled-form-field>
+<labelled-form-field [errors]="errors.locationId" [labelFor]="model.locationId" [label]="'Sala'">


Before you comment that it could be just [label]="Sala" -> yes it could, but in most of other places we have either space or polish character with which this syntax doesn't work and single quotes are needed, so I think it's better to just use the same syntax in whole file.

…our. Course title responsivness left for some other time.

megawebmaster · 2016-10-10T06:28:25Z

app/Http/Controllers/CoursesController.php

+      return $course;
+    });
+
+    $course->load('times.events');


But as you see the $course already contains all events and times you need, don't you?

megawebmaster · 2016-10-10T06:31:45Z

frontend/src/app/_commons/labelled-form-field/labelled-form-field.component.ts

+    selector: 'labelled-form-field',
+    template: `
+      <form-field [errors]="errors" class="row m-r-0">
+        <label for="labelFor" class="col-xs-12 col-sm-3 col-xl-2 col-form-label">{{ label }}</label>


for="{{ labelFor }}"

a tutaj znalazłem wyjaśnienie, czemu angular/angular#3803
ale na szczęście [attr.for]="labelFor" już śmiga. przy okazji znalazłem buga, że podawałem tam pole modelu, a nie id obiektu, więc podwójne dzięks :).

megawebmaster · 2016-10-10T06:46:52Z

frontend/src/app/_commons/labelled-form-field/labelled-form-field.component.ts

+)
+export class LabelledFormField {
+  @Input() errors: string[] = [];
+  @Input() labelFor: string;


Why not simple for?

megawebmaster · 2016-10-10T06:50:22Z

frontend/src/app/manager/courses/create/course-properties/course-properties.component.ts

@@ -0,0 +1,33 @@
+import { Component, Input, Output, EventEmitter, OnInit } from '@angular/core';
+import { InstructorsService } from '../../../instructors/instructors.service';


megawebmaster · 2016-10-10T07:00:22Z

frontend/src/app/manager/courses/manager-calendar/manager-calendar.component.ts

@@ -43,6 +44,7 @@ export class ManagerCalendarComponent {

  onDayClick(e: CalendarDayClick) {
    console.log('clicked on day in manager with date: ' + e.date.format());
-    this.router.navigate(['/manager/courses']);
+    this.service.broadcastCalendarDateClick(e.date);
+    this.router.navigate(['/manager/courses/create-course']);


I won't agree with you on that as you seem to miss the point. How can calendar know what to do with selected course? It's up to the container to decide and this is why ManagerCalendarComponent became responsible for multiple purposes: showing data (with fetching them!) and navigating to details page (what if we wanted it to behave differently when used in different place?). The reason to abstract it is to make sure that we can reason about the program easily and this means we can open ManagerCoursesComponent to know what we want to know.

This is course clicked event rather than date clicked event so we are abstracting things and pushing "hey, somebody clicked on course 1" to the parent.

BTW. I really would like to hear me as I worked on large project in AngularJS and I know this simple things will make it far easier to update and maintain in the future.

…endar components.

megawebmaster · 2016-10-11T07:27:13Z

frontend/src/app/course/courses.service.ts

+  courseCreated = this.courseCreatedSource.asObservable();
+  calendarItemsCreated = this.courseCreated.map(this.courseToCalendarItems);
+
+  recentlyClickedTimeSource = new ReplaySubject<CreateCourseTime>(1);


This should be private as well.

megawebmaster · 2016-10-11T07:30:24Z

frontend/src/app/manager/courses/create/create-course.component.ts

+
+  createCourse() {
+    this.coursesService.create(this.model).subscribe(
+      (course) => {


You know you don't need braces if you have single line command? :)

megawebmaster · 2016-10-11T07:40:42Z

frontend/src/app/reception/reception.component.ts

+    this.router.navigate(['/reception/course-details', course.id]);
+  }
+
+  onDayClick(moment: Moment) {


No need for this :)

tlegutko added 10 commits October 1, 2016 23:06

[DNCR-27] Created backend course creation, frontend mock and helper s…

025b286

…cripts dartisan.sh and prepenv.sh.

[DNCR-27] Renamed html and scss files to .component.

c4a0ecd

[DNCR-27] Added course-times component.

9f4a581

[DNCR-27] Added course properties view.

f9ba798

[DNCR-27] Added company and location to courses creation.

0463118

[DNCR-27] Form validation on both front-end and back-end.

26f5ad1

[DNCR-27] Added to form locations and instructors fetched from backend.

b7c62c3

[DNCR-27] Moved course request definitions to course module.

f3ee09d

[DNCR-27] Created courses are immediately displayed on calendar.

9c739a9

[DNCR-27] Added passing time from click on calendar to create-course-…

fd1cf3e

…component. Removed start_date parameter from front-end request.

tlegutko force-pushed the DNCR-27 branch from 3ffa5d6 to 199dcc5 Compare October 2, 2016 17:00

[DNCR-27] Added updating course time on calendar click.

7fa1f00

tlegutko force-pushed the DNCR-27 branch from 199dcc5 to 7fa1f00 Compare October 3, 2016 06:08

megawebmaster requested changes Oct 7, 2016

View reviewed changes

[DNCR-27] Added transaction on backend and code style improvements on…

de0847b

… frontend. Todo csses and removal of errors.

tlegutko force-pushed the DNCR-27 branch from 309f682 to de0847b Compare October 8, 2016 06:57

megawebmaster reviewed Oct 8, 2016

View reviewed changes

[DNCR-27] Simplified course creation code in transaction on backend. …

df487a1

…Improved observables code style in CoursesService. Removed toast error handling and added date check.

tlegutko commented Oct 8, 2016

View reviewed changes

tlegutko force-pushed the DNCR-27 branch from ff58682 to 7aeec40 Compare October 8, 2016 15:52

[DNCR-27] Implemented labelled-form-field. Improved responsive behavi…

8f114c1

…our. Course title responsivness left for some other time.

tlegutko force-pushed the DNCR-27 branch from 7aeec40 to 8f114c1 Compare October 8, 2016 16:42

[DNCR-27] Fixed multiple form errors styling.

00212b0

megawebmaster requested changes Oct 10, 2016

View reviewed changes

[DNCR-27] Improved labels in labelled field. Moved routing out of cal…

9725767

…endar components.

tlegutko force-pushed the DNCR-27 branch from ebea117 to 9725767 Compare October 10, 2016 15:39

[DNCR-27] Fix small code issues

42acc3d

megawebmaster approved these changes Oct 11, 2016

View reviewed changes

megawebmaster merged commit ef2803d into develop Oct 11, 2016

tlegutko deleted the DNCR-27 branch October 11, 2016 09:58

		constructor(private http: AuthHttp) {
		this.recentlyClickedTimeSource.next(CreateCourseTime.defaultTime()); // default for page refresh

		@@ -0,0 +1,33 @@
		import { Component, Input, Output, EventEmitter, OnInit } from '@angular/core';
		import { InstructorsService } from '../../../instructors/instructors.service';

[DNCR-27] Course creation #25

[DNCR-27] Course creation #25

Conversation

tlegutko commented Oct 2, 2016

megawebmaster Oct 7, 2016 • 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

tlegutko Oct 7, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

megawebmaster Oct 7, 2016 • edited Loading

Choose a reason for hiding this comment

tlegutko Oct 7, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tlegutko Oct 7, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tlegutko Oct 8, 2016 • 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

tlegutko Oct 8, 2016 • 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

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

megawebmaster Oct 7, 2016 •

edited

Loading

tlegutko Oct 7, 2016 •

edited

Loading

megawebmaster Oct 7, 2016 •

edited

Loading

tlegutko Oct 7, 2016 •

edited

Loading

tlegutko Oct 7, 2016 •

edited

Loading

tlegutko Oct 8, 2016 •

edited

Loading

tlegutko Oct 8, 2016 •

edited

Loading