changed the source of Owner and updated field to be searchable and so… #3095

YuliaKrimerman · 2024-08-15T13:35:20Z

…rtable

https://issues.redhat.com/browse/RHOAIENG-7566

Description

Update the source of the Owner - now it's just a field that we get from the API response. I've attached the screenshot that shows where this field on the page now is coming from.
In addition I've made the owner a sortable field as well as a Search option. See screenshots attached.

How Has This Been Tested?

Tested on the Model Registry Cluster

Test Impact

Updated the tests to cover now both field options scenarios( with Author and when "-" )

Request review criteria:

Self checklist (all need to be checked):

The developer has manually tested the changes and verified that the changes work
Testing instructions have been added in the PR body (for PRs involving changes that are not immediately obvious).
The developer has added tests or explained why testing cannot be added (unit or cypress tests for related changes)

If you have UI changes:

Included any necessary screenshots or gifs if it was a UI change.
Included tags to the UX team if it was a UI/UX change.

After the PR is posted & before it merges:

The developer has tested their solution on a cluster by using the image produced by the PR to main

gitdallas · 2024-08-15T14:40:56Z

frontend/src/__tests__/cypress/cypress/tests/mocked/modelRegistry/modelRegistry.cy.ts

-      registeredModelRow.findOwner().contains('Author 1');
+      registeredModelRow.findOwner().should(($owner) => {
+        const text = $owner.text();
+        expect(text).to.be.oneOf(['Author 1', '-']);


I think we should test both ways independently here (that the author shows up if the author prop is populated, that the '-' shows up if not).

That line of code started failing with the new change, and I wasn't quite able to figure out why ( findOwner has the same textId 'registered-model-owner' so it should be good) . It's complaining that :"Timed out retrying after 10000ms: Expected to find content: 'Author 1' within the element:
but never did". Do you have maybe a suggestion what can be missing?

It looks like you haven't updated the mock types and data. They still have the author in the modelVersion.

So if I understood correctly I should delete "mockModelVersion({ author: 'Author 1' })" line 70 in that test file? - I tried, that didn't help. I also tried to delete that field from all the 3 parts within MockModelVersionType but then the test suite didn't even run.

You'll need to also add the author prop to the RegisteredModel where your code is looking for it.

As for the tests not running in MockModelVersionType after removing all the model versions. I'm not sure, is that completely removed from everywhere or just within this case? If removed generally and tests fail after mocking that way, then there must be some test that is still falsely depending on that.

ah, actually i can't do that as the mockRegisteredModel.ts file wasn't touched yet. but basically:

then you could update the calls to mockRegisteredModel in modelRegistry.cy.ts to whatever owner/author you want, and the test to match. could use Author 1 if you want, but i think something with "Owner" would make more sense for the owner field so people don't get the two mixed up in reviewing/making tests.

or is there even an author field? you may have just had a typo below inModelDetailView. Seems odd the detail view would use author for "Owner" while the table row would use owner for "Owner"

Sorry, I just wrapped my head around the story and changes a bit more. I think you need to still treat author the same way and keep it in modelVersion and just add owner to the registeredModel. So the RM has an "owner" while the version has an "author".

Maybe @mturley has some input on this

Yes, it's confusing but the RegisteredModel has an owner but no author and the ModelVersion (and ModelArtifact, though that's not relevant) has an author but no owner. Currently in the UI we are showing "Owner" and under the hood we're using the version's author field.

There are two changes that prompted this story:

The database now has owner on RegisteredModel, which previously didn't have either owner or author (which is why we were fetching the author from the model's latest version).

The UX microcopy is being updated so that we call it in the UI what it is in the database -- for models we call it "Owner" and use the .owner field, and for versions we'll call it "Author" and use the .author field. (edit: the actual microcopy change for using the text "Author" on the versions is part of a separate story).

That latter change to use "Author" in the UI wasn't originally part of this Jira issue, it was from a followup discussion, but I think making that change in this PR would be least confusing. Is that ok @YuliaKrimerman ?
Ignore this, I was growing the scope of the PR

Actually --- scratch that last point, sorry. We don't have to touch anything with how Versions display the owner/author, that's out of scope here.

mturley

A couple changes here.

mturley · 2024-08-15T19:03:45Z

frontend/src/__mocks__/mockRegisteredModel.ts

+  author?: string;
+  owner?: string;


we should only have owner here, not author. Same for the other two instances below in this file.

mturley · 2024-08-15T19:06:32Z

frontend/src/concepts/modelRegistry/types.ts

+  author?: string;
+  owner?: string;


We shouldn't add either of these to ModelRegistryBase - I know I had originally added author here when we were pairing on this @YuliaKrimerman but I was wrong, sorry. I hadn't realized it was called owner for RegisteredModels.

The type updates we need are:

ModelRegistryBase - no change
RegisteredModel - add owner
ModelArtifact - no change (don't remove author)
ModelVersion - no change (don't remove author)

mturley · 2024-08-15T19:09:56Z

frontend/src/pages/modelRegistry/screens/RegisterModel/utils.ts

@@ -33,6 +33,7 @@ export const registerModel = async (
      name: formData.modelName,
      description: formData.modelDescription,
      customProperties: {},
+      author,


This line will need to be:

owner: author,

because the property we're adding on RegisteredModel is owner, but we can leave the function argument called author because we're using the same value for the author of the ModelVersion and ModelArtifact in the registerVersion call below.

Alternatively, if you want you can change this registerModel function so it takes an owner instead of author, then this line is just owner, and then when we call registerVersion() below you can pass owner instead of author.

I feel like I'm talking in circles, hopefully this makes sense.

mturley · 2024-08-15T19:10:50Z

frontend/src/pages/modelRegistry/screens/__tests__/utils.spec.ts

+      author: 'Author 1',
+      owner: 'Author 1',


Same deal here, we only need owner for models.

mturley · 2024-08-15T19:11:00Z

frontend/src/pages/modelRegistry/screens/__tests__/utils.spec.ts

@@ -286,9 +288,11 @@ describe('getPatchBody', () => {
    );
    expect(result).toEqual({
      description: 'Description here',
+      author: 'Author 1',


So we'll want to remove author here as well.

mturley · 2024-08-15T19:14:22Z

frontend/src/__tests__/cypress/cypress/tests/mocked/modelRegistry/modelRegistry.cy.ts

-      registeredModelRow.findOwner().contains('Author 1');
+      registeredModelRow.findOwner().should(($owner) => {
+        const text = $owner.text();
+        expect(text).to.be.oneOf(['Author 1', '-']);


Actually --- scratch that last point, sorry. We don't have to touch anything with how Versions display the owner/author, that's out of scope here.

YuliaKrimerman · 2024-08-15T20:31:09Z

Ready for hopefully final review @mturley @gitdallas

manaswinidas

/lgtm

Checked with Postman... the owner field gets picked up fine

…rtable

'

mturley

LGTM!

openshift-ci · 2024-08-16T17:23:09Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: gitdallas, manaswinidas, mturley

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~frontend/OWNERS~~ [mturley]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

openshift-ci bot requested review from christianvogt and DaoDaoNoCode August 15, 2024 13:35

gitdallas requested changes Aug 15, 2024

View reviewed changes

openshift-ci bot assigned gitdallas Aug 15, 2024

mturley reviewed Aug 15, 2024

View reviewed changes

gitdallas approved these changes Aug 15, 2024

View reviewed changes

openshift-ci bot added the lgtm label Aug 15, 2024

manaswinidas approved these changes Aug 16, 2024

View reviewed changes

openshift-ci bot assigned manaswinidas Aug 16, 2024

YuliaKrimerman added 5 commits August 16, 2024 12:46

changed the source of Owner and updated field to be searchable and so…

d3b8faa

…rtable

fixed lint and test

06caa32

updated conflicts

eeb5deb

take 2

24fdd2e

'

e98646d

'

YuliaKrimerman force-pushed the RHOAIENG-7566 branch from c283776 to e98646d Compare August 16, 2024 17:02

openshift-ci bot removed the lgtm label Aug 16, 2024

fixed last issues

e1fa4d6

mturley approved these changes Aug 16, 2024

View reviewed changes

openshift-ci bot assigned mturley Aug 16, 2024

openshift-ci bot added the lgtm label Aug 16, 2024

openshift-ci bot added the approved label Aug 16, 2024

openshift-merge-bot bot merged commit 8030ed4 into opendatahub-io:main Aug 16, 2024
6 checks passed

YuliaKrimerman deleted the RHOAIENG-7566 branch October 10, 2024 19:45

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

changed the source of Owner and updated field to be searchable and so… #3095

changed the source of Owner and updated field to be searchable and so… #3095

YuliaKrimerman commented Aug 15, 2024

gitdallas Aug 15, 2024 •

edited

Loading

YuliaKrimerman Aug 15, 2024

gitdallas Aug 15, 2024

YuliaKrimerman Aug 15, 2024

gitdallas Aug 15, 2024 •

edited

Loading

gitdallas Aug 15, 2024 •

edited

Loading

gitdallas Aug 15, 2024

mturley Aug 15, 2024 •

edited

Loading

mturley Aug 15, 2024 •

edited

Loading

mturley Aug 15, 2024

mturley left a comment

mturley Aug 15, 2024

mturley Aug 15, 2024

mturley Aug 15, 2024

mturley Aug 15, 2024

mturley Aug 15, 2024

mturley Aug 15, 2024

YuliaKrimerman commented Aug 15, 2024

manaswinidas left a comment •

edited

Loading

mturley left a comment

openshift-ci bot commented Aug 16, 2024

changed the source of Owner and updated field to be searchable and so… #3095

changed the source of Owner and updated field to be searchable and so… #3095

Conversation

YuliaKrimerman commented Aug 15, 2024

Description

How Has This Been Tested?

Test Impact

Request review criteria:

gitdallas Aug 15, 2024 • edited Loading

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

gitdallas Aug 15, 2024 • edited Loading

Choose a reason for hiding this comment

gitdallas Aug 15, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mturley Aug 15, 2024 • edited Loading

Choose a reason for hiding this comment

mturley Aug 15, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mturley 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

YuliaKrimerman commented Aug 15, 2024

manaswinidas left a comment • edited Loading

Choose a reason for hiding this comment

mturley left a comment

Choose a reason for hiding this comment

openshift-ci bot commented Aug 16, 2024

gitdallas Aug 15, 2024 •

edited

Loading

gitdallas Aug 15, 2024 •

edited

Loading

gitdallas Aug 15, 2024 •

edited

Loading

mturley Aug 15, 2024 •

edited

Loading

mturley Aug 15, 2024 •

edited

Loading

manaswinidas left a comment •

edited

Loading