fixed typos for zero order credit memo feature#19288
fixed typos for zero order credit memo feature#19288magento-engcom-team merged 1 commit intomagento:2.3-developfrom
Conversation
magento-engcom-team
added
Partner: Wagento
|
Hi @viral-wagento. Thank you for your contribution
For more details, please, review the Magento Contributor Assistant documentation |
|
@sivaschenko , can you please look into this? |
|
Hi @viral-wagento. Thanks for the collaboration. Please add the translation for "Allow Zero GrandTotal for Credit Memos" and "Allow Zero GrandTotal for Credit Memos" phrases to en_US.csv |
|
@viral-wagento The build on Travis was terminated. It's some infrastructure problem, so don't worry about it. Thanks |
orlangur
left a comment
orlangur
left a comment
There was a problem hiding this comment.
Please revert wrong changes and squash all into single commit.
There was a problem hiding this comment.
This is not needed - understandable from context
There was a problem hiding this comment.
I think we should add label - 'Allow Zero GrandTotal for Credit Memos'
because if we have only label - 'Allow Zero GrandTotoal' - how can one understand about what we are talking?
It is fine from your point that - I will name filed - 'Allow Zero GrandTotal' - that is understandable.
@orlangur , once you respond, i will commit my changes
There was a problem hiding this comment.
@viral-wagento it was just fine: "Allow Zero GrandTotal" for [potential] group of fields, "Allow Zero GrandTotal for Creditmemo" for specific one.
There was a problem hiding this comment.
@orlangur
Ohh, I have updated my changes earlier, let me know if any.
Thanks!
There was a problem hiding this comment.
@viral-wagento to summarize: changes in app/code/Magento/Sales/etc/adminhtml/system.xml are not needed, changes creditmemo -> credit memo are not needed, please squash all needed changes into a single commit and force push.
There was a problem hiding this comment.
@orlangur Do you think it should be "Allow Zero GrandTotal" or "Allow Zero GrandTotal for Credit Memos" in system xmk? @viral-wagento can you please also remove unused translation from csv, once decided with system.xml
There was a problem hiding this comment.
@orlangur and @sivaschenko
once system.xml changes are finalised from both of you, i will do changes in system.xml and its respected changes in translation csv.
Thanks!
There was a problem hiding this comment.
@sivaschenko should not be changed: "Allow Zero GrandTotal" for group, "Allow Zero GrandTotal for Credit Memos" for field.
There was a problem hiding this comment.
Done, as per your last comment - @orlangur
There was a problem hiding this comment.
There are hundreds of "reditmemo" occurrences in code, including csv files. Please revert "credtimemo" -> "credit memo" changes.
There was a problem hiding this comment.
Please generate csv properly so that quotes remain in place.
|
@magento-engcom-team give me 2.3-develop instance |
|
Hi @orlangur. Thank you for your request. I'm working on Magento 2.3-develop instance for you |
|
Hi @orlangur, here is your Magento instance. |
There was a problem hiding this comment.
@viral-wagento please revert changes in app/code/Magento/Sales/Model/Order.php related to credit memos too.
There was a problem hiding this comment.
@viral-wagento please check https://github.com/magento/magento2/pull/19288/files to see all items which are not done, apply changes and then squash them into a single commit.
There was a problem hiding this comment.
@viral-wagento almost there: https://github.com/magento/magento2/pull/19288/files
Please remove unneeded space "credit memo" -> "creditmemo" and squash changes into a single commit performing a force push.
There was a problem hiding this comment.
@orlangur Ok, so i should do this change 'Allow Zero GrandTotal for Credit memo' => 'Allow Zero GrandTotal for Creditmemo' - in system.xml and csv file. Right?
There was a problem hiding this comment.
@viral-wagento and in comment
// Case when Adjustment Fee (adjustment_negative) has been used for first credit memo
There was a problem hiding this comment.
@orlangur , okay going to change there too.
But then what about Retrieve order credit memo (refund) availability Retrieve credit memo for zero total refunded availability. and Retrieve credit memo for zero total availability.
Should i change - credit memo => creditmemo here?
They are function description
There was a problem hiding this comment.
@viral-wagento thanks! No need to change, as they were there before, not really worth fixing.
Changes look good to me now, please squash them into a single commit. Talk to me if you need any assistance with that.
viral-wagento
force-pushed
the
feature-creditmemo-typos
branch
from
83aaeb6 to
6918011
Compare
|
Hi @sidolov, thank you for the review. |
|
Hi @viral-wagento. Thank you for your contribution. |
6 participants
Description
Fixed typos and added proper variable names in below files:
app/code/Magento/Sales/Model/Order.php
app/code/Magento/Sales/etc/adminhtml/system.xml
Manual testing scenarios
Changes done as below:
app/code/Magento/Sales/Model/Order.php
Line:668 - replaced $hasActinFlag to $hasActionFlag
Line:688 - replaced $dueAmountCondition to $hasDueAmount
Line:690 - replaced $creditmemos to $creditMemos
app/code/Magento/Sales/etc/adminhtml/system.xml
Line:52 - replaced Allow Zero GrandTotal to Allow Zero GrandTotal for Credit Memos
Line:54 - replaced Allow Zero GrandTotal for Creditmemo to Allow Zero GrandTotal for Credit Memos
Contribution checklist (*)