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

[Sponsor by MEL] Add feature Metadata Quality Widget #497

Merged
merged 3 commits into from
Oct 25, 2023

Conversation

gkeimeHDF
Copy link
Contributor

@gkeimeHDF gkeimeHDF commented May 30, 2023

Copy link
Member

@fgravin fgravin left a comment

Choose a reason for hiding this comment

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

Thanks @gkeimeHDF for the contribution !

It looks overall quite good.

I've just done a very quick first review, more about the "forme" than the "fond" 😄
I will deeply test it and review it next week, from there, you can still start with

  • rebase your work on the last main
  • add tests to your UI components
  • add tests to the components that you've changed (to check if the widget is present or not for instance)
  • add stories to your UI components
  • address the first comments

Note that it's easier if you name your PR branch with a description of what it's about, rather than using the main tag.

In term of rendering, I really think that the progress bar component is not adapted for the metadata quality widget, it should stick to something similar to what we have on data.gouv.

Here is a first mockup about what the customer wants for the design, it's way better integrated and discret.

Screenshot from 2023-06-15 10-13-38

We will test the behavior and the computation better next week, thanks again for your great work.
Cheers

apps/search/src/assets/i18n/en.json Outdated Show resolved Hide resolved
@Input() value: boolean

get display() {
const name_snake_upper = this.name.replace(/[A-Z]/g, (letter) => `_${letter.toLowerCase()}`).toUpperCase();
Copy link
Member

Choose a reason for hiding this comment

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

Variable name must be camel case.

@fgravin
Copy link
Member

fgravin commented Jul 4, 2023

Hello @gkeimeHDF, any news on the topic, regarding my first bunch of comments ?
Cheers

@gkeimeHDF
Copy link
Contributor Author

@fgravin i was waiting some time if u have more comments, looks like it was completed. Today i had fixed all the issues.

Have a good day

@fgravin
Copy link
Member

fgravin commented Jul 5, 2023

@fgravin i was waiting some time if u have more comments, looks like it was completed. Today i had fixed all the issues.

Ok no worry sorry, we haven't tested it yet deeply.
Thanks for the update, we'll have a look shortly.
Did you change the ui component, not to use the default progress bar which is not satisfying in terms of rendering ?
Best.

@gkeimeHDF
Copy link
Contributor Author

gkeimeHDF commented Jul 5, 2023

No because it's the progress bar is the one that MEL has choosen, the meter component was denied by them.

Best regards

Guillaume

@fgravin
Copy link
Member

fgravin commented Jul 5, 2023

No because it's the progress bar is the one that MEL has choosen, the component was denied by them.

The screenshot that I provided above comes from MEL UX/UI work, it is what they expect.
I would suggest that you talk with Florent Berault about that, to see if their need has changed or not since he talked about the actual progress bar.
Thanks !

@fgravin fgravin added enhancement Proposal for a new feature major feature labels Jul 5, 2023
Copy link
Member

@fgravin fgravin left a comment

Choose a reason for hiding this comment

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

Thanks for the enhancements.
I've done a deeper review and unfortunately there is an architecture issue with the way your dumb components initiate their state (from the config).

I've made several comments as well.
Also, please add the documentation in the datahub readme for now, otherwise, people won't know how to use the new fields to sort on the quality.

Thanks a lot.

@@ -7,6 +7,9 @@ body {
margin: 0;
}

.container-xs {
Copy link
Member

Choose a reason for hiding this comment

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

I wonder if this shouldn't be supported by https://tailwindcss.com/docs/container

.container-sm shouldn't exist (it's our responsibility to remove it), do not take it as an exemple.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

On this comment, we should do something or let the code like this?

Copy link
Collaborator

Choose a reason for hiding this comment

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

You should remove the code because we try as much as possible to avoid having global CSS (which is very hard to maintain in the long run). For styling you can either use tailwind classes or, if styling is too elaborate, use the component stylesheet, which is encapsulated.

Copy link
Contributor Author

@gkeimeHDF gkeimeHDF Aug 1, 2023

Choose a reason for hiding this comment

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

i can remove the code but when we will reduce the screen the the title can overflow above the widget. Is it ok? @jahow

Copy link
Collaborator

Choose a reason for hiding this comment

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

No, we still need to have a working layout

# if you add an indexed field to calculate the qualityScore, the datahub search allow you to sort on this field with this parameter
# sortable = true

# by default the wudget appear in 2 location in search list and in detail page
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
# by default the wudget appear in 2 location in search list and in detail page
# by default the widget appears in 2 locations: in the search list and in the detail page

# by default the wudget appear in 2 location in search list and in detail page
# display_widget_in_detail = false // allow you to hide the widget in detail
# display_widget_in_search = false // allow you to hide the widget in search list
# If you want see the widget in the two location, don't fill theses configurations
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
# If you want see the widget in the two location, don't fill theses configurations
# If you want see the widget in the two locations, don't fill theses configurations

@@ -60,7 +62,11 @@ export class RecordMetadataComponent {
private searchService: SearchService,
private sourceService: SourcesService,
private orgsService: OrganisationsServiceInterface
) {}
) { }
Copy link
Member

@fgravin fgravin Jul 12, 2023

Choose a reason for hiding this comment

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

the formatting is wrong, you should install a prettier plugin within your IDE to auto format the files.
just run a npm run format to fix all formatting at once.

MD_LegalConstraintsUseLimitationObject: (output, source) => {
const legalConstraints = getAsArray(
selectField<SourceWithUnknownProps[]>(source, 'MD_LegalConstraintsUseLimitationObject')
).map((MD_LegalConstraintsUseLimitationObject) => selectTranslatedValue<string>(MD_LegalConstraintsUseLimitationObject));
Copy link
Member

Choose a reason for hiding this comment

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

variables should always be camel case.

})
export class MetadataQualityInfoComponent {

metadataQualityConfig: MetadataQualityConfig = getMetadataQualityConfig();
Copy link
Member

Choose a reason for hiding this comment

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

ui library, even every library are not allow to import the config.
an application could run without the configuration, it means that no dependency are allowed, configuration is only available from the applications.
We have set up linting rules for that, I would suggest you to run npm run lint before pushing a PR to check that everything is correct. The CI should do it I will enable it on your PR.

This means that you'll have to refactor your approach. You should read the configuration, extract meaningful informations out of it, and pass those flags to the dedicated components like "displayQualityInRecordPage" and drill them in.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The config is not required because there is a default config

@Input() name: string
@Input() value: boolean

get display() {
Copy link
Member

Choose a reason for hiding this comment

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

I think that the display should be handled outside of the component as well

Copy link
Contributor Author

@gkeimeHDF gkeimeHDF Aug 1, 2023

Choose a reason for hiding this comment

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

Where do you wish this display should be handled? @jahow @fgravin

font-size: 15px;
}

.menu {
Copy link
Member

Choose a reason for hiding this comment

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

should be done with tailwind CSS, no css is needed here

Copy link
Member

Choose a reason for hiding this comment

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

Try to remove all css properties here and use tailwind classes with the HTML template instead.

it('content', () => {
expect(component.metadata?.contact?.organisation).toBe("Ifremer");
})
})
Copy link
Member

Choose a reason for hiding this comment

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

You should test the sub component inputs as well with component mocks.

@Component({
  selector: 'gn-ui-metadata-quality-info',
  template: '',
})
class MockMetadataQualityInfoComponent {
  @Input() name: string;
  @Input() value: any;
}
..
await TestBed.configureTestingModule({
  declarations: [MetadataQualityComponent, MockMetadataQualityInfoComponent],
}).compileComponents();
...
component.metadata = {
  qualityScore: 80, // Sample metadata for testing
  title: 'Sample Title',
  abstract: 'Sample Abstract',
  topic: ['Topic 1', 'Topic 2'],
  keywords: ['Keyword 1', 'Keyword 2'],
  legalConstraints: ['Legal Constraint 1', 'Legal Constraint 2'],
  contact: {
    organisation: 'Sample Organization',
    email: '[email protected]',
  },
  updateFrequency: 'Monthly',
};
...
it('should display sub-components with correct inputs', () => {
const metadataInfoElements = fixture.nativeElement.querySelectorAll('gn-ui-metadata-quality-info');
expect(metadataInfoElements.length).toBe(8);

const titleElement = metadataInfoElements[0];
expect(titleElement.componentInstance.name).toBe('title');
expect(titleElement.componentInstance.value).toBe('Sample Title');

const abstractElement = metadataInfoElements[1];
expect(abstractElement.componentInstance.name).toBe('description');
expect(abstractElement.componentInstance.value).toBe('Sample Abstract');

const topicElement = metadataInfoElements[2];
expect(topicElement.componentInstance.name).toBe('topic');
expect(topicElement.componentInstance.value).toBe(true);

const keywordsElement = metadataInfoElements[3];
expect(keywordsElement.componentInstance.name).toBe('keywords');
expect(keywordsElement.componentInstance.value).toBe(true);

const legalConstraintsElement = metadataInfoElements[4];
expect(legalConstraintsElement.componentInstance.name).toBe('legalConstraints');
expect(legalConstraintsElement.componentInstance.value).toBe(true);

const organizationElement = metadataInfoElements[5];
expect(organizationElement.componentInstance.name).toBe('organisation');
expect(organizationElement.componentInstance.value).toBe('Sample Organization');

const contactElement = metadataInfoElements[6];
expect(contactElement.componentInstance.name).toBe('contact');
expect(contactElement.componentInstance.value).toBe('[email protected]');

const updateFrequencyElement = metadataInfoElements[7];
expect(updateFrequencyElement.componentInstance.name).toBe('updateFrequency');
expect(updateFrequencyElement.componentInstance.value).toBe('Monthly');
});

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 just need to put this snippet in the test page ?

Copy link
Collaborator

Choose a reason for hiding this comment

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

the snippet is just intended as an example, I don't think it'll work as-is. The idea is that you test things in a more complete way by checking that the children component get the right inputs for each field in the list.

DISPLAY_ORGANISATION: parsedMetadataQualitySection.display_organisation,
} as MetadataQualityConfig)

searchConfig =
Copy link
Member

Choose a reason for hiding this comment

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

why do you have code about the geometry ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

there is no geometry? Nothing to do here?

Copy link
Collaborator

Choose a reason for hiding this comment

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

the lines 249 from 255 look like they were duplicated by mistake, as they are about the filter geometry parameter

@@ -54,6 +54,9 @@ background_color = "#fdfbff"
# title_font = "'My Custom Title Font', fallback-font-title"
# fonts_stylesheet_url = "https://fonts.googleapis.com/css2?family=Open+Sans:wght@400;700&family=Permanent+Marker&display=swap"

# This optional parameter allow you to change the default class on progress bar, by default is font-bold
# progress_bar_text_class = 'font-normal'
Copy link
Member

Choose a reason for hiding this comment

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

I forgot this point but we don't want to introduce this kind of config. You can just get rid of it.
Thanks

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 need remove the code? did we change the default font by font-normal and let it to font-bold?

Copy link
Collaborator

@jahow jahow Aug 1, 2023

Choose a reason for hiding this comment

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

I think you can remove this change and enforce the font weight of the progress bar from the parent component.

One solution is to use CSS variables in the progress bar component, such as:

.label {
  font-weight: var(--progress-bar-font-weight, 'bold');
}

And then from the parent component:

:host {
  --progress-bar-font-weight: 'normal';
}

See a concrete example with this component: https://github.com/geonetwork/geonetwork-ui/blob/main/libs/ui/inputs/src/lib/star-toggle/star-toggle.component.css

Copy link
Contributor Author

@gkeimeHDF gkeimeHDF Aug 1, 2023

Choose a reason for hiding this comment

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

i can revert my code and let it like before without change the font-weight and let it bold for all in tailwind css. Is it ok ? @jahow

Copy link
Collaborator

Choose a reason for hiding this comment

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

No I think you should find a solution like the one I suggested

@gkeimeHDF
Copy link
Contributor Author

Snyk security is failed but it's not from the code provided ?

@jahow
Copy link
Collaborator

jahow commented Aug 1, 2023

Snyk security is failed but it's not from the code provided ?

No that's probably because the snyk workflow is lacking permission.

@@ -3,10 +3,12 @@ import { marker } from '@biesbjerg/ngx-translate-extract-marker'
import { SortByEnum } from '@geonetwork-ui/util/shared'
import { SearchFacade } from '../state/search.facade'
import { SearchService } from '../utils/service/search.service'
import { getMetadataQualityConfig } from '@geonetwork-ui/util/app-config'
Copy link
Collaborator

Choose a reason for hiding this comment

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

We try to avoid importing the app config from libs because components can exist without an app configuration (e.g. in web components); in this situation such an import might cause a crash.

See other comment by @fgravin

Comment on lines 205 to 209
if (check('TITLE')) {
if (selectField(source, 'resourceTitleObject')) {
success++;
}
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

nested ifs can be combined 🙂

@jahow
Copy link
Collaborator

jahow commented Aug 1, 2023

Big thanks for your continued work on this @gkeimeHDF, I have added a few comments to hopefully make things clearer for you. I feel like we're getting close to having something mergeable! Did you manage to run the lint and format tasks?

Edit: also keep in mind that the PR will need to be rebased before merging, as conflicts have appeared with the main branch, thanks!

@gkeimeHDF
Copy link
Contributor Author

@jahow How to launch lint and format ?

I tried :
npm run format

> [email protected] format
> nx format:write
tsconfig.base.json

npm run lint

> [email protected] lint
> nx affected:lint


 >  NX   Affected criteria defaulted to --base=remotes/origin/main --head=HEAD

 >  NX   Successfully ran target lint for 0 projects (3ms)
 
   View logs and run details at https://nx.app/runs/rqIj8pipxz

@f-necas
Copy link
Collaborator

f-necas commented Aug 1, 2023

Hi @gkeimeHDF,

npm run lint and npm run format should work. Do you execute them on geonetwork-ui/ root folder ?

@gkeimeHDF
Copy link
Contributor Author

Hi @gkeimeHDF,

npm run lint and npm run format should work. Do you execute them on geonetwork-ui/ root folder ?

yes else i will got error if no package.json is present.

@f-necas
Copy link
Collaborator

f-necas commented Aug 2, 2023

Indeed, my bad.

Maybe you could have a try with npm run lint:all and for format directly with npx nx format:write ? That's just some suppositions.

We have released the 1.1.0 version this morning, you can rebase on it.

Let me know if it works !

@jahow
Copy link
Collaborator

jahow commented Aug 25, 2023

Hi, #464 was just merged and it changes significantly the internal data format used in geonetwork-ui. Please use this issue if you need any pointers or indications on how to adapt your work consequently, thanks.

@gkeimeHDF
Copy link
Contributor Author

gkeimeHDF commented Sep 4, 2023

@jahow @fgravin

Hello, i submit a new version about the quality widget code that contains yours comments.

Can you have a look and tell me if u have some others comments?

Thank you

@fgravin
Copy link
Member

fgravin commented Sep 5, 2023

Can you have a look and tell me if u have some others comments?

Hi @gkeimeHDF sure we'll have a look today or tomorrow, thanks for your work.

@fgravin fgravin self-assigned this Sep 7, 2023
@fgravin fgravin linked an issue Sep 7, 2023 that may be closed by this pull request
Copy link
Member

@fgravin fgravin left a comment

Choose a reason for hiding this comment

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

Thanks for the refactoring @gkeimeHDF it looks very good.
I have made some minor comments for the final adjustments.

There is so much prop drilling that I wonder if a service wouldn't have been more appropriate... This is good enough though.

Thanks 👍

@@ -11,7 +11,14 @@ export const ES_SOURCE_SUMMARY = [
'linkProtocol',
'contactForResource.organisation',
'contact.organisation',
'contact.email',
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 is redundant with FIELDS_SUMMARY.
I think that ES_SOURCE_SUMMARY should be removed, what do you think @jahow .

@gkeimeHDF it's not your responsability, but I don't think that you need this change, as this constant is just used in the related records request.

Copy link
Collaborator

Choose a reason for hiding this comment

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

no big deal, there's probably some more cleanup to do but that doesn't have to be part of this PR; I'm fine either way

@@ -58,10 +58,14 @@ export interface BaseRecord {
keywords: Array<string> // TODO: handle thesaurus and id
accessConstraints: Array<AccessConstraint>
useLimitations: Array<string>
legalConstraints?: Array<string>
Copy link
Member

Choose a reason for hiding this comment

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

I don't know what to think with these changes, you are updating the pivot format which has been carefuly designed from various standards. I let @jahow judge, maybe those properties were missing.

BTW How do we manage topic over themes ?

Copy link
Collaborator

Choose a reason for hiding this comment

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

legal constraints are accessConstraints of type legal, so no need for a new field I would say

Copy link
Collaborator

Choose a reason for hiding this comment

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

Topics and themes are semantically the same thing I would say. In ISO they are in two different places but we can probably append them together in the themes array for now. Non-INSPIRE records will get their themes from the cl_topic field, whereas INSPIRE records will get them from the inspireTheme field.

Copy link
Contributor Author

@gkeimeHDF gkeimeHDF Sep 11, 2023

Choose a reason for hiding this comment

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

@jahow legalConstraints doesn't go to accessConstraints, it go to useLimitations which is a string Array.

https://github.com/geonetwork/geonetwork-ui/blob/7a84a5b053a3d56ae57f563d8793a40b4cca528f/libs/api/metadata-converter/src/lib/gn4/gn4.field.mapper.ts#L248C12-L248C12

The field is MD_LegalConstraintsUseLimitationObject

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jahow Des nouvelles sur le sujet?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jahow @fgravin i still need an answer about legalConstraints. Should i use accessConstraints type "legal" then actually it won't works because there no accessContraints for this field but a useLimitations ?

Copy link
Collaborator

Choose a reason for hiding this comment

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

@gkeimeHDF sorry for taking so long to answer. It's a tricky subject, I think we can leave it like that and improve it later on. Thanks!

@@ -34,6 +39,12 @@ export class SortByComponent {
private searchService: SearchService
) {}

ngOnInit(): void {
if (!this.isQualitySortable) {
Copy link
Member

Choose a reason for hiding this comment

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

I would prefer that you do the opposite, cause this method won't work if we want a setting for another sort property.

If it's enable, then add the option in the choices array.

font-size: 15px;
}

.menu {
Copy link
Member

Choose a reason for hiding this comment

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

Try to remove all css properties here and use tailwind classes with the HTML template instead.

}

ngOnChanges(changes: SimpleChanges): void {
if (changes['metadata'] || changes['metadataQualityDisplay'])
Copy link
Member

Choose a reason for hiding this comment

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

Does it enter here (except on component init) ? I don't understand how this component could see its input changed 🤔

Ok it does on the metadata view, because we first load a ghost page with a small metadata object, and when the full object is loaded, we hydrate the content with the new object.

Anyway, ngOnChanges is also called on init so there is a redundancy here, remove the ngOnInit method.

I would load the widget only when the full metadata object is fetched, not on pre-loading though.

@@ -50,6 +52,8 @@ import { PaginationButtonsComponent } from './pagination-buttons/pagination-butt
RelatedRecordCardComponent,
MetadataContactComponent,
MetadataCatalogComponent,
MetadataQualityComponent,
MetadataQualityInfoComponent,
Copy link
Member

Choose a reason for hiding this comment

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

Honestly, it's hard to understand at first sight that MetadataQualityInfoComponent is one item of the MetadataQualityComponent component, the name is not talkative enough IMO.

@@ -0,0 +1,3 @@
.text-4 {
Copy link
Member

Choose a reason for hiding this comment

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

why not keeping font-bold ?

Copy link
Collaborator

@jahow jahow left a comment

Choose a reason for hiding this comment

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

Added some comments, I hope it clarifies things a bit. We're almost there @gkeimeHDF, thanks for the continued work.

licenses: Array<License>
overviews: Array<GraphicOverview>
extras?: Record<string, unknown>
landingPage?: URL
topic?: Array<string>
updateFrequency?: UpdateFrequency
qualityScore?: number
Copy link
Collaborator

Choose a reason for hiding this comment

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

This should go in extras I think

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 don't understand this comment, can you more details?

Copy link
Collaborator

Choose a reason for hiding this comment

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

So the mapper can output things that are too specific for the pivot format in an extras field, see for example:

userSavedCount: (output, source) =>
this.addExtra(
{
favoriteCount: parseInt(selectField(source, 'userSavedCount')),
},
output
),

I think you should also use addExtra for the qualityScore field

@@ -58,10 +58,14 @@ export interface BaseRecord {
keywords: Array<string> // TODO: handle thesaurus and id
accessConstraints: Array<AccessConstraint>
useLimitations: Array<string>
legalConstraints?: Array<string>
Copy link
Collaborator

Choose a reason for hiding this comment

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

legal constraints are accessConstraints of type legal, so no need for a new field I would say

@@ -11,7 +11,14 @@ export const ES_SOURCE_SUMMARY = [
'linkProtocol',
'contactForResource.organisation',
'contact.organisation',
'contact.email',
Copy link
Collaborator

Choose a reason for hiding this comment

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

no big deal, there's probably some more cleanup to do but that doesn't have to be part of this PR; I'm fine either way

@@ -58,10 +58,14 @@ export interface BaseRecord {
keywords: Array<string> // TODO: handle thesaurus and id
accessConstraints: Array<AccessConstraint>
useLimitations: Array<string>
legalConstraints?: Array<string>
Copy link
Collaborator

Choose a reason for hiding this comment

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

Topics and themes are semantically the same thing I would say. In ISO they are in two different places but we can probably append them together in the themes array for now. Non-INSPIRE records will get their themes from the cl_topic field, whereas INSPIRE records will get them from the inspireTheme field.

licenses: Array<License>
overviews: Array<GraphicOverview>
extras?: Record<string, unknown>
landingPage?: URL
topic?: Array<string>
Copy link
Collaborator

Choose a reason for hiding this comment

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

See comment above, this new field should not be necessary.

@gkeimeHDF
Copy link
Contributor Author

@jahow @fgravin i did a new version taking account the last comments

@fgravin
Copy link
Member

fgravin commented Oct 18, 2023

@jahow @fgravin i did a new version taking account the last comments

Thank you @gkeimeHDF, we have been busy with the 2.0.0 release sorry for the delay.
We'll do a last review and should merge it straight after 🙏

@fgravin
Copy link
Member

fgravin commented Oct 18, 2023

@gkeimeHDF could you please fix the issues pointed by the CI ?
I would suggest that you run the format,lint,test commands locally before pushing.
Thanks

@gkeimeHDF
Copy link
Contributor Author

gkeimeHDF commented Oct 18, 2023

@gkeimeHDF could you please fix the issues pointed by the CI ? I would suggest that you run the format,lint,test commands locally before pushing. Thanks

@fgravin i had already runned format lint and test without issue. Which is the issue?

@fgravin
Copy link
Member

fgravin commented Oct 18, 2023

@gkeimeHDF
Copy link
Contributor Author

The CI is failing there https://github.com/geonetwork/geonetwork-ui/actions/runs/6302671953/job/17815157041#step:6:13

Yes i see, it works in local without prompt any errors, so i guess you need to fix the issue in your ci.

@fgravin
Copy link
Member

fgravin commented Oct 18, 2023

The CI is failing there https://github.com/geonetwork/geonetwork-ui/actions/runs/6302671953/job/17815157041#step:6:13

Yes i see, it works in local without prompt any errors, so i guess you need to fix the issue in your ci.

The CI rarely lies 😉

If you run npm run format:check you will see the list of invalid files, if there is at least one file in the list, the CI fails.
Run npm run format to autofix the formatting. Usually, it's recommended to configure your code editor with prettier so the formatting is automatic.

Thanks

@gkeimeHDF
Copy link
Contributor Author

gkeimeHDF commented Oct 18, 2023

The CI is failing there https://github.com/geonetwork/geonetwork-ui/actions/runs/6302671953/job/17815157041#step:6:13

Yes i see, it works in local without prompt any errors, so i guess you need to fix the issue in your ci.

The CI rarely lies 😉

If you run npm run format:check you will see the list of invalid files, if there is at least one file in the list, the CI fails. Run npm run format to autofix the formatting. Usually, it's recommended to configure your code editor with prettier so the formatting is automatic.

Thanks

Did you try the branch ? i had even downgraded my node version to match your ci node version without any errors.

npm run format:check --verbose
npm info using [email protected]
npm info using [email protected]
npm verb title npm run format:check
npm verb argv "run" "format:check" "--loglevel" "verbose"
npm verb logfile logs-max:10 dir:/Users/kembris/.npm/_logs/2023-10-18T11_05_07_512Z-
npm verb logfile /Users/kembris/.npm/_logs/2023-10-18T11_05_07_512Z-debug-0.log

> [email protected] format:check
> nx format:check

npm verb exit 0
npm info ok 
geonetwork-ui % npm run format                

> [email protected] format
> nx format:write

tsconfig.base.json
geonetwork-ui % git status
Sur la branche main
Votre branche est à jour avec 'origin/main'.

rien à valider, la copie de travail est propre

i would suggest you to add --verbose to view what are the issue from your CI.

@fgravin
Copy link
Member

fgravin commented Oct 18, 2023

I fetched your fork, and set me on detached HEAD on your branch. Then running the format gives me errors (the exact same files as in the CI):

fgravin@wkr19:~/dev/geonetwork-ui$ git st
HEAD detached at neogeo/main
no changes added to commit (use "git add" and/or "git commit -a")

fgravin@wkr19:~/dev/geonetwork-ui$ npx nx format:check
libs/api/repository/src/lib/gn4/elasticsearch/constant.ts
libs/api/repository/src/lib/gn4/elasticsearch/elasticsearch.service.spec.ts
libs/ui/search/src/lib/results-list-item/results-list-item.component.ts
fgravin@wkr19:~/dev/geonetwork-ui$ git branch

Are you sure that you pushed everything ? Don't you have anything in your stage ? Would be weird if not.

@gkeimeHDF
Copy link
Contributor Author

Yes all is pushed i don't have issue.

I did:
git clone https://github.com/gkeimeHDF/geonetwork-ui.git
cd geonetwork-ui

npm install then:

% npx nx format:check

% git status
Sur la branche main
Votre branche est à jour avec 'origin/main'.
rien à valider, la copie de travail est propre

% git push
Everything up-to-date

@fgravin
Copy link
Member

fgravin commented Oct 18, 2023

Ok, weird. I can't see any reason why the format command passes on your environment while the formatting of some files are obviously wrong (cf gkeimeHDF#1).

Anyway, to move forward, let's merge my PR and re-run the CI.
Thanks

@fgravin fgravin added this to the 2.1.0 milestone Oct 18, 2023
@gkeimeHDF
Copy link
Contributor Author

i have merged the PR, maybe the formatter had an issue on macos ?

@f-necas
Copy link
Collaborator

f-necas commented Oct 18, 2023

i have merged the PR, maybe the formatter had an issue on macos ?

Hello @gkeimeHDF, what IDE do you use ? In IntelliJ, I had to update the Prettier configuration.

@gkeimeHDF
Copy link
Contributor Author

@f-necas i use vscode with macos

@fgravin
Copy link
Member

fgravin commented Oct 19, 2023

This is not an issue about the IDE, cause it does not work apparently from the command line either.
I really think that it did work on your environment cause otherwise it would be impossible to have so few formatting errors from your PR.
Maybe the version of prettier, or nx, or prettier is confused about the configuration it uses ... ¯\_(ツ)_/¯

@gkeimeHDF
Copy link
Contributor Author

npx prettier --version
2.7.1

Copy link
Collaborator

@jahow jahow left a comment

Choose a reason for hiding this comment

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

All green except Snyk which is a problem that is on the CI, not on this PR

Thanks @gkeimeHDF for the ongoing work

@jahow jahow merged commit 2901ae6 into geonetwork:main Oct 25, 2023
5 of 6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Proposal for a new feature major feature
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

Add a metadata quality widget
4 participants