zulip-terminal icon indicating copy to clipboard operation
zulip-terminal copied to clipboard

Update email_address to be fetched from new API

Open rsashank opened this issue 1 year ago • 3 comments

What does this PR do, and why?

Added helper function to send request and get stream email_address, and called the helper function when user is viewing Stream Information.

External discussion & connections

  • [x] Discussed in #zulip-terminal in topic https://chat.zulip.org/#narrow/stream/206-zulip-terminal/topic/Stream.20Information.20ZT.20Crash
  • [ ] Fully fixes #
  • [ ] Partially fixes issue #
  • [ ] Builds upon previous unmerged work in PR #
  • [ ] Is a follow-up to work in PR #
  • [ ] Requires merge of PR #
  • [ ] Merge will enable work on #

How did you test this?

  • [ ] Manually - Behavioral changes
  • [ ] Manually - Visual changes
  • [ ] Adapting existing automated tests
  • [ ] Adding automated tests for new behavior (or missing tests)
  • [ ] Existing automated tests should already cover this (only a refactor of tested code)

Self-review checklist for each commit

  • [x] It is a minimal coherent idea
  • [ ] It has a commit summary following the documented style (title & body)
  • [ ] It has a commit summary describing the motivation and reasoning for the change
  • [ ] It individually passes linting and tests
  • [ ] It contains test additions for any new behavior
  • [ ] It flows clearly from a previous branch commit, and/or prepares for the next commit

Visual changes

rsashank avatar Dec 14 '23 05:12 rsashank

@zulipbot add "PR needs review"

rsashank avatar Dec 25 '23 21:12 rsashank

@zulipbot remove "PR awaiting update" @zulipbot add "PR needs review"

rsashank avatar Feb 15 '24 10:02 rsashank

When you update the suggested additional model method to call the _fetch, that would be a good time to include what is currently in the first commit - and using NotRequired would be a good change at that point too.

Did you understand what I meant here? I missed following up on this.

Reading this again, I think my original idea was to add the model methods in two stages: firstly to wrap the direct data access (pure refactor) and secondly update that method and add the new one (so only in the model) to try and fetch the data instead - combined with the NotRequired change.

This isn't critical, so focus on the other points first, but consider how the code changes would read differently in two commits as above.

neiljp avatar May 11 '24 01:05 neiljp

Updated this PR! Thanks for the review @Niloth-p :)

rsashank avatar May 31 '24 08:05 rsashank

Thanks for the detailed review, @neiljp! I've updated this PR and added a new commit to change fetched to expected_fetched for another test written previously. Thanks for pointing out that type annotations aren't checked for test_model.py. I'll look into this at a later point.

rsashank avatar Jun 26 '24 03:06 rsashank

@rsashank Thanks for your work on this, and everyone's reviews too :+1:

I made some minor changes and pushed back to merge. I dropped the last commit since while it was small and would not be a problem, this would likely make sense with other similar changes.

I'll be merging shortly, and plan to post the range-diff to the GSoC stream to discuss :)

neiljp avatar Jun 27 '24 06:06 neiljp