{"action":"created","issue":{"url":"https://api.github.com/repos/kaltura/kmc-ng/issues/257","repository_url":"https://api.github.com/repos/kaltura/kmc-ng","labels_url":"https://api.github.com/repos/kaltura/kmc-ng/issues/257/labels{/name}","comments_url":"https://api.github.com/repos/kaltura/kmc-ng/issues/257/comments","events_url":"https://api.github.com/repos/kaltura/kmc-ng/issues/257/events","html_url":"https://github.com/kaltura/kmc-ng/pull/257","id":270556235,"number":257,"title":"Playlists adjustments","user":{"login":"diamond-darrell","id":4395889,"avatar_url":"https://avatars2.githubusercontent.com/u/4395889?v=4","gravatar_id":"","url":"https://api.github.com/users/diamond-darrell","html_url":"https://github.com/diamond-darrell","followers_url":"https://api.github.com/users/diamond-darrell/followers","following_url":"https://api.github.com/users/diamond-darrell/following{/other_user}","gists_url":"https://api.github.com/users/diamond-darrell/gists{/gist_id}","starred_url":"https://api.github.com/users/diamond-darrell/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/diamond-darrell/subscriptions","organizations_url":"https://api.github.com/users/diamond-darrell/orgs","repos_url":"https://api.github.com/users/diamond-darrell/repos","events_url":"https://api.github.com/users/diamond-darrell/events{/privacy}","received_events_url":"https://api.github.com/users/diamond-darrell/received_events","type":"User","site_admin":false},"labels":[],"state":"open","locked":false,"assignee":{"login":"diamond-darrell","id":4395889,"avatar_url":"https://avatars2.githubusercontent.com/u/4395889?v=4","gravatar_id":"","url":"https://api.github.com/users/diamond-darrell","html_url":"https://github.com/diamond-darrell","followers_url":"https://api.github.com/users/diamond-darrell/followers","following_url":"https://api.github.com/users/diamond-darrell/following{/other_user}","gists_url":"https://api.github.com/users/diamond-darrell/gists{/gist_id}","starred_url":"https://api.github.com/users/diamond-darrell/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/diamond-darrell/subscriptions","organizations_url":"https://api.github.com/users/diamond-darrell/orgs","repos_url":"https://api.github.com/users/diamond-darrell/repos","events_url":"https://api.github.com/users/diamond-darrell/events{/privacy}","received_events_url":"https://api.github.com/users/diamond-darrell/received_events","type":"User","site_admin":false},"assignees":[{"login":"amirch1","id":3839950,"avatar_url":"https://avatars0.githubusercontent.com/u/3839950?v=4","gravatar_id":"","url":"https://api.github.com/users/amirch1","html_url":"https://github.com/amirch1","followers_url":"https://api.github.com/users/amirch1/followers","following_url":"https://api.github.com/users/amirch1/following{/other_user}","gists_url":"https://api.github.com/users/amirch1/gists{/gist_id}","starred_url":"https://api.github.com/users/amirch1/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/amirch1/subscriptions","organizations_url":"https://api.github.com/users/amirch1/orgs","repos_url":"https://api.github.com/users/amirch1/repos","events_url":"https://api.github.com/users/amirch1/events{/privacy}","received_events_url":"https://api.github.com/users/amirch1/received_events","type":"User","site_admin":false},{"login":"diamond-darrell","id":4395889,"avatar_url":"https://avatars2.githubusercontent.com/u/4395889?v=4","gravatar_id":"","url":"https://api.github.com/users/diamond-darrell","html_url":"https://github.com/diamond-darrell","followers_url":"https://api.github.com/users/diamond-darrell/followers","following_url":"https://api.github.com/users/diamond-darrell/following{/other_user}","gists_url":"https://api.github.com/users/diamond-darrell/gists{/gist_id}","starred_url":"https://api.github.com/users/diamond-darrell/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/diamond-darrell/subscriptions","organizations_url":"https://api.github.com/users/diamond-darrell/orgs","repos_url":"https://api.github.com/users/diamond-darrell/repos","events_url":"https://api.github.com/users/diamond-darrell/events{/privacy}","received_events_url":"https://api.github.com/users/diamond-darrell/received_events","type":"User","site_admin":false},{"login":"esakal","id":4751797,"avatar_url":"https://avatars3.githubusercontent.com/u/4751797?v=4","gravatar_id":"","url":"https://api.github.com/users/esakal","html_url":"https://github.com/esakal","followers_url":"https://api.github.com/users/esakal/followers","following_url":"https://api.github.com/users/esakal/following{/other_user}","gists_url":"https://api.github.com/users/esakal/gists{/gist_id}","starred_url":"https://api.github.com/users/esakal/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/esakal/subscriptions","organizations_url":"https://api.github.com/users/esakal/orgs","repos_url":"https://api.github.com/users/esakal/repos","events_url":"https://api.github.com/users/esakal/events{/privacy}","received_events_url":"https://api.github.com/users/esakal/received_events","type":"User","site_admin":false}],"milestone":null,"comments":2,"created_at":"2017-11-02T07:20:37Z","updated_at":"2017-11-06T07:24:29Z","closed_at":null,"author_association":"COLLABORATOR","pull_request":{"url":"https://api.github.com/repos/kaltura/kmc-ng/pulls/257","html_url":"https://github.com/kaltura/kmc-ng/pull/257","diff_url":"https://github.com/kaltura/kmc-ng/pull/257.diff","patch_url":"https://github.com/kaltura/kmc-ng/pull/257.patch"},"body":"### PR information\r\n**What kind of change does this PR introduce?** (check one with \"x\")\r\n- [] Bugfix\r\n- [] Feature\r\n- [] Code style update (formatting, local variables)\r\n- [] Refactoring (no functional changes, no api changes)\r\n- [] Build related changes\r\n- [] CI related changes\r\n- [] Other... Please describe:\r\n\r\n**What is the current behavior?** (You can also link to an open issue here)\r\n\r\n\r\n\r\n**What is the new behavior?**\r\n\r\n\r\n\r\n**Does this PR introduce a breaking change?** (check one with \"x\")\r\n- [] Yes\r\n- [] No\r\n\r\n\r\nIf this PR contains a breaking change, please describe the impact and migration path for existing applications: ...\r\n\r\n---\r\n\r\n### Other information\r\n\r\n**Where should the reviewer start?**\r\n\r\n**How should this be manually tested?**\r\n\r\n**Any background context you want to provide?**\n\n<!-- Reviewable:start -->\n---\nThis change is [<img src=\"https://reviewable.io/review_button.svg\" height=\"34\" align=\"absmiddle\" alt=\"Reviewable\"/>](https://reviewable.io/reviews/kaltura/kmc-ng/257)\n<!-- Reviewable:end -->\n"},"comment":{"url":"https://api.github.com/repos/kaltura/kmc-ng/issues/comments/342066321","html_url":"https://github.com/kaltura/kmc-ng/pull/257#issuecomment-342066321","issue_url":"https://api.github.com/repos/kaltura/kmc-ng/issues/257","id":342066321,"user":{"login":"diamond-darrell","id":4395889,"avatar_url":"https://avatars2.githubusercontent.com/u/4395889?v=4","gravatar_id":"","url":"https://api.github.com/users/diamond-darrell","html_url":"https://github.com/diamond-darrell","followers_url":"https://api.github.com/users/diamond-darrell/followers","following_url":"https://api.github.com/users/diamond-darrell/following{/other_user}","gists_url":"https://api.github.com/users/diamond-darrell/gists{/gist_id}","starred_url":"https://api.github.com/users/diamond-darrell/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/diamond-darrell/subscriptions","organizations_url":"https://api.github.com/users/diamond-darrell/orgs","repos_url":"https://api.github.com/users/diamond-darrell/repos","events_url":"https://api.github.com/users/diamond-darrell/events{/privacy}","received_events_url":"https://api.github.com/users/diamond-darrell/received_events","type":"User","site_admin":false},"created_at":"2017-11-06T07:24:29Z","updated_at":"2017-11-06T07:24:29Z","author_association":"COLLABORATOR","body":"\n\n\n\nReviewed 21 of 21 files at r2.\nReview status: all files reviewed at latest revision, 11 unresolved discussions.\n\n---\n\n*[a discussion](https://reviewable.io:443/reviews/kaltura/kmc-ng/257#-KyB3wt4YBSR1mnC9i0g:-KyFAJI-X7vSiD1tS-qR:b32w0x) (no related file):*\n<details><summary><i>Previously, esakal (Eran Sakal) wrote…</i></summary><blockquote>\n\nThere are some issues with the handling of entries bulk selection. \nPlease review them [here](https://my.pcloud.com/publink/show?code=XZuxMV7ZSeSTopQRbKB3yaeRtvomPzUe5YoV):\n1. reorder multiple entries\n2. duplicating, deleting single entry should clear bulk selection\n3. bulk delete show a confirmation message which it shouldn't. You can see the legacy system as a reference. If needed fix the PRD.\n4. bulk delete after single delete is deleting other entries randomly. Once you will fix issue # 2 it will not happen but I think you should try this out and make sure you fixed this because it indicate on problematic handling.\n</blockquote></details>\n\nOK\n\n---\n\n*[src/applications/content-playlists-app/content-playlists-app.module.ts, line 76 at r2](https://reviewable.io:443/reviews/kaltura/kmc-ng/257#-KyBJi8QEDQST6bPqyrV:-KyFAUihrqnxCrxYjn4w:ba9x77v) ([raw file](https://github.com/kaltura/kmc-ng/blob/306a7b674cc0f758f5f36bf71968318118c69e27/src/applications/content-playlists-app/content-playlists-app.module.ts#L76)):*\n<details><summary><i>Previously, esakal (Eran Sakal) wrote…</i></summary><blockquote>\n\nI don't think it should be singleton. Please confirm. \n\nIf it is here because `PlaylistCanActivate` has dependency to it then you can use the component which has better support (you can check how we did it in other places). \n\nalso, I noticed that you already handling this in the service, see my comment below\n</blockquote></details>\n\nYes, you right, I've moved the store service here because of guard, I'll check it again\n\n---\n\n*[src/applications/content-playlists-app/playlist/playlist-can-activate.service.ts, line 2 at r2](https://reviewable.io:443/reviews/kaltura/kmc-ng/257#-KyBajAokSIPyfmey5Hi:-KyFAtMBgYtlFJdzNab-:byp6w2w) ([raw file](https://github.com/kaltura/kmc-ng/blob/306a7b674cc0f758f5f36bf71968318118c69e27/src/applications/content-playlists-app/playlist/playlist-can-activate.service.ts#L2)):*\n<details><summary><i>Previously, esakal (Eran Sakal) wrote…</i></summary><blockquote>\n\nit seems that you already handle this scenario in the service, why do you need the guard?\n</blockquote></details>\n\nTo prevent popup component error, I'm not sure why it's happening because I haven't changed template, will check it again so we can figure something out together\n\n---\n\n*[src/applications/content-playlists-app/playlist/playlist-store.service.ts, line 114 at r2](https://reviewable.io:443/reviews/kaltura/kmc-ng/257#-KyBa9znt0cClu8E2MQ6:-KyFBDNg3v962D3EY9CU:b32w0x) ([raw file](https://github.com/kaltura/kmc-ng/blob/306a7b674cc0f758f5f36bf71968318118c69e27/src/applications/content-playlists-app/playlist/playlist-store.service.ts#L114)):*\n<details><summary><i>Previously, esakal (Eran Sakal) wrote…</i></summary><blockquote>\n\nremove this log\n</blockquote></details>\n\nOK\n\n---\n\n*[src/applications/content-playlists-app/playlist/playlist-store.service.ts, line 147 at r2](https://reviewable.io:443/reviews/kaltura/kmc-ng/257#-KyB3rAH3A8I8tD2aV0y:-KyFBY1_I9iC2bfejMUp:b-dcjew1) ([raw file](https://github.com/kaltura/kmc-ng/blob/306a7b674cc0f758f5f36bf71968318118c69e27/src/applications/content-playlists-app/playlist/playlist-store.service.ts#L147)):*\n<details><summary><i>Previously, esakal (Eran Sakal) wrote…</i></summary><blockquote>\n\nshould clear `_loadPlaylistSubscription` inside the subscriber\n</blockquote></details>\n\nActually, I think there's no need to keep subscription on `_getPlaylist`, since it's a cold observable and for destroying teardown we have `cancelOnDestroy` operator\n\n---\n\n*[src/applications/content-playlists-app/playlist/playlist-store.service.ts, line 156 at r2](https://reviewable.io:443/reviews/kaltura/kmc-ng/257#-KyBaRNdLyGbaeOXjbQN:-KyFBPTmuPk6Gmt-tr3r:b-yzsebw) ([raw file](https://github.com/kaltura/kmc-ng/blob/306a7b674cc0f758f5f36bf71968318118c69e27/src/applications/content-playlists-app/playlist/playlist-store.service.ts#L156)):*\n<details><summary><i>Previously, esakal (Eran Sakal) wrote…</i></summary><blockquote>\n\nwhat will happen if you remove the explicit casting?\n</blockquote></details>\n\nNot sure, it might be left from Konstantine's code\n\n---\n\n*[src/applications/content-playlists-app/playlist/playlist-store.service.ts, line 176 at r2](https://reviewable.io:443/reviews/kaltura/kmc-ng/257#-KyB3ukHUyee8XUbZ6fK:-KyFCCDSTGNBKgg4YUtJ:b-iobuo6) ([raw file](https://github.com/kaltura/kmc-ng/blob/306a7b674cc0f758f5f36bf71968318118c69e27/src/applications/content-playlists-app/playlist/playlist-store.service.ts#L176)):*\n<details><summary><i>Previously, esakal (Eran Sakal) wrote…</i></summary><blockquote>\n\nshould clear `_loadPlaylistSubscription`\n</blockquote></details>\n\nSee my comment above\n\n---\n\n*[src/applications/content-playlists-app/playlist/playlist-store.service.ts, line 263 at r2](https://reviewable.io:443/reviews/kaltura/kmc-ng/257#-KyBbM_Gpoe9hjkOSq_j:-KyFCc8NMDeYG-65G0eP:b-4uev60) ([raw file](https://github.com/kaltura/kmc-ng/blob/306a7b674cc0f758f5f36bf71968318118c69e27/src/applications/content-playlists-app/playlist/playlist-store.service.ts#L263)):*\n<details><summary><i>Previously, esakal (Eran Sakal) wrote…</i></summary><blockquote>\n\n+@amirch1 Let's discuss\n</blockquote></details>\n\nJust in case, I haven't changed this request 🙂\n\n---\n\n*[src/applications/content-playlists-app/playlist/playlist-store.service.ts, line 271 at r2](https://reviewable.io:443/reviews/kaltura/kmc-ng/257#-KyBdHrXlZZGTqBY6cM_:-KyFCNYlnq1S3SjN1C5U:b-o2l2hc) ([raw file](https://github.com/kaltura/kmc-ng/blob/306a7b674cc0f758f5f36bf71968318118c69e27/src/applications/content-playlists-app/playlist/playlist-store.service.ts#L271)):*\n<details><summary><i>Previously, esakal (Eran Sakal) wrote…</i></summary><blockquote>\n\nnot needed... Let's discuss\n</blockquote></details>\n\nYes, I can use multiple moving functions, will remove it\n\n---\n\n*[src/applications/content-playlists-app/playlist/playlist-widgets-manager.ts, line 9 at r2](https://reviewable.io:443/reviews/kaltura/kmc-ng/257#-KyBbgkvMDX87KgZOf5U:-KyFCj8zJNkYu6lfBZAj:but0a17) ([raw file](https://github.com/kaltura/kmc-ng/blob/306a7b674cc0f758f5f36bf71968318118c69e27/src/applications/content-playlists-app/playlist/playlist-widgets-manager.ts#L9)):*\n<details><summary><i>Previously, esakal (Eran Sakal) wrote…</i></summary><blockquote>\n\nthe data type is wrong. Let's discuss\n</blockquote></details>\n\nOkay, let's 🙂\n\n---\n\n\n*Comments from [Reviewable](https://reviewable.io:443/reviews/kaltura/kmc-ng/257)*\n<!-- Sent from Reviewable.io -->\n"}}
{"actor":{"display_login":"diamond-darrell"}}