-
Notifications
You must be signed in to change notification settings - Fork 0
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
Fix/backend/emailing bugs #91
Conversation
… production database
…configuring double qouted strings
Fix/backend/mongo test db
chore: Add /benchmarks to .gitignore
…-monitoring-badges Feat/documentation/adding monitoring badges
Fix/mobile/styling
… yaml files to facilitate and make development easier for test, dev-local, dev-deployed and prod environments
…yaml files for easier development and environment management
…env-environments Fix/backend/reorganizing env environments
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #91 +/- ##
===========================================
+ Coverage 84.23% 85.76% +1.53%
===========================================
Files 5 5
Lines 241 295 +54
===========================================
+ Hits 203 253 +50
- Misses 28 30 +2
- Partials 10 12 +2 ☔ View full report in Codecov by Sentry. |
<img src="https://raw.githubusercontent.com/COS301-SE-2024/occupi/develop/presentation/Occupi/Occupi-black.png" alt="Business Banner" style="width:100%;"> | ||
<p>140 Lunnon Road, Hillcrest, Pretoria. PO Box 14679, Hatfield, 0028</p> | ||
</div> | ||
return ` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Rethakgetse-Manaka did you verify that the styling of this footer looks alright? Because I noticed that you are using fixed width's like 600px which may cause issues on people who have monitors that are really wide
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah I did test it to verify on the other one was very big and looked weird
@@ -52,3 +54,65 @@ func SendMultipleEmailsConcurrently(emails []string, subject, body string) []str | |||
|
|||
return emailErrors | |||
} | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Rethakgetse-Manaka , I noticed that you are still sending emails using concurrent methods, which we had highlighted might slow api response times significantly if say an email was to be sent out to 1000 people.
Did you consider using bcc to send the email to multiple people. If it did not work, could I suggest you creating some other branch and writing unit tests to ensure this function is stable, and doesn't get blocked by google if we say try to send many emails in seconds
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll do that
@Rethakgetse-Manaka , now that we have a working local mongodb to test with, could you consider making a new branch once you have merged this branch to make integration tests for api handlers |
Okay cool. @waveyboym The reason why I requested your review is because of that linting fail I am experiencing. I wanted your perspective |
@Rethakgetse-Manaka , It's saying that for the two test functions you wrote, TestCheckIn and TestCancelBooking, the programming logic is the same so either one of them have to be deleted. |
Description
Updated integration testing for 80% of api-endpoints, changed structures. Need to update docs before merge.
Fixes # (issue)
#94
Type of change
Added more validation in api-endpoint functions.
Added more integration tests
Fixed database models.
Please delete options that are not relevant.
How Has This Been Tested?
Integration test ran for 80% functions. Did an invalid request, valid request and a 404 test.
Checklist: