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

Mage_Usa: added "USPS Ground Advantage" and removed "First Class Package" #3413

Merged
merged 6 commits into from
Aug 29, 2023
Merged

Conversation

fballiano
Copy link
Contributor

Untested, taken from #3300 (comment)

@github-actions github-actions bot added the Component: Usa Relates to Mage_Usa label Jul 27, 2023
@fballiano fballiano changed the title Added "USPS Ground Advantage" to Mage_Usa Mage_Usa: added "USPS Ground Advantage" and removed "First Class Package" Jul 27, 2023
@fballiano fballiano added the review needed Problem should be verified label Jul 29, 2023
@sreichel
Copy link
Contributor

sreichel commented Aug 3, 2023

Untested? Just to have another commit?

@Flyingmana
Copy link
Contributor

Untested? Just to have another commit?

This response has no substance and contains apparently a personal attack, see this as a Warning for a Code of Conduct Violation

@fballiano
Copy link
Contributor Author

It's only a way to take a user's discussion (you can read that there were interactions in order to understand what we should do) and put it into code in order not to let it be forgotten.
I've always tested (as far as my abilities go) other's people code and mine, in this case I can't test because I don't have enough knowledge about USPS systems to understand if it's working as intended.

@sreichel
Copy link
Contributor

sreichel commented Aug 3, 2023

Untested? Just to have another commit?

This response has no substance and contains apparently a personal attack, see this as a Warning for a Code of Conduct Violation

Thats just wrong.

Why to commit untested code? Cant understand ... QA?

@addison74
Copy link
Contributor

We focus the discussion on the word "untested" instead of losing sight of the big picture. When I came here, I kept hearing very often the phrase "if you have an idea, create a PR" from the maintenance side. I believe that any proposed PR starts from the beginning with some predetermined conditions such as: it is not a problem in the code, yes it is a problem but it does not solve it. Here, everyone can be the devil's advocate and do well as long as he has solid arguments.

Fabrizio took a code suggested by someone and mentioned that it is untested. In order for this PR to be approved, it needs at least 2 green, 1 green and 2 gray approvals. As you can see, no one has approved it, if they do, it means that they have tested it and can provide feedback. The fact that he opened it is beneficial, those with similar problems can checkout and determine if something is solved or not.

Let's not get attached to one word in a sentence. I pretend that we are mature people.

@fballiano
Copy link
Contributor Author

tested with a customer which confirms this patch solves the problem!

Screenshot 2023-08-04 alle 08 45 13

this should be released as soon as possible because at the moment whoever's using USPS is missing on the cheaper shipping options and it is a big deal.

@fballiano
Copy link
Contributor Author

@michaelj000 @johnvoncolln

@johnvoncolln
Copy link

@fballiano this is great, thanks for doing it!

@fballiano
Copy link
Contributor Author

it seems we have 2 positive tests, can we please try to merge this?

@fballiano
Copy link
Contributor Author

@johnvoncolln could you please approve this PR at this link: https://github.com/OpenMage/magento-lts/pull/3413/files clicking the green button on the right?

Without enough votes I can't merge it and release it.

In this case, having a broken USPS module is a disservice we should fix, we have the fix and we get stuck forever on such a minor thing.

Copy link

@johnvoncolln johnvoncolln left a comment

Choose a reason for hiding this comment

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

Looks good to me!

Copy link
Contributor

@kiatng kiatng left a comment

Choose a reason for hiding this comment

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

Agreed with @fballiano to merge as First-Class will be deprecated in September.

@fballiano fballiano merged commit 621f21f into OpenMage:main Aug 29, 2023
15 checks passed
@fballiano fballiano deleted the usps branch August 29, 2023 07:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Component: Usa Relates to Mage_Usa review needed Problem should be verified
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants