Improved: Invoice Payments for user with 'VIEW' permission (OFBIZ-12384) #344
Conversation
renamed CreateApplicationList.groovy to InvoicePayments.groovy adjusted reference in acc-invoices.adoc regarding invoice payments renamed HELP_editInvoiceApplications.adoc to HELP_invoicePayments.adoc AccountingMenus.xml: - changed menu-item for invoice payments (name and label and link for better perception alignment) InvoiceScreens.xml: re screen InvoiceOverview - removed redundant property-map references - adjusted groovy script reference to new file name. - removed link definitions in screenlets (duplicate of menu-items related to the invoice) - adjusted references to invoice payment definitions in InvoiceForms.xml for better perception alignment re screen for invoice payment - renamed screen from 'InvoiceApplication' to 'InvoicePayments' - adjusted references to titleProperty, tabButtonItem and helpAnchor for better perception alignment - adjusted groovy script reference to new file name. - added conditions for CREATE and UPDATE permission to the start section - adjusted reference to the form (grid) to update existing invoice payments (for users with CREATE or UPDATE Permissions) - adjusted reference to the form (grid) for invoice payments (for users with CREATE or UPDATE Permissions) InvoiceForms.xml - renamed 'ListInvoiceApplications' to 'InvoiceOverviewApplications. Changed from form definition to grid definition, adjusted field order. - renamed 'EditInvoiceApplications' to 'InvoicePayments' for user with VIEW permission. Changed from form definition to grid definition. adjusted field order. - added 'UpdateInvoicePayments for user with 'CREATE' or 'UPDATE' permissions. controller.xml - adjusted requests related to invoice payments for better perception alignment - renamed view-map related to invoice payments for better perception alignment.
* trunk: Improved: Try to reduce the SARIF file used by CodeQL for Java classes (OFBIZ-12361) Improved: Try to reduce the SARIF file used by CodeQL for Java classes (OFBIZ-12361) Fixed: Fix some bugs Spotbugs reports (OFBIZ-12386)
|
Kudos, SonarCloud Quality Gate passed!
|
|
This change does not only fix the issue but also introduces various name changes, also for requests which possibly breaks custom uses. Please provide a PR which only fixes the issue and maybe another one for proposed improvements or name changes. Thanks. |
|
Also, please avoid merge commits, you can better update your personal repository using a rebase. |
|
Please list for each offence:
|
|
Please list every custom code it breaks. That makes that the project, nor the contributor can be held accountable for custom code (in a downstream repository, or implementation). |
Thanks for the suggestion. |
|
You missed my point. If you want to fix something which is broken, please do it solely in a PR and avoid to introduce additional changes. It's harder to test/review and also has more risks to introduce additional bugs if you mix it up with other changes. Changes like name changes also have to be dicussed and reviewed diffrently from bug fixes. They should not slip in with a bug fix. Thanks. |
|
When talking about a bug fix, there is a (potential) validity. However, this is not the case. |
|
I may not be up to date with latest 'rule' changes, but again: please state for each offence, which rule it violates and point out where that violated rule can be found. I am, as I have been, always motivated to work with the community. |
|
The description of the corresponding issue reads: "Currently, a user with only 'VIEW' permissions, as demonstrated in trunk demo with userId = auditor, accessing the payments screen on an invoice sees fields editable and triggers to requests reserved for users with 'CREATE' or 'UPDATE' permissions." If you call it an improvement or bug (which I would call it), the description explains what should be fixed/improved. The changes in this PR are a lot more than just fixing the permission bug decribed there. |
|
Tak for pointing that out. I will change the subject of the ticket. |
|
Quoting Michael Also it's often quite harder to revert... |
|
Pierre, You need to refrain yourself to put unrelated changes into a sole PR. It complicates things for committers, TIA |
|
Sounds a bit weird to now change the issue to match the PR instead of changing the PR to match the issue... |
Thanks for the tip, Jacques. |
|
As I said in the Jira it works |
|
Thanks Jacques. If members of the community feels that there must be a rule for having each minor or trivial improvement to be raised as a separate ticket, and having its own PR, instead of being combined with another ticket (and the PR associated with that), they can of course start that discussion in dev. I believe that any new contributor would say that such processes would be overbearing, cumbersome and counter-productive. Addressing those minor and trivial issues (even without their own tickets) in one go with other tickets (and even without), has been done by every contributor - non-privileged and others - in the past 15 years without raising eye-brows. |
|
It's simple, create sufficient PRs inside the same Jira to separate things . That's what most savvy contributors did for years, using patches before. Then it's easier for committers to:
If you don't do that you will always front what you seem to feel as negligence from committers. We just prefer an easier and especially safer work. |
|
Hi Pierre, First I must say that in general it's not a good idea to edit comment sin GitHub. Because there is no history or way to compare once done, or I don't know how. Also I think that mixing comments in Jira and GH PR is not helping. Anyway, here is an answer to your last comment above. You ask about rules, etc. You don't need rules here, just common sense. And it was clearly explained by Michael above. Not all can be ruled, and even with rules you need a judge. Michael and I were your judges here. Though I wonder how for a "Proud contributor of Apache OFBiz since 2008 (without privileges)" you don't see what the problems are. You are lucky I'm in a good day. Here are more details. If you feel cosmetic changes, like renaming things, should be done you should have made them in another PR. I mean changed/renamed AccountingUiLabels.xml, InvoicePayments.groovy, acc-invoices.adoc, HELP_invoicePayments.adoc, the controller.xml, AccountingMenus.xml , in another PR. Only InvoiceForms.xml and InvoiceScreens.xml should have be part of this PR but without renaming in them. You remember 6727f46 ? That's what I want to avoid. Even if I must say it certainly has not the same extend here. Also, I don't know if it's related to GitHub patches or how Eclipse apply patches. When using apply patch from clipboard in Eclipse to locally test, file renaming is not working. Maybe it's due to how you rename files and should read https://www.patrick-wied.at/blog/rename-files-and-folders-with-git or something alike? That's what I get in cmd line after saving https://patch-diff.githubusercontent.com/raw/apache/ofbiz-framework/pull/344.patch locally. So I can't test your changes locally and something must be done. |
renamed CreateApplicationList.groovy to InvoicePayments.groovy
adjusted reference in acc-invoices.adoc regarding invoice payments
renamed HELP_editInvoiceApplications.adoc to HELP_invoicePayments.adoc
AccountingMenus.xml:
InvoiceScreens.xml:
re screen InvoiceOverview
re screen for invoice payment
InvoiceForms.xml
controller.xml