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: add hidden properties #6484

Merged
merged 1 commit into from Oct 12, 2020
Merged

feat: add hidden properties #6484

merged 1 commit into from Oct 12, 2020

Conversation

mdbetancourt
Copy link
Contributor

@mdbetancourt mdbetancourt commented Oct 2, 2020

Implements: #6385
Related to: #1914

Checklist

  • DCO (Developer Certificate of Origin) signed in all commits
  • npm test passes on your machine
  • New tests added or existing tests modified to cover all changes
  • Code conforms with the style guide
  • Affected artifact templates in packages/cli were updated
  • API Documentation in code was updated
  • Documentation in /docs/site was updated
  • Affected example projects in examples/* were updated

@achrinza achrinza added community-contribution Repository Issues related to @loopback/repository package labels Oct 2, 2020
Copy link
Member

@bajtos bajtos left a comment

Choose a reason for hiding this comment

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

Thank you @mdbetancourt for the pull request! I have few comments on what to improve, see below.

I am little bit concerned that now we have to update every code dealing with hidden properties, are you sure you found all places? For example, hiddenProperties setting is passed to loopback-datasource-juggler, but properties with hidden: true are not. I am not able to tell if that could be a problem or not.

Have you considered a different approach, where properties with hidden: true are handled by model definition object and copied to hiddenProperties setting? That way we don't need to modify any code dealing with hidden properties, just the code building model definitions.

@raymondfeng @hacksparrow do you have any opinions on this?

packages/repository/src/__tests__/unit/model/model.unit.ts Outdated Show resolved Hide resolved
packages/repository/src/model.ts Show resolved Hide resolved
packages/repository/src/model.ts Outdated Show resolved Hide resolved
@bajtos bajtos linked an issue Oct 2, 2020 that may be closed by this pull request
2 tasks
@bajtos
Copy link
Member

bajtos commented Oct 2, 2020

Also please update the documentation in Model >> Hidden properties to explain both ways how to hide a property.

@hacksparrow
Copy link
Contributor

Have you considered a different approach, where properties with hidden: true are handled by model definition object and copied to hiddenProperties setting? That way we don't need to modify any code dealing with hidden properties, just the code building model definitions.

Yes, that's what I'd prefer too, { hidden: true } as a sugar for pushing the property to hiddenProperties. As such { hidden: true } should not be involved in any logic.

@mdbetancourt please consider this approach. It will be less complex and less disruptive.

@hacksparrow
Copy link
Contributor

With the approach suggested by @bajtos and some minor text changes, this should be good to land.

Copy link
Member

@bajtos bajtos left a comment

Choose a reason for hiding this comment

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

The new version looks much better 👍🏻

I have few more comments to address, PTAL.

docs/site/Model.md Outdated Show resolved Hide resolved
packages/repository/src/__tests__/unit/model/model.unit.ts Outdated Show resolved Hide resolved
packages/repository/src/model.ts Show resolved Hide resolved
packages/repository/src/model.ts Outdated Show resolved Hide resolved
packages/repository/src/model.ts Outdated Show resolved Hide resolved
@hacksparrow
Copy link
Contributor

With the previous assertion also added and UserHidden renamed to Customer, this looks ready to land for me.

@dhmlau
Copy link
Member

dhmlau commented Oct 7, 2020

@hacksparrow, assigning this PR to you. Please help it to land. Thanks!

@hacksparrow
Copy link
Contributor

hacksparrow commented Oct 8, 2020

@mdbetancourt the update looks good. However, we will have to add some more test case(s) to address the drop in test coverage. Writing tests which cover any newly introduced if-else conditions will fix it.

Eg: Add a test case to confirm the property is not hidden if it does not have hidden set to true. Basically an inverse of your existing test case.

One may wonder, "What's the point of doing this?" This will ensure, the latest changes does not accidentally hide any property - now or in future.

bajtos
bajtos approved these changes Oct 8, 2020
Copy link
Member

@bajtos bajtos left a comment

Choose a reason for hiding this comment

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

Code changes LGTM.

The drop in code coverage looks like a bug in coveralls infrastructure to me. If you look at the coverage report, it complains about missing coverage for an existing block which didn't have coverage before either.

I don't have a strong opinion on whether we need to add a new test to verify that properties with no hidden flag are not added to hiddenProperties array - I would expect this case is already covered by existing tests. Maybe not? I'll leave it up to @hacksparrow to decide.

@mdbetancourt can you please rebase your patch on top of the latest master, to see if it fixes the unexpected coverage drop? While doing that, please squash all commits into a single one and provide a descriptive error message. See our commit message guidelines and final rebase and squashing of commits if you are not familiar with the process.

@mdbetancourt
Copy link
Contributor Author

mdbetancourt commented Oct 9, 2020

I don't have a strong opinion on whether we need to add a new test to verify that properties with no hidden flag are not added to hiddenProperties array - I would expect this case is already covered by existing tests. Maybe not? I'll leave it up to @hacksparrow to decide.

yes is already covered by a lot of test (almost every test doesn't use that flag)

The drop in code coverage looks like a bug in coveralls infrastructure to me. If you look at the coverage report, it complains about missing coverage for an existing block which didn't have coverage before either.

i added a test to cover this (i did a rebase and only left the feature and this fix)

@@ -32,7 +32,7 @@ export const MODEL_WITH_PROPERTIES_KEY = MetadataAccessor.create<
ClassDecorator
>('loopback:model-and-properties');

export type PropertyMap = MetadataMap<PropertyDefinition>;
export type PropertyMap = MetadataMap<Partial<PropertyDefinition>>;
Copy link
Member

Choose a reason for hiding this comment

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

This change seems a bit unrelated to the test being added, I am also not sure if is a good idea to allow only partial property definitions in PropertyMap. Shouldn't we (the @model decorator?) guarantee that PropertyMap always contains valid (full) property definitions only?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

only "type" is required (but is inferred when is not provided)

https://github.com/strongloop/loopback-next/blob/11267618b58d437cd9a3c28f4c2f4f48cfe930c0/packages/repository/src/decorators/model.decorator.ts#L87-L89

here designType does not provide a properly type (string | Function | undefined) instead provide (string | Function) because propDef.type is not nullable, when in fact it is (that's why we check and throw CANNOT_INFER_PROPERTY_TYPE error). i'm open to use another approach thought. like only change
https://github.com/strongloop/loopback-next/blob/11267618b58d437cd9a3c28f4c2f4f48cfe930c0/packages/repository/src/decorators/model.decorator.ts#L83

Copy link
Member

Choose a reason for hiding this comment

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

I though that PropertyMap is being used by ModelDefinition too. I did a quick search and see that this type is used only for decorator-related metadata, in which case this change is fine with me. (I was worrying about changing ModelDefinition contract, which is not the case.)

…decorator

Signed-off-by: Michel Betancourt <michelbetancourt23@gmail.com>

test(repository): add missing test when is not provided a type for fields

Signed-off-by: Michel Betancourt <michelbetancourt23@gmail.com>
bajtos
bajtos approved these changes Oct 12, 2020
Copy link
Member

@bajtos bajtos left a comment

Choose a reason for hiding this comment

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

LGTM 👍🏻

@hacksparrow Would you like to take another look? Can we merge this PR?

@bajtos bajtos self-assigned this Oct 12, 2020
@bajtos bajtos merged commit 3160424 into loopbackio:master Oct 12, 2020
2 checks passed
@bajtos
Copy link
Member

bajtos commented Oct 12, 2020

Landed, thank you @mdbetancourt for the contribution! ❤️

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Repository Issues related to @loopback/repository package
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Model: hidden property shortcut
5 participants