[WIP]model/messages/api_types: alert_words mark up

[Subhasish-Behera]

What does this PR do?

Partial fix for #588

Tested?

• Manually
• Existing tests (adapted, if necessary)
• New tests added (for any new behavior)
• Passed linting & tests (each commit)

Commit flow

first commit:Fetches alert words and registers for any alert_word event and updates the alerts words.
second commit: adds transform_content_alert_words which adds <span class=alert-word> around alert-words

Notes

-> This PR(specifically the second commit) is based on the alert word implementation of the Zulip web client which was later adapted by other clients.
link for the web implementation code: link
-> Other helpful documentations for contributors:
1. regex wrt python
2. re module
-> Whats Done?

1. fetching and registering of alert_words
2. finding the alert_word patterns and applying the

-> What needs to be done?

1. Even though applying span logic is ready but the logic for applying markup for alert words inside of multiple elements is not ready. So basically the markup works for the alert-words without any other tags indie soup2markup. Before that we have to decide whether we want to apply different markup when multiple tags are present for a alert_word or we have to choose the markup from any one of them.

2. Currently the markup applied for alert-words is same as code tag i.e msg_code as the markup for different zt_themes are yet to be discussed.

3. Tests are not added as the non-test code is yet to reach a stable state.

[neiljp]

@Subhasish-Behera Thanks for looking at this other piece of the alert words puzzle 👍

I've focused on the first commit, since we need the alert words first in order to make progress, at least in the application itself.

It'd be good to split the first commit into two parts:

• getting the initial data
• updating the data

While forgetting to include the updating wouldn't be great (and we have that with other data right now), alert words aren't often updated, so you should be able to manually test a new first commit easily - and even the highlighting if you wanted. That first commit could be merged sooner on it's own once it looks good.

Please note my point about the alert words being 'private' to the model; while other parts of the code simply access inside the model, ideally we want to use a method for this (and migrate the others too). That would sit in the new first commit, and could form part of a test that the data from the server comes out via that function. For that, I'd suggest you check out the API link I gave, as well as the fixtures in tests/conftest.py.

The updating part does look very simple, and in a data sense it should be, based on the event data, but note that a change in the data should affect how messages are rendered. That is, not just ones that are constructed based on the message content and alert words at the time, but they need updating too. We could ignore that initially, but we shouldn't forget it :)

As a final quick point, if you've not, I'd suggest installing gitlint as per the README. It should complain about your long commit title ;)
That section should also give you pointers on the general format.
If you find you need to say too much, or include too many files, that can indicate the commit can be split to be simpler.

[neiljp]

@Subhasish-Behera Let's still address the first commit first, but I just took a look at the second commit as it stands.

I can see how this mirrors web-app code, but we should test it ourselves too, to ensure that it works how we expect in this translated form.

If we can demonstrate this manually and with tests, this should be a good v1 implementation of this feature.

Beyond that, if we have more tests, and can compare what the web app highlights (and tests), we should be able to determine whether we can do the same using this current approach. This will affect if we need to think differently to cover more cases.

We may want to consider an alternative way of formatting this kind of text, and maybe search text too, if it crosses tag/style boundaries.

I expect the web app applies tags dynamically since it uses CSS to style the text, and I'm guessing the spans can overlap? That's complex with urwid - we process the HTML into urwid-styled Text instead.

If so, we might end up considering processing the content into text only, finding matches, then mapping those into adjacent pieces in the HTML or urwid-processed text.

However, let's focus on testing this v1 first :)
(and the v0, ie. the data handling, before that!)

[neiljp]

These new parameters could be combined into one. If the message doesn't have alert words flagged, then the function doesn't need to know/process them.

[neiljp]

If this is translated from a server implementation, it would be useful to add a comment to note that, and in the commit text too.

That could refer to the file as it is now, but also a commit ref, so that if code changes later then it is easier to track what has changed in the original code.

Having this function in a commit on it's own first, with detailed tests to cover various cases, would be good before we hook it into the processing in a subsequent commit.

[Subhasish-Behera]

Yeah, that would be great. will do it.
But it's tricky to translate the code changes of js to python directly in this case because of the difference in libraries. In js, the lodash library uses _.escapeRegExp() but in python we have to use re.compile for creating objects and then we call the built-in functions like object.search() etc

[Subhasish-Behera]

@zulipbot add "PR needs review"

[neiljp]

@Subhasish-Behera I focused on the first 3 commits, since I think that was the recent focus?

The current overall PR fails when I run it right now, at line 843 of messages.py, compiling a regex (multiple repeat at position 27).

Otherwise, improving your commit text would improve the speed of merging, and the first three seem close, if needing some adjustments.

[neiljp]

Is this always true?

[Subhasish-Behera]

Yep, this is always true. But it is more context-dependent. Basically, currently, this test is designed to test the most minimal of changes in the active_conversation_info i.e change(start/stop) of a single user. The inital_alerted_words means just before that change and not at the start of the whole typing event series.
So it is true because of the values that are passed in mark.parametrize i.e it is an approach followed in the test.

[neiljp]

If that is true, then it is good to add a suffix to the test function name to indicate that.

[Subhasish-Behera]

I think the name test__handle_alert_words_event_minimal_change would do the job.

[neiljp]

Hmm. Do we get an event if the changes aren't made?

I'd suggest a double underscore from the function name to any 'meaning'.

[Subhasish-Behera]

I believe by changes you mean no typing in the writebox(of any client).
That would be a event with op=stop. So yes.

[Subhasish-Behera]

test__handle_alert_words_event_minimal_change sounds bit subjective,so i will be changing it to test__handle_alert_words_event__single_user. Though it's natural as a single event can't inform about more than 1 user in this case.

[neiljp]

I didn't comment on this before, but this function placement in the file seems strange too - between two related functions?

[Subhasish-Behera]

will fix it asap

[neiljp]

Note that strictly this could go into a separate commit with a test to eg. ensure that one can't modify the data externally, but this is fine.
(also to indicate it returns the expected data, as a minimal test)

[theViz343]

@Subhasish-Behera Thanks for working on this 👍! This seems to work well. There are a few cases I found during testing where it seems to be a bit unpredictable.

• \something. Prefixing with a backslash seems to not get picked up by the regex if the alert word is \something. If the alert word is just something, it gets picked up when it shouldn't (according to the behavior on the webapp).
• wqdnasj\ Suffixing with a backslash when the alert word is also wqdnasj\ seems to crash ZT with the following error - re.error: missing ), unterminated subpattern at position 23.

Apart from this, I've left some suggestions for minor code changes in the comments.

[theViz343]

Regarding the typing of match, Union[str, str, str] results in just Union[str] so you could shorten it. Also, a match object's type is generally Union[Match[str], None] for the case that there is no match, which could be in our case because not every alert word is guaranteed to be present.

[Subhasish-Behera]

agreed upto Match[Union[str]] . I agree that the Match object can be None when the pattern is not found but we know from the flag that an alert word is present. Re the case that not every alert word is present, the code iterates over every alert word as configured by the User and then puts span if it identifies that particular word in the content(so in every time one alert word is used to create the pattern). so None doesn't seem to be a possible value in our case.

Also, if I add None in typing of match , the errors shown is

 error: Item "None" of "Optional[Match[str]]" has no attribute "group"  [union-attr]
          Item "None" of "Optional[Match[str]]" has no attribute "start" 

[theViz343]

Minor: You could use match.group(1,2,3) here, which returns a tuple consisting of the three values.

[Subhasish-Behera]

The only use these 3 have is in here:

 if in_tag:
                return f"{before}{word}{after}"
 return f"{before}<span class='alert-word'>{word}</span>{after}"

I could have directly used match. group(n) but didn't. I think separating them into 3 variables gives better readability
By following this suggestion of match.group(1,2,3), I need it to be stored in some var and break it using the index to use it in the above code which will not clarify any further of what is being done.
So why group it when it can be accessed individually without creating any tuple

[neiljp]

If I follow the above correctly, would it help if you used tuple unpacking?

one, two, three = function_returning_tuple_of three_values()

[Subhasish-Behera]

Yep! Multiple assignments in one line can do the same thing.

before,word,after = match.group(1,2,3)

[neiljp]

@Subhasish-Behera Is this awaiting an update from you, or have you addressed the feedback?
Just checking based on my earlier feedback and that from Vishwesh :)
(If so, you can add a 👍 emoji or a comment to specific comments to indicate this)