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

refactor(project landing page): use metadata endpoint to get data from backend (DSP-1199) #400

Merged
merged 16 commits into from Mar 2, 2021

Conversation

@waychal
Copy link
Contributor

@waychal waychal commented Feb 26, 2021

resolves DSP-1199

In this PR:

  • remove hardcoded metadata
  • remove interfaces written for project metadata
  • use data types (ProjectMetadata. Dataset, Person, Organisation, etc.) directly from js-lib
  • use Ajax request to get metadata from backend
@waychal waychal requested a review from mpro7 Feb 26, 2021
@waychal waychal self-assigned this Feb 26, 2021

// return the type of agent to use correct template to display it
getAgentType(agent: IPerson | IOrganisation) {
if (agent.hasOwnProperty('familyName')) {
setAgent (agent: Person | Organization | IId) {
Copy link
Contributor

@mpro7 mpro7 Mar 1, 2021

Choose a reason for hiding this comment

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

Method return type is missing.

Loading

let atype = this.getAgentType(agent);
if (atype) {
this.currentAgent = agent;
return atype;
}
this.currentAgent = this.subProperties[agent.id];
atype = this.getAgentType(this.currentAgent);
return atype;
Copy link
Contributor

@mpro7 mpro7 Mar 1, 2021

Choose a reason for hiding this comment

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

why not if...else with one return outside?

Loading

Copy link
Contributor Author

@waychal waychal Mar 1, 2021

Choose a reason for hiding this comment

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

When I used if..else before, linter was complaining about else condition and my tests were failing because of that. So I updated the code like this.

Loading

Copy link
Contributor

@mpro7 mpro7 Mar 2, 2021

Choose a reason for hiding this comment

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

Same here, this answer.

Loading

getAgentType (agent: Person | Organization | IId) {
if (agent instanceof Person) {
return 'person';
}
if (agent.hasOwnProperty('url')) {
if (agent instanceof Organization) {
return 'organisation';
}
return undefined;
}
Copy link
Contributor

@mpro7 mpro7 Mar 1, 2021

Choose a reason for hiding this comment

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

method return type is missing and if...else

Loading

Copy link
Contributor Author

@waychal waychal Mar 1, 2021

Choose a reason for hiding this comment

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

linter was complaining same for else block here so I removed it.

Loading

Copy link
Contributor

@mpro7 mpro7 Mar 2, 2021

Choose a reason for hiding this comment

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

I've applied below and on vscode, linter doesn't complain. Could you show me (screen) how it looks on your side?

    setAgent (agent: Person | Organization | IId): string {
        let atype = this._datasetMetadataService.getContactType(agent);
        if (atype) {
            this.currentAgent = agent;
        } else {
            this.currentAgent = this.subProperties[agent.id];
            atype = this._datasetMetadataService.getContactType(this.currentAgent);
        }
        return atype;
    }

Loading

Copy link
Contributor Author

@waychal waychal Mar 2, 2021

Choose a reason for hiding this comment

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

Here it is: https://github.com/dasch-swiss/dsp-app/runs/1987482010?check_suite_focus=true.
It shows misplaced else error in multiple places.

Loading

Copy link
Contributor

@mpro7 mpro7 Mar 2, 2021

Choose a reason for hiding this comment

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

It's like that because of wrong formatting (see your code in screen below). There shouldn't be ENTER between if and else blocks. See my example above (this code is from another comment - just miss clicked answering right comment, but the overall rule applies here too). Please adjust.

image

Loading

Copy link
Contributor Author

@waychal waychal Mar 2, 2021

Choose a reason for hiding this comment

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

Thanks for pointing it out. Changes are done in d8b4993.

Loading

<!-- Functionality to be implemented -->
<a href *ngFor="let dformat of metadataDownloadFormats" class="download-metadata"> {{ dformat }} </a>
Copy link
Contributor

@mpro7 mpro7 Mar 1, 2021

Choose a reason for hiding this comment

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

indention

Loading

[cdkCopyToClipboard]="selectedProject.id"
(click)="copyToClipboard('Persistent identifier')"
class="btn-copy-to-clipboard">
<mat-icon [inline]="true" class="icon-arkurl">content_copy</mat-icon>
Copy link
Contributor

@mpro7 mpro7 Mar 1, 2021

Choose a reason for hiding this comment

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

indention

Loading

<div class="property-label display-inline-block">
Email:
</div>
Copy link
Contributor

@mpro7 mpro7 Mar 1, 2021

Choose a reason for hiding this comment

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

Indention or you can make it one-liner.

Loading


ngOnInit() {
// check if members list is the list of [Organization] or [Iid]
let isOrganizationType: boolean = false;
Copy link
Contributor

@mpro7 mpro7 Mar 1, 2021

Choose a reason for hiding this comment

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

If property is initiated with value, type annotation is redundant. It's not wrong, but not necessary for primitive types.

Loading

SingleProject,
DataManagementPlan,
Person,
Organization,
IId,
Grant
Copy link
Contributor

@mpro7 mpro7 Mar 1, 2021

Choose a reason for hiding this comment

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

alphabetise imports

Loading


}

getDMP(currenDmps: DataManagementPlan | IId[]) {
Copy link
Contributor

@mpro7 mpro7 Mar 1, 2021

Choose a reason for hiding this comment

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

return type is missing

Loading

getFunderType(funder: Person | Organization | IId) {
if (funder instanceof Person) {
return 'person';
}
if (funder instanceof Organization) {
return 'organization';
}
return undefined;
}
Copy link
Contributor

@mpro7 mpro7 Mar 1, 2021

Choose a reason for hiding this comment

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

like few comments above, this can be reused from service

Loading

@waychal waychal force-pushed the wip/dsp-1199-use-metadata-endpoint-to-get-data branch from b688416 to 6bb29d8 Mar 1, 2021
@waychal
Copy link
Contributor Author

@waychal waychal commented Mar 2, 2021

Requested changes are done in 6bb29d8.

Loading

@waychal waychal requested a review from mpro7 Mar 2, 2021
Copy link
Contributor

@mpro7 mpro7 left a comment

Where possible, bring back correct form of if...else or if...else if blocks.

Loading

getContactType(obj: Person | Organization | IId): string {
if (obj instanceof Person) {
return 'person';
}
if (obj instanceof Organization) {
return 'organization';
}
return undefined;
}
Copy link
Contributor

@mpro7 mpro7 Mar 2, 2021

Choose a reason for hiding this comment

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

Use if..else if block here, have a look at this answer.

Loading

Copy link
Contributor Author

@waychal waychal Mar 2, 2021

Choose a reason for hiding this comment

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

Thanks. Changes are done in d8b4993.

Loading

@Injectable({
providedIn: 'root'
})
export class DatasetMetadataService {
Copy link
Contributor

@mpro7 mpro7 Mar 2, 2021

Choose a reason for hiding this comment

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

I would just name it MetadataService, or MetadataUtilService where all helpers method could be stored.

Also this repository has using 4 spaces indentions.

Loading

Copy link
Contributor Author

@waychal waychal Mar 2, 2021

Choose a reason for hiding this comment

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

done in d8b4993.

Loading

Snehal Kumbhar added 16 commits Mar 2, 2021
…ntact page for new metadata schema

Note: need to work on UI for person and organisation templates (mainly used in funder, grant,
attribution and contact sections)
@waychal waychal force-pushed the wip/dsp-1199-use-metadata-endpoint-to-get-data branch from 6bb29d8 to d8b4993 Mar 2, 2021
@waychal waychal requested a review from mpro7 Mar 2, 2021
Copy link
Contributor

@mpro7 mpro7 left a comment

;)

Loading

}
return undefined;
Copy link
Contributor

@mpro7 mpro7 Mar 2, 2021

Choose a reason for hiding this comment

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

ending else is missing here

Loading

Copy link
Contributor Author

@waychal waychal Mar 2, 2021

Choose a reason for hiding this comment

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

From my understanding if the case is default (and there is no further code to run in the function, we can add it without using else block. isn't it right?

Loading

Copy link
Contributor

@mpro7 mpro7 Mar 2, 2021

Choose a reason for hiding this comment

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

Correct.

Loading

mpro7
mpro7 approved these changes Mar 2, 2021
}
return undefined;
Copy link
Contributor

@mpro7 mpro7 Mar 2, 2021

Choose a reason for hiding this comment

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

Correct.

Loading

@waychal waychal merged commit 5dde42f into main Mar 2, 2021
6 checks passed
Loading
@waychal waychal deleted the wip/dsp-1199-use-metadata-endpoint-to-get-data branch Mar 2, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants