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

167 - Upload of branding and upload of partners #340

Merged
merged 14 commits into from
Sep 14, 2020

Conversation

latin-panda
Copy link
Contributor

Description

This PR will add two new commands:

  1. upload-branding will update the medic DB's branding document with: Title, resources (logo and favicon) and attachments.
  2. upload-partners will update the medic DB's partners document with: resources (parner name and partner image) and attachments.

Additionally will add support to include as attachment the files with extension: ".icon"

#167

Code review items

  • Readable: Concise, well named, follows the style guide, documented if necessary.
  • Documented: Configuration and user documentation on medic-docs
  • Tested: Unit and/or e2e where appropriate
  • Internationalised: All user facing text
  • Backwards compatible: Works with existing data and configuration or includes a migration. Any breaking changes documented in Changes.md.

License

The software is provided under AGPL-3.0. Contributions to this project are accepted under the same license.

1. upload-branding will update the medic DB's branding document with: Title, resources (logo and favicon) and attachments.
2. upload-partners will update the medic DB's partners document with: resources (parner name and partner image) and attachments.
Additionally will add support to include as attachment the files with extension: ".icon"
@latin-panda
Copy link
Contributor Author

Hi @garethbowen, since branding and partners are separate documents in the DB, I thought on making two separate commands as well.
upload-branding
upload-partners

@MaxDiz
Copy link

MaxDiz commented Jul 9, 2020

If you can also update this documentation upon completion of this ticket, it would be appreciated!

Copy link
Member

@garethbowen garethbowen left a comment

Choose a reason for hiding this comment

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

Awesome!

The main change is a refactor to reduce code duplication - reach out in Slack if you want to talk through the options here.

We should also add both of these to the list of defaultActions in /src/lib/main.js so they run by default.

src/fn/upload-partners.js Show resolved Hide resolved
src/lib/attachment-from-file.js Outdated Show resolved Hide resolved
test/fn/upload-partners.spec.js Show resolved Hide resolved
…upload-partners.js and upload-resources.js to a reusable library called upload-configuration-docs.js. Additionally adds the unit test from the mentioned files.
.eslintrc Outdated Show resolved Hide resolved
Copy link
Member

@garethbowen garethbowen left a comment

Choose a reason for hiding this comment

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

Awesome work! I love the reuse.

One comment about the linting configuration and then it'll be good to go!

.eslintrc Outdated Show resolved Hide resolved
src/fn/upload-partners.js Show resolved Hide resolved
src/lib/main.js Show resolved Hide resolved
Copy link
Member

@garethbowen garethbowen left a comment

Choose a reason for hiding this comment

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

Great work!

@garethbowen
Copy link
Member

Actually, I just noticed reviewing the documentation that you changed the format of the resouces.json file. I don't think we can do this because it would be a breaking change for everyone who's using it. Is this correct? Can you find a way to do it without breaking backwards compatibility?

@latin-panda
Copy link
Contributor Author

Hi @garethbowen I could add any of the following options, but it would add some ugly noise in the code that I'd like to avoid if it's possible to announce the breaking change (maybe in conjunction with a node upgrade 🙂).

Option 1:

if command === upload_resources then 
      add json content inside doc.resources
else 
      extend doc with json content (my solution)

Option 2:
A dynamic option is to pass as parameter a "mapping object", so to instruct our generic function where to take the data, this will restrict what we are copying from the json (advantage or disadvantage 🤔) for example:
Branding mapping

{
  title: 'title', // equivalent json.title
  icons: 'images.icon', // json.images.icon
}

Resources mapping

{
  otherProp: 'otherProp' // equivalent json.otherProp
  resources: '' // equivalent assigning the whole json
}

@garethbowen
Copy link
Member

I'm strongly against breaking backwards compatibility. Even if we announce it there's a chance someone will miss it, and even if they don't it's annoying for every app builder!

Option 3 would be for each implementation to pass in the doc JSON rather than the path to the JSON so each caller has to parse the file but they can pass a subset of the file if they like.

Option 4 would be to pass an optional mapping function to turn the JSON file into the expected CouchDB doc, eg:

return uploadConfigurationDocs(configurationPath, directoryPath, 'resources', file => file.resources);

By default no mapping would be applied.

I like Option 4 - what do you think?

@latin-panda
Copy link
Contributor Author

latin-panda commented Jul 15, 2020

Hi @garethbowen I proceeded with option 4, added unit test and also manually tested that resources is working as before.
Documentation is updated as well.

@garethbowen
Copy link
Member

Thanks! Looks good.

@latin-panda latin-panda merged commit 27bfcc7 into master Sep 14, 2020
@latin-panda latin-panda deleted the 167-upload-branding branch June 14, 2021 04:23
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.

Allow configuring of whitelabelling options
3 participants