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

DSP-652 FIX: Boolean add button #182

Merged
merged 7 commits into from Sep 21, 2020
Expand Up @@ -36,10 +36,7 @@
</div>
<!-- Add button -->
<!-- temporary hack to hide add button for an XML value -->
<div *ngIf="prop.guiDef.propertyIndex !== 'http://0.0.0.0:3333/ontology/0001/anything/v2#hasRichtext' &&
addButtonIsVisible &&
prop.guiDef.cardinality !== 1 &&
propID !== prop.propDef.id">
<div *ngIf="addValueIsAllowed(prop) && prop.guiDef.propertyIndex !== 'http://0.0.0.0:3333/ontology/0001/anything/v2#hasRichtext'">
Copy link
Contributor

Choose a reason for hiding this comment

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

Why does the condition contain a property IRI from the anything test project?

Copy link
Contributor

Choose a reason for hiding this comment

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

ah I get it: but then this should be more generic than 'http://0.0.0.0:3333/ontology/0001/anything/v2#hasRichtext', you could use the gui ele

Copy link
Contributor

Choose a reason for hiding this comment

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

I think we should integrate the XML / CKeditor functionality asap, so this becomes unnecessary

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree, I'll make the small change to use the gui element

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed in 94ab6a8

<button class="create" (click)="showAddValueForm(prop)" title="Add a new value">
<mat-icon>add_box</mat-icon>
</button>
Expand Down
Expand Up @@ -217,12 +217,34 @@ describe('PropertyViewComponent', () => {
testHostFixture.detectChanges();
});

it('should show an add button under each property that has a value component and for which the cardinality is not 1', () => {
it('should show an add button under each property that has a value component and value and appropriate cardinality', () => {
const addButtons = propertyViewComponentDe.queryAll(By.css('button.create'));
expect(addButtons.length).toEqual(13);

});

it('should show an add button under a property with a cardinality of 1 and does not have a value', () => {

// show all properties so that we can access properties with no values
testHostComponent.showAllProps = true;
testHostFixture.detectChanges();

let addButtons = propertyViewComponentDe.queryAll(By.css('button.create'));

// current amount of buttons should equal 18 because the boolean property shouldn't have an add button if it has a value
expect(addButtons.length).toEqual(18);

// remove value from the boolean property
testHostComponent.propArray[9].values = [];

testHostFixture.detectChanges();

// now the boolean property should have an add button so the amount of add buttons on the page should increase by 1
addButtons = propertyViewComponentDe.queryAll(By.css('button.create'));
expect(addButtons.length).toEqual(19);

});

it('should show an add value component when the add button is clicked', () => {
const addButtonDebugElement = propertyViewComponentDe.query(By.css('button.create'));
const addButtonNativeElement = addButtonDebugElement.nativeElement;
Expand Down
Expand Up @@ -53,7 +53,6 @@ export class PropertyViewComponent implements OnInit, OnDestroy {
addButtonIsVisible: boolean; // used to toggle add value button
addValueFormIsVisible: boolean; // used to toggle add value form field
propID: string; // used in template to show only the add value form of the corresponding value
readOnlyProp: boolean; // used in template to not show an "add" button for properties we do not yet have a way to create/edit

valueOperationEventSubscription: Subscription;

Expand Down Expand Up @@ -86,10 +85,8 @@ export class PropertyViewComponent implements OnInit, OnDestroy {
* Called from the template when the user clicks on the add button
*/
showAddValueForm(prop: PropertyInfoValues) {

this.propID = prop.propDef.id;
this.addValueFormIsVisible = true;

}

/**
Expand All @@ -100,4 +97,24 @@ export class PropertyViewComponent implements OnInit, OnDestroy {
this.addButtonIsVisible = true;
this.propID = undefined;
}

/**
* Given a resource property, check if an add button should be displayed under the property values
*
* @param prop the resource property
*/
addValueIsAllowed(prop: PropertyInfoValues): boolean {
// check if cardinality allows for a value to be added
if (prop.guiDef.cardinality !== 1 ||
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@tobiasschweizer How can I get an instance of a ResourceClassDefinition to use for CardinalityUtil.createdValueForPropertyAllowed? I think that's what I'd want to use instead of this logic.

Copy link
Contributor

Choose a reason for hiding this comment

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

the definition should be contained in the ReadResource's entityInfo.classes

Copy link
Contributor Author

Choose a reason for hiding this comment

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

entityInfo.classes has a type of ResourceClassDefinitionWithPropertyDefinition which is an extension of ResourceClassDefinition but I'm getting an error when I try to pass it in.

Copy link
Contributor

Choose a reason for hiding this comment

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

there are some methods to get only certain defs

Copy link
Contributor

Choose a reason for hiding this comment

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

but I think these work only for property defs

Copy link
Contributor

Choose a reason for hiding this comment

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

What kind of error do you get? Can you use type assertions to make it work?

Otherwise we can look at it on Monday.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Screen Shot 2020-09-18 at 15 51 33

We can look at it on Monday; here's the error message. Type assertion doesn't seem to work either and it suggests to assert it as type unknown first, which also didn't help.

Copy link
Contributor

Choose a reason for hiding this comment

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

I will try this myself and get back to you later this morning.

Copy link
Contributor

Choose a reason for hiding this comment

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

@mdelez Could you try this CardinalityUtil.createValueForPropertyAllowed(prop.propDef.id, prop.values.length, this.parentResource.entityInfo.classes[this.parentResource.type]);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That was the missing part :) Thanks!!

prop.guiDef.cardinality === 1 && prop.values.length === 0) {

// check that the value component does not already have an add value form open
// and that the user has write permissions
if (this.propID !== prop.propDef.id && this.addButtonIsVisible) {
return true;
}
}

return false;
}
}