WikiEducationFoundation / WikiEduDashboard

Wiki Education Foundation's Wikipedia course dashboard system
https://dashboard.wikiedu.org
MIT License
392 stars 631 forks source link

Fix: Failing test in pull request #5783 #6027

Closed Formasitchijoh closed 1 day ago

Formasitchijoh commented 2 weeks ago

What this PR does

This PR fixes the failing test from pull request #5783, and update functionality of sending instructor alerts. All necessary tests have been written to cover various scenarios and edge cases.

ScreenShot of Email Sent

Screenshot 2024-11-19 at 05 56 07 Screenshot 2024-11-19 at 05 57 14

@ragesoss please can you review this when you are chanced

Formasitchijoh commented 2 weeks ago

My Findings

The test failures were inconsistent and unstable, with different tests failing on each build. While many tests pass consistently when run locally, some tests fail both locally and in production, suggesting potential changes in the code or resources that are not aligned across environments.

Some observations included.

I believe identifying these flaky tests, addressing potential race conditions and shared state issues, and adding explicit waits in feature tests could help resolve some of these failures. I can start working on this after I am done with this task, if that’s okay with you

ragesoss commented 1 week ago

Thanks! Can you rebase this so that in only includes the changes relevant to the new feature?

Formasitchijoh commented 1 week ago

Thanks! Can you rebase this so that in only includes the changes relevant to the new feature?

@ragesoss i've rebased to the feature changes

Formasitchijoh commented 1 week ago

Hello @ragesoss Just checking in regarding this task. Is there anything specific I need to do to complete it or any additional updates required from my end?

ragesoss commented 1 week ago

I still need to manually test this. However, I note that this PR includes an unexpected removal of a large number of messages from en.yml. Perhaps in needs to be rebased?

Formasitchijoh commented 1 week ago

I still need to manually test this. However, I note that this PR includes an unexpected removal of a large number of messages from en.yml. Perhaps in needs to be rebased?

Alright updating that

ragesoss commented 1 week ago

It looks like you've squashed the commits from @prathamVaidya's PR into the first commit, but it also includes additional changes that you've made? I'd prefer to either leave the commits from the other PR as separate commits, or squash them into a single one that represents only those commits, and add any additional changes in new commits.

Formasitchijoh commented 6 days ago

It looks like you've squashed the commits from @prathamVaidya's PR into the first commit, but it also includes additional changes that you've made? I'd prefer to either leave the commits from the other PR as separate commits, or squash them into a single one that represents only those commits, and add any additional changes in new commits.

Alright I have updated it

Formasitchijoh commented 1 day ago

Could i start with something else while you are reviewing this

ragesoss commented 1 day ago

Could i start with something else while you are reviewing this

Yes

ragesoss commented 1 day ago

Looks good! Thanks @prathamVaidya and @Formasitchijoh !