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

fix: guard against missing data elements by using lodash.get #19

Conversation

fluffynuts
Copy link
Contributor

@fluffynuts fluffynuts commented Oct 23, 2018

Motivation

Which issue does this fix? Fixes #18 (perhaps only partially -- I used lodash, which was already referenced by the project; if this approach is acceptable, it could be applied elsewhere)

Checklist

  • Code follows coding conventions held in this repo
  • Automated tests have been added (I don't see any existing testing?)
  • Tests are passing
  • Docu has been updated (if applicable)
  • Temporary settings (e.g. project ID used during development and testing) have been reverted to defaults

How to test

Should solve the problem with partial data causing dereference errors in Articles and Article components

@petrsvihlik
Copy link
Contributor

thanks for the PR, @fluffynuts ! we're on it :)

Copy link
Contributor

@Simply007 Simply007 left a comment

Choose a reason for hiding this comment

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

I am on it

@Simply007
Copy link
Contributor

Simply007 commented Nov 2, 2018

Hello @fluffynuts,

when I run the application and open articles. I got an error:

TypeError: Cannot read property 'formatDate' of undefined
    at mapArticle (MapArticle.js?4863:7)
    ...

That is caused by the extraction of mapArticle to the separate module and this is not a component you are calling it from.

  • Personally, I would prefer to have all the utility functions to be the pure functions and to not depend on context (this context in that case).

Ideal case

Have a universal solver module that would resolve the path to an object or return the localized string. Lodash offers that functionality with combination with VueI18n from vue-i18n which is already used in application

// For resolution of i.e. title.value of the article in Articles component
_.get(article, 'title.value', $t('Articles.notitlevalue'))

And have the record in src/Localization/en-US.json and src/Localization/es-ES.json in format:

// Format
{
{
    "locale": "en-US",
     ...
    "Articles": { // component name
        "notitlevalue": "(Article has no title)"
    }
   ...

I am OK to use this on article + articles as a showcase and I would raise the issue to implement is for the rest of the components.

If anything is unclear, feel free to ask @fluffynuts!

Copy link
Contributor

@Simply007 Simply007 left a comment

Choose a reason for hiding this comment

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

see comment above

     - require dateFormat and i18n functions from caller
fix: add translations for fallback values
@fluffynuts
Copy link
Contributor Author

I hope this is more in line with what you'd prefer?

  1. apologies about the rogue this. I don't have a Kentico site to test against -- so this is a good case for code review (:
  2. apologies for missing i18n -- it's so important to use it throughout when an app is supposed to have translations

@Simply007
Copy link
Contributor

Hello @fluffynuts,

for the testing purposes it is possible to generate your own project and then modify Client.js to use the preview API.

Basically what you would do is:

  1. run npm install
  2. run npm run serve
  3. App should show the configuration page (if not you have already project ID set in cookies - just delete prijectId from cookies and refresh or go to /Administration/Configuration page)
    • Hit Get Project ID from Kentico Cloud
    • Hit Create a sample project
  4. After the new project is generated app should display its content
  5. Edit Client.js and place projectId and preview API key as described [here] https://github.com/Kentico/cloud-sample-app-vue#connecting-to-your-project-manually

Please see the inline comments in code.

src/components/Articles.vue Outdated Show resolved Hide resolved
@Simply007
Copy link
Contributor

Hey @fluffynuts,

we would like to thank you for contributing to Kentico's open-source project. As a sign of our appreciation, we would like to send you some Kentico Developer Community SWAG!

Please fill out your postal address if you are interested.

This comment was automatically generated. If you've submitted more than one pull request, it's OK to fill out the form just once.

Kentico Developer Community

If there’s anything we can do to help, please don’t hesitate to reach out to us at developerscommunity@kentico.com

@fluffynuts
Copy link
Contributor Author

@Simply007 hey, thanks! I appreciate it & have filled out the form (:

@Simply007
Copy link
Contributor

Hello @fluffynuts,

I have gone through the changes you have made and I have tried to show you, how to implement guarding for all the occurrences of article data (home page, articles and article detail).

I have created a pull request to your branch:

Go through the changes I have made and then please polish the code:

  • remove hardcoded projectId and preview API key from Client.js

I have also provided a vue.cli.config for easies debugging:

Hope that would be usefull!

After you approve the pull request to your branch and remove hardcoded API keys I am more than happy to merge this pull request!

@fluffynuts
Copy link
Contributor Author

@Simply007 I've merged in your branch -- I remember thinking that I needed to remember not to include client & project id's -- and then I did anyway facepalm. But that makes me think -- wouldn't it make sense to perhaps get those bits of information from another file which is mentioned in .gitignore? I would imagine that a company would really prefer not to make the mistake that I just did, letting their secrets out into the wild.
I tried reading from process.env, but that didn't work out-the-box -- not sure if there's a "proper" way to do that, since I didn't find a definitive answer from a quick web search. Thoughts?

Copy link
Contributor

@Simply007 Simply007 left a comment

Choose a reason for hiding this comment

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

Could you please fix one error in LatestArticles.js component - incl. suggestion?

Then everything is OK to merge the pull requests!

About the extraction - I have summarized an issue and proposed a solution using dotenv library.

src/components/LatestArticles.vue Show resolved Hide resolved
@Simply007
Copy link
Contributor

I have also created an issue #23 for API key extraction.

@Simply007
Copy link
Contributor

Simply007 commented Nov 23, 2018

I have decided to merge your pull request and then fix the latest articles in a separate one, to have the feature in place.

Thanks @fluffynuts for the contribution!

@Simply007 Simply007 merged commit 71f7fbc into kontent-ai:master Nov 23, 2018
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.

Handle missing values in the preview
3 participants