#29 User configures Adobe Stock integration #52
Conversation
* develop: #53: Code review fixes Fixed typo in type name Introduced request builder Fixed method annotation #11 Implement setup script for user metadata Fix details_url column Add setup script (#10) #29 User configures Adobe Stock integration Introduced search builder interface Add documentation in classes, in the form of comments.
There was a problem hiding this comment.
Hi @diazwatson , thanks for the pull request!
Please see my review comments
AdobeStockImageAdminUi/Model/Block/Adminhtml/System/Config/TestConnection.php
Outdated
Show resolved
Hide resolved
* upstream/develop: #11 requested changes during PR review #11 Add a separate table to link languages and users Fixed installation link issue Removed email from slack signup text Improved GitHUb pull request template and added useful links to the README.MD #11 requested changes during PR review Added title to the slide panel. Removed default buttons #11 Change type text to varchar with lengh #11 changes suggested by lewisvoncken #11 Implement setup script for user metadata
* upstream/develop: Removed unused class import #5: Introduce pagination for image listing UI component #17: Implement search filter for the "orientation" attribute #3: Filters support #3: Client refactoring #3: Refactored API structure #11 remove db_schema from AdobeStockImage #11 Add additional foreign key constraints Move locale assignment to RequestBuilder Add Magento admin locale to request
There was a problem hiding this comment.
Thanks for the updates @diazwatson ! Please see my review comments, also I think that all the code in this pull request should be moved to AdobeStockAsset module as it is general configuration for the integration and is not specifically related to Image. @sidolov can you please confirm my opinion?
AdobeStockImageAdminUi/Controller/Adminhtml/System/Config/TestConnection.php
Outdated
Show resolved
Hide resolved
| ]; | ||
|
|
||
| try { | ||
| $response = $this->getImageList->execute($this->getSearchCriteria()); |
There was a problem hiding this comment.
Considering that the configuration is general and not specifically related to image I would avoid dependency on AdobeStockImage module. For this purpose, I propose to introduce a getAssetList or testConnection API to AdobeStockAssetApi and use it
There was a problem hiding this comment.
Hey guys, I have applied the requested changes. Could you please review when you get a chance?
Note: I am not very happy with the implementation of testConnection method, can you see a better way to implement it?
There was a problem hiding this comment.
Thanks for the contribution, @diazwatson ! I think test connection implementation is perfect for now, also, we can improve it later if there will be ideas.
The only adjustments I would do now - is changing the return type to bool
There was a problem hiding this comment.
Thanks @sivaschenko I'll change that tonight
There was a problem hiding this comment.
@diazwatson actually, I did this already. Pull request is merged! Please take a look at my changes and feel free to create a new pull request if you'd like to adjust anything
magento-engcom-team
added
Award: category of expertise
and removed
Award: category of expertise
labels
Resolved Issue: #29
Blocked by: magento/magento2#22844