Rule-based playlists

[diamond-darrell]

PR information

What kind of change does this PR introduce? (check one with "x")

• [] Bugfix
• Feature
• [] Code style update (formatting, local variables)
• [] Refactoring (no functional changes, no api changes)
• [] Build related changes
• [] CI related changes
• [] Other... Please describe:

What is the current behavior? (You can also link to an open issue here)

What is the new behavior?

Does this PR introduce a breaking change? (check one with "x")

• [] Yes
• [] No

If this PR contains a breaking change, please describe the impact and migration path for existing applications: ...

---

Other information

Where should the reviewer start?

How should this be manually tested?

Any background context you want to provide?

---

This change is 

[diamond-darrell]

Review status: 0 of 60 files reviewed at latest revision, 1 unresolved discussion.

---

src/applications/content-playlists-app/playlist/playlist-content/rule-based/playlist-rule/playlist-rule-parser.service.ts, line 22 at r6 (raw file):

  }

  // TODO [kmcng] get rid of duplicated requests for metadataProfiles

+@esakal where should I move this metadata profile loading. It's needed to parse the server value of advanced filter to the application format. The reverse parsing is done in the data-provider service as we discussed

---

Comments from Reviewable

[esakal]

Reviewed 29 of 66 files at r1, 4 of 31 files at r2, 17 of 35 files at r3, 11 of 18 files at r5, 7 of 8 files at r6, 1 of 2 files at r7, 2 of 2 files at r8.
Review status: all files reviewed at latest revision, 26 unresolved discussions, some commit checks failed.

---

a discussion (no related file):
I think this place holder is not the desired although it was taken from invision.
confirm with Amir that the value for the placeholder should be 'Rule Name'

---

a discussion (no related file):
did you fix he problem when trying to open a rule with local changes without sending to the server

---

src/app-config/index.ts, line 186 at r8 (raw file):

        "contentPlaylists" : {
            "createdAtDateRange" : "2005:2030",
            "bulkActionsLimit": 2,

shouldn't it be 50?

---

src/applications/content-entries-app/entry/entry-store.service.ts, line 346 at r8 (raw file):

				if (response.allowed)
				{
					this._router.navigate(["entry", entryId, "metadata"],{ relativeTo : this._entryRoute.parent});

wasn't it taken from default routing to entry? why is this needed?

---

src/applications/content-playlists-app/content-playlists-app.routes.ts, line 26 at r8 (raw file):

          { path: 'metadata', component: PlaylistMetadataComponent, data: { sectionKey: PlaylistWidgetKeys.Metadata } },
          { path: 'content', component: PlaylistContentComponent, data: { sectionKey: PlaylistWidgetKeys.Content } },
          { path: 'content', component: PlaylistContentComponent, data: { sectionKey: PlaylistWidgetKeys.ContentRuleBased } },

I'm not sure how it works for you, maybe the second is overriding the previous?

anyhow I assume you did it for the route to key mapping?
Consider moving the routing to the service with a function like private _mapRoutes(key: PlaylistWidgetKeys): string

---

src/applications/content-playlists-app/playlist/playlist-content/rule-based/rule-based-content-widget.service.ts, line 48 at r8 (raw file):

  protected onValidate(wasActivated?: boolean): Observable<{ isValid: boolean }> {
    if (this.data.playlistType === KalturaPlaylistType.dynamic) { // validate only rule-based playlist
      return Observable.of({ isValid: !wasActivated || !!this.rules.length });

should it be valid if was not activated? if not activated how do you allow saving it even if no rules were found?

consider checking data.filters if wasn't activated and this.rules otherwise

---

src/applications/content-playlists-app/playlist/playlist-content/rule-based/rule-based-content-widget.service.ts, line 57 at r8 (raw file):

    if (data.playlistType === KalturaPlaylistType.dynamic) { // handle only rule-based playlist
      if (typeof data.totalResults === 'undefined' || data.totalResults <= 0) {
        data.totalResults = environment.modules.contentPlaylists.ruleBasedTotalResults;

is this the default value? if so it should be set when loading the data and then here it should always be available

---

src/applications/content-playlists-app/playlist/playlist-content/rule-based/rule-based-content-widget.service.ts, line 90 at r8 (raw file):

      .cancelOnDestroy(this, this.widgetReset$)
      .map(responses => {
        responses.forEach(({ result = [] }, index) => {

how do you handle errors if you are asking for result with default value?

---

src/applications/content-playlists-app/playlist/playlist-content/rule-based/rule-based-content-widget.service.ts, line 106 at r8 (raw file):

        this._updateDurationAndCount();
        super._hideLoader();
        this._state.next({ loading: false, error: false });

why do you manage loading and error state both in the super and in this class?

---

src/applications/content-playlists-app/playlist/playlist-content/rule-based/rule-based-content-widget.service.ts, line 171 at r8 (raw file):

        this._moveDownRules([rule]);
        break;
      case 'view':

ActionSelected is part of the component logic. it is ok to leave it here if you although it is better to keep component logic in the component.ts and not in its service.

the view action is different from the others because it is pure component logic that doesn't involve the service. consider moving that action to the component

---

src/applications/content-playlists-app/playlist/playlist-content/rule-based/rule-based-content-widget.service.ts, line 187 at r8 (raw file):

  }

  public updateRules(rule: PlaylistRule): void {

shouldn't you handle only add new rule? because existing rules already has selection id and are part of rules

---

src/applications/content-playlists-app/playlist/playlist-content/rule-based/rule-based-content-widget.service.ts, line 190 at r8 (raw file):

    const relevantRuleIndex = this.rules.findIndex(item => item.selectionId === rule.selectionId);
    if (relevantRuleIndex !== -1) {
      this.rules[relevantRuleIndex] = rule;

can you elaborate when this use case is relevant?

---

src/applications/content-playlists-app/playlist/playlist-content/rule-based/rule-based-content.component.ts, line 34 at r8 (raw file):

    this._widgetService.attachForm();

    this._widgetService.selectedRule$.subscribe(rule => {

are you missing .cancelOnDestroy?

---

src/applications/content-playlists-app/playlist/playlist-content/rule-based/rule-based-content.component.ts, line 43 at r8 (raw file):

      .filter(state => state.loading)
      .subscribe(() => {
        this._clearSelection();

I understand the use case but since this is the first time I'm seeing such an implementation I'm wondering if this is needed, which use cases does it address?

---

src/applications/content-playlists-app/playlist/playlist-content/rule-based/rule-based-content.component.ts, line 67 at r8 (raw file):

  public _saveRule(rule: PlaylistRule): void {
    this._selectedRule = null;
    this._widgetService.updateRules(rule);

see my comments in the service

---

src/applications/content-playlists-app/playlist/playlist-content/rule-based/playlist-rule/playlist-entries-data-provider.service.ts, line 62 at r8 (raw file):

  }

  public getServerFilter(data: EntriesFilters, forRequest = true): Observable<KalturaMediaEntryFilterForPlaylist> {

consider renaming forRequest to something more self-explained... maybe mediaTypesDefault`...

---

src/applications/content-playlists-app/playlist/playlist-content/rule-based/playlist-rule/playlist-rule-parser.service.ts, line 22 at r6 (raw file):

Previously, diamond-darrell wrote…

+@esakal where should I move this metadata profile loading. It's needed to parse the server value of advanced filter to the application format. The reverse parsing is done in the data-provider service as we discussed

It is fine to use this request here since _metadataProfileService is caching the results. wdyt?

---

src/applications/content-playlists-app/playlist/playlist-content/rule-based/playlist-rule/playlist-rule-parser.service.ts, line 31 at r8 (raw file):

      })
      .cancelOnDestroy(this)
      .first()

are you sure you need the first operator? because it is a request that should complete it self

---

src/applications/content-playlists-app/playlist/playlist-content/rule-based/playlist-rule/playlist-rule.component.ts, line 22 at r8 (raw file):

export class PlaylistRuleComponent {
  @Input() set rule(value: PlaylistRule) {  // rule-based playlist specific
    if (value) {

I understand that this logic is relevant for editing while the defaults are for new rule?

consider moving this logic into _prepare() that will be executed from onInit() because:

• it is hard to flow/understand the flow by other future developers
• you want to handle it only when opening the view I assume. using setter property will handle it anytime the value changes.

---

src/applications/content-playlists-app/playlist/playlist-content/rule-based/playlist-rule/playlist-rule.component.ts, line 31 at r8 (raw file):

      this._saveBtnLabel = this._appLocalization.get('applications.content.playlists.save');

      this._playlistRuleParser.toEntriesFilters(value)

is this operation async or operation that can fail? if so it should be managed as we usually are doing (loading, errors and retries). if you will decide to use a _prepare() method as suggested above it should fit naturally.

---

src/applications/content-playlists-app/playlist/playlist-content/rule-based/playlist-rule/playlist-rule.component.ts, line 113 at r8 (raw file):

    if (ruleName) {
      this._playlistRuleParser.toPlaylistRule({

is this an async operation or operation that can fail? if so you should manage the flow (loading, error, retry)

---

src/applications/content-playlists-app/playlist/playlist-content/rule-based/playlist-rules-table/playlist-rules-table.component.ts, line 55 at r8 (raw file):

    let loadedOnce = false;

    this._widgetService.state$

we moved the loading/errors logic in other tables to the list component. please do the same with this table. once the refactoring is complete I think you will not needed to use _widgetService in this component

---

src/applications/content-playlists-app/playlist/playlist-details/playlist-details.component.ts, line 24 at r8 (raw file):

      .subscribe(data => {
        this._currentPlaylist = data;
        this._isNew = !this._currentPlaylist.id;

ensure that _currentPlaylist is defined

---

src/shared/content-shared/playlist-rule.interface.ts, line 1 at r8 (raw file):

import { KalturaMediaEntryFilterForPlaylist } from 'kaltura-ngx-client/api/types/KalturaMediaEntryFilterForPlaylist';

Is this file in content-shared on purpose instead of part of playlists app?

---

src/shared/content-shared/entries/pipes/entries-total-duration.pipe.ts, line 8 at r8 (raw file):

  transform(entries: KalturaMediaEntry[] = []): number {
    return Array.isArray(entries) ? entries.reduce((total, entry) => total + entry.duration, 0) : 0;

since this action might be performance heavy, check if you can calculate this value in the relevant component instead and bind to a property

---

src/shared/kmc-shared/playlist-creation/create-new-playlist.event.ts, line 9 at r8 (raw file):

  description?: string;
  playlistContent?: string; // entry ids separated by comma
  ruleBasedSub?: boolean;

check if you need some further cleanups

---

Comments from Reviewable

[diamond-darrell]

Review status: 55 of 60 files reviewed at latest revision, 26 unresolved discussions.

---

a discussion (no related file):

Previously, esakal (Eran Sakal) wrote…

I think this place holder is not the desired although it was taken from invision.
confirm with Amir that the value for the placeholder should be 'Rule Name'

OK

---

src/app-config/index.ts, line 186 at r8 (raw file):

Previously, esakal (Eran Sakal) wrote…

shouldn't it be 50?

probably copy-paste from another place, OK

---

src/applications/content-entries-app/entry/entry-store.service.ts, line 346 at r8 (raw file):

Previously, esakal (Eran Sakal) wrote…

wasn't it taken from default routing to entry? why is this needed?

I don't remember exactly, but when we discussed routing between sections for playlists, there was some issue with default tab for manual and rule-based playlist and along with this discussion you said me to set hardcoded value for entry's section to "metadata".

---

src/applications/content-playlists-app/content-playlists-app.routes.ts, line 26 at r8 (raw file):

Previously, esakal (Eran Sakal) wrote…

I'm not sure how it works for you, maybe the second is overriding the previous?

anyhow I assume you did it for the route to key mapping?
Consider moving the routing to the service with a function like private _mapRoutes(key: PlaylistWidgetKeys): string

I don't get it

---

src/applications/content-playlists-app/playlist/playlist-content/rule-based/rule-based-content-widget.service.ts, line 48 at r8 (raw file):

Previously, esakal (Eran Sakal) wrote…

should it be valid if was not activated? if not activated how do you allow saving it even if no rules were found?

consider checking data.filters if wasn't activated and this.rules otherwise

OK

---

src/applications/content-playlists-app/playlist/playlist-content/rule-based/rule-based-content-widget.service.ts, line 57 at r8 (raw file):

Previously, esakal (Eran Sakal) wrote…

is this the default value? if so it should be set when loading the data and then here it should always be available

OK

---

src/applications/content-playlists-app/playlist/playlist-content/rule-based/rule-based-content-widget.service.ts, line 90 at r8 (raw file):

Previously, esakal (Eran Sakal) wrote…

how do you handle errors if you are asking for result with default value?

OK

---

src/applications/content-playlists-app/playlist/playlist-content/rule-based/rule-based-content-widget.service.ts, line 106 at r8 (raw file):

Previously, esakal (Eran Sakal) wrote…

why do you manage loading and error state both in the super and in this class?

OK

---

src/applications/content-playlists-app/playlist/playlist-content/rule-based/rule-based-content-widget.service.ts, line 171 at r8 (raw file):

Previously, esakal (Eran Sakal) wrote…

ActionSelected is part of the component logic. it is ok to leave it here if you although it is better to keep component logic in the component.ts and not in its service.

the view action is different from the others because it is pure component logic that doesn't involve the service. consider moving that action to the component

OK

---

src/applications/content-playlists-app/playlist/playlist-content/rule-based/rule-based-content-widget.service.ts, line 187 at r8 (raw file):

Previously, esakal (Eran Sakal) wrote…

shouldn't you handle only add new rule? because existing rules already has selection id and are part of rules

why? I'm using selectionId as indicator that a rule is a new, if passed rule doesn't have it I push it to the list, if it has - update it in the list

---

src/applications/content-playlists-app/playlist/playlist-content/rule-based/rule-based-content-widget.service.ts, line 190 at r8 (raw file):

Previously, esakal (Eran Sakal) wrote…

can you elaborate when this use case is relevant?

when rule isn't new I need to update it in the list

---

src/applications/content-playlists-app/playlist/playlist-content/rule-based/rule-based-content.component.ts, line 34 at r8 (raw file):

Previously, esakal (Eran Sakal) wrote…

are you missing .cancelOnDestroy?

OK

---

src/applications/content-playlists-app/playlist/playlist-content/rule-based/rule-based-content.component.ts, line 43 at r8 (raw file):

Previously, esakal (Eran Sakal) wrote…

I understand the use case but since this is the first time I'm seeing such an implementation I'm wondering if this is needed, which use cases does it address?

if you select a couple of entries then hit save, after data is loaded the selection is still shown, but it shouldn't

---

src/applications/content-playlists-app/playlist/playlist-content/rule-based/rule-based-content.component.ts, line 67 at r8 (raw file):

Previously, esakal (Eran Sakal) wrote…

see my comments in the service

OK

---

src/applications/content-playlists-app/playlist/playlist-content/rule-based/playlist-rule/playlist-entries-data-provider.service.ts, line 62 at r8 (raw file):

Previously, esakal (Eran Sakal) wrote…

consider renaming forRequest to something more self-explained... maybe mediaTypesDefault`...

OK

---

src/applications/content-playlists-app/playlist/playlist-content/rule-based/playlist-rule/playlist-rule-parser.service.ts, line 22 at r6 (raw file):

Previously, esakal (Eran Sakal) wrote…

It is fine to use this request here since _metadataProfileService is caching the results. wdyt?

OK

---

src/applications/content-playlists-app/playlist/playlist-content/rule-based/playlist-rule/playlist-rule-parser.service.ts, line 31 at r8 (raw file):

Previously, esakal (Eran Sakal) wrote…

are you sure you need the first operator? because it is a request that should complete it self

OK

---

src/applications/content-playlists-app/playlist/playlist-content/rule-based/playlist-rule/playlist-rule.component.ts, line 22 at r8 (raw file):

Previously, esakal (Eran Sakal) wrote…

I understand that this logic is relevant for editing while the defaults are for new rule?

consider moving this logic into _prepare() that will be executed from onInit() because:

• it is hard to flow/understand the flow by other future developers
• you want to handle it only when opening the view I assume. using setter property will handle it anytime the value changes.

I'm not sure I understand it correctly, I can move only else part to the _prepare function, but if value not null I need to init component

---

src/applications/content-playlists-app/playlist/playlist-content/rule-based/playlist-rule/playlist-rule.component.ts, line 31 at r8 (raw file):

Previously, esakal (Eran Sakal) wrote…

is this operation async or operation that can fail? if so it should be managed as we usually are doing (loading, errors and retries). if you will decide to use a _prepare() method as suggested above it should fit naturally.

OK

---

src/applications/content-playlists-app/playlist/playlist-content/rule-based/playlist-rule/playlist-rule.component.ts, line 113 at r8 (raw file):

Previously, esakal (Eran Sakal) wrote…

is this an async operation or operation that can fail? if so you should manage the flow (loading, error, retry)

OK

---

src/applications/content-playlists-app/playlist/playlist-content/rule-based/playlist-rules-table/playlist-rules-table.component.ts, line 55 at r8 (raw file):

Previously, esakal (Eran Sakal) wrote…

we moved the loading/errors logic in other tables to the list component. please do the same with this table. once the refactoring is complete I think you will not needed to use _widgetService in this component

OK

---

src/applications/content-playlists-app/playlist/playlist-details/playlist-details.component.ts, line 24 at r8 (raw file):

Previously, esakal (Eran Sakal) wrote…

ensure that _currentPlaylist is defined

it is, the subscribe function won't be invoked if data is not defined or null...

---

src/shared/content-shared/playlist-rule.interface.ts, line 1 at r8 (raw file):

Previously, esakal (Eran Sakal) wrote…

Is this file in content-shared on purpose instead of part of playlists app?

by mistake, OK

---

src/shared/content-shared/entries/pipes/entries-total-duration.pipe.ts, line 8 at r8 (raw file):

Previously, esakal (Eran Sakal) wrote…

since this action might be performance heavy, check if you can calculate this value in the relevant component instead and bind to a property

OK

---

src/shared/kmc-shared/playlist-creation/create-new-playlist.event.ts, line 9 at r8 (raw file):

Previously, esakal (Eran Sakal) wrote…

check if you need some further cleanups

OK

---

Comments from Reviewable

[esakal]

Reviewed 1 of 31 files at r2, 1 of 36 files at r3, 1 of 5 files at r9, 21 of 21 files at r10.
Review status: all files reviewed at latest revision, 6 unresolved discussions.

---

a discussion (no related file):

Previously, esakal (Eran Sakal) wrote…

did you fix he problem when trying to open a rule with local changes without sending to the server

I will check it tomorrow

---

src/applications/content-playlists-app/content-playlists-app.routes.ts, line 26 at r8 (raw file):

Previously, diamond-darrell wrote…

I don't get it

you provided the same path token 'content' to two different routing. I don't understand how it works. Instead you should have a single content component and internally use *ngSwitch or *ngIf to handle rule based and manual list accordingly. wdyt?

---

src/applications/content-playlists-app/playlist/playlist-store.service.ts, line 153 at r10 (raw file):

          if (playlist.playlistType === KalturaPlaylistType.dynamic) {
            if (typeof playlist.totalResults === 'undefined' || playlist.totalResults <= 0) {
              playlist.totalResults = environment.modules.contentPlaylists.ruleBasedTotalResults;

you are setting the totalResults while you load the playlist, is this according to the PRD

---

src/applications/content-playlists-app/playlist/playlist-content/rule-based/rule-based-content-widget.service.ts, line 190 at r8 (raw file):

Previously, diamond-darrell wrote…

when rule isn't new I need to update it in the list

ok, If I understand correctly you are providing a new rule instance every time you edit a rule? if so I understand what you did here. if this is the same instance then it doesn't add to the table. We can discuss about it if I wasn't that clear.

---

src/applications/content-playlists-app/playlist/playlist-content/rule-based/rule-based-content.component.ts, line 43 at r8 (raw file):

Previously, diamond-darrell wrote…

if you select a couple of entries then hit save, after data is loaded the selection is still shown, but it shouldn't

I will check it tomorrow

---

src/applications/content-playlists-app/playlist/playlist-content/rule-based/playlist-rule/playlist-rule.component.ts, line 22 at r8 (raw file):

Previously, diamond-darrell wrote…

I'm not sure I understand it correctly, I can move only else part to the _prepare function, but if value not null I need to init component

Let's discuss

---

Comments from Reviewable

[diamond-darrell]

Review status: 59 of 61 files reviewed at latest revision, 6 unresolved discussions.

---

src/applications/content-playlists-app/content-playlists-app.routes.ts, line 26 at r8 (raw file):

Previously, esakal (Eran Sakal) wrote…

you provided the same path token 'content' to two different routing. I don't understand how it works. Instead you should have a single content component and internally use *ngSwitch or *ngIf to handle rule based and manual list accordingly. wdyt?

I'm already doing it in the 'holder' component, this duplication was needed to define different keys for different types of playlists. But I understand your doubts about this approach, despite the fact it works fine, it can confuse other developers. So, how about to define keys array and handle array value in the mapping?

---

src/applications/content-playlists-app/playlist/playlist-store.service.ts, line 153 at r10 (raw file):

Previously, esakal (Eran Sakal) wrote…

you are setting the totalResults while you load the playlist, is this according to the PRD

I've moved it from onDataSaving hook here, as you suggested me and yes, it's according to the PRD

---

src/applications/content-playlists-app/playlist/playlist-content/rule-based/rule-based-content-widget.service.ts, line 190 at r8 (raw file):

Previously, esakal (Eran Sakal) wrote…

ok, If I understand correctly you are providing a new rule instance every time you edit a rule? if so I understand what you did here. if this is the same instance then it doesn't add to the table. We can discuss about it if I wasn't that clear.

kinda, it's a copy of original rule with modified fields, let's discuss because I'm not sure what you want me to change here 🙂

---

src/applications/content-playlists-app/playlist/playlist-content/rule-based/rule-based-content.component.ts, line 43 at r8 (raw file):

Previously, esakal (Eran Sakal) wrote…

I will check it tomorrow

OK

---

src/applications/content-playlists-app/playlist/playlist-content/rule-based/playlist-rule/playlist-rule.component.ts, line 22 at r8 (raw file):

Previously, esakal (Eran Sakal) wrote…

Let's discuss

Let's 🙂

---

Comments from Reviewable

[diamond-darrell]

Review status: 59 of 61 files reviewed at latest revision, 7 unresolved discussions.

---

src/applications/content-playlists-app/playlist/playlist-store.service.ts, line 185 at r11 (raw file):

      if (routeSectionType !== null) {
        if (Array.isArray(routeSectionType)) {

+@esakal here I'm handling array of keys for manual and rulebased content section

---

Comments from Reviewable

[diamond-darrell]

Review status: 58 of 61 files reviewed at latest revision, 1 unresolved discussion.

---

src/applications/content-playlists-app/playlist/playlist-content/rule-based/playlist-rule/playlist-rule.component.ts, line 22 at r8 (raw file):

Previously, diamond-darrell wrote…

Let's 🙂

OK

---

Comments from Reviewable

[esakal]

---

Reviewed 2 of 2 files at r11, 1 of 1 files at r12.
Review status: all files reviewed at latest revision, all discussions resolved.

---

Comments from Reviewable

[amirch1]

Reviewed 27 of 67 files at r1, 2 of 31 files at r2, 11 of 36 files at r3, 7 of 18 files at r5, 3 of 8 files at r6, 1 of 5 files at r9, 18 of 21 files at r10, 2 of 2 files at r11, 1 of 1 files at r12.
Review status: all files reviewed at latest revision, 1 unresolved discussion, all commit checks successful.

---

a discussion (no related file):
Saving a new rule-based playlist fails

---

Comments from Reviewable

[diamond-darrell]

Review status: 59 of 61 files reviewed at latest revision, 1 unresolved discussion.

---

a discussion (no related file):

Previously, amirch1 (Amir Chervinsky) wrote…

Saving a new rule-based playlist fails

Fixed

---

Comments from Reviewable

[esakal]

Reviewed 2 of 2 files at r13.
Review status: all files reviewed at latest revision, 1 unresolved discussion.

---

Comments from Reviewable

[amirch1]

Review status: all files reviewed at latest revision, 2 unresolved discussions, some commit checks failed.

---

a discussion (no related file):
Slider of result limits - when trying to drag the knob - moves slowly and can't be set correctly

---

Comments from Reviewable

[diamond-darrell]

Review status: all files reviewed at latest revision, 2 unresolved discussions, some commit checks failed.

---

a discussion (no related file):

Previously, amirch1 (Amir Chervinsky) wrote…

Slider of result limits - when trying to drag the knob - moves slowly and can't be set correctly

primefaces/primeng#2145

---

Comments from Reviewable

[esakal]

Reviewed 3 of 67 files at r1, 8 of 10 files at r14.
Review status: all files reviewed at latest revision, all discussions resolved.

---

Comments from Reviewable