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

EmailQuotaService has unnecessary ISession.SaveAsync() calls (OSOE-848) #118

Closed
Piedone opened this issue May 3, 2024 · 5 comments · Fixed by #125
Closed

EmailQuotaService has unnecessary ISession.SaveAsync() calls (OSOE-848) #118

Piedone opened this issue May 3, 2024 · 5 comments · Fixed by #125
Assignees
Labels
bug Something isn't working good first issue Good for newcomers

Comments

@Piedone
Copy link
Member

Piedone commented May 3, 2024

While those shouldn't be necessary. See OrchardCMS/OrchardCore#15290 (comment).

Jira issue

@Piedone Piedone added bug Something isn't working good first issue Good for newcomers labels May 3, 2024
@github-actions github-actions bot changed the title EmailQuotaService has unnecessary ISession.SaveAsync() calls EmailQuotaService has unnecessary ISession.SaveAsync() calls (OSOE-848) May 3, 2024
@Psichorex
Copy link
Contributor

I removed all and after that just some of the SaveAsync() calls and experienced broken behavior and failing UI tests. I sense that they might indeed be necessary.

@Piedone
Copy link
Member Author

Piedone commented Jun 16, 2024

Broken how?

@Psichorex
Copy link
Contributor

"in the EmailQuotaService all the methods updating the quota instance also call SaveAsync but it's not necessary since it's either coming from the database or being created and then tracked"

For example when removing it from IncreaseEmailUsageAsync it is just not counting the emails sent.

If I remove the others the tests are still working but this can be due to the Tests not touching those funcionalities that would be broken upon removing these.

As far this is all info I have collected.

@Psichorex
Copy link
Contributor

But I highly suspect that if emailQuota.CurrentEmailUsageCount++ is not working without SaveAsync then emailQuota.CurrentEmailUsageCount = 0; shouldn't be working aswell it is just not tested.

@Piedone
Copy link
Member Author

Piedone commented Jun 16, 2024

I see. Let's keep them, then, but please add a comment about this above the class so we don't try to remove them again.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working good first issue Good for newcomers
Projects
None yet
2 participants