Skip to content

Removes incorrect csp_whitelist.xml files #30640

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

Merged

Conversation

hostep
Copy link
Contributor

@hostep hostep commented Oct 24, 2020

Description (*)

Somehow 2 csp_whitelist.xml files were incorrectly renamed.
They previously existed in the Braintree and Authorizenet modules, but those modules no longer exist in Magento core, so they should have been removed instead of put in another module.

I assume these were incorrectly solved merge conflicts.

Commits which caused this:

Related Pull Requests

None

Fixed Issues (if relevant)

  1. Fixes Somehow csp_whitelist.xml from Magento_Authorizenet ended up in Magento_CatalogUrlRewrite #30607: Somehow csp_whitelist.xml from Magento_Authorizenet ended up in Magento_CatalogUrlRewrite

Manual testing scenarios (*)

Not relevant, this is just a cleanup

Questions or comments

Maybe somebody needs to review the two commits mentioned above to see if no other strange things happened in there?

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 Oct 24, 2020

Hi @hostep. 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

❗ Automated tests can be triggered manually with an appropriate comment:

  • @magento run all tests - run or re-run all required tests against the PR changes
  • @magento run <test-build(s)> - run or re-run specific test build(s)
    For example: @magento run Unit Tests

<test-build(s)> is a comma-separated list of build names. Allowed build names are:

  1. Database Compare
  2. Functional Tests CE
  3. Functional Tests EE,
  4. Functional Tests B2B
  5. Integration Tests
  6. Magento Health Index
  7. Sample Data Tests CE
  8. Sample Data Tests EE
  9. Sample Data Tests B2B
  10. Static Tests
  11. Unit Tests
  12. WebAPI Tests

You can find more information about the builds here

ℹ️ Please run only needed test builds instead of all when developing. Please run all test builds before sending your PR for review.

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

⚠️ According to the Magento Contribution requirements, all Pull Requests must go through the Community Contributions Triage process. Community Contributions Triage is a public meeting.

🕙 You can find the schedule on the Magento Community Calendar page.

📞 The triage of Pull Requests happens in the queue order. If you want to speed up the delivery of your contribution, please join the Community Contributions Triage session to discuss the appropriate ticket.

🎥 You can find the recording of the previous Community Contributions Triage on the Magento Youtube Channel

✏️ Feel free to post questions/proposals/feedback related to the Community Contributions Triage process to the corresponding Slack Channel

@hostep
Copy link
Contributor Author

hostep commented Oct 24, 2020

@magento run all tests

@hostep
Copy link
Contributor Author

hostep commented Oct 24, 2020

If somebody knows the developers behind the Braintree module, can they redirect them to this comment: #30607 (comment) so we are sure no regressions occur with their module?

@rogyar rogyar self-assigned this Oct 24, 2020
@rogyar
Copy link
Contributor

rogyar commented Oct 24, 2020

That's a really interesting issue. Thank you @hostep for addressing it.

@ghost ghost added Priority: P3 May be fixed according to the position in the backlog. Severity: S3 Affects non-critical data or functionality and does not force users to employ a workaround. labels Oct 24, 2020
@rogyar rogyar added Priority: P3 May be fixed according to the position in the backlog. and removed Priority: P3 May be fixed according to the position in the backlog. labels Oct 24, 2020
@hostep
Copy link
Contributor Author

hostep commented Dec 28, 2020

Any updates here? Anything that needs to be done before this can move forwards? Thanks! 🙂

@rogyar
Copy link
Contributor

rogyar commented Jan 4, 2021

@magento run all tests

@rogyar rogyar added the Auto-Tests: Not Required Changes in Pull Request does not require coverage by auto-tests label Jan 5, 2021
Copy link
Contributor

@rogyar rogyar left a comment

Choose a reason for hiding this comment

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

The failing tests are related to the date issue with the year change but no to the current changeset

@magento-engcom-team
Copy link
Contributor

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

@hostep
Copy link
Contributor Author

hostep commented Mar 6, 2021

Any updates here? PR has been open for almost half a year now 😉

@hostep
Copy link
Contributor Author

hostep commented May 6, 2021

Hi @Den4ik, it looks like you are quite active here lately and maybe you can help move this PR forward a bit?

Let me know if I should still do something here.

@Den4ik
Copy link
Contributor

Den4ik commented May 6, 2021

@magento run all tests

@magento-automated-testing
Copy link

The requested builds are added to the queue. You should be able to see them here within a few minutes. Please re-request them if they don't show in a reasonable amount of time.

Copy link
Contributor

@Den4ik Den4ik left a comment

Choose a reason for hiding this comment

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

@hostep Totally agree with this changes

@magento-engcom-team
Copy link
Contributor

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

@Den4ik Den4ik added the Cleanup label May 6, 2021
@hostep
Copy link
Contributor Author

hostep commented Sep 7, 2021

One more month until the anniversary of this PR. I don't really understand why it takes so long to get processed?

@Den4ik
Copy link
Contributor

Den4ik commented Sep 9, 2021

Hi @hostep
On one of the community Hangouts core team said that currently they are looking only on high priority PRs. This PR has P3 priority that is not high and unfortunately I'm not sure that it will be merged soon

@hostep
Copy link
Contributor Author

hostep commented Sep 9, 2021

But that's not the way PR's should be handled. Priorities can apply to issues but shouldn't be applied to PR's, that's the wrong attitude.
Sure, super high prio PR's can have precedence, but you can't ignore PR's that have been open for many months, that's very discouraging to the contributors. You want contributors to come back to keep investing their precious free time into this open source product. Ignoring PR's is not encouraging at all to contributors. That's not they way you should treat your community.

The end result of priorities on PR's is that the ones with priority 3 or 4 will just never be handled, because there will always be higher priority PR's. So that priority handling of PR's is just dumb.

And I'm not only talking about this PR, there are plenty of other PR's out there that have the same problem. So don't just now decide to merge this PR just to keep me from complaining while ignoring all the other long time open PR's please.

@Den4ik
Copy link
Contributor

Den4ik commented Sep 10, 2021

@hostep I'm totally agree with you

@hostep
Copy link
Contributor Author

hostep commented Sep 14, 2021

Thanks @Den4ik, can you maybe discuss this with the other community maintainers and try to change their minds with the current PR priority system?

This will probably be a problem of the past once the new community organization driven fork comes into play somewhere in the future, but until that happens, we can try to improve the community relations as well over here I believe.

@hostep
Copy link
Contributor Author

hostep commented Oct 22, 2021

I've learned yesterday in the Magento Association Connect event that PR's with prio P3 and P4 will never be merged in Magento core.
Even though I strongly believe that this should get merged because Adobe's original intentions were to remove this. But due to incorrect merge conflict solving it didn't happen.

@nathanjosiah: since this is related to the CSP module, what do you think? Do you agree this shouldn't get merged in core, or should the prio be increased so it does get into the core eventually?

@Den4ik
Copy link
Contributor

Den4ik commented Oct 22, 2021

I believe that we can increase priority for this PR

@hostep
Copy link
Contributor Author

hostep commented Oct 27, 2021

Hi @Den4ik: any updates? Who should we discuss this with? @sidolov maybe?

@Den4ik
Copy link
Contributor

Den4ik commented Oct 27, 2021

@sidolov Please pay attention to this PR

@sidolov sidolov added Priority: P2 A defect with this priority could have functionality issues which are not to expectations. and removed Priority: P3 May be fixed according to the position in the backlog. labels Nov 2, 2021
@hostep
Copy link
Contributor Author

hostep commented Mar 30, 2022

Any movement here?

@sidolov : I've seen some PR's with prio P2 getting merged the last couple of days, so maybe it's finally time that this one gets merged as well after almost 2 years? 😉

@hostep
Copy link
Contributor Author

hostep commented Apr 6, 2022

Thanks! 🙂

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Auto-Tests: Not Required Changes in Pull Request does not require coverage by auto-tests Cleanup Component: CatalogUrlRewrite Component: LoginAsCustomerAdminUi Event: MageCONF CD 2020 Priority: P2 A defect with this priority could have functionality issues which are not to expectations. Progress: accept Release Line: 2.4 Risk: low Severity: S3 Affects non-critical data or functionality and does not force users to employ a workaround.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Somehow csp_whitelist.xml from Magento_Authorizenet ended up in Magento_CatalogUrlRewrite
6 participants