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

[Proof Of Concept] Enable plugin mode #335

Closed
wants to merge 2 commits into from

Conversation

adrianq
Copy link

@adrianq adrianq commented Jun 13, 2024

@amcgee @KaiVandivier @Birkbjo This is just a proof of concept of what we have been discussing these days. It is very rough - we are still keeping the header, button, etc. which ideally we would like to hide with some props (this can be done in our fork as I understand it may not be interesting for you). For us, it would be great if we could have this infrastructure in capture-app, dashboard and potentially some other apps. This is how it looks like in our app. I am happy to show you the example in the DHIS2 instance if we have time later. Thank you!
image (62)

@tokland thanks for this!

@adrianq adrianq requested review from a team as code owners June 13, 2024 08:05
@KaiVandivier
Copy link

Hey Adrian — thanks for the proposal! It definitely did start a conversation 😊

The main thing we can say is that we’re moving toward a general app architecture that should end up achieving what you want here: we’re moving toward a “global shell” way of loading apps in an iframe, which will entail building each app in plugable way (i.e. without the header bar and with a communication channel from its parent), which seems like it would satisfy the intention of this PR 👍 so I think that will be useful to the use case you have here!

In the mean time, as our tech is now, we don’t think it’s the right thing to build an entire app as a plugin, but I have some advice below on making it easy to implement in your fork if you like!

These are our main reasons for holding off on this plugin build in the core repository:

  • If we made this change, we would end up undoing it soon when we move to the global shell way of “pluginifying” apps
  • It would be a little weird to have this available for some apps, but not all of them (we would also want a standardized “props interface” to pass from parent to child if we were to do that, which again would change with the global shell)
  • Plugins as they are now have a bit of a special meaning and usage in the platform, and having a whole app as a plugin may confuse consumers of Capture and Dashboard plugins
  • Plugin building is unoptimized right now, and building a whole app as a plugin would end up making basically two apps in the bundle, which is a bit clunky (this will get better with Vite though)

If you want make a plugin build like this in your own fork, however, it can actually be just a small change, so hopefully it’s easy to maintain 🙂 Here are some tips:

  1. The only change you really need to make is in d2.config.js, which is to add a plugin entry point that is just the same as the app’s:
    1. entrypoints: { app: './src/App.js', plugin: './src/App.js' }
    2. All the other changes in this PR seem to be copied over from a Dashboard plugin, which needs to handle some complicated offline features, so fortunately the same logic wouldn’t be needed for the Data Approvals app
  2. In the app that will be consuming the plugin, you can use the Plugin component from the App Runtime, which adds some nice utilities: https://developers.dhis2.org/docs/app-runtime/components/Plugin
    1. The examples linked at the bottom of the page are a useful guide for how they’re used
    2. The Dashboard plugins like the one this PR got copied were made before this component was released; they’re being migrated to using this component now which makes them simpler
    3. (If you don’t want to use the Plugin component, you can use the skipPluginLogic: true config option in the d2.config.js of the app with a plugin entrypoint to skip some of the communication & prop handling that’s added to the plugin)
  3. And another tip: If you’re using the Data Approval app in an iframe, you can take advantage of the deep linking feature if you want to load the app with a particular context selection already selected by providing a URL to the iframe with the right parameters: https://play.im.dhis2.org/dev/dhis-web-approval/index.html#/?ou=%2FImspTQPwCqd%2FO6uvpzGd5pu%2FYuQRtpLP10I%2FDiszpKrYNg8&ouDisplayName=Ngelehun%20CHC&pe=202412&wf=rIUL3hYOjJc

Hopefully these things help you out and make it easy to do what you want to do! 🙏

Thanks again for your input here! We value hearing your use case, and it’s helpful to get new ideas and perspectives. Let us know if you have any other questions or ideas!

@adrianq
Copy link
Author

adrianq commented Jul 10, 2024

Hi @KaiVandivier Thanks for your thoughtful response! It helps a lot. We have been testing some of your ideas and we have some comments:

  • It is true the build size increases a bit: app (2050K) -> app+plugin (2470K)
  • Thanks for this tip: entrypoints: { app: './src/App.js', plugin: './src/App.js' } We tested and it worked!
  • The Plugin component from the App Runtime works nicely and seems to be useful. The only thing is that we had to update the @dhis2/cli-app-scripts dependency to 10.4.0 so that the parent and approval app dependencies are aligned.
  • Deep linking: it worked. In the URL, I guess you meant plugin.html instead of index.html. In this case, we are missing only two parameters: attributeOptionCombo=ID and hideSelectors=true. We understand the hideSelectors parameter is something for our fork but is it reasonable to add the attributeOptionCombo in this app?
    Once again, many thanks for your help! We really appreciate having your input on this.

@KaiVandivier
Copy link

Hey Adrian! Glad to hear the plugin is working for you! 🎉

Ah yeah, I forgot to mention the cli-app-scripts version to support the plugin logic — sorry about that, and glad you got it working 🙂

For the deep linking, that’s right that you would use plugin.html for the iframe src 👍 for attributeOptionCombo, is that for the Data Approval app? I’m not very familiar with the app myself, but it doesn’t seem to use an attribute option combo anywhere — let me know if there’s something else that you mean by that, and I can inquire further 🔍

For the Data Entry app, the attributeOptionCombo can be deep-linked using the query param format attributeOptionComboSelection=<categoryOneId>-<categoryOptionOneId>_<categoryTwoId>-<categoryOptionTwoId>…, e.g. this one: https://play.im.dhis2.org/dev/dhis-web-aggregate-data-entry/index.html#/?attributeOptionComboSelection=LFsZ8v5v7rq-C6nZpLKjEJr_yY2bQYqNt0o-i4Nbp8S2G6A&dataSetId=lyLU2wR22tC&orgUnitId=DiszpKrYNg8&periodId=202403 — maybe that can be helpful; I think I remember you mentioning you use that one as a plugin as well?

Happy to help out with anything else if I can 🙂

@tokland
Copy link

tokland commented Oct 14, 2024

Hi @KaiVandivier !

Sorry for the delay, we finally got the opportunity to integrate Data Approval as an iframe in one of our apps ( using query strings, not props) and is working great. We just needed the minor modifications we discussed. Thanks for all your help!

for attributeOptionCombo, is that for the Data Approval app? I’m not very familiar with the app myself, but it doesn’t seem to use an attribute option combo anywhere — let me know if there’s something else that you mean by that, and I can inquire further 🔍

As you know, aggregated data values are associated to some attributeOptionCombo (one of the COCs of the dataSet categoryCombo), so typically we will need to disaggregate on these dimension to approve the data. In 2.36, the old Data Approval App showed a selector:

data-approva36-aoc-selection

However, that capability does not exist in the new React Data Approval App (or we have missed it!)

@adrianq

@KaiVandivier
Copy link

KaiVandivier commented Oct 14, 2024

Ahh - am I understanding right that you're looking for that feature in the new Approvals app, where you can disaggregate data by the AoC too? (Sorry if I misunderstood earlier!)

If so, I can do a little research to find out why that isn't included in the new app, and if it's possible to add! 🙂

@KaiVandivier
Copy link

@tokland Aha, this actually known to be a missing feature, and there is interest in adding it (here is one issue about supporting it in the backend, for example) -- I'll chime in to the discussion with your use case, which will help prioritize it 🙂

I'll try to share updates!

@tokland
Copy link

tokland commented Oct 15, 2024

you're looking for that feature in the new Approvals app

Yes, that would be great! For the moment, we've added the filter param in our fork (https://github.com/EyeSeeTea/approval-app/pull/3/files#diff-5f723d69ee9cc69ee25c7a33059f7a22668a657886975d1b82976bcf57b63fe9R22), so now passing filter: "ao:COCID" shows only the data we are interested in.

It was our understanding that the backend already supports approving by the tuple orgUnit/dataSet/aoc/period:

https://docs.dhis2.org/en/develop/using-the-api/dhis-core-version-241/data-approval.html#webapi_data_approval_approve_data

https://dhis2.atlassian.net/browse/DHIS2-11518)

Cool, we didn't know about this one, we'll be watching it! (@adrianq)

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

Successfully merging this pull request may close these issues.

3 participants