List constraint logic

[LakshSingla]

@AdrianBZG I have added the list filter which is functioning properly, although I have not tested extensively, specially with user defined lists. Here are some of my concerns:

• I have made such that only one list can be chosen as a constraint. I had earlier made it for multiple lists, but I have changed it. Lemme know is it for a single list or can multiple lists be added as constraints (in the latter, how to specify those multiple lists). Depending upon it, I would need to remove a few dead code.
• I have modified the positioning of Apply button from the screenshot in the issue, because of UX. One need not scroll to the bottom, to click on Apply. Also I have removed Close because it is the default behavior of the modal.

I may need to clean up a few bits of code, apart from that it is ready from my side. Lemme know if you want any changes to be made :)

Refers #27

[AdrianBZG]

@LakshSingla Nice work!

I think we need to make it possible to filter by multiple lists. To do so, in the IN constraint, you just need to pass comma-separated lists names. For instance: "ListA,ListB", just as is done for the GO annotation filter. The constraint should be a ONE OF. Does that sound all right for you? 😄

About the Close button, do you mean you have removed it from the modal? If so, I think we prefer to keep it, as for UX is better.

[LakshSingla]

@AdrianBZG I have made the required changes except for the updateTableWithConstraints() part.

I have tried following queries, but they do not seem to work:

// Using operator ONE OF and array of lists as values
"path": sessionStorage.getItem('currentClassView'),
"op": "ONE OF",
"values": window.imTableConstraint["savedLists"],
"code": "A"

// Using operator ONE OF and array of lists as comma seperated list
"path": sessionStorage.getItem('currentClassView'),
"op": "ONE OF",
"values": window.imTableConstraint["savedLists"].join(''),
"code": "A"

// Using operator ONE OF and array of lists as comma seperated list as value
"path": sessionStorage.getItem('currentClassView'),
"op": "IN",
"value": window.imTableConstraint["savedLists"].join(''),
"code": "A"

It is not supporting "ONE OF" operator and while using "IN", it requires a single list name to be entered as value parameter.

What seems to be working is

// Using operator ONE OF and array of lists as comma seperated list as value
"path": sessionStorage.getItem('currentClassView'),
"op": "IN",
"value": window.imTableConstraint["savedLists"], 
"code": "A"
//Where only 1 list is present in the array,
//In case where more than one list is added, following error is thrown: 

I am pushing the code and highlighting the location where constraint logic is added. It would be helpful if you take a look 😸 LIST CONSTRAINT ON IMTABLE(L263-270)

According to me "ONE OF" as an operator and "VALUES" as constraint information are not supported.
Only "IN" as an operator and "VALUE" as a constraint information is supported by the query.

[AdrianBZG]

@LakshSingla It's looking great! A few comments:

• When I choose more than 1 list and press Apply, I get a "Oops! - Sorry we cannot get that table for you." error. I suppose is due to the fact that the constraint for multiple lists is not working. So there are two options:

1. Only make it possible to choose 1 list, hence allowing only to filter with 1 list (easy way)

or

2. Format properly the constraint with multiple lists; see Yo code in Discord for multiple list filtering (hard way)

What do you think?

[LakshSingla]

@AdrianBZG I just realized that if a user selects 2 lists, maybe we should limit ourselves to only those results which is present in both the lists. This is in accordance with the rest of the app when user adds more than one constraint. For example if someone enters Pathway Name: Signal Transduction and Phenotype: Edema, only results satisfying both queries are selected.
But still as the View Saved Lists options is different from rest of the filters, I cannot comment on how the filter should behave on multiple lists.

What are your opinions about it? As I am unfamiliar with the usage pattern of queries like these (in an actual scenario), you make the call whether only queries passing both lists be displayed or any one be displayed ... 😃

[AdrianBZG]

@LakshSingla So constraints in the same filter are trated as ORs, whereas constraints across different filters are treated as ANDs. So in this case since it's a constraint in same filter, we want to display results from both lists. Does that make sense for you?

[LakshSingla]

@AdrianBZG The following error is thrown while trying the above way. I have displayed the query alongside the result:

[AdrianBZG]

@LakshSingla What line is giving this error in the code (not in the min version, as it's only 1 very big line)?

[LakshSingla]

var minimumValue = result['results'][0]['min'];

This line of code L1669 in common.js. Although, this implies that the resulting query has no row as a result, which means that the logic is not applied correctly (because individual lists had many rows satisfying it).

[LakshSingla]

@LakshSingla It's looking great! A few comments:

* When I choose more than 1 list and press Apply, I get a "Oops! - Sorry we cannot get that table for you." error. I suppose is due to the fact that the constraint for multiple lists is not working. So there are two options:


1. Only make it possible to choose 1 list, hence allowing only to filter with 1 list (easy way)

or

1. Format properly the constraint with multiple lists; see Yo code in Discord for multiple list filtering (hard way)

What do you think?

@AdrianBZG I think that choosing only one list would be the way to go. I am trying to union multiple lists but to no avail. In the original mine's query interface for lists, when I try to take union of 2 lists, it asks me to build another, rather than directly showing the result. What do you say ? :)

[LakshSingla]

@yochannah With respect to your comment on Discord, I tried the solution but it seems to be failing. Can you please suggest any other way to add multiple list constraints if possible ::))

Tagged below are the relevant parts of the conversation related to this issue.

@AdrianBZG The following error is thrown while trying the above way. I have displayed the query alongside the result:

var minimumValue = result['results'][0]['min'];

This line of code L1669 in common.js. Although, this implies that the resulting query has no row as a result, which means that the logic is not applied correctly (because individual lists had many rows satisfying it).

@LakshSingla It's looking great! A few comments:

* When I choose more than 1 list and press Apply, I get a "Oops! - Sorry we cannot get that table for you." error. I suppose is due to the fact that the constraint for multiple lists is not working. So there are two options:


1. Only make it possible to choose 1 list, hence allowing only to filter with 1 list (easy way)

or

1. Format properly the constraint with multiple lists; see Yo code in Discord for multiple list filtering (hard way)

What do you think?

@AdrianBZG I think that choosing only one list would be the way to go. I am trying to union multiple lists but to no avail. In the original mine's query interface for lists, when I try to take union of 2 lists, it asks me to build another, rather than directly showing the result. What do you say ? :)

[AdrianBZG]

@LakshSingla Maybe it's useful to print the query object associated with the im-table, to see if there is some error in the formating of the constraints? Or maybe we can just take into account 1 list, instead of multiple, what do you think @yochannah ?

[LakshSingla]

@AdrianBZG I have displayed the query object in one of my comments above.

@AdrianBZG The following error is thrown while trying the above way. I have displayed the query alongside the result:

[LakshSingla]

@AdrianBZG @yochannah After a bit of digging I found this issue: intermine/imjs#18
It seems that we cannot set the constraint logic after loading the table although I am not entirely sure of the im-tables part.
I think we should roll out this feature for a single list only, while the imjs issue gets resolved. I am planning to read a little bit up on CoffeeScript to contribute to imjs's issues.
What are your opinions on it ?

[AdrianBZG]

@LakshSingla We are good with single list for the moment, let's leave multiple lists as a future feature. So let's make sure the user can only select one list in the modal, how does that sound for you?

[LakshSingla]

Alright !! I will revert the necessary changes by tomorrow and create a separate issue regarding multiple selection of lists :)

[AdrianBZG]

@LakshSingla Sounds great to me!

[LakshSingla]

@AdrianBZG Please review the PR now !! I believe it is free of any bugs and now it implements single list save logic. I will open the issue regarding multiple lists (referencing this issue) after your review.

[AdrianBZG]

@LakshSingla Looks good! Just three little things left:

1. Change "View Saved Lists" to "Lists", as it is shorter.
2. Change the icon of the button to something characteristic, like fa-list (https://fontawesome.com/icons/list?style=solid)
3. Add a third button in the modal, that says "Reset", and that removes the list constraint. Just in the middle between Apply and Close is ok... maybe same color as Reset button in the left side filters? (see the Expression one)

[LakshSingla]

@AdrianBZG I have made the required changes 😄 😄

[AdrianBZG]

@LakshSingla Wonderful! Just change the text color of the Reset buton to white, as they are all white, and it will be ready to merge.

[LakshSingla]

@AdrianBZG Missed out that one 😅 Now its fixed :) Added that change to the previous commit only.

[AdrianBZG]

@LakshSingla Nice work!