[Proposal][WIP] List DiscountConditions by type

[olivermrbl]

Goal

We want to be able to retrieve a specific type of conditions from a DiscountCondition. Can be products, collections, tags, types, and customer groups.

Proposal

Endpoint

[GET] /admin/discounts/:discount_id/conditions/:condition_id/types?type=products&offset=0&limit=20

Middleware handler Access control

export async function canAccessDiscountCondition(req, res, next) {
  const { discount_id, condition_id } = req.params

  const discountService: DiscountService = req.scope.resolve("discountService")

  const discount = await discountService.retrieve(discount_id, {
    relations: ["rule", "rule.conditions"],
  })

  const existsOnDiscount = discount.rule.conditions.some(
    (c) => c.id === condition_id
  )

  if (!existsOnDiscount) {
    throw new MedusaError(
      MedusaError.Types.NOT_FOUND,
      `Condition with id ${condition_id} does not belong to Discount with id ${discount_id}`
    )
  }

  next()
}

Controller Let's discuss the response object here. Think we need to think of something else than discount_conditions, since that would not be semantically correct

export default async (req, res) => {
  const { condition_id } = req.params

  const validatedParams = validator(SomeValidatorClassParams, req.params) // { type, offset, limit, ... }

  const discountConditionService: DiscountConditionService = req.scope.resolve("discountConditionService")

  const config = getListConfig<DiscountCondition>(
      ...someFields,
      ...someRelations,
      validatedParams?.fields?.split(",") as (keyof DiscountCondition)[],
      validatedParams?.expand?.split(","),
      validatedParams?.limit,
      validatedParams?.offset,
    )
  
  const discountConditions = await discountConditionService.listAndCountConditionType(config)

  res.json({ discount_conditions: discountConditions })
}

[adrien2p]

Guard few updates:

export async function getRequestedDiscount<TEntity extends BaseEntity>(
  queryConfig?: Pick<QueryConfig<TEntity>, "defaultFields" | "defaultRelations">
) {
  return (req: Request, res: Response, next: NextFunction) => {
    const discountService: DiscountService =
      req.scope.resolve("discountService")

    try {
      const { discount_id } = req.params
      const retrieveConfig = queryConfig
        ? prepareRetrieveQuery(
          {},
          {
            queryConfig.defaultFields as (keyof TEntity)[],
            (queryConfig.defaultRelations ?? []) as string[]
          }
        )
        : (req.retrieveConfig ?? {})
      req.discount = await discountService.retrieve(discount_id, retrieveConfig)
    } catch (error) {
      return next(error)
    }

    next()
  }
}

export async function canAccessDiscountCondition(req: Request, res: Response, next: NextFunction) {
  const { condition_id } = req.params
  
  const discount = req.discount
  const existsOnDiscount = discount.rule.conditions.some(
    (c) => c.id === condition_id
  )

  if (!existsOnDiscount) {
    return next(
      new MedusaError(
        MedusaError.Types.NOT_FOUND,
        `Condition with id ${condition_id} does not belong to Discount with id ${discount_id}`
      )
    )
  }

  next()
}

Handler

export default async (req, res) => {
  const discountConditionService: DiscountConditionService = req.scope.resolve("discountConditionService")
  const discountConditions = await discountConditionService.listAndCountConditionType(req.listConfig)

  /* EDIT */
  res.json({ conditions: discountConditions, type: req.filterableFields.type })
}

[olivermrbl]

Yes, good point. With 1593, we don't need to handle the list config in the controller.

Also, I like this response. We could also do:

res.json({ conditions: discountConditions, type: req.filterableFields.type })

Wdyt?

[adrien2p]

Yes, good point. With 1593, we don't need to handle the list config in the controller.

Also, I like this response. We could also do:

res.json({ conditions: discountConditions, type: req.filterableFields.type })

Wdyt?

Yes this is even better, that way there is no dynamic value and it is much easier to handle on the front side 👍

[srindom]

@kasperkristensen and I have discussed an alternative approach that would be good to get your thoughts on.

The main thought is that the retrieval of the resources on a condition will be delegated to either ProductService, CollectionService, etc. (Will stick to Product for simplicity, but should be extended to the other resource types too)

All the list logic etc. will be handled by the existing admin-list-products controller. This would make it possible to do this:

GET /admin/products?discount_condition_id=[whatever]

To make it simple to discover details about a condition we can change the condition endpoints to work like this:

GET /admin/discounts/:id/conditions

// Response
{
  conditions: [
    {
      id: "cond_1234",
      type: "product",
      products: {
        count: 352,
        limit: 10,
        offset: 0,
        next: "/admin/products?discount_condition_id=cond_1234&offset=10&limit=10",
        items: [
          { 
            id: "prod_...",
            [other product data]
          },
          [9 other products]
        ]
      }
    },
    [rest of the conditions]
  ],
  [list response stuff],
}

In the controller for retrieving conditions we would make use of the productService to perform the actual listing:

const condition = await conditionService.retrieve(id)

const productLimit = 10

const [products, productCount] = await productService.listAndCount({ discount_condition_id: id }, { take: productLimit })
condition.products = {
  limit: productLimit,
  offset: 0,
  count: productCount,
  next: `/admin/products?discount_condition_id=${id}&limit=${productLimit}&offset=10`,
  items: products
}

res.json({ condition })

This way we avoid making tricky implementations within the ConditionService (i.e. listAndCountConditionType) and it will be a super nice API experience.

The biggest issue that I see with this implementation is that it introduces a new pattern for nested arrays (namely the { count, items: [....] pattern`). However, I think we will have to introduce this pattern at some point anyway so this might as well be the first step towards that.

Wdyt?

[adrien2p]

Like the idea @srindom , I would just add something,

I think the easiest idea would be to always limit sub resources to be loaded by a limited number (let say 50 max and we stick with that as a practice) and they must always be ordered by the created_at by default.

If you want then to have the rest of them, you can call the appropriate list end point with the correct expand/fields and manage the pagination from there.

I don't think we need a new collection object-like as the sub resource, we can always compute all the parameters we need to fetch the next one if needed.

That way, no new end points and we only reuse the existing that already provide those features.

we were thinking solving the actual problem, but in fact it will appears on all potential entities with sub resources

[adrien2p]

Finally like the proposal @srindom, after thinking of some use cases, specially if you want to show some stats before going into a page details. The only think is that if we want to provide the next page, i think it should also integrate the previous no?

generally speaking, one thing to think about is that we will need a generic way of building that since the relations can be dynamic, if the user expand them, so we also have to know that a relation will add a collection

[srindom]

Finally like the proposal @srindom, after thinking of some use cases, specially if you want to show some stats before going into a page details.

Just to clarify: you agree with the idea of having:

products: {
  count: xx,
  items: [...],
  [other fields]
}

right? :)

we were thinking solving the actual problem, but in fact it will appears on all potential entities with sub resources

Yes - I think this is a pattern we should eventually introduce everywhere, but we will have to change from relying on the joins by TypeORM to listing through our own services. This will also make everything more modular as services will consume less data that other services are "responsible" for.

[adrien2p]

😅 sorry for the clarity, yes I agree on that approach.

we have to be careful with that, from a perf point of view I think it would be fine since it would always be a limited number of objects that we will manipulate, but from the architecture point of view, we don’t want all the services to inject all the other services.

in the other hand, we could still rely on the join from typeorm, but we would have to add something to check if the relation is a many or a one and if it is a many then limiting the query and add some internal metadata. Maybe through building the join as a custom sub query?? Then later we can build the object-like (don’t know how to name it ^^). Potentially we can override the join methods from typeorm to integrate what we need, specially mapping the items to there respective entities while being in an object not corresponding to the entity, does it make sense? Or just building subQueries that include the pagination and then get the raw query to include it in the join 😂

relying on the services would break (I think) the expand/fields feature, wdyt?

[adrien2p]

After a bit of research, I didn’t find much info on the ability to paginate the relationship’s with typeorm. Which means either going with the services as you mentioned @srindom or the sub query builder as I mentioned. After looking into the code, i can see that the query of the relationship are generally builded in a loop, which means that we could create a sub query builder to limit and prepare the result object and then join the sub query as part of the original query builder. Wdyt?