cms block, get by identifier and #635

[GovindaSharma]

fixed issue #635 cms block, get by identifier

Description (*)

fixed issue #635 cms block, get by identifier

Fixed Issues (if relevant)

1. cms block, get by identifier and  #635: cms block, get by identifier

Manual testing scenarios (*)

Steps to reproduce (*)

Create atleast 2 store views
Create 2 cms blocks, with the same identifier, set storeview scope two the 2 different store views created at the top
3: Get the cms block with the identifier - it will ALWAYS get the cms block from the default webstore.
4: (You can check with storeConfig: { code } that you actually are running with a different scope.

Now the query will be

{
  storeConfig {
    code
  }
  cmsBlocks(identifiers: "test_page",
  scope_code: ""
  ) {
    items {
      content
    }
  }
}

in scope_code store code need to be passed and it will fetch according to store

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 on Travis CI are green)

[TomashKhamlai]

TestCase 1: (changes required)
getCmsScopeCodeNoSuchEntity
message should be "message": "Store View Does Not Exist"
and probably it is will even better to provide more detail so that we get "message": "Store View with scope_code \"%scope_code\" Does Not Exist" but the main idea is not to confuse people with Store/Store View

[TomashKhamlai]

Test Case 2: (no changes are required, test coverage only)
Create additional website with store and store view
Create CMS Block for the Main Website on Default Store View that has some text content like: <p>text{{number of store from StoreManagerInterface}}</p>
Create CMS Block for the store view on additional website that has some text content like: <p>text{{number of store from StoreManagerInterface}}</p>
Assert the content of the block from the first store.
Assert the content of the block from the second store.
Assert that contents are different

[TomashKhamlai]

@naydav, I will create few issues found during testing this PR as a separate tasks.
@GovindaSharma, we encourage people to cover pull requests with api-functional tests. You are the author of the PR and this right primarily belongs to you. But if it is too complicated for you, please create an issue like it was done here

[TomashKhamlai]

#671 reproducible here

[TomashKhamlai]

@GovindaSharma you have uncompleted tasks:

• All new or changed code is covered with unit/integration tests (if applicable)
• All automated tests passed successfully (all builds on Travis CI are green)

[TomashKhamlai]

Case: send

query {
  storeConfig {
    id
    website_id
  }
  cmsBlocks(
    identifiers: null
    scope_code: null
  ) {
    items {
      content
    }
  }
}

and get this "message": "\"identifiers\" of CMS blocks should be specified"
send another

query {
  storeConfig {
    id
    website_id
  }
  cmsBlocks(
    identifiers: ""
    scope_code: null
  ) {
    items {
      content
    }
  }
}

and get "message": "Scope Code of CMS blocks should be specified"
then send

query {
  storeConfig {
    id
    website_id
  }
  cmsBlocks(
    identifiers: ""
    scope_code: ""
  ) {
    items {
      content
    }
  }
}

and get "The CMS block with the "" ID doesn't exist."

@naydav, why identifiers is not required field. What is the point of sending empty strings. I have seen many times that sending "" or null results in error that some field is not-nullable. Maybe the better logic is to check if the property is omitted and thn address it to all or default equivalent in REST rather than allowing empty strings?

[GovindaSharma]

hi @TomashKhamlai thanks for your review.Just wanted to know only unit test i have to fix right? or is there any other thing i have to check also?

[TomashKhamlai]

@GovindaSharma, in GraphQL we cover with api-functional tests, but fixtures are taken from integration. To learn more check accepted/merged PRs or better visit these pages:
Web API
GraphQL

And if you have some free time, I invite you join our conversation #635, because the issue you are trying to fix was reproducible but now it isn't. Perhaps it is still reproducible in certain cases.

[TomashKhamlai]

Generally, it works. You tried to implement scope selection as an input parameter, but we already use headers for defining the scope. @GovindaSharma, do you want to continue to work on this issue? I've added a comment today.

Please respond to this message. We are thinking about closing this PR but your feedback is important for us.

In case you are reading this message when PR is closed, don't be shy to create another PR for the same issue. Thank you for your interest.

[naydav]

Generally, it works. You tried to implement scope selection as an input parameter, but we already use headers for defining the scope. @GovindaSharma, do you want to continue to work on this issue? I've added a comment today.

Please respond to this message. We are thinking about closing this PR but your feedback is important for us.

In case you are reading this message when PR is closed, don't be shy to create another PR for the same issue. Thank you for your interest.
@GovindaSharma

Agee with @TomashKhamlai. The scope should be passed as a header.
https://devdocs.magento.com/guides/v2.3/graphql/send-request.html
So we need to change implementation.

[ghost]

Hi @GovindaSharma, 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.