Added more Locales and Currencies. Made som fixes for ProductValueDat…#18
Added more Locales and Currencies. Made som fixes for ProductValueDat…#18objecta wants to merge 1 commit into
Conversation
…aConverter, when more than one currency and some of them are null
pardahlman
left a comment
pardahlman
left a comment
There was a problem hiding this comment.
Thanks a bunch for this PR! I have reviewed your changes and added some comments. I am interested to hear your thoughts.
In order to easier follow the changes made, feel free to create multiple PRs for the different fixes you have done.
Thanks again!
| Value = date | ||
| }; | ||
| Value = date.ToString("yyyy-MM-dd HH:mm:ss") | ||
| }; |
| Value = date | ||
| }; | ||
| Value = date.ToString("yyyy-MM-dd HH:mm:ss") | ||
| }; |
| Value = date | ||
| }; | ||
| Value = date.ToString("yyyy-MM-dd HH:mm:ss") | ||
| }; |
| { | ||
| Operator = Operators.Equal, | ||
| Value = date | ||
| Value = date.ToString("yyyy-MM-dd HH:mm:ss") |
There was a problem hiding this comment.
I'm a bit hesitant to add a call to ToString here. Is this date format accepted throughout the Akeneo API? If so, we should update the AkeneoSerializerSettings to set DateFormatHandling accordingly.
| return FilterAsync<TModel>(queryString, limit, ct); | ||
| } | ||
|
|
||
| public async Task<PaginationResult<TModel>> FilterAsync<TModel>(string queryString, int limit = 10, CancellationToken ct = default(CancellationToken)) where TModel : ModelBase |
There was a problem hiding this comment.
Just thinking out load here... the signature for FilterAsync has a string argument for queryString, and this overload also accepts a limit that is combined with the queryString... I think this overload is redundant, as the client could create the &limiy= query string herself. Thoughts?
| } | ||
|
|
||
| public async Task<PaginationResult<TModel>> FilterAsync<TModel>(string queryString, CancellationToken ct = default(CancellationToken)) where TModel : ModelBase | ||
| public Task<PaginationResult<TModel>> SearchAsync<TModel>(IEnumerable<Criteria> criterias, int limit = 10, CancellationToken ct = default(CancellationToken)) where TModel : ModelBase |
There was a problem hiding this comment.
I think it makes sense with a limit parameter here. But instead of creating a new method, we should extend the existing one and have a check like
if(limit > 0)
{
// append limit to query string
}2 participants
The ProductValueDataConverter is failing when there are more then one currency and one or more of these currencies are null. Sugesting a fix that I have tested and used in my nopCommerce Integration plugin.
Also I have added all Locals and Currencies and made a fix for the dates string used in search. They were failing in my setup.