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

feat: app override initial support #122

Merged
merged 5 commits into from
Aug 28, 2020
Merged

Conversation

amcgee
Copy link
Member

@amcgee amcgee commented Aug 26, 2020

Requires dhis2/dhis2-core#5977

This adds a noticebox if an app is found on the App Hub with:

  1. The name App Management
  2. The developer organization DHIS2 (should probably be UiO or maybe an org just for UiO core apps?)
  3. A version higher than the version of the current app

This is the noticebox in action. Clicking the link will install the app from the App Hub and then reload the page. @cooper-joe any feedback on messaging? The nice thing here is that we can update this after the 2.35 release, using this feature!!

Screen Shot 2020-08-26 at 6 40 46 PM

This also adds a CORE APP indicator to any installed app which is overriding a bundled application (see screenshot)

@amcgee amcgee force-pushed the feat/app-override-initial-support branch from fdcba6c to a81129d Compare August 26, 2020 16:51
@amcgee amcgee marked this pull request as draft August 26, 2020 16:55
Copy link
Member

@Birkbjo Birkbjo left a comment

Choose a reason for hiding this comment

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

LGTM, just some minor comments.

src/.babelrc.js Show resolved Hide resolved
@cooper-joe
Copy link
Member

Looking good 👍 I think we could adjust the messaging to make it clear what will happen:

Clicking the link will install the app from the App Hub and then reload the page.

I think the button should read "Update and reload app", just to make it completely clear that clicking that button will refresh the page and potentially remove any inputs/changes the user has not committed. Perhaps we'd catch this with a dialog anyway? But either way, I think it's good to put it up front in the button.

@Philip-Larsen-Donnelly
Copy link
Contributor

Very nice @amcgee !

Regarding point 2 in your note above, I agree that we should have a clear way of distinguishing core apps on the App Hub. Having a specific organisation could work, though it feels a bit hacky. I feel like it would be better to have a specific attribute - but, of course, the dhis2 organisation, or admin, should be the only one that can set that attribute.

I like the CORE APP indicator, but I wonder if we should be more explicit that it is a "CORE APP OVERRIDE"? However, I don't like the latter very much. Maybe we eventually need to add the info about which version is overridden, like:
CORE APP v31.0.0 (overrides bundled version v29.5.0)
But I think as it is is fine for the release

@amcgee
Copy link
Member Author

amcgee commented Aug 27, 2020

Having a specific organisation could work, though it feels a bit hacky.

Maybe, but it does do exactly what we want - only developers we explicitly grant access can push to an organization we own. That's basically the "specific attribute" but using a system we already have in place. Is there a reason to require a separate attribute?

I like the CORE APP indicator, but I wonder if we should be more explicit that it is a "CORE APP OVERRIDE"? However, I don't like the latter very much. Maybe we eventually need to add the info about which version is overridden, like:
CORE APP v31.0.0 (overrides bundled version v29.5.0)
But I think as it is is fine for the release

I agree the UI could be improved, but I don't think we actually want to specify that these are "overrides" - they are just core apps which have been upgraded. Eventually we should have a "Core Apps" section of this app which includes all bundled and overridden apps, has buttons to update each one if an update exists, and allows "rollback" to the bundled version in exceptional circumstances. But I think that's out of scope of this change - we can push that update to App Management once we have this self-update mechanism in place.

@Birkbjo
Copy link
Member

Birkbjo commented Aug 27, 2020

Having a specific organisation could work, though it feels a bit hacky.

Maybe, but it does do exactly what we want - only developers we explicitly grant access can push to an organization we own. That's basically the "specific attribute" but using a system we already have in place. Is there a reason to require a separate attribute?

I've also thought about this, and I think I agree on having something like DHIS2 Core as an organisation for these apps. In addition to the authority-system you mention, it would also mean we could use existing filtering on both the client and server without introducing a new concept/attribute and functionality for that.

@amcgee amcgee marked this pull request as ready for review August 28, 2020 10:22
@amcgee
Copy link
Member Author

amcgee commented Aug 28, 2020

@cooper-joe updated the wording of the button

Screen Shot 2020-08-28 at 12 44 33 PM

There's a live demo here http://d2.winged.tech:8080/dhis-web-app-management/index.html

Note that the version is artificially set to always show the banner, but when the version of the current app is <= the highest compatible version on the App Hub it will disappear

Copy link
Contributor

@Philip-Larsen-Donnelly Philip-Larsen-Donnelly left a comment

Choose a reason for hiding this comment

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

OK. Agreed w.r.t. having core organisation, and also that CORE APPS are not always "overrides".

Copy link
Member

@cooper-joe cooper-joe left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@amcgee amcgee merged commit 2868355 into master Aug 28, 2020
dhis2-bot added a commit that referenced this pull request Nov 30, 2020
# [28.2.0](v28.1.4...v28.2.0) (2020-11-30)

### Bug Fixes

* **translations:** sync translations from transifex (master) ([9f02cbd](9f02cbd))
* **translations:** sync translations from transifex (master) ([4e66e91](4e66e91))
* **translations:** sync translations from transifex (master) ([70f17c1](70f17c1))
* **translations:** sync translations from transifex (master) ([bc54a78](bc54a78))
* **translations:** sync translations from transifex (master) ([61ea206](61ea206))
* **translations:** sync translations from transifex (master) ([6d665a1](6d665a1))
* **translations:** sync translations from transifex (master) ([1435171](1435171))
* remove deploy-build dep ([83919ff](83919ff))
* update travis.yaml ([60fdc4b](60fdc4b))
* use core AppHub proxy instead of apps.dhis2.org ([#137](#137)) ([#138](#138)) ([3eee6c6](3eee6c6))
* **translations:** sync translations from transifex (master) ([ee159fa](ee159fa))
* **translations:** sync translations from transifex (master) ([7e5574b](7e5574b))
* **translations:** sync translations from transifex (master) ([6e6aee1](6e6aee1))
* add favicon path to avoid 404 error ([8da3f98](8da3f98))
* remove trailing slash on baseUrl before /api ([9065fd5](9065fd5))
* use unversioned endpoint for backwards compat ([358a92a](358a92a))
* **DHIS2-4788:** replaced folderName with key to fix icon URL. ([#38](#38)) ([3ecfa8a](3ecfa8a))

### Features

* support self-updates and label core apps in list ([#122](#122)) ([2868355](2868355))
* **app-hub:** use the app-hub directly ([#88](#88)) ([18d2154](18d2154))
* load apps directly from app store backend ([74192b5](74192b5))
@dhis2-bot
Copy link
Contributor

🎉 This PR is included in version 28.2.0 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

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

Successfully merging this pull request may close these issues.

5 participants