Stream info view #1127
Stream info view #1127
Conversation
|
@kingjuno The second commit seems unrelated to the title of the PR. Could you please extract that commit into a new PR? |
There was a problem hiding this comment.
@kingjuno Thanks for working on this. This is a nice addition to our StreamInfo view and it works fine. 👍
I've mostly focused on the code for this review and below are a few points for the first iteration. When working on features that leverage a new field from API responses, it's always important to check if that field was added sometimes later in the server, in which case, you'd need to gracefully handle it for older server versions too. Other than that, rest are minor points, and considering this is your first PR in this repo, Great work! 🎉
zulipterminal/ui_tools/views.py
Outdated
| stream_posting_policy = { | ||
| 1: "Any user can post", | ||
| 2: "Only administrators can post", | ||
| 3: "Only full members can post", | ||
| 4: "Only moderators can post", | ||
| } | ||
| posting_policy = stream_posting_policy[stream["stream_post_policy"]] |
There was a problem hiding this comment.
As per API docs, the stream_post_policy field was added in Server version 3.0; there's no feature level mentioned there, so you can either query that in the #api-design stream in chat.zulip.org or you could just check if the field is absent so use the older is_announcement_only field.
There was a problem hiding this comment.
@kingjuno This looks good - thanks for giving this a go 👍
As @zee-bit mentions, some of these fields are dependent on the server version, so may fail badly depending on which server one uses (we claim to support 2.1+ if you check the README). Due to that you may want to limit this first PR to those which are fully backwards compatible to that point, to avoid needing to get into version/feature detection initially.
For tests, as I note, you've made the existing tests (and parts of tests - "case"s) pass, but there are not yet new ones which cover the range of new values which are being output. It's true that the existing test only examines the height, which doesn't give much detail, but perhaps we can improve upon that.
zulipterminal/ui_tools/views.py
Outdated
| stream_posting_policy = { | ||
| 1: "Any user can post", | ||
| 2: "Only administrators can post", | ||
| 3: "Only full members can post", | ||
| 4: "Only moderators can post", | ||
| } |
There was a problem hiding this comment.
We may want to extract this into ui_mappings.py when we add this feature - which is dependent upon server version.
tests/conftest.py
Outdated
| "stream_post_policy": 1, | ||
| "history_public_to_subscribers": True, | ||
| "role": 20, |
There was a problem hiding this comment.
This updates the data for the general stream, but not for other similar test data - which we likely should do too.
neiljp
added
area: UI
kingjuno
force-pushed
the
StreamInfo_view
branch
3 times, most recently
from
0fc646b
to
a707463
Compare
zulipbot
added
size: XL
kingjuno
force-pushed
the
StreamInfo_view
branch
from
1f90ddc
to
a707463
Compare
zulipbot
added
size: M
kingjuno
force-pushed
the
StreamInfo_view
branch
2 times, most recently
from
1e88d7a
to
8ee1e89
Compare
kingjuno
force-pushed
the
StreamInfo_view
branch
from
8ee1e89
to
c5017fa
Compare
|
@kingjuno Thanks for working on this 👍 I just made some very slight alterations to your commit and merged it manually as 4509d26 🎉 You're welcome to continue with the other aspects we discussed and that you started including here. |
|
Heads up @kingjuno, we just merged some commits that conflict with the changes your made in this pull request! You can review this repository's recent commits to see where the conflicts occur. Please rebase your feature branch against the |
What does this PR do?
Include more elements to StreamInfo View like :
Also fixed convert-unicode-emoji-data from displaying wrong file name when
zulipterminal.unicode_emoji_dictisn't downloaded.Tested?