Me/dh/records published by my org

[cmoinier]

TO-DO

In the "My Organization" menu item:

• The title should be the logo + name of the organization
• Right of the title, the amount of published records should be shown in a numerical figure; when clicked, the user is taken to the datahub with the corresponding filter (amount of records should match!)
• Right of the title, the amount of users should be shown in a numerical figure; when clicked, the user is taken to the "/users/my-org" route
• On this route, a list of users in the organization is shown (using the same layout as the records list); fields shown are name, username, email)

How to test

For testing purposes, I left commented lines at :

• my-org.service (lines 41 and 59)

You might need to restore the commented line depending on the user you're using/your docker compo.
For instance, the user Barbie has the organization name Barbie Inc. which doesn't match any record or user organization and prevents testing here.
The comments will be removed before merging.

• In default.toml, (line 27) the link to the datahub is to be set to your localhost where datahub is running.

[github-actions]

Affected libs: api-repository, feature-catalog, feature-record, feature-router, feature-search, feature-map, feature-dataviz, feature-auth, common-domain, api-metadata-converter, feature-editor, ui-search, common-fixtures, ui-elements, ui-catalog, util-shared, ui-widgets, ui-inputs, ui-map, ui-dataviz, util-app-config,
Affected apps: datahub, metadata-editor, demo, webcomponents, search, map-viewer, datafeeder, metadata-converter, data-platform,

• 🚀 Build and deploy storybook and demo on GitHub Pages
• 📦 Build and push affected docker images

[coveralls]

coverage: 82.713% (-3.5%) from 86.261% when pulling 608ff82 on ME/records-published-by-my-org into 7c8b49c on main.

[f-necas]

Nicely done @cmoinier for this huge work.
I've let some comments, some debatable. Feel free to remove the comments either.

Maybe a nice things to have is a 'no record for this organisation/ no user for this organisation' placeholder instead of blank page.

[f-necas]

Thanks Camille for those nice improvements ! If @jahow can tell us what she thinks about it.

Otherwise, it seems to be good :)

[f-necas]

I am skeptical for this. I may need @jahow advice there ...

Instead of subscribing to 3 observables, it may be good to do it with rxjs with some kind of (with combineLatest or zip, or something else if it exists.) :

const orgData$ = combineLatest([users$, allUsers$, organisations]).pipe(
  map(([a,b,c]) => resolve resolve),
);

and in html using an async pipe to get datas.

[jahow]

Indeed this is a good exercise for observables 🙂

RxJS offers you tools to avoid creating your own observable and calling next() yourself. Something like what @f-necas was suggesting:

myOrgData$ = combineLatest([this.authService.user$, this.authService.allUsers$, this.orgService.organisations$]).pipe(
  map(([user, allUsers, orgs]) => {
      const orgName = user.organisation
      const org = orgs.find((org) => org.name === orgName)
      const logoUrl = org?.logoUrl?.href.toString()
      const recordCount = org?.recordCount
      const userList = users.filter((user) => user.organisation === orgName)
      const userCount = userList.length
      return {
        logoUrl,
        recordCount,
        userList,
        userCount,
      }
  })
);

(this is not tested!)

As for what you did originally, the problem is that you "break" the observable pattern by using the myOrgDataSubject to share information between various contexts (namely the orgName value). Although it works, it might be difficult to read for others because of the convoluted logic.

As a rule of thumb, an ideal use of observables is:

• no manual subscribing (only in the html with | async)
• no creation of observables yourself (most observables should be derived from something else: HTTP requests, events, etc.)

Don't hesitate to ask if something is unclear! And yes, this should go in the docs eventually. 👼

[cmoinier]

Got it, thanks for the extensive explaination 🙂
Following this guidance, should I change the way my-org-records.component.html receives its data?
Go from : orgData?.logoUrl to something like myOrgRecordsService.myOrgData$.logoUrl | async ?

[jahow]

Thanks for the extensive work! It feels like you tackled quite a complex task with this story. I've added some comments, and I think it could be great if you could address them.

The metadata-editor components will still need some work for the future and that's fine, let's keep this an iterative process. I hope my comments are clear enough 🙂

[jahow]

Indeed this is a good exercise for observables 🙂

RxJS offers you tools to avoid creating your own observable and calling next() yourself. Something like what @f-necas was suggesting:

myOrgData$ = combineLatest([this.authService.user$, this.authService.allUsers$, this.orgService.organisations$]).pipe(
  map(([user, allUsers, orgs]) => {
      const orgName = user.organisation
      const org = orgs.find((org) => org.name === orgName)
      const logoUrl = org?.logoUrl?.href.toString()
      const recordCount = org?.recordCount
      const userList = users.filter((user) => user.organisation === orgName)
      const userCount = userList.length
      return {
        logoUrl,
        recordCount,
        userList,
        userCount,
      }
  })
);

(this is not tested!)

As for what you did originally, the problem is that you "break" the observable pattern by using the myOrgDataSubject to share information between various contexts (namely the orgName value). Although it works, it might be difficult to read for others because of the convoluted logic.

As a rule of thumb, an ideal use of observables is:

• no manual subscribing (only in the html with | async)
• no creation of observables yourself (most observables should be derived from something else: HTTP requests, events, etc.)

Don't hesitate to ask if something is unclear! And yes, this should go in the docs eventually. 👼

[cmoinier]

Thanks @jahow for this review and for your advice. I think the last two commits cover pretty much everything 🙂

For some reason I couldn't comment back on your comment . Here it is :

I know, the union type is behaving weirdly, I thought it would work too when I first tried.
Yes I know... At first, I did start to make a whole different table in my-org-records, but ended up with almost the same code as this component, in both template and functionalities, and figured it was going to be a whole lot of duplicate code, as you say. Hence why I ended up doing this and I know it's far from ideal. And yes, I think your idea about having an agnostic presentation component is great, but doing it now for this PR is going to be a lot of work.

[jahow]

Thanks for getting my comments addressed, I think it can be merged now 🙂

Could you please rebase the branch on develop before merging and remove the merge commit in the PR history? We generally try to avoid merge commits in Pull Requests as much as possible, thanks!

changes made & checked by other team member