ofbiz-framework icon indicating copy to clipboard operation
ofbiz-framework copied to clipboard

Improved: Role handling visavis Agreements (OFBIZ-5996)

Open PierreSmits opened this issue 3 years ago • 2 comments

With the change to the PartyRole entity

improved/changed:

  • AgreementServices.groovy: added function to update AgreementRole (or create if needed)
  • services-agreement.xml: added expireAgreementRole, replaced updateAgreementRole, cleanup
  • controler.xml in accounting: deleted createAgreementRole, replaced deleteAgreementRole with expireAgreementRole
  • AgreementForms.xml: adjusted grid for ListAgreementRoles, adjusted AddAgreementRole (plus rename)
  • AgreementScreens.xml: adjusted AgreementRoles (incl. rename, rearrange layout, labels), added screen for AddAgreementRole
  • AccountingDemoData.xml: improved data related to agreement parties
  • OrderDemoData.xml: improved data related to agreement
  • PartySeedData.xml: added roletype records for agreements
  • party-entitymodel.xml: added fromDate and thruDate fields, adjusted prim-key, key-map of PartyRole relation
  • PartyServices.groovy: added funtion to get valid role parties
  • services in party: cleanup updatePartyRole
  • services-view in party: added service to get valid role parties
  • controller in party: added request map to get valid role parties
  • Labels.xml: adjustment of labels to reflect the change

PierreSmits avatar Nov 03 '21 16:11 PierreSmits

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

sonarqubecloud[bot] avatar Nov 08 '21 08:11 sonarqubecloud[bot]

Is this PR considered final @PierreSmits ?

If yes, please provide a new PR with all changes committed with proper commit messages (they differ between commits and do not stick to the commit message template).

Generally, commits should not contain different changes like a bug fix and an improvement (like you can read in https://github.com/apache/ofbiz-framework/pull/333/commits/302b1308ecfc367f5ae607221bd85575b103d298).

Also: why do we now have a new service which mixes create and update? Why not have a create and an update service?

The code in updateAgreementRole() does not provide the logic which is described in the comment (selecting entities without thruDate).

Why all those name changes for requests, forms etc.?

Why does some of the demo data has changed timestamps?

Why the change of ManufacturingUiLabels within a commit with completely different scope?

Why is it necessary to add the timestamp fields for AgreementRole and PartyRole? They are generated automatically like in all other entities which do not have the "no-auto-stamp" set.

I'll have a second review pass when a clean PR is provided. Thanks.

mbrohl avatar Nov 08 '21 15:11 mbrohl