Skip to content
This repository was archived by the owner on May 20, 2019. It is now read-only.

Add topic description feature #30

Open
wants to merge 1 commit into
base: 2.3-develop
Choose a base branch
from

Conversation

phoenix128
Copy link

@phoenix128 phoenix128 commented Sep 24, 2018

This PR adds the ability to define a topic name both via \Magento\AsynchronousOperations\Model\MassSchedule::publishMass parameter and di injection.

This will avoid to expose obscure messages from the user's perspective in the Magento backend when an async task is completed.

Currently we get messages like:

While we should have something more user friendly like:

@magento-cicd2
Copy link

magento-cicd2 commented Sep 24, 2018

CLA assistant check
All committers have signed the CLA.

@magento-cicd2
Copy link

CLA assistant check
Thank you for your submission, we really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.

@phoenix128 phoenix128 force-pushed the issue-13-define-user-friendly-topic-names branch from 18aa516 to ed7ff2f Compare September 24, 2018 16:35
@phoenix128 phoenix128 changed the title Issue #13 - Add topic description feature Add topic description feature Sep 24, 2018
@nuzil
Copy link

nuzil commented Oct 4, 2018

Hey, if you will be able to fix tests will be awesome :-)
Related issue:
https://github.com/magento/async-import/issues/13

@phoenix128
Copy link
Author

That is the plan :) . I will be back on it very soon

array $entitiesArray,
$groupId = null,
$userId = null,
$bulkDescription = null
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we really have a strong reason to change method arguments list or we may delay them till the moment when we will have some code that passes this argument?

@vkublytskyi
Copy link
Collaborator

vkublytskyi commented Oct 10, 2018

@phoenix128, @nuzil in #22 we already disabled PHPMD check for coupling between objects in MassSchedule. The reason was that it is not possible to implement the required functionality in a backward compatible way. Backward compatibility is more important than coupling limit.

#22 were merged to 2.3.0-release branch. As far as 2.3.0-release will be merged back to 2.3-develop static test should not fail

Copy link
Collaborator

@vkublytskyi vkublytskyi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Current implementation expects that topic descriptions are provided to descriptions pool in constructor. So this would be declared in di.xml. However, topics are defined at communication.xml. It is highly desirable to have all declarations in the same place. So if we don't have strong reasons to not to do that I think we should implement a description pool that fetches descriptions from a config file.

@nuzil
Copy link

nuzil commented Oct 26, 2018

@vkublytskyi
Can you please maybe give a tip for a good location? Cause for me logical way will be define description in communication.xml
BUT we are generating communication.xml automatically based on webapi.xml, so only way currently I see or use as was proposed di.xml, or extend webapi.xml.

@nuzil
Copy link

nuzil commented Nov 8, 2018

@phoenix128 we decided to extend webapi.xml with "description" attribute and then generate communication.xml also with "description" attribute and then it can be used on those messages.

@nuzil
Copy link

nuzil commented Nov 21, 2018

Field name: "successMessage"
Message like "%s items were successfully unassigned from stock inventory" can be defined in xml.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants