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

[Reverse relation] Support configuration of visible fields #13569

Merged

Conversation

BlackbitDevs
Copy link
Contributor

@BlackbitDevs BlackbitDevs commented Nov 14, 2022

For (advanced) many-to-many object relations you can configure the fields to be shown for the relation field grid in the editing panel.
This PR adds the same for reverse relations:

Снимок экрана 2022-11-14 в 16 00 57

Снимок экрана 2022-11-15 в 15 06 37

@github-actions
Copy link

github-actions bot commented Nov 14, 2022

Review Checklist

  • Target branch (10.5 for bug fixes, others 11.x)
  • Tests (if it's testable code, there should be a test for it - get help)
  • Docs (every functionality needs to be documented, see here)
  • Migration incl. install.sql (e.g. if the database schema changes, ...)
  • Upgrade notes (deprecations, important information, migration hints, ...)
  • Label
  • Milestone

@BlackbitDevs BlackbitDevs changed the title add visible fileds to reverse object relation field [Reverse relation] Support configuration of visible fields Nov 15, 2022
@BlackbitDevs BlackbitDevs marked this pull request as ready for review November 15, 2022 13:07
@kingjia90 kingjia90 added this to the 11.0.0 milestone Nov 15, 2022
@mcop1 mcop1 self-assigned this Nov 21, 2022
@mcop1 mcop1 assigned Corepex and unassigned mcop1 Dec 12, 2022
@Corepex
Copy link
Collaborator

Corepex commented Dec 14, 2022

@BlackbitDevs could you please resolve the conflicts? :) thanks

@BlackbitDevs
Copy link
Contributor Author

@Corepex Done.

@Corepex
Copy link
Collaborator

Corepex commented Dec 19, 2022

@BlackbitDevs, could you also please merge the 11.x branch into your branch again - this should fix the test errors - thanks

@BlackbitDevs
Copy link
Contributor Author

@Corepex Done.

@Corepex
Copy link
Collaborator

Corepex commented Dec 27, 2022

@BlackbitDevs could you please fix the tests 😺

@brusch
Copy link
Member

brusch commented Jan 2, 2023

@BlackbitDevs 🏓😉 Just a quick reminder - Thanks!

@BlackbitDevs
Copy link
Contributor Author

@Corepex, @brusch Done.

@Corepex
Copy link
Collaborator

Corepex commented Jan 24, 2023

@BlackbitDevs, thanks for this really cool PR. I really like the idea of customizing the columns of the reverse relation.

One weird thing I found is that if I save the class multiple times (10 - 20 times), I sometimes get the following error: Typed property Pimcore\Model\DataObject\ClassDefinition\Data\ReverseObjectRelation::$ownerClassName must not be accessed before initialization

Seems like this is called in ReverseObjectRelation::getClasses() and throws the error at ReverseObjectRelation::getOwnerClassId -> ($class = DataObject\ClassDefinition::getByName($this->ownerClassName))

image

@BlackbitDevs
Copy link
Contributor Author

BlackbitDevs commented Jan 25, 2023

@Corepex Is fixed. It was caused by wrong property type in

When this shall be initialized with null (what it does when no default value is given and the property does not get set in the __constructor) the type has to be ?string. For more information see https://madewithlove.com/blog/software-engineering/typed-property-must-not-be-accessed-before-initialization/#uninitialized-fields

Copy link
Contributor

@blankse blankse left a comment

Choose a reason for hiding this comment

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

We should init it with null otherwise it will occure a error when you read it without writing

@Corepex
Copy link
Collaborator

Corepex commented Jan 25, 2023

@BlackbitDevs is that PR ready for testing again?

@BlackbitDevs
Copy link
Contributor Author

@Corepex Yes

@Corepex
Copy link
Collaborator

Corepex commented Jan 26, 2023

@BlackbitDevs now i get another error when saving the class multiple times. The following error message gets displayed: Pimcore\Model\DataObject\ClassDefinition::getByName(): Argument #1 ($name) must be of type string, null given, called in /var/www/html/dev/pimcore/pimcore/models/DataObject/ClassDefinition/Data/ReverseObjectRelation.php on line 99 (see screenshot).

image

It seems that the owner_class field gets cleared for some reason after saving ...

image

@Corepex
Copy link
Collaborator

Corepex commented Feb 8, 2023

@BlackbitDevs friendly reminder ⏰ 🍭

@BlackbitDevs
Copy link
Contributor Author

Ok, wrapped another if condition. Can you please try again, @Corepex?

@Corepex
Copy link
Collaborator

Corepex commented Feb 14, 2023

It seems that the owner_class field gets cleared for some reason after saving ...

@BlackbitDevs, this error doesn't seem to be fixed - the owner_class field is still getting cleared out sometimes.

@Corepex
Copy link
Collaborator

Corepex commented Mar 17, 2023

@BlackbitDevs final reminder. ⏰
Please respond latest by 24.03 - otherwise, I will close the ticket due to inactivity.

@BlackbitDevs
Copy link
Contributor Author

@Corepex Please check again. I cannot reproduce the issue. I clicked multiple times on save - with set owner class as well as without. Everything works as expected.

@fashxp fashxp modified the milestones: 11.0.0, 11.1.0 Mar 28, 2023
@Corepex
Copy link
Collaborator

Corepex commented Mar 28, 2023

@BlackbitDevs I just set up a new pimcore 11.x instance. The owner_class issue seems to be fixed now - good job 🎉

Sadly I found some new bugs that need to be fixed.

  1. Empty Columns
    If I add a new entry to the relation it seems that some of the columns are empty ... maybe you could find a way to load the column data when adding the object to the relation.

image

  1. Missing scrollbar
    I also tested the option to add an image field to the grid.
    There we should add a scrollbar to the relation field because it only shows some (not all) entries.

image

I used the Category object from the demo and added the following fields:

  • fullpath
  • key
  • published
  • modificationDate
  • filename
  • name
  • location
  • (genericImages)

@BlackbitDevs
Copy link
Contributor Author

@Corepex Fixed loading the visible field data after adding via drag & drop or search window.

The height issue was caused by

if (nicePathRequested) {
window.setTimeout(function () {
this.component.getView().refresh();
}.bind(this), 500);
}

This was introduced in #5378 (after I suggested a similar solution in #4337)
For fields which do not use optimizedAdminLoading, the field data already gets fetched by loadObjectData, for example in
toBeRequested.add(this.loadObjectData(initData, this.visibleFields));

For those fields fieldsstays empty in
var fields = [];
var context = this.getContext();
var loadEditModeData = false;
if(isInitialLoad && this.fieldConfig.optimizedAdminLoading && context['containerType'] == 'object') {
loadEditModeData = true;
if(this.visibleFields) {
fields = fields.concat(this.visibleFields);
}
if(this.fieldConfig.columnKeys) {
fields = fields.concat(this.fieldConfig.columnKeys);
}
}

and thus
var nicePathRequested = pimcore.helpers.requestNicePathData(
returns nicePathRequested = undefined
and thus we did not get to
if (nicePathRequested) {
window.setTimeout(function () {
this.component.getView().refresh();
}.bind(this), 500);
}
},

And for this reason the height was not adjusted. The same problem existed for normal many-to-many object relations with a visible image field: After adding something via drag & drop, the image is cut off.

Now it should work.

@robertSt7 robertSt7 self-assigned this Sep 20, 2023
@robertSt7
Copy link
Contributor

@BlackbitDevs Could you please resolve the conflicts? Thanks

@BlackbitDevs BlackbitDevs force-pushed the feature/reverse-relation-visible-fields branch from f1100c7 to 939edb7 Compare September 25, 2023 09:51
@BlackbitDevs
Copy link
Contributor Author

@robertSt7 As the JS files are now part of https://github.com/pimcore/admin-ui-classic-bundle I have splitted this PR into 2. This one contains the core changes and pimcore/admin-ui-classic-bundle#290 contains the UI changes.

Co-authored-by: Divesh Pahuja <divesh.pahuja@pimcore.com>
@sonarcloud
Copy link

sonarcloud bot commented Oct 4, 2023

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 47 Code Smells

No Coverage information No Coverage information
2.1% 2.1% Duplication

@dvesh3
Copy link
Contributor

dvesh3 commented Oct 9, 2023

@robertSt7 ready for merge?

@robertSt7 robertSt7 merged commit 906ab3d into pimcore:11.x Oct 10, 2023
14 checks passed
@robertSt7
Copy link
Contributor

@BlackbitDevs Thanks for the PR

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

10 participants