Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

System information error when error is fixed but page wasn't refreshed #919

Closed
snky1987 opened this issue Jan 6, 2015 · 12 comments
Closed
Labels
bug report Issue: Ready for Work Gate 4. Acknowledged. Issue is added to backlog and ready for development

Comments

@snky1987
Copy link

snky1987 commented Jan 6, 2015

When you open admin panel in few windows and you do the change in the layout then system throws System Message about invalidated cache.

Then, when you fix this issue in a new window and don't refresh the old one you will still see a bar about system error but when you click on it there is no message in a pop up.

I believe it would be worth to throw a message like "All system warning have been fixed. Please refresh the page" or similar. Just in case as empty message looks a bit suspicious.

Screen prove: http://prntscr.com/5p48py

@otoolec
Copy link
Contributor

otoolec commented Jan 22, 2015

Thanks for reporting this bug. I've opened up MAGETWO-32969 internally to track and resolve the issue.

@sshrewz sshrewz added bug report Issue: Ready for Work Gate 4. Acknowledged. Issue is added to backlog and ready for development labels Jan 30, 2015
@marcozheng
Copy link
Contributor

Hi @snky1987, thank you for spending time and efforts trying to make Magento 2 better!

The M2 development team has reviewed this issue and evaluated the changes you are proposing.
We believe that this is a very important and necessary improvement we should add to M2.

There are a few things we'd like to ask you to kindly address before we merge in your commits:

  • Severity level - instead using the value returned from $this->getRequest()->getParam('severity'), let's go ahead and set the severity to be the "notice" level for all cases. So basically, let's set (string)\Magento\Framework\Notification\MessageInterface::SEVERITY_NOTICE as the value for 'severity' for the default message.
  • Default message - let's move the declaration of default message into the if (empty($result)) block, because it will be only used here - no need to declare the default message when the $result is not empty. Basically something like this:
        if (empty($result)) {
            $result[] = [
                'severity' => (string)\Magento\Framework\Notification\MessageInterface::SEVERITY_NOTICE,
                'text' => 'You have viewed and resolved all recent system notices. '
                    . 'Please refresh the web page to clear the notice alert.',
            ];
        }
  • Message content: let's use something like 'You have viewed and resolved all recent system notices. Please refresh the web page to clear the notice alert.' The reason for this is that the system notices may not always be "issues", they can also be "warnings" or simple "notices".
  • We have just refactored our copyright docblocks recently, so please revert your change there and take whatever the latest upstream has.
  • Please revert the code style change you proposed for the following lines:
        $this->getResponse()->representJson(
            $this->_objectManager->get('Magento\Core\Helper\Data')->jsonEncode($result)
        );
  • All the other code style changes look good and reasonable

Again, we really appreciate your time and efforts, and we are looking forward to accepting your contribution!

@anupdugar
Copy link
Contributor

Also we encourage use of Dependency Injection wherever possible instead of using objectManager directly:
e.g.:

    /**
     * Initialize ListAction
     * 
     * @param \Magento\Backend\App\Action\Context $context
     * @param \Magento\Core\Helper\Data $coreHelper
     */
    public function __construct(\Magento\Backend\App\Action\Context $context, \Magento\Core\Helper\Data $coreHelper)
    {
        $this->coreHelper = $coreHelper;
        parent::__construct($context);
    }

and then

$this->getResponse()->representJson(
            $this->coreHelper->jsonEncode($result)
        );

@anupdugar
Copy link
Contributor

@snky1987 Did you get a chance to look at the suggested code changes for your PR?

@snky1987
Copy link
Author

@anupdugar Hi, I will today. Recently i had too much work with eBay service in AngularJS

@ilol ilol added the TECH label Feb 17, 2015
@vpelipenko vpelipenko removed the TECH label Feb 17, 2015
@anupdugar
Copy link
Contributor

No problem @snky1987. Feel free to get back if you need any inputs or have any suggestions or recommendations.

@sshrewz
Copy link

sshrewz commented Mar 3, 2015

Hi @snky1987, we haven't heard back from you for quite some time. To avoid closure of this issue, can you update us by end of this week?

@anupdugar
Copy link
Contributor

Closing this issue since the PR is being tracked in #922.

@otoolec
Copy link
Contributor

otoolec commented Mar 7, 2015

Re-opening this since #922 was closed.

@otoolec otoolec reopened this Mar 7, 2015
@marcozheng
Copy link
Contributor

The M2 dev team has already taken in the commits from @snky1987 and applied the recommended changes from the comments above. The internal ticket has been closed and merged into mainline codebase.

@vpelipenko
Copy link
Contributor

@snky1987, wait for update public repository. We will inform you when fix is available.

magento-team pushed a commit that referenced this issue Mar 23, 2015
…but page wasn't refreshed #919

 - Rephrased the notice message content per feedback from product management
 - Changed the severity level of default message to be "Notice"
 - Replaced usages of object manager with dependency injections
 - Code style changes
@sshrewz
Copy link

sshrewz commented Mar 23, 2015

@snky1987, this is now resolved in 0.74.0-beta1. Thank you again for submitting this issue. Feel free to reach out to us should you have any questions.

@sshrewz sshrewz closed this as completed Mar 23, 2015
magento-team pushed a commit that referenced this issue Mar 15, 2017
Stories:
- MAGETWO-65181: Remove serialize/unserialize in \Magento\Framework\Unserialize\Unserialize
- MAGETWO-65183: Remove serialize/unserialize in \Magento\Framework\Acl\Cache
magento-engcom-team added a commit that referenced this issue Oct 2, 2019
 - Merge Pull Request magento/graphql-ce#919 from XxXgeoXxX/graphql-ce:2.3-develop#911
 - Merged commits:
   1. fa8f566
   2. aaee0c8
   3. e9879d1
   4. da62bd1
   5. 2c4adbf
   6. d167013
   7. 33d8148
   8. adb6517
   9. f52ba88
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug report Issue: Ready for Work Gate 4. Acknowledged. Issue is added to backlog and ready for development
Projects
None yet
Development

No branches or pull requests

8 participants