NCI-Agency / anet

Advisor Network
MIT License
24 stars 11 forks source link

Epic 2293 - 'Nice to have' issues #2293

Open jose-west opened 5 years ago

jose-west commented 5 years ago

Originally posted by Vassil on Slack on the 4th and 6th of October 2019

I am at the training event. A few "nice to have" low hanging fruits:

Some other observations that require a bit more thought:

Related to the rollup usability:

Related to the search functionality:

gjvoosten commented 4 years ago
  • [ ] Searching with not filled filters
  • I search on reports, add the author filter, but don't fill this in
  • I click search => get lots of results
  • I do save search
  • I go to homepage, select the just saved search and under the saved searches I see the results preview for it => as expected
  • I click on show search => goes to the search page and displays "You did not enter any search criteria" => I would expect to have the same search results

@vassil This one is caused by JSON.stringify({authorUuid: undefined}) returning {} because JSON doesn't have undefined. A solution might be to call: JSON.stringify(obj, (k, v) => v === undefined ? null : v) (change undefined values into null), but we need to investigate if that has any side effects.

@vassil Edit: yes, it does. Search filter deserialization currently does at some point: if (query[queryKey]) { which means null filter values get ignored. We could naively change that to something like if (queryKey in query) {, but that will eventually fail because the AdvancedSelectFilter deserialization will make a GraphQL request for a Person with uuid: null, which fails because the uuid parameter is non-nullable ("graphql.execution.NonNullableValueCoercedAsNullException: Field 'uuid' of variable 'uuid' has coerced Null value for NonNull type 'String!'"). You might think that changing that to be nullable would help, but then PersonResource::getByUuid will throw new WebApplicationException("Person not found", Status.NOT_FOUND). You're in a maze of twisty little passages…