search

status-im / open-bounty

Enable communities to distribute funds to push their cause forward.
https://openbounty.status.im/
GNU Affero General Public License v3.0
118 stars 36 forks source link

[FIX #355] persist bounty filter to query string #373

Closed msuess closed 6 years ago

msuess commented 6 years ago

fixes #355

Persist bounty filters to query string using bide query params as described in #355.

This needs additional testing, as I could not get the github integration to work locally yet.

The filter UI works as expected, but without actual bounties in the DB it's hard to tell if it really works.

@martinklepsch do you think you could take a look at this?

msuess commented 6 years ago

Thanks for your feedback @martinklepsch, I'll be looking into this tonight!

msuess commented 6 years ago

@martinklepsch I fixed the stuff you commented on. Regarding calling nav! in the UI components: I went down this path for a little while but hit a road block: in order to add to the query string, one needs to have access to all the current filters when calling nav!. This is actually one of the reasons that made me move this logic into an fx handler, now that I think of it. Do you have any ideas as to how to solve this?

martinklepsch commented 6 years ago

Could it be that some changes went missing here? Particularly the changes in the UI code seemingly got lost?

msuess commented 6 years ago

@martinklepsch there are no changes in the UI code, since the call to nav! is still in the handlers.

martinklepsch commented 6 years ago

@msuess I see what you mean. Did not anticipate that properly, sorry for leading you into that 🙂 Dabbling with this for some time lead me to some other ideas to improve this code but we probably shouldn't be tackling these as part of this PR/bounty. Some notes below just to keep record.

Basically we're duplicating the information in the query params in (::db/open-bounties-filters db). Duplication like this is problematic because different parts of the app might rely on this duplication, causing pain when refactoring.

Idea for a solution

Ultimately that key ::db/open-bounties-filters in the DB should be replaced by a subscription which derives the same information from the query params. These are currently not stored in the DB but you could imagine that the [page params query] tuple is stored in the DB (e.g. under a :route key). This would be trivial to add to :set-active-page.

The subscription would then also be responsible for the transformation of that data into the format the UI components expect (i.e. ui-model/query-param->bounty-filter-type).

::set-open-bounty-filter-type (which gets db as arg) would simply extract the current routing information from the DB (query params) and call nav! with the updated query params (basic assoc).

As you rightfully noted it's problematic to call nav! in the components themselves because that requires them to have a full understanding of the current routing state.

Don't worry if this doesn't make sense right now, jotting this down in a bit of a rush. 😅

msuess commented 6 years ago

@martinklepsch fixed the issue with the currencies filter. the problem was that the value for ETH was a string and the other token-ids are keywords. I changed the value for ETH to a keyword as well, let me know if you prefer strings for everything :)

martinklepsch commented 6 years ago

Just played with this again and noticed that the owner selection now has a problem:

msuess commented 6 years ago

@martinklepsch sorry, I should have catched that. Will fix.

martinklepsch commented 6 years ago

No worries & thanks! 👍

churik commented 6 years ago

@msuess thanks for your contribution!

I addition to last @martinklepsch comment: when applying the filter by the Owner, the bounties are not filtered (always 'No matching bounties found.')

Video: http://take.ms/PwIxp Please, ping me when PR will be ready.

msuess commented 6 years ago

@martinklepsch @churik fixed the issue when filtering by owner.

churik commented 6 years ago

Thanks @msuess!

Environments

Tested functionality