Skip to content

chat: Fix create post without a topic#4543

Closed
AlaaElden98 wants to merge 1 commit intozulip:mainfrom
AlaaElden98:create_post_without_topics
Closed

chat: Fix create post without a topic#4543
AlaaElden98 wants to merge 1 commit intozulip:mainfrom
AlaaElden98:create_post_without_topics

Conversation

@AlaaElden98
Copy link
Copy Markdown
Contributor

@AlaaElden98 AlaaElden98 commented Mar 18, 2021

When the option 'require topics in stream messages' is checked,the user
shouldn't be able to post a message without a topic.

  • Store the setting in RealmState
  • Populate it in realmReducer
  • Add a selector for it in directSelector
  • Add the setting to ComposeBox props and connect call,and use it

Fixes: #4378

@AlaaElden98 AlaaElden98 force-pushed the create_post_without_topics branch from 1b3e092 to 5e01427 Compare March 18, 2021 05:14
Copy link
Copy Markdown
Contributor

@chrisbobbe chrisbobbe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, @AlaaElden98! See a few small comments below.

Also, I think the commit message can be tightened up a bit:

chat: Fix create post without a topic

Why i did these changes?
-When the option 'require topics in stream messages' is checked,the user
shouldn't be able to post a message without a topic.

What changes:
* Store the setting in `RealmState`
* Populate it in `realmReducer`
* Add a selector for it in `directSelector`
* Add the setting to `ComposeBox` props and `connect` call,and use it
* Edit `realmReducer-test` to test the new changes

Fixes: #4378
  • One thing is that the "What changes" list duplicates what's already clear by looking at the code changes, so I think that can just be removed. 🙂 The changes are pretty small, straightforward, and unsurprising.

  • Let's give the summary line a bit more detail. How about something like, "ComposeBox: Prevent sending empty-topic messages if topics are mandatory."

  • Then, the summary line plus the Fixes: line should pretty much answer the question of why the changes were made. So the "Why i did these changes?" section can disappear too, to leave us with the following:

    ComposeBox: Prevent sending empty-topic messages if topics are mandatory.
    
    Fixes: #4378
    

    That's shorter, and still gets the job done. 🙂

Comment thread src/compose/ComposeBox.js Outdated
Comment thread src/compose/ComposeBox.js
Comment thread src/realm/realmReducer.js
twentyFourHourTime: false,
canCreateStreams: true,
isAdmin: false,
isTopicMandatory: false,
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's add a dropCache migration in src/boot/store.js for this. Something like,

  // Add `isTopicMandatory` to `state.realm`.
  '27': dropCache,

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's new, I understand what this does, but when should I add a dropCache?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And i think you mean '28': dropCache ? there is already
'27': state => ({ ...state, accounts: state.accounts.filter(a => a.email !== ''), }),

Comment thread src/realm/realmReducer.js Outdated
Comment thread src/compose/ComposeBox.js Outdated
@AlaaElden98 AlaaElden98 force-pushed the create_post_without_topics branch from 5e01427 to ffe3427 Compare May 7, 2021 04:23
@AlaaElden98 AlaaElden98 requested a review from chrisbobbe May 7, 2021 04:34
@AlaaElden98
Copy link
Copy Markdown
Contributor Author

@chrisbobbe, Thanks for the review, the PR is ready for another one.

@gnprice
Copy link
Copy Markdown
Member

gnprice commented Oct 19, 2021

This issue was resolved by #4798. Thanks @AlaaElden98 for your work on this!

@gnprice gnprice closed this Oct 19, 2021
@AlaaElden98 AlaaElden98 deleted the create_post_without_topics branch October 20, 2021 01:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Mobile app can create post without a topic, when permissions are set not to allow that.

3 participants