Create shared assets folder

[IshavSohal]

Related Item(s)

#385

Changes

• Added a shared assets folder.
  • When an asset is uploaded within the config of one lang that already exists in the other langs config, it gets removed from the other lang's assets folder, and added to the shared assets folder
  • When an asset is removed from a lang's assets folder, the asset in the current lang's assets folder has its source count set to 0, and it is added onto to the source count of the asset in the shared assets folder. This is because all instances of that asset within the current lang's config will be set to refer to the shared assets folder (once save changes is clicked, that is)
• Modified deleteVideo() to decrement the source count of video asset (if its local), and to remove it from configFileStructure
• Added a shared-asset events, which takes as input the name of an asset
  • This event is emitted when an asset uploaded to the current lang's config already exists in the opposite lang's config, and thus needs to be moved to the shared assets folder
  • When this event is emitted, it modifies the src of each instance of the uploaded asset (within the opposite lang's config) to refer to the shared assets folder

Notes

• When deleting an asset and its source count hits 0, it gets removed from configFileStructure. However the asset doesn't get removed from the assets folder of the product. Due to this, when the app is reloaded and configFileStructure is initialized, the deleted asset returns. If this is a bug, I can open a new issue for this.
• Once this PR is finalized and merged, I can begin working on Copying slides between languages should copy to appropriate language folders as well #213. When copying slides from one languages config to another, my idea is to remove all of the assets from the current language's asset folder, and add them to the shared assets folder. Then the slides copied over will reference assets from the shared assets folder
• console logs are just for testing, will be removed before merging

Testing

Perform the below steps using video panels as well. It should also work in the same way

Creating product

1. Create a new storylines product and fill in necessary metadata. Be sure to include a logo
2. Click Next

Adding an asset to configs of both langs

3. Within the en config, create a new slide, and then an image panel within it
4. Create an image panel within it
5. Upload an image
6. Observe, in the console, that configFileStrucuture stores the asset within the assets/en folder
7. Click save changes
8. Switch to the fr config
9. Create new slide, and then an image panel within it
10. Upload the exact same image that you uploaded in the en config
11. Observe, in the console, that configFileStrucuture stores the asset within the assets/shared folder, and has removed it from the assets/en folder
12. Click save changes
13. Check the config file for each language, and observe that the src of the asset now refers to the shared assets folder

Deleting an asset with a source count greater than 1

14. Within the en config, delete the image that you uploaded
15. Observe, in the console, that the image still exists in configFileStructure within the assets/shared folder (since its source count has gone from 2 to 1)
16. Click save changes

Deleting an asset with a source count of 1

17. Switch over to the fr config
18. Delete the image that you uploaded here
19. Observe, in the console, that the image now doesn't exists within the assets/shared folder (since its source count has gone from 1 to 0)
20. Click save changes

Changing the type of a panel that contains assets from shared assets folder

21. Upload a (new) image to the en and fr configs
22. Click save changes
23. Within one config, switch the panel type of the image panel
24. Observe, in the console, that the image still exists within the assets/shared folder (since its source count has gone from 2 to 1)
25. Observe, in the advanced editor, that the image uploaded now doesn't exist
26. Click save changes
27. Switch to the other config, and switch the panel type of the image panel
28. Observe, in the console, that the image now doesn't exists within the assets/shared folder (since its source count has gone from 1 to 0)
29. Observe, in the advanced editor, that the image uploaded now doesn't exist
30. Click save changes

---

This change is 

[github-actions]

Your demo site is ready! 🚀 Visit it here: https://ramp4-pcar4.github.io/storylines-editor/issue-385

[RyanCoulsonCA]

Haven't tested the demo for this PR just yet as I'm a bit tied up with TCEI changes, but this is my first-pass review just taking a look at the code. Will test the demo tomorrow!

Reviewable status: 0 of 6 files reviewed, 9 unresolved discussions (waiting on @IshavSohal)

---

src/components/image-editor.vue line 327 at r1 (raw file):

this.configFileStructure.assets['shared'].file(this.imagePreviews[idx].id) ? 'shared' : this.lang

This is used both here and in the const fileSource line assignment above, it may be worth assigning it to its own variable at the top of the code block to prevent code duplication.

---

src/components/image-editor.vue line 351 at r1 (raw file):

            console.log('imagePreviews');
            console.log(JSON.parse(JSON.stringify(this.imagePreviews)));
            for (let i = 0; i < this.panel.items.length; i++) {

If we don't need to use the item index for anything, I'm thinking we can just use the following to save a couple of lines:

for(asset in this.panel.items) {
  const assetSrc = asset.src.split('/');
  ...
}

---

src/components/image-editor.vue line 358 at r1 (raw file):

                const assetFolder = assetSrc[2];
                const assetId = assetSrc[3];
                if (this.configFileStructure.assets['shared'].file(assetId) && assetFolder !== 'shared') {

You can just use the following here:

return this.configFileStructure.assets['shared'].file(assetId) && assetFolder !== 'shared';

---

src/components/image-editor.vue line 372 at r1 (raw file):

            const assetFolder = assetSrc[2];
            const assetId = assetSrc[3];
            if (this.configFileStructure.assets['shared'].file(assetId) && assetFolder !== 'shared') {

Same as above, you can just use this here:

return this.configFileStructure.assets['shared'].file(assetId) && assetFolder !== 'shared'

---

src/components/image-editor.vue line 425 at r1 (raw file):

this.configFileStructure.assets['shared'].file(imageFile.id) ? 'shared' : this.lang

Same as before, since this is used multiple times (here and in the else block below), it may be worth assigning it earlier to reduce code duplication.

---

src/components/metadata-editor.vue line 879 at r1 (raw file):

        // If uploadLogo is set, upload the logo to the directory.
        // Q: if we create a shared assets folder, should the logo just go there, since both langs

Someone can shut me down if I'm wrong here, but I don't know if we should necessarily assume that the logo will be the same for both products. The best example I can think of would be if the product logo has some sort of text in it and the author wants to have an English and a French version of it.

I'm not sure how common of a case that would be though. It may be worth a discussion at some point.

---

src/components/video-editor.vue line 333 at r1 (raw file):

this.configFileStructure.assets['shared'].file(this.videoPreview.id) ? 'shared' : this.lang

Same as the image file, this should probably be stored assigned to a variable earlier to prevent code duplication.

---

src/components/video-editor.vue line 377 at r1 (raw file):

            // An assets src value is "out of date" if the asset exists in the shared asset folder, but still
            // references the curr lang's assets folder
            if (this.configFileStructure.assets['shared'].file(assetId) && assetFolder !== 'shared') {

return this.configFileStructure.assets['shared'].file(assetId) && assetFolder !== 'shared'

---

src/components/video-editor.vue line 407 at r1 (raw file):

this.configFileStructure.assets['shared'].file(this.videoPreview.id) ? 'shared' : this.lang

Same as the previous comments about this!

[szczz]

When deleting an asset and its source count hits 0, it gets removed from configFileStructure. However the asset doesn't get removed from the assets folder of the product. Due to this, when the app is reloaded and configFileStructure is initialized, the deleted asset returns. If this is a bug, I can open a new issue for this.

This seems like a bug to me. We don't want stale assets floating around in storylines products. Someone please correct me if I'm wrong and missing a use case for this.

Reviewed 6 of 6 files at r1, all commit messages.
Reviewable status: all files reviewed, 10 unresolved discussions (waiting on @IshavSohal and @RyanCoulsonCA)

---

src/components/image-editor.vue line 225 at r1 (raw file):

            oppositeSourceCount = this.sourceCounts[oppositeFileSource] ?? 0;
            this.sourceCounts[oppositeFileSource] = 0;
            this.configFileStructure.assets[oppositeLang].remove(file.name);

I think we need to perform this on this.lang's folder. While following the PR test steps, I noticed that the img remains in the original langs asset folder after it has been moved to the shared folder.

Scenario "Adding an asset to configs of both langs" indicates that the image should be removed from the assets/en folder, however, it persists.

---

src/components/metadata-editor.vue line 879 at r1 (raw file):

Previously, RyanCoulsonCA (Ryan Coulson) wrote…

Someone can shut me down if I'm wrong here, but I don't know if we should necessarily assume that the logo will be the same for both products. The best example I can think of would be if the product logo has some sort of text in it and the author wants to have an English and a French version of it.

I'm not sure how common of a case that would be though. It may be worth a discussion at some point.

Agreed and I think that will be a relatively common case.

[RyanCoulsonCA]

When deleting an asset and its source count hits 0, it gets removed from configFileStructure. However the asset doesn't get removed from the assets folder of the product. Due to this, when the app is reloaded and configFileStructure is initialized, the deleted asset returns. If this is a bug, I can open a new issue for this.

If I'm understanding this correctly, it's probably this same issue we've had for a while. See the following: #343, #111

Reviewable status: all files reviewed, 11 unresolved discussions (waiting on @IshavSohal)

---

src/components/image-editor.vue line 221 at r1 (raw file):

        // in the shared nor the opposite langs assets folder, the file should be uploaded to the current
        // langs assets folder
        if (this.configFileStructure.assets[oppositeLang].file(file.name)) {

I've found a bug that most likely relates to this section. Try out these steps:

1. Create a new image panel using the English configuration.
2. Upload an image.
3. Save the product.
4. Swap to the French configuration and create a new image panel.
5. Upload the same image you uploaded to the English slide.
6. Save the product.

The image is correctly moved to the shared folder, however, the image source for the panel in the English configuration still references the en folder when it should be referencing the shared folder.

This block will need additional code that updates the source path in the opposite language config with the new shared path. It looks like the video editor is missing this too, so we'll need there too.

[RyanCoulsonCA]

Reviewable status: all files reviewed, 11 unresolved discussions (waiting on @IshavSohal)

---

src/components/image-editor.vue line 351 at r1 (raw file):

Previously, RyanCoulsonCA (Ryan Coulson) wrote…

If we don't need to use the item index for anything, I'm thinking we can just use the following to save a couple of lines:

for(asset in this.panel.items) {
  const assetSrc = asset.src.split('/');
  ...
}

In hindsight, I don't think it actually works as I've written it here. You can ignore it, or this.panel.items.forEach(asset => { ... }); should work.

[IshavSohal]

Yeah that's exactly what I was referring to

Reviewable status: 2 of 7 files reviewed, 11 unresolved discussions (waiting on @RyanCoulsonCA and @szczz)

---

src/components/image-editor.vue line 221 at r1 (raw file):

Previously, RyanCoulsonCA (Ryan Coulson) wrote…

I've found a bug that most likely relates to this section. Try out these steps:

1. Create a new image panel using the English configuration.
2. Upload an image.
3. Save the product.
4. Swap to the French configuration and create a new image panel.
5. Upload the same image you uploaded to the English slide.
6. Save the product.

The image is correctly moved to the shared folder, however, the image source for the panel in the English configuration still references the en folder when it should be referencing the shared folder.

This block will need additional code that updates the source path in the opposite language config with the new shared path. It looks like the video editor is missing this too, so we'll need there too.

I ended up creating a shared-asset event, which is emitted whenever an asset is to be moved to the shared assets folder. The handler for this event will look for each instance of the uploaded asset within the opposite lang's config, and will update its source to refer to the shared asset folder. Is this the approach you had in mind? Let me know if you still have this bug.

---

src/components/image-editor.vue line 225 at r1 (raw file):

Previously, szczz (Matt Szczerba) wrote…

I think we need to perform this on this.lang's folder. While following the PR test steps, I noticed that the img remains in the original langs asset folder after it has been moved to the shared folder.

Scenario "Adding an asset to configs of both langs" indicates that the image should be removed from the assets/en folder, however, it persists.

I think this may be related to the bug I mentioned (the assets not being deleted from the product in the server). Whenever the product is re-fetched from the server, configFileStructure gets updated such that it contains any/all deleted assets.

If the product is not re-fetched from the server, this issue shouldn't occur, but correct me if I'm wrong.

---

src/components/image-editor.vue line 327 at r1 (raw file):

Previously, RyanCoulsonCA (Ryan Coulson) wrote…

this.configFileStructure.assets['shared'].file(this.imagePreviews[idx].id) ? 'shared' : this.lang

This is used both here and in the const fileSource line assignment above, it may be worth assigning it to it's own variable at the top of the code block to prevent code duplication.

Donethanks, I created a method srcFolder() to do this

---

src/components/image-editor.vue line 351 at r1 (raw file):

Previously, RyanCoulsonCA (Ryan Coulson) wrote…

In hindsight, I don't think it actually works as I've written it here. You can ignore it, or this.panel.items.forEach(asset => { ... }); should work.

Due to the new shared-asset event this method isn't needed anymore, so I just deleted it.

---

src/components/image-editor.vue line 358 at r1 (raw file):

Previously, RyanCoulsonCA (Ryan Coulson) wrote…

You can just use the following here:

return this.configFileStructure.assets['shared'].file(assetId) && assetFolder !== 'shared';

Deleted this method, all good now

---

src/components/image-editor.vue line 372 at r1 (raw file):

Previously, RyanCoulsonCA (Ryan Coulson) wrote…

Same as above, you can just use this here:

return this.configFileStructure.assets['shared'].file(assetId) && assetFolder !== 'shared'

Deleted this method, all good now

---

src/components/image-editor.vue line 425 at r1 (raw file):

Previously, RyanCoulsonCA (Ryan Coulson) wrote…

this.configFileStructure.assets['shared'].file(imageFile.id) ? 'shared' : this.lang

Same as before, since this is used multiple times (here and in the else block below), it may be worth assigning it earlier to reduce code duplication.

Donethanks

---

src/components/metadata-editor.vue line 879 at r1 (raw file):

Previously, szczz (Matt Szczerba) wrote…

Agreed and I think that will be a relatively common case.

Ah fair point. However upon creating a product, both configs now have a src for their respective logos that refers to the assets/en folder.

---

src/components/video-editor.vue line 333 at r1 (raw file):

Previously, RyanCoulsonCA (Ryan Coulson) wrote…

this.configFileStructure.assets['shared'].file(this.videoPreview.id) ? 'shared' : this.lang

Same as the image file, this should probably be stored assigned to a variable earlier to prevent code duplication.

Donethanks

---

src/components/video-editor.vue line 377 at r1 (raw file):

Previously, RyanCoulsonCA (Ryan Coulson) wrote…

return this.configFileStructure.assets['shared'].file(assetId) && assetFolder !== 'shared'

Deleted this method, all good now

---

src/components/video-editor.vue line 407 at r1 (raw file):

Previously, RyanCoulsonCA (Ryan Coulson) wrote…

this.configFileStructure.assets['shared'].file(this.videoPreview.id) ? 'shared' : this.lang

Same as the previous comments about this!

Donethanks

[RyanCoulsonCA]

Reviewed 2 of 6 files at r1, 4 of 5 files at r2, all commit messages.
Reviewable status: 6 of 7 files reviewed, 4 unresolved discussions (waiting on @IshavSohal and @szczz)

---

src/components/editor.vue line 218 at r2 (raw file):

Is this the approach you had in mind?

I think this approach should work 👍

---

src/components/editor.vue line 229 at r2 (raw file):

        console.log(JSON.parse(JSON.stringify(oppositeConfig.slides)));

        for (let i = 0; i < oppositeConfig.slides.length; i++) {

This (and the panel loop just below this) can be simplified with something like the following:

oppositeConfig.slides.forEach(oppositeSlide => { ... });

The variable oppositeSlide can be changed to whatever you think works!

---

src/components/editor.vue line 239 at r2 (raw file):

                console.log(JSON.parse(JSON.stringify(currPanel)));

                // if panel is slideshow, image or video then proceed

This will miss the dynamic panel, which like slideshows can have any slide type as a child (including slideshows).

It may be nice to use a recursive solution here like we use in other parts of the app in order to reduce code duplication (see an example here, although we can simplify it).

The recursive function would look something like the following (disclaimer: wrote this directly in Reviewable and haven't tested it so it may have some syntax errors or bugs):

const recursiveHelper = (panel: BasePanel) => {
  switch (panel.type) {
    case 'slideshow':
      panel.items.forEach(item => recursiveHelper(item));
      break;
    case 'dynamic':
      panel.children.forEach(child => recursiveHelper(child));
      break;
    default:
      // your code to handle the individual panels here.
      // If we want to ensure this only runs on images and videos, we can add a check for that here too.
      break;
  }
}

After you have this, you should just need to call it like this:

currSlide.panel.forEach(panel => { recursiveHelper(panel); }

[IshavSohal]

Reviewable status: 2 of 9 files reviewed, 5 unresolved discussions (waiting on @RyanCoulsonCA and @szczz)

---

a discussion (no related file):
I've made one additional change. With the new redesigns merged in, I noticed that the configLang prop was not updating appropriately when changing from the en config to the fr config (since they now both reside in the same editor view). I've created a new lang-change event that is emitted whenever a slide-change event is handled by the editor component. The lang-change event is handled in the metadata-editor component.

---

src/components/editor.vue line 229 at r2 (raw file):

Previously, RyanCoulsonCA (Ryan Coulson) wrote…

This (and the panel loop just below this) can be simplified with something like the following:

oppositeConfig.slides.forEach(oppositeSlide => { ... });

The variable oppositeSlide can be changed to whatever you think works!

Donethanks

---

src/components/editor.vue line 239 at r2 (raw file):

Previously, RyanCoulsonCA (Ryan Coulson) wrote…

This will miss the dynamic panel, which like slideshows can have any slide type as a child (including slideshows).

It may be nice to use a recursive solution here like we use in other parts of the app in order to reduce code duplication (see an example here, although we can simplify it).

The recursive function would look something like the following (disclaimer: wrote this directly in Reviewable and haven't tested it so it may have some syntax errors or bugs):

const recursiveHelper = (panel: BasePanel) => {
  switch (panel.type) {
    case 'slideshow':
      panel.items.forEach(item => recursiveHelper(item));
      break;
    case 'dynamic':
      panel.children.forEach(child => recursiveHelper(child));
      break;
    default:
      // your code to handle the individual panels here.
      // If we want to ensure this only runs on images and videos, we can add a check for that here too.
      break;
  }
}

After you have this, you should just need to call it like this:

currSlide.panel.forEach(panel => { recursiveHelper(panel); }

Donethanks, appreciate the help!

[RyanCoulsonCA]

Reviewed 5 of 7 files at r3, all commit messages.
Reviewable status: 7 of 9 files reviewed, 3 unresolved discussions (waiting on @IshavSohal and @szczz)

---

server/index.js line 12 at r3 (raw file):

const simpleGit = require('simple-git');
const responseMessages = [];
require('dotenv').config();

I think this file snuck into this PR somehow! Other than that, all of the tests I've done look good!

[IshavSohal]

Reviewable status: 5 of 9 files reviewed, 3 unresolved discussions (waiting on @RyanCoulsonCA and @szczz)

---

server/index.js line 12 at r3 (raw file):

Previously, RyanCoulsonCA (Ryan Coulson) wrote…

I think this file snuck into this PR somehow! Other than that, all of the test I've done look good!

This was just me fixing some formatting issues I noticed. Everything should be working the same as before.

[RyanCoulsonCA]

Reviewed 1 of 7 files at r3, 3 of 3 files at r4, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @IshavSohal and @szczz)

---

server/index.js line 12 at r3 (raw file):

Previously, IshavSohal (Ishav Sohal) wrote…

This was just me fixing some formatting issues I noticed. Everything should be working the same as before.

👍

[szczz]

Adding an asset to configs of both langs

After performing these steps, but with a slight modification of copying the config from the en slide. The assets still remain in the en folder. They get moved to the shared folder if you manually add the same image to the FR slide though.

Reviewed 1 of 7 files at r3.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @IshavSohal and @RyanCoulsonCA)

---

server/index.js line 12 at r3 (raw file):

Previously, RyanCoulsonCA (Ryan Coulson) wrote…

I think this file snuck into this PR somehow! Other than that, all of the test I've done look good!

Yep please remove this file from the PR

[szczz]

slight modification**

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @RyanCoulsonCA)

[IshavSohal]

Donethanks. As I was making the fix for this, I decided to create a helper method, panelHelper(), which can be provided a callback (as well as additional arguments for it) to be called on each ImagePanel/VideoPanel within a given BasePanel. This may be a bit overkill, so let me know your thoughts/opinions.

Reviewable status: 4 of 10 files reviewed, 1 unresolved discussion (waiting on @RyanCoulsonCA and @szczz)

---

server/index.js line 12 at r3 (raw file):

Previously, szczz (Matt Szczerba) wrote…

Yep please remove this file from the PR

Donethanks

[RyanCoulsonCA]

Reviewed 4 of 6 files at r5, all commit messages.
Reviewable status: 8 of 10 files reviewed, 1 unresolved discussion (waiting on @IshavSohal and @szczz)

---

a discussion (no related file):

Previously, IshavSohal (Ishav Sohal) wrote…

I've made one additional change. With the new redesigns merged in, I noticed that the configLang prop was not updating appropriately when changing from the en config to the fr config (since they now both reside in the same editor view). I've created a new lang-change event that is emitted whenever a slide-change event is handled by the editor component. The lang-change event is handled in the metadata-editor component.

Found a bug:

1. Create a new blank slide.
2. Change both the English and French slide to an "image" slide.
3. Upload a photo to the English slide.
4. Upload the same photo to the French slide.
5. Switch back to the English slide and notice that the image doesn't appear. Switching over to the Advanced panel shows that the src still points to the en folder instead of shared.

This seems to work fine for slideshow panels, just breaks with image panels.

[yileifeng]

Note: loading in existing products with old configuration asset folder structure will freeze the metadata page and eventually resolve with all N/A values. This breaks backwards compatibility and should be considered before merging without a plan in place to migrate old products.

Reviewed 1 of 6 files at r1, 2 of 7 files at r3, 1 of 3 files at r4, 6 of 6 files at r5, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @IshavSohal)

---

src/components/image-editor.vue line 130 at r1 (raw file):

    mounted(): void {
        console.log('');

Blocker for console logs

[IshavSohal]

Reviewable status: 2 of 10 files reviewed, 2 unresolved discussions (waiting on @RyanCoulsonCA and @yileifeng)

---

a discussion (no related file):

Previously, RyanCoulsonCA (Ryan Coulson) wrote…

Found a bug:

1. Create a new blank slide.
2. Change both the English and French slide to an "image" slide.
3. Upload a photo to the English slide.
4. Upload the same photo to the French slide.
5. Switch back to the English slide and notice that the image doesn't appear. Switching over to the Advanced panel shows that the src still points to the en folder instead of shared.

This seems to work fine for slideshow panels, just breaks with image panels.

Donethanks, should be all good now

[IshavSohal]

This may have been due to the console logs that I've now removed, which were problematic for large products.

Reviewable status: 1 of 10 files reviewed, 3 unresolved discussions (waiting on @RyanCoulsonCA, @szczz, and @yileifeng)

---

a discussion (no related file):
Removed all console logs.

[szczz]

Couple of issues:

1. If you create a new storyline, upload a logo to the EN config and then save it, the logo gets added to the en assets folder as expected. If you return to the dashboard and load the storyline and then change to the french config, the original logo remains. If you go to the editor page and then press save, it results in the logo being stored in both the en fold and fr folder even though its exactly the same.

2. If you create a new slide and upload an image with the name "test.jpeg" in the en config, then flip to the fr config and upload a different image with the name "test.jpeg", returning to the en config will display the second image. If you save the en config before the french one, the second image will show up in the shared folder, but the first image will remain in the en folder. Perhaps a hash function or looking at image metadata could help handle this situation.

Reviewable status: 1 of 10 files reviewed, 3 unresolved discussions (waiting on @IshavSohal, @RyanCoulsonCA, and @yileifeng)