ott-web-app icon indicating copy to clipboard operation
ott-web-app copied to clipboard

Ambiguous interpretation of 'free'

Open marcovandeveen opened this issue 2 years ago • 10 comments

The access attributefree has ambiguous behavior

  • "" --> blocked
  • false --> open to anonymous users
  • true --> open to anonymous users
  • False --> open to anonymous users

NOT TESTED:

  • 'FALSE'
  • 'True'
  • 'TRUE"

Note: this also impact the entitlement service of videodock

marcovandeveen avatar Jul 04 '22 20:07 marcovandeveen

@marcovandeveen for free param we can accept true, True and other case variation as TruE. Other values will be considered to be falsy. However there may also be a problem with requiresSubscription param which we also use. Not only false value is accepted. In case we set it to False or FaLSE, the item would be considered to be free.

It seems that we need a general approach for these cases. I suggest:

  1. We accept false / true string values and their upper / lower case variations.
  2. Empty string value is a falsy one.
  3. Boolean values (not sure if it may happen) have their original values.

https://github.com/jwplayer/ott-web-app/blob/8cc30171fe76282f60f53868ed6f18dcede4aded/src/utils/entitlements.ts#L15

@dbudzins fyi

AntonLantukh avatar Jul 13 '22 14:07 AntonLantukh

I would also accept 0 and 1 as strings to be false and true respectively. Actually, we only need to talk about what is false and then everything else we'll assume to be true. So false is:

  • blank value / empty string
  • any casing of the word false
  • 0

dbudzins avatar Jul 13 '22 14:07 dbudzins

@dbudzins then for free param we can set any other value which would be true. I don't know whether it is what we want.

AntonLantukh avatar Jul 13 '22 14:07 AntonLantukh

Oh I see... some properties may be better defaulted to false if not explicitly true... that's fine. So we do:

  • requiresSubscription - true unless:
    • blank value / empty string
    • any casing of the word false
    • 0
    • any casing of the word No
  • free - false unless:
    • any casing of the word true
    • any non-zero number
    • any casing of the word Yes

Works?

dbudzins avatar Jul 13 '22 15:07 dbudzins

@dbudzins looks great. I would only suggest to use just 1 as a number which equals to true value.

AntonLantukh avatar Jul 13 '22 15:07 AntonLantukh

Ya, that's what I started with, but then I thought maybe companies would have some conventions that we don't know, like free types 2, 3, 4, etc. Maybe I'm overthinking it, but we could do any non-zero decimal perhaps?

dbudzins avatar Jul 13 '22 15:07 dbudzins

@dbudzins makes sense. I also wonder why we decided to use requiresSubscription param as an indication that the item is free (so basically item is not free by default, and it could be more logical to call it isFree or useNoSubscription). For me it seems to be a bit confusing as we also support free which requires the opposite value for the item to be free. Maybe we can just use free for both cases? In this case we won't have to integrate different logic for two params (default true for requiresSubscription and default false for free). @marcovandeveen maybe you have better understanding of the possible reason.

AntonLantukh avatar Jul 14 '22 09:07 AntonLantukh

The reason is that Applicaster uses attribute 'free'.

Consistency between applicaster and web app is more important than having a pure model at this moment. Also, the attribute 'productIds' is not a clean solution.Asking Applicaster to change is too much of a burden now. Especially as we need to productize our access model in future iterations further

marcovandeveen avatar Jul 14 '22 21:07 marcovandeveen

@marcovandeveen I am not against using free param :) I suggest to use it for both us (and get rid of requiresSubscription ) and Applicaster

AntonLantukh avatar Jul 18 '22 08:07 AntonLantukh

I suggest to deprecate requiresSubscription but don't remove it.

Its being used Also documentation still states you need to use 'requiresSubscription'. New docs are in progress

marcovandeveen avatar Jul 19 '22 15:07 marcovandeveen