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

Lookup: In multiselect we are getting array of string instead of array of object #986

Closed
Assignees
Labels
priority: high Team: GRC Issues for the Infor GRC team type: enhancement ✨ [5] Velocity rating (Fibonacci)

Comments

@dhumalkishor
Copy link

Describe the bug
In lookup control we are getting array of string instead of array of object when we select more than 6 records.

We have forked and made our changes in https://github.com/JangleShubham/enterprise-ng
Please install this project and run it and go to the path http://localhost:4200/ids-enterprise-ng-demo/lookup.

To Reproduce
Steps to reproduce the behavior:

  1. Go to 'Open lookup multiselect under label "Products (Async Results) Multi Select"'
  2. Popup with grid will get open, apply for filter and select one value, repeat this until we select at least 3 values.
  3. After close at console we have logged selected records, and those records should be array of object but instead we are getting array of string.

Expected behavior
It should provide array of object.

Version

  • ids-enterprise-ng: [e.g. v4.9.0 or v4.10.0]

Screenshots
If applicable, add screenshots to help explain your problem.

Platform

  • Device (if applicable) [e.g. iPhone 6 or Samsung Galaxy S6]
  • OS Version: [e.g. Windows 10 or iOS 8]
  • Browser Name [e.g. chrome, safari, stock browser]
  • Browser Version [e.g. 22, 66.0.3359.181 (Official Build) (64-bit)]

Additional context
As we deals with ids and because if this issue when we get array of string we can't get id assigned to object.

@tmcconechy tmcconechy changed the title lookup multiselect - we are getting array of string instead of array of object Lookup: In multiselect we are getting array of string instead of array of object Feb 9, 2021
@tmcconechy tmcconechy added [8] Velocity rating (Fibonacci) type: enhancement ✨ labels Feb 9, 2021
@tmcconechy
Copy link
Member

I dont think we can change this at this time. The Dirty event is just firing and giving you the string thats in the lookup field. That cant handle objects.

try watching the change event instead...

this.element.val(value.join(this.settings.delimiter)).trigger('change', [this.selectedRows]);

I think this should give you the selectedRows object. Feel free to close if that works.

@tmcconechy tmcconechy added type: breaking change 💥 [5] Velocity rating (Fibonacci) and removed [8] Velocity rating (Fibonacci) labels Feb 9, 2021
@JangleShubham
Copy link

Hello Tim,

We are using change event already, please see URL: https://github.com/JangleShubham/enterprise-ng/blob/master/src/app/lookup/lookup.demo.html#L218
Please take latest code of https://github.com/JangleShubham/enterprise-ng.git and see, we have commented dirty and pristine events but still we are getting same output in console.
Please let us know if we are doing something incorrect.

Thanks,
Shubham Jangle

@tmcconechy
Copy link
Member

I think the only thing "incorrect" is you dont use unique ID's so you cant use the value as a string. I did think that the change event would have objects but i was wrong...

What about using the selected event?

https://github.com/infor-design/enterprise-ng/blob/master/src/app/lookup/lookup-dialog.demo.html#L4
https://github.com/infor-design/enterprise-ng/blob/master/src/app/lookup/lookup-dialog.demo.ts#L34

?

@JangleShubham
Copy link

JangleShubham commented Feb 15, 2021

Hello Tim,
https://github.com/infor-design/enterprise-ng/blob/master/src/app/lookup/lookup-dialog.demo.html#L4
https://github.com/infor-design/enterprise-ng/blob/master/src/app/lookup/lookup-dialog.demo.ts#L34
This event seems to be of DataGridView, can you please suggest us how to use it in lookup multiselect?
Thanks,
Shubham Jangle

@tmcconechy
Copy link
Member

Hmm yes your right. This example i think is just "left out" and shows datagrid. Im not seeing a selected event you can access on the lookup. But you can try to get to the underlying example maybe.

Perhaps a solution here is to expose the selected event in the datagrid to the lookup. This will require some work we will have to schedule down the line. Perhaps you can find a workaround meanwhile on your end (using strings). I'm out of ideas than that for the moment

@tmcconechy tmcconechy added [3] Velocity rating (Fibonacci) and removed type: breaking change 💥 [5] Velocity rating (Fibonacci) labels Feb 16, 2021
@tmcconechy tmcconechy added this to To do in Enterprise 4.51.x (Apr 2021) Sprint via automation Feb 16, 2021
@ghost ghost removed this from To do in Enterprise 4.51.x (Apr 2021) Sprint Apr 5, 2021
@ghost ghost added this to To do in Enterprise 4.52.x (May 2021) Sprint via automation Apr 5, 2021
@ghost ghost removed this from To do in Enterprise 4.52.x (May 2021) Sprint Apr 28, 2021
@ghost
Copy link

ghost commented Apr 28, 2021

@dhumalkishor Can you please tell us what team you're on? Thanks!

@dhumalkishor
Copy link
Author

Infor GRC

@tmcconechy tmcconechy added the Team: GRC Issues for the Infor GRC team label Apr 29, 2021
@dhumalkishor
Copy link
Author

Hi Tim,

Can you please let me know when we can expect this issue will get fix? One of the our customer is waiting for this.

Regards,
Kishor

@tmcconechy
Copy link
Member

I can try and add it to next version but we are moving to quarterly releases so will be end of Q3. I think a possible fix is to use the settings we added as intended. i.e.

Set the field and the match option https://github.com/infor-design/enterprise/blob/main/src/components/lookup/lookup.js#L916

https://github.com/infor-design/enterprise/blob/main/app/views/components/lookup/test-custom-matching.html#L50-L53

I think you might be able to just use that. If you can put the example up in a stack blitz https://stackblitz.com/edit/ids-quick-start-950 fork i can fork it and try it out for you.

Here is the running example: https://main-enterprise.demo.design.infor.com/components/lookup/test-custom-matching.html

@tmcconechy tmcconechy added this to To do in Enterprise 4.54.x (July 2021) Sprint via automation May 11, 2021
@dhumalkishor
Copy link
Author

In which version we get above mentioned code?

@tmcconechy
Copy link
Member

@JangleShubham
Copy link

Hello Tim,

We checked with latest SOHO version and tried with stackbliz code you shared.
We are getting results in array now but we have found three issues with it as below:

  1. Shows all records selected if duplicate field is same
    Steps:
    i. Go to page https://ids-quick-start-950-wqyk9n.stackblitz.io/lookup
    ii. Click on "Soho miltiselect issue" lookup
    iii. Select any two records of column"Application Instance Name" named "Soxtest" and click Apply
    iv. Again, click on same lookup, you will find all records named "Soxtest" are selected
    Expected: Records selected in steps iii should only be selected in step iv. you suggested match function, how can we use it here?

  2. Shows all records selected on second page:
    steps:
    i. Go to page: https://ids-quick-start-950-wqyk9n.stackblitz.io/lookup
    ii. Click on "Soho miltiselect issue" lookup
    iii. Select any two records and click on next page button of pagination
    iv. At second page, you will find all records are selected
    Expected: Only earlier selected records should show selected, (if no records then no records should be selected).

  3. Removes already selected entries if we don't visit selected record page:
    steps:
    i. Go to page: https://ids-quick-start-950-wqyk9n.stackblitz.io/lookup
    ii. Click on "Soho miltiselect issue" lookup
    iii. Go to second page and select any two records and click Apply.
    iv. Again click on same lookup and click on Apply without visiting second page.
    v. Selected Records from page two will not be there in the list showed below lookup.
    Expected: Whether user travels the page of record or not lookup should return those records as well.

If there is some turnaround for all this please let us know.

Thanks,
Shubham Jangle

@tmcconechy tmcconechy added [5] Velocity rating (Fibonacci) and removed [3] Velocity rating (Fibonacci) labels May 13, 2021
@tmcconechy
Copy link
Member

Did you checkout the match example? Basically it's just a function that you set the code in the settings it runs when trying to find the selected items.
https://github.com/infor-design/enterprise/blob/main/app/views/components/lookup/test-custom-matching.html#L54

Is it possible to make a very simple example that just shows the problem (seems like a lot in this one and im not sure where the code is?)

My other suggestions/ideas would be to use a unique key here for matching. Or maybe even use the multiselect https://master-enterprise.demo.design.infor.com/components/multiselect/example-index.html

@tmcconechy
Copy link
Member

tmcconechy commented Jul 13, 2021

@dhumalkishor this should be working now. we had to add an example with NON unique ids in the field.

I still think you should use the component as intended but we exposed afterpaging and selected and allowDuplicates so you can store your own ID's and it it manually.

See the new example:
https://github.com/infor-design/enterprise-ng/blob/main/src/app/lookup/lookup-desc.demo.ts
https://github.com/infor-design/enterprise-ng/blob/main/src/app/lookup/lookup-desc.demo.html

You can do it this way or you could just use field: 'id' which would have worked before. Hope this helps 👍🏻

Also these events are in 10.2.0 and 4.53.2

@JangleShubham
Copy link

Hello @tmcconechy ,

Thanks for the extra events to handle this. It seems to be working.
However we found one small issue. When there is single record selected and we unselect that record then at that time selected event does not get invoke.
This results variable previouslySelectedRows contains that one last value even after deselecting all the values.
As we need results in object format, so we need variable previouslySelectedRows for further operations as per business requirements.

Thanks,
Shubham Jangle

@dhumalkishor
Copy link
Author

Hi Tim,

We found one more issue with multiselect lookup. Please refer below steps to reproduce the same.

  1. Go to "http://localhost:4200/ids-enterprise-ng-demo/lookup-desc"
  2. Click on browse button and select first record of the first page
  3. Go to 2nd page and select first records and click on Apply.
  4. Now again click on browse button and uncheck the first page record and click on apply button.
  5. See both records are got removed.

It should remove only one record.

Regards,
Kishor

@tmcconechy
Copy link
Member

@deep7102 do you want to look into these two cases or should I? Thanks

@tmcconechy
Copy link
Member

No not yet needs to be investigated by one of us. Since i can connect very well right now can you have a look? Ideally we just can update the example and make it work

@nganotice
Copy link

This has been QA tested and passed on v4.54.0-dev.

https://main-enterprise.demo.design.infor.com/components/lookup/test-multiselect-description-only.html
Screen Shot 2021-07-22 at 6 12 31 PM

Moving this ticket to Done.

Thanks!

@nganotice nganotice moved this from Ready for QA (beta) to Done in Enterprise 4.54.x (July 2021) Sprint Jul 22, 2021
@tmcconechy tmcconechy moved this from Done to Ready for QA (beta) in Enterprise 4.54.x (July 2021) Sprint Jul 22, 2021
@tmcconechy
Copy link
Member

@dhumalkishor and @JangleShubham all additional fixes are merged and patched into 4.53.4 also made an NG version patch with the fix https://www.npmjs.com/package/ids-enterprise-ng/v/10.2.1

@CindyMercadoReyes
Copy link

This issue is now resolved.

@CindyMercadoReyes CindyMercadoReyes moved this from Ready for QA (beta) to Done in Enterprise 4.54.x (July 2021) Sprint Jul 23, 2021
@JangleShubham
Copy link

JangleShubham commented Jul 26, 2021

Hello @tmcconechy ,

Thanks for the update and issues reported last time seems to be resolved.
But there is one small issue observe (maybe we are missing some step).

In GRC application, when we select records, it gets displayed below the lookup control as a soho list view and this list view has a cancel/delete button as well.
This means user can unselect the record before opening the model popup grid by clicking on that button.
It is shown in below image:

image

We have iterated the list we get in event (selected) and used as onSelected in component level.
We are getting values by using code:
this.previouslySelectedRows = e.selectedRows?.slice();

When user click on cancel/delete button of list, we have removed the items from list this.previouslySelectedRows and also from array model.desc(ngModel).
And after clicking on lookup we iterate this.previouslySelectedRows to show selected records as below (The code is written in event (afterpaging)):
e.lookup.grid.unSelectAllRows(true);
for (const selectedRow of this.previouslySelectedRows) {
e.lookup.selectRowByValue('id', selectedRow.data.id);
}

But after this as well it shows those records which were cancelled/deleted.
It doesn't appear in e.selectedRows but it shows as selected in grid after opening.

Are we missing something?
If there is any event/function already present to handle this, please let us know.

Please let us know if more details required from our side.

Thanks,
Shubham Jangle

@tmcconechy
Copy link
Member

tmcconechy commented Jul 26, 2021

@JangleShubham wha you just added i dont 100% follow. I think we should make a follow up issue with simple steps to reproduce? I dont get how its got now a "lookup control as a soho list view and this list view has a cancel/delete button as well.". This seems like something entirely new as i've never seen a lookup with a listview.

But anyways this issue is already very confusing so lets start with a new issue. If its small i'd like to save it for later

@JangleShubham
Copy link

JangleShubham commented Jul 26, 2021

Hello @tmcconechy ,

In short we just want a way to unselect the record without opening lookup popup.
We are removing a record we want to remove from list this.previouslySelectedRows and also from model.desc, but still after opening model popup those records shows selected.

We are using below code to show selected records in afterpaging event:

e.lookup.grid.unSelectAllRows(true);
for (const selectedRow of this.previouslySelectedRows) {
  e.lookup.selectRowByValue('id', selectedRow.data.id);
}

I think we should we go with same issue as we can't go ahead with this issue.

Thanks,
Shubham Jangle

@tmcconechy
Copy link
Member

tmcconechy commented Jul 26, 2021

OK whats the part about listview? that's what is confusing. Can you modify the existing example in the repo so we can try what your doing? So we can try what your talking about / trying to do?

https://main-enterprise.demo.design.infor.com/components/lookup/test-multiselect-description-only.html

From what you say about "unselect the record without opening lookup popup". This might be difficult as the lookup is managing the state as well.
This seems like adding new requirements and this feature was already difficult and non standard. Is this anything you can skip for now? Whats the use case?

@deep7102
Copy link
Contributor

Hi @JangleShubham I think instead of use that previouslySelectedRows[], it should sync with lookupApi.selectedRows[]. Because lookup modal take selected rows from lookupApi.selectedRows[] array before open the modal.

JangleShubham added a commit to JangleShubham/enterprise-ng that referenced this issue Jul 26, 2021
lookup unselect after clicking on cancel/delete button reproduced for issue: infor-design#986
JangleShubham added a commit to JangleShubham/enterprise-ng that referenced this issue Jul 26, 2021
lookup unselect after clicking on cancel/delete button reproduced for issue: infor-design#986
@JangleShubham
Copy link

Hello @tmcconechy / @deep7102 ,
We have forked the code and added our code in https://github.com/JangleShubham/enterprise-ng, in file src/app/lookup/lookup-desc.demo.ts and src/app/lookup/lookup-desc.demo.html.
We are unselecting/removing record in method delSelectedTree
Could you please tell how do we use lookup API there?

@dhumalkishor can you explain use case for this?

Thanks,
Shubham Jangle

@tmcconechy
Copy link
Member

Thanks first did you try @deep7102 answer. That actually sounds like it could work. #986 (comment)

@JangleShubham
Copy link

Hello @tmcconechy and @deep7102 ,
Thanks for the solution. Solution suggested is seems to be working, we are in process of integrating it in GRC now.
Thanks for your help.
Regards,
Shubham Jangle

@tmcconechy
Copy link
Member

tmcconechy commented Aug 3, 2021

Your welcome, we're glad it works now 😓 👍🏻

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment