Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

refactor(condo): DOMA-6931 refactored news sending #4501

Merged
merged 32 commits into from Mar 29, 2024

Conversation

Alllex202
Copy link
Contributor

@Alllex202 Alllex202 commented Mar 21, 2024

what's going on here?

changes in the NewsItem schema:

Before:

  • the field sendAt is set in when the news should be postponed

After:

  • the sendAt field is now always set in (сan be null if not ready to send yet when isPublished = false), and indicates the date when the news sending should begin

changes in tasks

Before:

  • the notifyResidentsAboutNewsItem task used setTimeout and because of this we could not track the sending process, since the worker completed the task before the news sending process was completed

After:

  • the notifyResidentsAboutNewsItem task has been refactored and now does not use setTimeout

changes in accesses

Before:

  • a dynamic filter was previously used to allow residents to read news

After:

  • the filter for residents to read news has been simplified

Other

  • Tests for sending news have been added
  • Fixed display of the date of sending news in CRM
  • Removed the button for editing if the news is already being sent out

@Alllex202 Alllex202 added the 🔬 WIP Not intended to be merged right now, it is a work in progress label Mar 21, 2024
@Alllex202 Alllex202 force-pushed the refactor/condo/DOMA-6931/refactor-news-sending branch from 09239d2 to 92ebb7f Compare March 21, 2024 15:09
@@ -12,7 +12,7 @@ const { ADDRESS_META_SUBFIELDS_TABLE_LIST } = require('@condo/domains/property/s

const COMMON_FIELDS = 'id dv sender { dv fingerprint } v deletedAt newId createdBy { id name } updatedBy { id name } createdAt updatedAt'

const NEWS_ITEM_FIELDS = `{ organization { id } number title body type validBefore sendAt sentAt isPublished publishedAt ${COMMON_FIELDS} compactScopes { count firstOnes { id unitType unitName property { address addressMeta { ${ADDRESS_META_SUBFIELDS_TABLE_LIST} } } } } }`
const NEWS_ITEM_FIELDS = `{ organization { id } number title body type validBefore sendAt deliverAt sentAt isPublished publishedAt ${COMMON_FIELDS} compactScopes { count firstOnes { id unitType unitName property { address addressMeta { ${ADDRESS_META_SUBFIELDS_TABLE_LIST} } } } } }`
Copy link
Member

Choose a reason for hiding this comment

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

Its better to call it deliveredAt, since delivered is in past tense

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, it's future tense, not past tense.
This is equivalent to:

  1. sendAt - for news to be sent in the future
  2. publishedAt + time to cancel sending 15 seconds - for news to be sent now

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I will add a description to this pr to make it clearer what is changing

Copy link
Member

Choose a reason for hiding this comment

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

Alright, got you

But this seems really counter intuitive.. Need docs or better naming

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Unfortunately, the name sendTo is already taken :(

Copy link
Contributor Author

@Alllex202 Alllex202 Mar 22, 2024

Choose a reason for hiding this comment

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

or better naming

Ok, I'll think about it

Copy link
Member

Choose a reason for hiding this comment

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

Just to be clear:

Why couldnt we set sendAt to publishedAt + time to cancel sending 15 seconds - for news to be sent now ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I wanted to do that, but...
Now the sendAt field is an indicator that the news was sent with a planned sending date in the future.
If it is full, then the news is planned; if it is empty, then the news is being sent right now.
I can’t yet say for sure whether we need this information now or whether this information may be useful in the future, but if we start always filling out the sendAt field, we will lose it.

However, we can add a separate flag for this. (If needed)

I think it's worth discussing this point. if it doesn't affect anything, then why not

type: 'DateTimeUtc',
},

deliverAt: {
// todo(DOMA-6931): update schemaDoc
Copy link
Member

Choose a reason for hiding this comment

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

6931 is solved by this PR, is it not? I think we should remove this PR

@@ -828,7 +828,8 @@ describe('NewsItems', () => {
)
})

test('must throw an error on trying to edit the news item which already been sent', async () => {
// todo(doma-6931): rewrite test
Copy link
Member

Choose a reason for hiding this comment

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

^^

})
})

describe('should be auto-calculate', () => {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
describe('should be auto-calculate', () => {
describe('should be auto-calculated', () => {

@@ -136,10 +136,40 @@ const NewsItem = new GQLListSchema('NewsItem', {
},

sendAt: {
Copy link
Member

Choose a reason for hiding this comment

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

what will happen with this value if user decides to click "No I do not want to send this newsItem"

Also, I think we should reuse 15 seconds from frontned part of our app. Otherwise we will change 15 to 30 in one place. and forget to change in other place

Copy link
Contributor Author

Choose a reason for hiding this comment

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

if user decides to click "No I do not want to send this newsItem"

This news item will be deleted (using deletedAt)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Also, I think we should reuse 15 seconds from frontned part of our app. Otherwise we will change 15 to 30 in one place. and forget to change in other place

Fixed

type: defineMessageType(newsItem),
meta: {
dv: 1,
title: truncate(newsItem.title, { length: TITLE_MAX_LEN, separator: ' ', omission: '...' }),
Copy link
Member

Choose a reason for hiding this comment

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

It might be better to use deidcated char here, since it might (im not sure) take less space in the resulting message

Need to check whether the target system support the encodings though

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think there is no need to change something that already works well :)

@Alllex202 Alllex202 force-pushed the refactor/condo/DOMA-6931/refactor-news-sending branch from 92ebb7f to 871fcd0 Compare March 22, 2024 09:20
@Alllex202 Alllex202 force-pushed the refactor/condo/DOMA-6931/refactor-news-sending branch from 9d87ebf to 536deef Compare March 26, 2024 16:46
@Alllex202 Alllex202 marked this pull request as ready for review March 26, 2024 17:29
@Alllex202 Alllex202 added ✋🙂 Review please Comments are resolved, take a look, please 🚨 Migrations We have a database migrations here! and removed 🔬 WIP Not intended to be merged right now, it is a work in progress labels Mar 26, 2024
@Alllex202 Alllex202 added 🔬 WIP Not intended to be merged right now, it is a work in progress and removed ✋🙂 Review please Comments are resolved, take a look, please labels Mar 27, 2024
@Alllex202 Alllex202 force-pushed the refactor/condo/DOMA-6931/refactor-news-sending branch from 536deef to 7e2f166 Compare March 27, 2024 12:15
@Alllex202 Alllex202 added ✋🙂 Review please Comments are resolved, take a look, please and removed 🔬 WIP Not intended to be merged right now, it is a work in progress labels Mar 27, 2024
@@ -83,6 +85,8 @@ const NewsItemCard: React.FC = () => {
const ConfirmDeleteMessage = intl.formatMessage({ id: 'news.ConfirmDeleteMessage' })
const CancelMessage = intl.formatMessage({ id: 'news.CancelMessage' })

const { publicRuntimeConfig: { newsItemsSendingDelay } } = getConfig()
Copy link
Member

Choose a reason for hiding this comment

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

move this outside of component

@Alllex202 Alllex202 force-pushed the refactor/condo/DOMA-6931/refactor-news-sending branch from 5b62394 to 4604ee4 Compare March 27, 2024 16:57
…eUntil" and "NewsItem.deliverAt" to "NewsItem.sendAt"
….postponeUntil" and "NewsItem.deliverAt" to "NewsItem.sendAt""

This reverts commit 536deef.
@Alllex202 Alllex202 force-pushed the refactor/condo/DOMA-6931/refactor-news-sending branch from 4604ee4 to a80e3ef Compare March 28, 2024 09:09
Copy link

sonarcloud bot commented Mar 28, 2024

Quality Gate Passed Quality Gate passed

Issues
2 New issues
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
0.0% Duplication on New Code

See analysis details on SonarCloud

@Alllex202 Alllex202 merged commit d352b18 into master Mar 29, 2024
24 checks passed
@Alllex202 Alllex202 deleted the refactor/condo/DOMA-6931/refactor-news-sending branch March 29, 2024 11:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🚨 Migrations We have a database migrations here! ✋🙂 Review please Comments are resolved, take a look, please
Development

Successfully merging this pull request may close these issues.

None yet

4 participants