[Discussion] Move `toReponse` to Entity file

[JustSamuel]

We currently have a lot of service classes with a function along the lines of revisionToResponse, take for example the banner-service.ts

  /**
   * Creates a banner response from a banner
   * @param {Banner.model} banner - banner
   * @returns {BannerResponse.model} - a banner response created with the banner
   */
  public static asBannerResponse(banner: Banner): BannerResponse {
    if (!banner) {
      return undefined;
    }

    let image;
    if (!banner.image) {
      image = null;
    } else {
      image = banner.image.downloadName;
    }

    return {
      id: banner.id,
      name: banner.name,
      image,
      duration: banner.duration,
      active: banner.active,
      createdAt: banner.createdAt.toISOString(),
      updatedAt: banner.updatedAt.toISOString(),
      startDate: banner.startDate.toISOString(),
      endDate: banner.endDate.toISOString(),
    };
  }

However I think it might be nicer to move this code to the entity? This also forces the idea that service deal with entities, not responses, and moves that in a more clearer fashion to the controller.

This is the current banner.ts

@Entity()
export default class Banner extends BaseEntity {
  @Column()
  public name: string;

  @Column({
    type: 'integer',
  })
  public duration: number;

  @Column({
    default: false,
  })
  public active: boolean;

  @Column({
    type: 'datetime',
    default: () => 'CURRENT_TIMESTAMP',
  })
  public startDate: Date;

  @Column({
    type: 'datetime',
  })
  public endDate: Date;

  // onDelete: 'CASCADE' is not possible here, because removing the
  // image from the database will not remove it form storage
  @OneToOne(() => BannerImage, { nullable: true, onDelete: 'RESTRICT' })
  @JoinColumn()
  public image?: BannerImage;
}

How about we add a .toResponse function? I suggest we add a param base which indicates if it needs any of its relations loaded, in the case of a base it will only return a response with its own properties. This is also way clearer to the programmer what a base response should be.

Using the entity based approach we can also make use of the extensions from the base entity, by calling .super for .toResponse

See this commit / branch as en example: https://github.com/GEWIS/sudosos-backend/commit/f7b31b520cf0d4e383df37946ca746b2674159c4

[Yoronex]

Looks nice, but we need to watch out that this might not work everywhere. In many places, we use Object.assign() to create new entities, but these entities do not necessarily have all entity attributes, especially methods will be missing.

Regarding the base parameter, I think it is much better to just define two separate methods, as that deals correctly with return types. Then the asBaseResponse() method returns a BaseResponse and the asResponse() method returns a Response type.

[JustSamuel]

Looks nice, but we need to watch out that this might not work everywhere. In many places, we use Object.assign() to create new entities, but these entities do not necessarily have all entity attributes, especially methods will be missing.

Should be fine? We usually save them in the meantime. I don't think there will be a lot of issues, but we could always check.

Regarding the base parameter, I think it is much better to just define two separate methods, as that deals correctly with return types. Then the asBaseResponse() method returns a BaseResponse and the asResponse() method returns a Response type.

Agree