search

systopia / de.systopia.resource

CiviCRM extension to specify generic resources
Other
5 stars 4 forks source link

Added resource demand linked entities #28

Closed CsarRamos closed 1 year ago

CsarRamos commented 1 year ago

Implemented logic in the resource demands class to add the referenced entities.

Tested with the event class.

Before:

In Searchkit it's not possible to join the event class to the resource demand class.

After:

Searchkit allows join event class to the resource demands class.

https://github.com/systopia/de.systopia.resource/issues/27

colemanw commented 1 year ago

Ok I've read this PR more carefully and I think I understand what you're trying to do. I recommend:

  1. Don't use the grouping column to store the table name, use value - that's how it's done elsewhere.
  2. You don't need a buildOptions function at all. Just add <pseudoconstant> tag pointing to your option group like this: https://github.com/civicrm/civicrm-core/blob/a6d0f90f8da1d21cd87e62f56d9bf4dc8f05d1ef/xml/schema/Core/EntityTag.xml#L26-L33
  3. After modifying xml you need to run civix generate:entity-boilerplate.
  4. Also, I think it would be better to package your new option group in a .mgd.php file instead of .json as that's a newer approach and we ought to be removing the .json stuff from this extension.
bjendres commented 1 year ago

Thanks for helping out, @colemanw.

@CsarRamos let us know if you get stuck.

CsarRamos commented 1 year ago

Thanks for helping out, @colemanw.

@CsarRamos let us know if you get stuck.

Great, thank you for the guidelines, I'll let you if there's any problem, thanks.

CsarRamos commented 1 year ago

Hello @bjendres I have added some comits that implement the functionality following @colemanw recommendations. I have tried to modify the resource class, but it seems that some decisions have to be made on how to implement it and I am quite unsure how to do it.

CsarRamos commented 1 year ago

Hello @bjendres I have added some comits that implement the functionality following @colemanw recommendations. I have tried to modify the resource class, but it seems that some decisions have to be made on how to implement it and I am quite unsure how to do it.

Apologies, I updated civix and did not detect that there were errors, I will check it carefully and update it as soon as possible, thanks.

CsarRamos commented 1 year ago

Hello @bjendres I have added some comits that implement the functionality following @colemanw recommendations. I have tried to modify the resource class, but it seems that some decisions have to be made on how to implement it and I am quite unsure how to do it.

Apologies, I updated civix and did not detect that there were errors, I will check it carefully and update it as soon as possible, thanks.

Hello, the problem was solved including the mixin in info and removing old function. Could you review if the aproach is correct or if there is a issue? Thank you.

bjendres commented 1 year ago

@jensschuppe, can we review this together some time soon?

colemanw commented 1 year ago

I've made 2 suggestions but otherwise this code looks very good.

CsarRamos commented 1 year ago

Hello, I added new change to allow to add the label field in the form build with afform. If it should be a new requirement, please let me know and I will create another issue/PR. Thanks!

bjendres commented 1 year ago

@jensschuppe Could you have a look at this?