{ASI} :- Error message to be cached for grid data storage component #26502

konarshankar07 · 2020-01-22T17:21:59Z

Description (*)

This PR is part of the ASI issue in which error message is not cached when user try to apply filter

Fixed Issues (if relevant)

Message disappear when the request with broken credentials is sent second time adobe-stock-integration#863: Message disappear when the request with broken credentials is sent second time

Manual testing scenarios (*)

Please check magento/adobe-stock-integration#884

Questions or comments

Contribution checklist (*)

Pull request has a meaningful description of its purpose
All commits are accompanied by meaningful commit messages
All new or changed code is covered with unit/integration tests (if applicable)
All automated tests passed successfully (all builds are green)

m2-assistant · 2020-01-22T17:22:03Z

Hi @konarshankar07. Thank you for your contribution
Here is some useful tips how you can test your changes using Magento test environment.
Add the comment under your pull request to deploy test or vanilla Magento instance:

@magento give me test instance - deploy test instance based on PR changes
@magento give me 2.4-develop instance - deploy vanilla Magento instance

For more details, please, review the Magento Contributor Guide documentation.

sivaschenko · 2020-01-22T21:33:02Z

@konarshankar07 thanks for opening the pull request! there was a jasmine test created for this method, could you include it with this pull request as well?

konarshankar07 · 2020-01-23T18:32:44Z

@sivaschenko Yes, I'm going to add those jasmine test with this PR but I think we should cover other methods with the jasmine test for data-storage so I'm working on it

…uest

diazwatson · 2020-02-04T22:26:18Z

Waiting for magento/adobe-stock-integration#884 to be merged in order to merge this one

konarshankar07 · 2020-02-05T05:02:00Z

@diazwatson ... Actually its opposite to your comment. We need to merge this PR in order to merge ASI PR

diazwatson · 2020-02-05T09:57:44Z

Thanks for clarifying @konarshankar07
What is left in this PR then?

konarshankar07 · 2020-02-05T10:14:16Z

@diazwatson... Code Review for jasmine test cause I'm very new to the jasmine test so I might have missed any test or needs an improvement
Thanks

diazwatson · 2020-02-05T10:53:16Z

Maybe @filmaj can help? 😅

konarshankar07 · 2020-02-05T11:07:45Z

@diazwatson ... Yes @sivaschenko or @filmaj both can review :)

filmaj

Nicely done on the tests! I left a couple of comments around labeling. Wording is important 😄

filmaj · 2020-02-05T17:39:55Z

dev/tests/js/jasmine/tests/app/code/Magento/Ui/base/js/grid/data-storage.test.js

+                expect(model.getRequest).toHaveBeenCalled();
+            });
+
+            it('Return "getRequestData" method', function () {


One thing I think we should try to do is provide good test names. This may seem like a silly recommendation, but as a first-time contributor to the project, one of the first things I do to familiarize myself with the project is read the tests! While I understand what the intention behind this test is, it only became clear to me after reading the data-storage getData method.
I would suggest naming the test something like "it returns cached request data if a cached request exists and no refresh option is provided". In this way, the name of the test alone is enough to explain to me some key functionality of this module.

I'd agree that test names should be more detailed to differentiate test between each other, however, I wouldn't make them too long. I don't think test results can be accessed without reading the code (by name only).

filmaj · 2020-02-05T17:41:03Z

dev/tests/js/jasmine/tests/app/code/Magento/Ui/base/js/grid/data-storage.test.js

+                expect(model.getRequestData).toHaveBeenCalled();
+            });
+
+            it('Return "requestData" method', function () {


Along the same lines as my last comment, let's qualify the test name further. How does it differentiate from the previous test? having multiple tests with the same name is not recommended - that can be quite confusing. Wording matters! Remember that tests are a tool to help us, the developers, be productive and guide us. Spending the extra few minutes to properly comment, document and name things goes a long way to making the code maintainable long term.

konarshankar07 · 2020-02-05T18:38:39Z

Thanks @filmaj for reviewing the pull request and will work on the your suggestions

sivaschenko

Hi @konarshankar07 thank you for the pull request! The implementation looks fine, the only comments I have are addressed to the test:

I don't think that abstract checks like toBeDefined, toBeTruthy, expect(type).toEqual provide enough information to assume that the method is working correctly
I'd limit the tests to verifying the actual return of the method depending on arguments

sivaschenko · 2020-02-07T14:28:05Z

dev/tests/js/jasmine/tests/app/code/Magento/Ui/base/js/grid/data-storage.test.js

+            it('Check returned value if method called with argument', function () {
+                var ids = [1,2,3];
+
+                expect(model.getByIds(ids)).toBeDefined();


I don't think toBeDefined is a specific enough case to be checked separately. I'd focus on testing the concrete return values instead

sivaschenko · 2020-02-07T14:29:47Z

dev/tests/js/jasmine/tests/app/code/Magento/Ui/base/js/grid/data-storage.test.js

+                dataScope: 'magento'
+            });
+
+            it('check for defined', function () {


Do you think check for defined and check method type are necessary? If the function is not declared the following tests will fail anyway and it wouldn't be hard to identify the problem.

sivaschenko · 2020-02-07T14:31:34Z

dev/tests/js/jasmine/tests/app/code/Magento/Ui/base/js/grid/data-storage.test.js

+                expect(model.initConfig()).toBeDefined();
+            });
+
+            it('Check returned value type if method called without arguments', function () {


I don't think Check for defined, Check method type, Check returned value if method called without arguments and Check returned value type if method called without arguments are necessary

sivaschenko · 2020-02-07T14:32:56Z

dev/tests/js/jasmine/tests/app/code/Magento/Ui/base/js/grid/data-storage.test.js

+                expect(model.getByIds(ids)).toEqual(false);
+            });
+
+            it('Return array if "getByIds" has been called', function () {


it would be good to test the case when ids parameter contains both existing and non-existing ids

sivaschenko · 2020-02-07T14:33:35Z

dev/tests/js/jasmine/tests/app/code/Magento/Ui/base/js/grid/data-storage.test.js

+                    }
+                });
+
+                expect(typeof model.getByIds(ids)).toEqual('object');


I'd check for the specific object

sivaschenko · 2020-02-07T14:34:31Z

dev/tests/js/jasmine/tests/app/code/Magento/Ui/base/js/grid/data-storage.test.js

+                var ids = [1,2,3],
+                    type = typeof model.getIds(ids);
+
+                expect(type).toEqual('object');


I'd check for specific return

sivaschenko · 2020-02-07T14:36:29Z

dev/tests/js/jasmine/tests/app/code/Magento/Ui/base/js/grid/data-storage.test.js

+                expect(typeof model.getIds).toEqual('function');
+            });
+
+            it('check returned value if method called with argument', function () {


I don't think it's beneficial to test booth return type and return value - only one check for return value would be enough.

It'd be better to check the case when no arguments are passed to getIds method instead.

sivaschenko · 2020-02-07T14:36:47Z

dev/tests/js/jasmine/tests/app/code/Magento/Ui/base/js/grid/data-storage.test.js

+            it('check returned value if method called with argument', function () {
+                var ids = [1,2,3];
+
+                expect(model.getIds(ids)).toBeDefined();


toBeDefined is not actually a check for value, it would be good to verify the actual returned value

konarshankar07 · 2020-02-10T05:21:22Z

dev/tests/js/jasmine/tests/app/code/Magento/Ui/base/js/grid/data-storage.test.js

                model.cacheRequest(data, params);
-                expect(typeof model._requests).toEqual('object');
-                expect(model.getIds).toHaveBeenCalled();
+                expect(model.getRequest(params)).toEqual(request);


I think here we need to also test model._requests

I think it's better to test it through getRequest as _requests property is a detail of internal implementation

Okay, _requests array pushed functionality is added in the cacheRequest method so I thought that we need to cover in the cacheRequest functionality

magento-engcom-team · 2020-02-10T09:19:40Z

Hi @sivaschenko, thank you for the review.
ENGCOM-6840 has been created to process this Pull Request

engcom-Bravo · 2020-02-10T11:10:00Z

✔️ QA Passed

The link to Configuration - System - Adobe Stock Integration doesn't disappear after changing between per page

…e component #26502

m2-assistant · 2020-02-13T06:27:04Z

Hi @konarshankar07, thank you for your contribution!
Please, complete Contribution Survey, it will take less than a minute.
Your feedback will help us to improve contribution process.

Error message to becached for grid data storage component

c9d5e8e

konarshankar07 requested a review from DrewML as a code owner January 22, 2020 17:21

konarshankar07 requested a review from sivaschenko January 22, 2020 17:22

sivaschenko requested a review from VladimirZaets January 22, 2020 21:31

konarshankar07 mentioned this pull request Jan 23, 2020

[MFTF] Message disappear when the request with broken credentials is sent second time magento/adobe-stock-integration#884

Merged

konarshankar07 added 2 commits January 24, 2020 01:11

Jasmine test added for initConfig, getByIds, getRequestData, cacheReq…

8357cf8

…uest

More jasmine tests added

a17c2a5

sivaschenko added Award: bug fix Auto-Tests: Covered All changes in Pull Request is covered by auto-tests Award: special achievement Award: test coverage labels Jan 28, 2020

More method cover with jasmine tests

7fafb2d

konarshankar07 changed the title ~~[WIP] : {ASI} :- Error message to be cached for grid data storage component~~ {ASI} :- Error message to be cached for grid data storage component Feb 1, 2020

Fixed static tests

9de1e59

filmaj reviewed Feb 5, 2020

View reviewed changes

improve test names

c3ba2ce

diazwatson requested a review from filmaj February 6, 2020 20:36

sivaschenko requested changes Feb 7, 2020

View reviewed changes

ghost assigned sivaschenko Feb 7, 2020

ghost added the Progress: needs update label Feb 7, 2020

feedback changes

0aef9ad

konarshankar07 requested a review from sivaschenko February 7, 2020 19:46

magento#26502: Code review changes

7ef0c88

konarshankar07 commented Feb 10, 2020

View reviewed changes

konarshankar07 added 2 commits February 10, 2020 11:25

Fixed static tests

a1d2a84

code cleanup

1f2ed90

sivaschenko approved these changes Feb 10, 2020

View reviewed changes

ghost added Progress: ready for testing and removed Progress: needs update labels Feb 10, 2020

magento-engcom-team added Release Line: 2.4 Area: Frontend Component: Ui labels Feb 10, 2020

engcom-Bravo self-assigned this Feb 10, 2020

ghost added Progress: testing in progress and removed Progress: ready for testing labels Feb 10, 2020

ghost added Progress: extended testing and removed Progress: testing in progress labels Feb 10, 2020

engcom-Charlie self-assigned this Feb 10, 2020

ghost added Progress: accept and removed Progress: extended testing labels Feb 10, 2020

magento-engcom-team mentioned this pull request Feb 13, 2020

How to switch base,thumbnail images in magento 2 back end #863

Closed

magento-engcom-team pushed a commit that referenced this pull request Feb 13, 2020

ENGCOM-6840: {ASI} :- Error message to be cached for grid data storag…

5e86f23

…e component #26502

magento-engcom-team merged commit 1f2ed90 into magento:2.4-develop Feb 13, 2020

sdzhepa mentioned this pull request May 9, 2022

tyutyu sdzhepa/magento2#5

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

{ASI} :- Error message to be cached for grid data storage component #26502

{ASI} :- Error message to be cached for grid data storage component #26502

konarshankar07 commented Jan 22, 2020

m2-assistant bot commented Jan 22, 2020

sivaschenko commented Jan 22, 2020

konarshankar07 commented Jan 23, 2020

diazwatson commented Feb 4, 2020

konarshankar07 commented Feb 5, 2020

diazwatson commented Feb 5, 2020

konarshankar07 commented Feb 5, 2020

diazwatson commented Feb 5, 2020

konarshankar07 commented Feb 5, 2020

filmaj left a comment

filmaj Feb 5, 2020

sivaschenko Feb 7, 2020

filmaj Feb 5, 2020

konarshankar07 commented Feb 5, 2020

sivaschenko left a comment

sivaschenko Feb 7, 2020

sivaschenko Feb 7, 2020

sivaschenko Feb 7, 2020

sivaschenko Feb 7, 2020

sivaschenko Feb 7, 2020

sivaschenko Feb 7, 2020

sivaschenko Feb 7, 2020

sivaschenko Feb 7, 2020

konarshankar07 Feb 10, 2020

sivaschenko Feb 10, 2020

konarshankar07 Feb 10, 2020

magento-engcom-team commented Feb 10, 2020

engcom-Bravo commented Feb 10, 2020

m2-assistant bot commented Feb 13, 2020

{ASI} :- Error message to be cached for grid data storage component #26502

{ASI} :- Error message to be cached for grid data storage component #26502

Conversation

konarshankar07 commented Jan 22, 2020

Description (*)

Fixed Issues (if relevant)

Manual testing scenarios (*)

Questions or comments

Contribution checklist (*)

m2-assistant bot commented Jan 22, 2020

sivaschenko commented Jan 22, 2020

konarshankar07 commented Jan 23, 2020

diazwatson commented Feb 4, 2020

konarshankar07 commented Feb 5, 2020

diazwatson commented Feb 5, 2020

konarshankar07 commented Feb 5, 2020

diazwatson commented Feb 5, 2020

konarshankar07 commented Feb 5, 2020

filmaj left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

konarshankar07 commented Feb 5, 2020

sivaschenko left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

magento-engcom-team commented Feb 10, 2020

engcom-Bravo commented Feb 10, 2020

m2-assistant bot commented Feb 13, 2020