Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Channel aware Assets #677

Closed
martijnvdbrug opened this issue Jan 29, 2021 · 11 comments
Closed

Channel aware Assets #677

martijnvdbrug opened this issue Jan 29, 2021 · 11 comments

Comments

@martijnvdbrug
Copy link
Collaborator

martijnvdbrug commented Jan 29, 2021

Currently, assets are shared between channels. If an asset is uploaded to channel1and the user switches to channel2, it will see the same set of assets. To fully support multi-channel shops, it's desirable to have assets that are channel aware.

Describe the solution you'd like
Assets that are uploaded in the context of the current channel should only be available in the current channel.

  • Focalpoints are global: Changing focalpoints changes the focalpoint for this asset on all channels
  • Deleting an Asset in a non-default channel will result in the asset still being available in the default channel
  • Optional deleteFromAllChannels boolean for deleteAsset mutation to delete asset from all channels
input AssetDeletionOptions {
  force: Boolean
  deleteFromAllChannels: Boolean
}

Considered out of scope for now: Assigning assets to multiple channels via the Admin UI.

Question Should there be something like global assets?

I am currently working on this feature.

@michaelbromley
Copy link
Member

Question Should there be something like global assets?

What do you mean by this? Assets that are available by default in all Channels without needing to be explicitly assigned?

@martijnvdbrug
Copy link
Collaborator Author

Question Should there be something like global assets?

What do you mean by this? Assets that are available by default in all Channels without needing to be explicitly assigned?

I was thinking about assets that are assigned to multiple channels, which seems like a must have now that I think about it.

@michaelbromley
Copy link
Member

Yes, they should work just like any of the ChannelAware entities:

  • They are always assigned to the default Channel.
  • They may be optionally assigned to one or more sub-channels.

@martijnvdbrug
Copy link
Collaborator Author

  • What is the desired behavior when an asset is deleted from a channel? Should it always stay in the default channel?
  • What if an asset is deleted from the default channel, but it's also in other channels?
  • I assume focalPoints should be channel-aware aswell?

@michaelbromley
Copy link
Member

Good questions:

What is the desired behavior when an asset is deleted from a channel? Should it always stay in the default channel?
What if an asset is deleted from the default channel, but it's also in other channels?

I think we need to provide a choice to the user. Here's the current deletion API:

deleteAsset(id: ID!, force: Boolean): DeletionResponse!

type DeletionResponse {
  message: String
  result: DeletionResult!
}

enum DeletionResult {
  "The entity was successfully deleted"
  DELETED
  "Deletion did not take place, reason given in message"
  NOT_DELETED
}

We make use of the "force" variable e.g. if attempting to delete an Asset which is set as the featuredAsset of a Product. The call to deleteAsset will return something like:

{ 
  "result": "NOT_DELETED",
  "message": "The selected Asset is featured by 1 Product"
}

and only setting "force" to true actually deletes it.

We could do something similar with channels, but we probably would then want a "AssetDeletionOptions" input something like:

input AssetDeletionOptions {
  force: Boolean
  deleteFromAllChannels: Boolean
}

I assume focalPoints should be channel-aware aswell?

No, I don't think this is necessary. It would be overly-complex and I can't easily think of a scenario where the focal point changes depending on what channel it is in. If you have some use-case in mind, let me know.

@martijnvdbrug
Copy link
Collaborator Author

I can't think of any use-case on having channel aware focalpoints, so focalpoints will always be global.

The solution of having deleteFromAllChannels seems fine for me, given that you can only use it in the context of the default channel. Maybe we should throw an error when deleteFromAllChannels=true is given in the context of a non-default channel?

@michaelbromley
Copy link
Member

I was thinking that deleteFromAllChannels: true in the context of a sub-channel would mean "also delete it from the default channel". I.e. it always means "delete from all channels" regardless of which channel it is called from.

Oh, another complication here - we need to take into account whether the current user actually has permissions to delete from other channels! You can't allow "Shop B Admin" who only has permissions on "Shop B Channel" to delete things from the default channel.

@martijnvdbrug
Copy link
Collaborator Author

Oh, another complication here - we need to take into account whether the current user actually has permissions to delete from other channels! You can't allow "Shop B Admin" who only has permissions on "Shop B Channel" to delete things from the default channel.

That's why I was thinking about throwing an error, but that might be unexpected behavior when you have permission to all channels.
So the right way is checking permissions and only throw an error when the current user does not have permissions.

@michaelbromley
Copy link
Member

but that might be unexpected behavior when you have permission to all channels.

Yes exactly. There will be cases where a superadmin-type account is doing work in a sub-channel and would want to be able to just delete assets without switching to the default channel, finding the asset again and then deleting.

@martijnvdbrug
Copy link
Collaborator Author

martijnvdbrug commented Jan 31, 2021

More questions:

  • If an asset is assigned to a product in the default channel, and the product is assigned to channel2, should the asset also be added to channel2? I assume so.
  • I don't think we need a removeAssetFromChannel mutation, because we can do deleteAsset(removeFromAllChannels=false)
  • Proposal for the mutations:
type Mutation {
    assignAssetsToChannel(input: AssignAssetsToChannelInput!): [Asset!]!
    deleteAsset(input: DeleteAssetInput!): DeletionResult!
    deleteAssets(input: DeleteAssetsInput!): DeletionResult!
}

input AssignAssetsToChannelInput {
    assetIds: [ID!]!
    channelId: ID!
}

input DeleteAssetInput {
    assetId: ID!
    force: Boolean
    deleteFromAllChannels: Boolean
}

input DeleteAssetsInput {
    assetIds: [ID!]!
    force: Boolean
    deleteFromAllChannels: Boolean
}

@michaelbromley
Copy link
Member

  1. Yes
  2. Agreed
  3. Looks good 👍

@michaelbromley michaelbromley added this to the v1.0.0 milestone Feb 2, 2021
martijnvdbrug added a commit to martijnvdbrug/vendure that referenced this issue Feb 11, 2021
Relates to vendure-ecommerce#677

BREAKING CHANGE: New DB relation Asset to Channel. Mutations deleteAsset and deleteAssets changed
michaelbromley pushed a commit that referenced this issue Feb 16, 2021
Relates to #677

BREAKING CHANGE: New DB relation Asset to Channel, requiring a migration. The Admin API mutations `deleteAsset` and `deleteAssets` have changed their argument signature.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants