Skip to content

Conversation

jake-bassett
Copy link
Contributor

No description provided.

@codecov
Copy link

codecov bot commented Nov 6, 2020

Codecov Report

Merging #333 (b1e583d) into main (09f2234) will decrease coverage by 0.00%.
The diff coverage is 78.70%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #333      +/-   ##
==========================================
- Coverage   86.24%   86.23%   -0.01%     
==========================================
  Files         723      725       +2     
  Lines       14733    14807      +74     
  Branches     1881     1760     -121     
==========================================
+ Hits        12706    12769      +63     
- Misses       1995     2007      +12     
+ Partials       32       31       -1     
Impacted Files Coverage Δ
projects/components/src/modal/modal.ts 100.00% <ø> (ø)
...hared/graphql/model/metadata/attribute-metadata.ts 100.00% <ø> (+50.00%) ⬆️
.../handler/metadata-graphql-query-handler.service.ts 100.00% <ø> (ø)
...y/src/pages/explorer/explorer-dashboard-builder.ts 88.88% <ø> (ø)
...lore-query-editor/explore-visualization-builder.ts 97.77% <0.00%> (ø)
...able/columns/table-edit-columns-modal.component.ts 25.00% <25.00%> (ø)
...ble/header/table-header-cell-renderer.component.ts 73.52% <40.00%> (-5.79%) ⬇️
projects/components/src/table/table.component.ts 91.50% <86.95%> (-0.36%) ⬇️
...ared/dashboard/widgets/table/table-widget.model.ts 81.57% <88.88%> (+0.93%) ⬆️
...ets/table/services/table-widget-columns.service.ts 92.10% <92.10%> (ø)
... and 8 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 09f2234...b1e583d. Read the comment docs.

@@ -165,15 +183,102 @@ export class TableWidgetModel {
})
public pageable?: boolean = true;

@ModelProperty({
key: 'fetchEditableColumns',
displayName: 'Query for additional columns not provided',
Copy link
Contributor

Choose a reason for hiding this comment

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

In what cases do we not want this? I'd assume to be implicit in whether we provide a scope or not - it's easier to add an explicit property later than to remove one.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Edit columns comes with two modes. The first mode only uses the columns specified in the json. The second mode also fetches attributes and turns those into columns.

This property is to enable the second mode. The scope is hard coded in the data model and always present so we can't use that as a way to enable this mode.

Copy link
Contributor

@aaron-steinfeld aaron-steinfeld Nov 7, 2020

Choose a reason for hiding this comment

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

So the assumption I built into my statement may not hold - I assumed, if a data source was used that supported attributes (as determined by the presence of a scope), we'd always want this. If we want to selectively enable, then makes sense.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Correct. In fact testing the attribute fetching mode, I'm finding issues with some attributes that can't be fetched. Going to have to work on those case by case, but it looks like using the json defined mode will be the typical case in the near term while we iron out the wrinkles with the troublesome attributes.

@jake-bassett jake-bassett marked this pull request as ready for review November 9, 2020 22:40
@jake-bassett jake-bassett requested a review from a team as a code owner November 9, 2020 22:40
@jake-bassett jake-bassett merged commit 2ff0a17 into main Nov 9, 2020
@jake-bassett jake-bassett deleted the add-remove-columns branch November 9, 2020 22:40
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.

2 participants