Allow to filter by keywords #6183
Conversation
moliholy
force-pushed
the
keywords
branch
5 times, most recently
from
53c2e8e to
238d101
Compare
|
I'm merging #6177 now so maybe see if you can use that for the dropdown field |
@ThiefMaster this PR is now using the new widget! 
|
|
|
||
| def _get_event_data(self, event_query): | ||
| def calculate_keyword_id(kw): | ||
| return crc32(kw) + 3 |
There was a problem hiding this comment.
I think the + 3 here deserves a comment - at least I have no clue after reading over the code (up until this line) why this is needed.
There was a problem hiding this comment.
It's in order not to collide with IDs 0 and 1, which are reserved. I'll add a comment.
There was a problem hiding this comment.
I think getting a CRC32 that's 0 or 1 is quite uncommon (but apparently not impossible, even though the examples I found on Stack Overflow did not yield 0 for me).
Do those IDs really need to be numeric though? If not we could even use the keyword itself for the keywords...
There was a problem hiding this comment.
Well, they are numerics (category ID, venue ID, room ID, etc.) so for consistency I'd definitely prefer it to be numeric. As for the +3, it's just to be 100% covered against collisions.
There was a problem hiding this comment.
crc32 by itself is not collision-proof :p
There was a problem hiding this comment.
No hashing algorithm is 100% collision proof, but it should be at least extremely unlikely to have a collision. In any case, what do you suggest we should do here? I don't have an strong opinion here, but I'd definitely prefer to keep the ID as an integer for consistency.
There was a problem hiding this comment.
I don't have a super strong opinion either, but if there's nothing that really depends on them being numeric it feels much nicer to be able to just use the strings. Also if you ever need to debug something and look at the actual data...
There was a problem hiding this comment.
Unless there's a strong reason (and inspecting the IDs is definitely not one of them, at least for me) to change the ID from integer to string my vote goes straightforwardly for maintaining it as a number. How that number gets generated I don't care though.
ThiefMaster
force-pushed
the
keywords
branch
from
cfd9f4b to
4f6ef8e
Compare
|
I think there's a bug somewhere in the keyword filtering logic. I have an event with the keywords
I think a filter for "multiple keywords" (clearer than "many keywords" IMHO) makes sense in case you want to see which events have multiple keywords. But this should not be a requirements to see such events at all: When I filter by a single keywords, I'd expect to see all events that contain this keyword! |
moliholy
force-pushed
the
keywords
branch
2 times, most recently
from
4768919 to
ae7c5ad
Compare
@ThiefMaster I just pushed a commit that I think implements the behaviour you describe. Would you mind to check it out? |
ThiefMaster
force-pushed
the
keywords
branch
from
3663208 to
689d95c
Compare
|
works fine now! :) |
Error only shows when someone messes around with the submitted data, no need to have people translate that
This broke when I was fixing a problem w/ the keywords field
2 participants
Closes #6127
Description
This PR adds the possibility to filter in the calendar by keywords.
Keywords can now be registered system-wise so that there is a limited amount of allowed one. If there is no system-wide allowed keywords then the option to filter by keyword is not shown.
Also when editing a booking's keywords different widgets are used to limit the possibilities if needed. Otherwise the old widget is used.
Screenshots