[ENG-4760] User secondary metadata #2031

futa-ikeda · 2023-10-17T14:48:41Z

Ticket: ENG-4760
Feature flag: n/a

Purpose

Show more user metadata for user search result cards

Summary of Changes

Add preprint relationship to user model
Handle user affiliation in search-result model
Update search-result-card to fetch user model if there is one
Update style of secondary-metadata card so paired dt and dd are closer

Screenshot(s)

Loading state
Loaded
other result cards (note bolded title and values are closer together)

Side Effects

QA Notes

futa-ikeda · 2023-10-17T14:58:13Z

lib/osf-components/addon/components/search-result-card/component.ts

+        const osfId = ids.find(id => id['@value'].includes(osfUrl))?.['@value'];
+        if (osfId) {
+            // remove osfUrl from id and any leading/trailing slashes
+            return osfId.replace(osfUrl, '').replace(/^\/|\/$/g, '');


This feels like a fragile way of fetching the user guid. Any thoughts/feedback on this would be appreciated!

would recommend startsWith (instead of includes) and slice (instead of replace), so the operation is anchored at the start of the string

for (const iri of ids.map(id => id['@value'])) { if (iri && iri.startsWith(osfUrl)) { return iri.slice(osfUrl.length).replace(/^\/|\/$/g, ''); } }

(does this happen other places? in backend code using IRIs as data, this sort of thing was common enough to warrant shared utility functions)

I suppose this could be done fairly simply with the URL object type by doing something like

for (const iri of irisArray ) { const url = new URL(iri); if (url.host === osfUrl) { return url.pathname.slice(1); // remove leading slash. Could also be slice(1,6)?? } }

Although this approach feels a bit too naive to make into a generalized util function

if you want to compare hosts, yeah the built-in URL seems better than parsing it yourself (tho you'd also need to extract the host from osfUrl, since it's a full url) -- at the moment i'm more a fan of the primitive startswith/slice approach for iris within namespaces (one way to look at osf-id (osf-guid)), tho there's also the complication of osf-urls that are not osf-ids but are still used as identifiers...

for (const iri of ids.map(id => id['@value'])) { if (iri && iri.startsWith(osfUrl)) { const pathSegments = iri.slice(osfUrl.length).split('/').filter(Boolean); if (pathSegments.length === 1) { return pathSegments[0]; // one path segment; looks like osf-id } } }

or using URL and comparing hosts:

const osfHost = new URL(osfUrl).host; for (const iri of ids.map(id => new URL(id['@value']))) { if (iri.host === osfHost) { const pathSegments = iri.pathname.split('/').filter(Boolean); if (pathSegments.length === 1) { return pathSegments[0]; // one path segment; looks like osf-id } } }

(the two would have different behavior if there were ever an osfUrl that had a non-empty path, which might be a nice future to aim for (supporting folks who want to deploy an instance at a subroute under their own domain) but is not the present we're in)

buuuut in any case, maybe the eventual helpful reusable thing would be a method on IndexCardModel to extract an osf-id from this.resourceIdentifier and/or this.resourceMetadata.identifier (and maybe another method to fetch the ember-data model instance for it) -- no pressure to add that now, tho

I would not have caught the subtle difference in these two approaches. I'll opt for the first one that uses startsWith, as I think there may be some cases where the pathname approach may cause problems. Will add this guid-extration method and a fetchOsfModel, as I think that would be handy for possible future improvements

brianjgeiger

Code looks good, but it doesn't look like the mockups. I'm guessing this is because of the data list? Can it be formatted to look like the mockups? If not, have you discussed that with Product?

coveralls · 2023-10-17T15:12:43Z

Pull Request Test Coverage Report for Build 6591621712

1 of 40 (2.5%) changed or added relevant lines in 4 files are covered.
1 unchanged line in 1 file lost coverage.
Overall coverage decreased (-0.5%) to 70.71%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
app/models/search-result.ts	0	3	0.0%
lib/osf-components/addon/components/search-result-card/user-secondary-metadata/component.ts	0	5	0.0%
lib/osf-components/addon/components/search-result-card/component.ts	0	7	0.0%
app/models/index-card.ts	1	25	4.0%

Files with Coverage Reduction	New Missed Lines	%
lib/osf-components/addon/components/search-result-card/component.ts	1	10.53%

Totals
Change from base Build 6552310919:	-0.5%
Covered Lines:	5942
Relevant Lines:	8171

💛 - Coveralls

futa-ikeda · 2023-10-17T16:29:47Z

Just so we are talking about the same thing, do you mean that the title and description are too far apart? I noticed that and I can update the styles so they're closer together. We have opted not to use the mocks where some fields are on the same line (mocks here)

brianjgeiger

It was the stuff on the same line that I was talking about. If you've decided as a group not to do that, I'm good.

aaxelb · 2023-10-18T13:45:36Z

lib/osf-components/addon/components/search-result-card/component.ts

+        const osfId = ids.find(id => id['@value'].includes(osfUrl))?.['@value'];
+        if (osfId) {
+            // remove osfUrl from id and any leading/trailing slashes
+            return osfId.replace(osfUrl, '').replace(/^\/|\/$/g, '');


would recommend startsWith (instead of includes) and slice (instead of replace), so the operation is anchored at the start of the string

for (const iri of ids.map(id => id['@value'])) { if (iri && iri.startsWith(osfUrl)) { return iri.slice(osfUrl.length).replace(/^\/|\/$/g, ''); } }

(does this happen other places? in backend code using IRIs as data, this sort of thing was common enough to warrant shared utility functions)

app/models/user.ts

app/models/search-result.ts

lib/osf-components/addon/components/search-result-card/template.hbs

lib/osf-components/addon/components/search-result-card/component.ts

brianjgeiger

One question, but it's fine if that's what we're doing.

brianjgeiger · 2023-10-20T13:25:59Z

lib/osf-components/addon/components/search-result-card/user-secondary-metadata/template.hbs

+                {{#if this.user.employment.length}}
+                    <dt>{{t 'osf-components.search-result-card.employment'}}</dt>
+                    <dd>{{this.user.employment.[0].institution}}</dd>
+                {{/if}}
+                {{#if this.user.education.length}}
+                    <dt>{{t 'osf-components.search-result-card.education'}}</dt>
+                    <dd>{{this.user.education.[0].institution}}</dd>


Are we only doing the latest education/employment, rather than all of them or the one marked 'current'?

Yea, I spoke with Eric, and he wants just the latest one, or more accurately, whichever one the users put at the top of the list

app/models/index-card.ts

aaxelb · 2023-10-20T17:30:39Z

lib/osf-components/addon/components/search-result-card/template.hbs

-        <CpPanel local-class='cp-panel' id={{secondaryMetadataPanelId}} @open={{this.isOpenSecondaryMetadata}} as |panel|>
-            <panel.body>
-                <hr>
-                {{component this.secondaryMetadataComponent result=@result}}
-            </panel.body>
-        </CpPanel>
+        {{component this.secondaryMetadataComponent
+            wrapperClass=(local-class 'cp-panel')
+            wrapperId=secondaryMetadataPanelId
+            result=@result
+            isOpen=this.isOpenSecondaryMetadata
+        }}


i gather that <CpPanel> was moved inside each secondaryMetadatComponent so that user-secondary-metadata knows when it's toggled and can fetch the user, but now there's duplicate <CpPanel> invocations and a more complicated shared interface -- instead, could user-secondary-metadata start fetching the user from its constructor? (or is it bad practice to start tasks from constructor? been a while)

Yea, this refactor took a little bit of doing. I had to move the <CpPanel>into the secondaryMetadata components, since the component would be destroyed every time the card was expaneded/closed if the <CpPanel> lived on the parent search-result-card component. If we tied the user fetching to the constructor of the secondaryMetadataComponent, it would result in fetching the user every time the card is expanded (if the <CpPanel> stays on the parent search-result-card component), or the search-result card is rendered. (Nothing inherently wrong with a task being associated with the constructor I think, but in this case, the component will get constructed a lot more frequently). Also ran into trouble using splattributes on the secondaryMetadataComponents when invoking it using the {{component}} helper which was interesting

aaxelb

just a lil nitpick

lib/osf-components/addon/components/search-result-card/user-secondary-metadata/component.ts

aaxelb

🤺

futa-ikeda and others added 3 commits October 16, 2023 11:53

Add preprint relation to users

de2cb25

Update result card for user info

b4f816c

Merge branch 'develop' into user-secondary-metadata

61b6624

futa-ikeda commented Oct 17, 2023

View reviewed changes

brianjgeiger requested changes Oct 17, 2023

View reviewed changes

brianjgeiger previously approved these changes Oct 17, 2023

View reviewed changes

Group dt and dl a little closer

4a77892

futa-ikeda dismissed brianjgeiger’s stale review via 4a77892 October 17, 2023 18:51

brianjgeiger previously approved these changes Oct 17, 2023

View reviewed changes

aaxelb reviewed Oct 18, 2023

View reviewed changes

CR feedback; Refactor secondary metatadata components

fe8e44b

futa-ikeda dismissed brianjgeiger’s stale review via fe8e44b October 19, 2023 15:23

Merge branch 'develop' into user-secondary-metadata

ed5f7bc

brianjgeiger previously approved these changes Oct 20, 2023

View reviewed changes

aaxelb reviewed Oct 20, 2023

View reviewed changes

CR feedback; Add test selector

6880aa9

futa-ikeda dismissed brianjgeiger’s stale review via 6880aa9 October 20, 2023 18:21

Better refactor

18bc4dd

aaxelb previously approved these changes Oct 20, 2023

View reviewed changes

lib/osf-components/addon/components/search-result-card/user-secondary-metadata/component.ts Outdated Show resolved Hide resolved

Remove leftover arg

9cbf67b

futa-ikeda dismissed aaxelb’s stale review via 9cbf67b October 20, 2023 18:54

aaxelb approved these changes Oct 20, 2023

View reviewed changes

brianjgeiger approved these changes Oct 20, 2023

View reviewed changes

futa-ikeda merged commit bfb3f12 into CenterForOpenScience:develop Oct 20, 2023

futa-ikeda deleted the user-secondary-metadata branch October 20, 2023 19:12

futa-ikeda added this to the 23.13.0 milestone Oct 23, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[ENG-4760] User secondary metadata #2031

[ENG-4760] User secondary metadata #2031

futa-ikeda commented Oct 17, 2023 •

edited

Loading

futa-ikeda Oct 17, 2023

aaxelb Oct 18, 2023

futa-ikeda Oct 18, 2023

aaxelb Oct 18, 2023

futa-ikeda Oct 19, 2023

brianjgeiger left a comment

coveralls commented Oct 17, 2023 •

edited

Loading

futa-ikeda commented Oct 17, 2023

brianjgeiger left a comment

aaxelb Oct 18, 2023

brianjgeiger left a comment

brianjgeiger Oct 20, 2023

futa-ikeda Oct 20, 2023

futa-ikeda Oct 20, 2023

aaxelb Oct 20, 2023

futa-ikeda Oct 20, 2023 •

edited

Loading

aaxelb left a comment

aaxelb left a comment

[ENG-4760] User secondary metadata #2031

[ENG-4760] User secondary metadata #2031

Conversation

futa-ikeda commented Oct 17, 2023 • edited Loading

Purpose

Summary of Changes

Screenshot(s)

Side Effects

QA Notes

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brianjgeiger left a comment

Choose a reason for hiding this comment

coveralls commented Oct 17, 2023 • edited Loading

Pull Request Test Coverage Report for Build 6591621712

💛 - Coveralls

futa-ikeda commented Oct 17, 2023

brianjgeiger left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brianjgeiger left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

futa-ikeda Oct 20, 2023 • edited Loading

Choose a reason for hiding this comment

aaxelb left a comment

Choose a reason for hiding this comment

aaxelb left a comment

Choose a reason for hiding this comment

futa-ikeda commented Oct 17, 2023 •

edited

Loading

coveralls commented Oct 17, 2023 •

edited

Loading

futa-ikeda Oct 20, 2023 •

edited

Loading