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

JavaScript: support optional byKeyUrl #417

Open
statler opened this issue Mar 4, 2020 · 5 comments
Open

JavaScript: support optional byKeyUrl #417

statler opened this issue Mar 4, 2020 · 5 comments

Comments

@statler
Copy link

statler commented Mar 4, 2020

Aleksey

Further to my PR and previous issue #150...

Having a separate byKeyUrl would be useful. The case you gave as achieving this result actually does not work in the case which necessitated me implementing this function.

The most common case I have for this is to do with users. I have several methods that return a subset of the user base. For example OrderApprovalUsers which is a subset of Users returned by users/GetOrderApprovalUsers.

When an Order is raised, the current list of approved users is returned for the selectbox, a user is selected e.g. user id 99. Lets say later that user leaves the business, or their authority is revoked - they are no longer returned by the users/GetOrderApprovalUsers controller method. As a result, no call to users/GetOrderApprovalUsers with userid:99 will ever return a value.

This means whenever I display this record, the ApprovedBy field is blank - even though I have data to complete it.

If however a separate byKeyUrl such as I have implemented can be specified, then the selectbox can call users/GetUserById?UserId=99 without impacting any other functionality. By default, the ByKey method uses the LoadUrl anyway, so all that is required is an additional property ByKeyUrl that is only actually used if it is specified. It is a very small change with a very powerful additional functionality.

Would you please reconsider?

@AlekseyMartynov
Copy link
Contributor

Hello
You're describing a common case when

  • For display, you need current collection + previous assignment
  • For edit, you need only the current collection

There can be different ways to achieve this. For example, by using two different data sources. However, I don't think that the following would be correct:

  • collection.ByKey(key) is not empty
  • collection.Where(i => Key(i) == key) is empty

@statler
Copy link
Author

statler commented Mar 4, 2020

I disagree. For display you only require previous assignment. If the previous assignment is not a member of the current collection, then it is no longer a valid selection, and the only reason it would be changed would be to make a valid selection - otherwise you would leave it as it is. Regardless, you describe this as a common problem, but provide no solution that works with the controls except to provide a heap of additional plumbing on the server to modify the datasource.

To support a separate byKeyUrl requires a change to 1 line of code. Compare this to having to provide additional datasources for every single time this happens (at least 30 times in my codebase) and it really isn't even close. That is why I have actually implemented it - I would prefer not to have to maintain a separate fork though.

The additional advantage is where I have collections that require significant computational power. The majority of calls to the datasource are simply getting the key, and this can be made trivial where required if I can use a separate byKeyUrl

This change is completely backwards compatible - I am not referring to the AllUrl in my original PR (for which your previous supplied workaround works well), just the ability to specify a separate byKeyURL which defaults to the loadURL if not specified.

Like you point out, there are other solutions which require a lot more code and a lot more effort, so what exactly would be the problem with doing this? It literally just requires to replace

loadUrl = options.loadUrl

with

loadUrl = options.byKeyUrl || options.loadUrl

and add byKeyUrl to the typescript definition.

@AlekseyMartynov
Copy link
Contributor

For display you only require previous assignment. If the previous assignment is not a member of the current collection, then it is no longer a valid selection, and the only reason it would be changed would be to make a valid selection - otherwise you would leave it as it is.

I see that this is how your app handles business rules. Another example that comes to mind is my Spotify profile page when I view it from a non-domestic location. The UI displays a dropdown list of two countries, and I am only allowed to save with the current location.

I think that I fully understand your arguments. Indeed, byKeyUrl might simplify integration with DevExtreme widgets for the specific scenario. The point regarding heavy collections is also valid. Though, I'm not sure whether all widgets can correctly handle a data source whose byKey returns an item that is not returned by load.

We can keep the ticket open for further research.

@statler
Copy link
Author

statler commented Mar 4, 2020

Thanks Aleksey

How would we go about researching / progressing the potential impact on other controls? Reason I ask is that it is important to my application, and now also teams I work with. At the moment they use the standard devex asp.net js library, but having come across exactly this issue we are going to probably have to implement my fork.

Now that we have managed to resolve the Dto issue using customaccessors, and the AllUrl suggestion from #150 works well, this is the only difference still remaining in that fork, and it would be really great to be able to return to the main repo.

@AlekseyMartynov
Copy link
Contributor

How would we go about researching / progressing the potential impact on other controls?

As usual, we cannot promise whether / when the suggested idea will be implemented. If you find that a certain DevExpress widget doesn't work well with a key that is not present among items, submit a Support ticket.

Consider also this approach which doesn't require a fork:

var store = DevExpress.data.AspNet.createStore(...);

store._byKeyFunc = function(key) {
    return $.getJSON("@Url.Action("CustomAction", "ApiController")", { key: key });
}

store.byKey(123).then(function(obj) {
    debugger
});

Similarly, you can override _loadFunc, _totalCountFunc, _insertFunc, _updateFunc, _removeFunc.

@AlekseyMartynov AlekseyMartynov changed the title Reconsider adding ByKeyUrl JavaScript: support optional byKeyUrl Mar 5, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants