Skip to content
This repository has been archived by the owner on Dec 19, 2019. It is now read-only.

Fixed #176 - Show only active CMS Blocks #221

Merged
merged 8 commits into from
Nov 23, 2018

Conversation

ronak2ram
Copy link
Member

Description

Removed only active block checking code.

Fixed Issues (if relevant)

  1. Show only active CMS Blocks #176 : Show only active CMS Blocks

Steps to reproduce

  1. Checkout to branch 31-cms-page-graphql-support
    Pull request #31 CMS page/block coverage #105
  2. Disable some CMS block: MagentoAdmin > Content > Blocks > choose_your_block ->
    Enable Block: NO -> Save
  3. Run this GraphQL query:
{
  cmsBlocks (identifiers: ["disabled_cms_block", "enabled_cms_block"]){
    items{
      title
      identifier
      content
    }
  }
}

disabled_cms_block: identifier of the disabled cms_block
enabled_cms_block: identifier of the enabled cms_block

Expected result

  1. Get only cms_blocks with status: enabled.
{
  "data": {
    "cmsBlocks": {
      "items": [
        {
          "title": "This is a enabled cms block",
          "identifier": "enabled_cms_block",
          "content": "Happy to help you"
        }
      ]
    }
  }
}

Actual result

{
  "errors": [
    {
      "message": "No such entity.",
      "category": "graphql-no-such-entity",
      "locations": [
        {
          "line": 2,
          "column": 3
        }
      ],
      "path": [
        "cmsBlocks"
      ]
    }
  ],
  "data": {
    "cmsBlocks": null
  }
}

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)

$blockData = $this->blockDataProvider->getData($blockIdentifier);
if (!empty($blockData)) {
$blocksData[$blockIdentifier] = $blockData;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

We need to log the fact that some of the requested blocks were not found, please add this to the else.

Copy link
Contributor

Choose a reason for hiding this comment

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

I provided a little refactoring of this PR
Now blocks data is located in data section, and errors are located in errors section

Also, I removed the logging, because there it’s way to log overflow logfile if I send non-existent block id
And at the same time for the client, there is no information what is the actual error (error is only on the server)

Copy link
Contributor

@paliarush paliarush left a comment

Choose a reason for hiding this comment

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

@ronak2ram thank you for making all necessary changes. The PR looks good now.

@naydav
Copy link
Contributor

naydav commented Nov 22, 2018

Result:

Request

{
  cmsBlocks (identifiers: ["disabled_block", "enabled_block"]){
    items{
      title
      identifier
      content
    }
  }
}

Response

{
  "errors": [
    {
      "message": "The CMS block with the \"disabled_block\" ID doesn't exist.",
      "category": "graphql-no-such-entity",
      "locations": [
        {
          "line": 3,
          "column": 5
        }
      ],
      "path": [
        "cmsBlocks",
        "items",
        0
      ]
    }
  ],
  "data": {
    "cmsBlocks": {
      "items": [
        null,
        {
          "title": "enabled_block",
          "identifier": "enabled_block",
          "content": "<p>enabled_block enabled_block enabled_block</p>"
        }
      ]
    }
  }
}

Valeriy Nayda added 2 commits November 22, 2018 16:48
* @param \Exception|null $cause
* @param int $code
*/
public function __construct(string $message, array $responseData, \Exception $cause = null, int $code = 0)
Copy link
Contributor

Choose a reason for hiding this comment

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

We should not swap the original arguments order to comply with Liskov substitution principle.

@paliarush
Copy link
Contributor

Looks like response structure does not match the request structure after the fix (there is additional data level). Usually in GraphQL they should match.

@naydav naydav removed this from the Release: 2.3.1 milestone Dec 7, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants