(perf, refactor) Remove unnecessary code in Media subscription.

[prinzdezibel]

Closes #3557

This ticket addresses:

• cost of ownership (number of LOC), because of dead selector variable
• performance , because it prevents unnecessary DDP roundtrips.

[mikemurray]

@prinzdezibel This publication might still be used. Uploading a logo for the shop brand, unless that has been removed from shop settings, uses this publication.

[mikemurray]

Verified this publication is still required for the shop logo upload.

This publication may also be needed for some media documents under revision control.

[brent-hoover]

@prinzdezibel Please always include test instructions in every PR you submit

[brent-hoover]

As @mikemurray notes we still need brandAssets but I wonder if we still need all this code which seems like it just gets negated by

return Media.find({
 -    "metadata.type": "brandAsset"
 -  });

at the bottom?

Do we still need this for reactivity or is this handled in the Products publication?

[spencern]

@zenweasel @mikemurray It does seem that this publication could be simplified significantly.

[prinzdezibel]

Thanks for the hint with the brand asset, @mikemurray. I've been thinking hard and could not imagine a place where this pub would have still been in use.

Now I know better.

I'm going to strip this pub down to something along the lines.

return Media.find({
     "metadata.type": "brandAsset"
   });

[prinzdezibel]

This publication may also be needed for some media documents under revision control.

Your statement would correlate with that code snippet, if I'm correct:

  if (RevisionApi.isRevisionControlEnabled()) {
    const revisionHandle = Revisions.find({
      "documentType": "image",
      "workflow.status": { $nin: [ "revision/published"] }
    }).observe({
      added: (revision) => {
        const media = Media.findOne(revision.documentId);
        if (media) {
          this.added("Media", media._id, media);
          this.added("Revisions", revision._id, revision);
        }
      },
      changed: (revision) => {
        const media = Media.findOne(revision.documentId);
        this.changed("Media", media._id, media);
        this.changed("Revisions", revision._id, revision);
      },
      removed: (revision) => {
        if (revision) {
          const media = Media.findOne(revision.documentId);
          if (media) {
            this.removed("Media", media._id, media);
            this.removed("Revisions", revision._id, revision);
          }
        }
      }
    });

I don't think it's necessary at all. For one all it does looks to me like an unnecessary roundtrip to the client, because it pushes revision and images when the revision is created and deletes it when the revision is published.

Also I'm wondering about the following two things:

a) The first push (the media object)
It pushes to an non-existent collection named "Media". But nowadays the collection is called "cfs.Media.filerecord" (I guess this happened when we changed to the implicit publishing process through products.js).

b) The second push (the Revision object)
Why should we bother to push the Revision to the user at all? Isn't this an admin-only thing?

NOTE In case you're asking where the 3.rd push is coming from: This results from my PR #3559.

Here is a gif for a better explanation (click on it to have it enlarged in a separate tab window):

[prinzdezibel]

Changed as requested, @mikemurray. Also this PR does now incorporate the already approved & merged PR#3559.

[mikemurray]

@prinzdezibel Isn't this equivalent to lines 546 and 571? All the media from the products and their variants should already be published unless I'm missing something here.

[brent-hoover]

Should probably be flagged as a breaking change since we are changing the name of a publication?

[prinzdezibel]

Should probably be flagged as a breaking change since we are changing the name of a publication?

Would probably only break, if somebody has overwritten the pub/sub, I guess.

[brent-hoover]

Or if they were subscribing to it in their own code. I would say changing the name of publication is probably one of the bigger API changes one could make.

[machikoyasuda]

@prinzdezibel - Assigning you this doc ticket reactioncommerce/reaction-docs#490 to update https://docs.reactioncommerce.com/reaction-docs/master/image-handling to reflect this name change