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.

Sign up for GitHub

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

Jump to bottom

Fix issue #27355 with too many cookies in admin #27364

Merged
merged 1 commit into from
Aug 8, 2020
Merged

Fix issue #27355 with too many cookies in admin #27364

merged 1 commit into from
Aug 8, 2020

Conversation

ihor-sviziev
Copy link
Contributor

@ihor-sviziev ihor-sviziev commented Mar 19, 2020 •
edited
Loading

Port MC-19247 for adminhtml in order to not use jQuery.localStorage plugin

Description (*)

As part of 71c781d there were significantly changed translation module for frontend, but for backend was left old implementation that caused #27355

Before my changes js translation file was loaded via
https://github.com/magento/magento2/blob/71c781df5536a2ec79c9cf42ade0c262396767a3/app/code/Magento/Translation/view/base/templates/translate.phtml

After - via https://github.com/magento/magento2/blob/71c781df5536a2ec79c9cf42ade0c262396767a3/app/code/Magento/Translation/view/base/web/js/mage-translation-dictionary.js
so it not creating any cookies anymore.

Related Pull Requests

Fixed Issues (if relevant)

  1. Fixes Adminhtml - Too many mage-translation-file-version cookies #27355 : Adminhtml - Too many mage-translation-file-version cookies
  2. Reduces time of reproducing Incorrect message: Unable to send the cookie. Maximum number of cookies would be exceeded #17195

Manual testing scenarios (*)

  1. See Adminhtml - Too many mage-translation-file-version cookies #27355

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
Copy link

m2-assistant bot commented Mar 19, 2020

Hi @ihor-sviziev. 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.

@ihor-sviziev ihor-sviziev linked an issue Mar 19, 2020 that may be closed by this pull request
Adminhtml - Too many mage-translation-file-version cookies #27355
Closed
@lbajsarowicz lbajsarowicz changed the title Fix issue with too many cookies in admin Fix issue #27355 with too many cookies in admin Mar 19, 2020
lbajsarowicz
lbajsarowicz previously approved these changes Mar 20, 2020
View reviewed changes
Copy link
Contributor

@lbajsarowicz lbajsarowicz left a comment

Choose a reason for hiding this comment

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

Verified that locally with different scenarios.
Change works as expected, translations (Polish) still work.
I'd love to provide Functional Tests as a separate PR.

@ghost ghost assigned lbajsarowicz Mar 20, 2020
@magento-engcom-team
Copy link
Contributor

Hi @lbajsarowicz, thank you for the review.
ENGCOM-7156 has been created to process this Pull Request
✳️ @lbajsarowicz, could you please add one of the following labels to the Pull Request?

Label Description
Auto-Tests: Covered All changes in Pull Request is covered by auto-tests
Auto-Tests: Not Covered Changes in Pull Request requires coverage by auto-tests
Auto-Tests: Not Required Changes in Pull Request does not require coverage by auto-tests

@ihor-sviziev
Copy link
Contributor Author

@lbajsarowicz in functional tests I believe we can’t handle specifically this case because AFAIK there is no way to find all cookies with the same domain and name, but different path.
What we can check here is just that cookie isn’t added, but not really sure it would be ok as if functionality will be adjusted in future by adding these cookies back - it will not mean that became works incorrect.
I still think it’s better not to cover this case at all

@lbajsarowicz
Copy link
Contributor

Ok. Its fine for me.

@lbajsarowicz lbajsarowicz added the Auto-Tests: Not Required Changes in Pull Request does not require coverage by auto-tests label Mar 22, 2020
@engcom-Alfa engcom-Alfa self-assigned this Mar 24, 2020
@engcom-Alfa
Copy link
Contributor

✔️ QA Passed

Before:
before

After:
after

Tested with Germany language and everything work as expected

@engcom-Echo engcom-Echo self-assigned this Mar 24, 2020
@engcom-Echo
Copy link
Contributor

Failed functional tests not related to the changes in this PR

@ghost ghost dismissed lbajsarowicz’s stale review March 27, 2020 10:12

Pull Request state was updated. Re-review required.

@magento-engcom-team
Copy link
Contributor

Hi @ihor-sviziev, here is your new Magento instance.
Admin access: https://pr-27364.instances.magento-community.engineering/admin_36e6
Login: 49fdcfb1 Password: 41e9d8d4a8fc
Instance will be terminated in up to 3 hours.

@ihor-sviziev
Copy link
Contributor Author

@engcom-Alfa I can't reproduce this issue on test instance.
image

Also I see the file mage-translation-directory is loaded, so we shouldn't have any cookies:
image

@engcom-Alfa
Copy link
Contributor

engcom-Alfa commented Aug 4, 2020 •
edited
Loading

Hi @ihor-sviziev . I rechecked it and everything works well.

Thanks!

@engcom-Alfa
Copy link
Contributor

engcom-Alfa commented Aug 5, 2020 •
edited
Loading

Hi @lbajsarowicz . Could you approve it again?
Thanks!

@magento-engcom-team
Copy link
Contributor

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

@engcom-Echo engcom-Echo added the QA: Ready to add to Regression Scope Should be analyzed and added to Regression Testing Scope(if applicable) label Aug 5, 2020
@engcom-Alfa
Copy link
Contributor

✔️ QA Passed

Before: ✖️

2020-08-04_12-26

After: ✔️

Screenshot from 2020-08-04 16-17-51

@engcom-Alfa engcom-Alfa added QA: Added to Regression Scope Scenario was analysed and added to Regression Testing Scope and removed QA: Ready to add to Regression Scope Should be analyzed and added to Regression Testing Scope(if applicable) labels Aug 6, 2020
@slavvka slavvka mentioned this pull request Aug 8, 2020
Closed
magento-engcom-team pushed a commit that referenced this pull request Aug 8, 2020
@slavvka
ENGCOM-7156: Fix issue #27355 with too many cookies in admin #27364
44862b3
@magento-engcom-team magento-engcom-team merged commit f803554 into magento:2.4-develop Aug 8, 2020
@m2-assistant
Copy link

m2-assistant bot commented Aug 8, 2020

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@engcom-Alfa engcom-Alfa engcom-Alfa requested changes

@lbajsarowicz lbajsarowicz lbajsarowicz approved these changes

@dcaccoun dcaccoun dcaccoun approved these changes

Assignees

@lbajsarowicz lbajsarowicz

@engcom-Alfa engcom-Alfa

@engcom-Echo engcom-Echo

Labels
Area: Frontend Auto-Tests: Not Required Changes in Pull Request does not require coverage by auto-tests Component: Backend Component: Translation Priority: P1 Once P0 defects have been fixed, a defect having this priority is the next candidate for fixing. Progress: accept QA: Added to Regression Scope Scenario was analysed and added to Regression Testing Scope Release Line: 2.4 Severity: S2 Major restrictions or short-term circumventions are required until a fix is available.
Projects
Pull Requests Dashboard
Archived in project
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Adminhtml - Too many mage-translation-file-version cookies
9 participants
@ihor-sviziev @magento-engcom-team @lbajsarowicz @engcom-Alfa @engcom-Echo @VladimirZaets @hostep @dcaccoun @sdzhepa