Skip to content

526 contrasting confirmation messages#571

Open
Kafui123 wants to merge 14 commits intodevelopmentfrom
526Contrasting_Confirmation_Messages
Open

526 contrasting confirmation messages#571
Kafui123 wants to merge 14 commits intodevelopmentfrom
526Contrasting_Confirmation_Messages

Conversation

@Kafui123
Copy link
Copy Markdown

@Kafui123 Kafui123 commented Apr 2, 2026

Fixes issue #526

Changes:

  • Fixed the issue where an error message was shown, but a successful flash confirmation still appeared afterward. Now the feedback is consistent... no mixed signals for the user.
  • Adjusted the logic so that success and failure states are clearly separated and reported correctly, instead of overlapping or triggering both paths.
  • Addressed the case where email sending could fail, but the form would still be created, which previously caused a misleading failure message even though the form existed.
  • Ensured that form creation and email sending happen within a single transaction block.

Testing:

  • Click on administration and then manage terms.
  • Select a valid term under any academic year.
  • Create a new labor status form and then click submit
  • If everything succeeded, no flash error message would appear
  • If the form got created, but no email was sent, the form would not get created.
Screenshot 2026-04-02 145031 Screenshot 2026-04-02 145042

@Kafui123 Kafui123 requested a review from Arohasina April 2, 2026 18:54
@JohnCox2211 JohnCox2211 self-requested a review April 9, 2026 18:11
bakobagassas
bakobagassas requested changes Apr 13, 2026
View reviewed changes
Copy link
Copy Markdown

@bakobagassas bakobagassas left a comment

Choose a reason for hiding this comment

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

Why are we changing 6 test files ? I don't think most of those changes are necessary

@Kafui123
Copy link
Copy Markdown
Author

Kafui123 commented Apr 14, 2026

Why are we changing 6 test files ? I don't think most of those changes are necessary

I had to make those changes because they(the test files) were failing as a result of the new changes I added.

bakobagassas
bakobagassas reviewed Apr 14, 2026
View reviewed changes
Comment thread app/logic/emailHandler.py Outdated
Arohasina
Arohasina reviewed Apr 14, 2026
View reviewed changes
Comment thread app/static/js/laborStatusForm.js
bakobagassas
bakobagassas requested changes Apr 14, 2026
View reviewed changes
Comment thread tests/code/test_adminManagement.py Outdated
Comment thread tests/code/test_adminManagement.py Outdated
Comment thread tests/code/test_adminManagement.py Outdated
Comment thread app/static/js/laborStatusForm.js
@Arohasina
Copy link
Copy Markdown
Contributor

Arohasina commented Apr 14, 2026

Your tests now handle success cases, but could you consider adding an integration test for partial/full submission failure scenarios after you have added the fixes mentioned above?

Arohasina
Arohasina reviewed Apr 14, 2026
View reviewed changes
Comment thread app/logic/statusFormFunctions.py
@Kafui123
Copy link
Copy Markdown
Author

Kafui123 commented Apr 16, 2026

Your tests now handle success cases, but could you consider adding an integration test for partial/full submission failure scenarios after you have added the fixes mentioned above?

Your tests now handle success cases, but could you consider adding an integration test for partial/full submission failure scenarios after you have added the fixes mentioned above?

Your tests now handle success cases, but could you consider adding an integration test for partial/full submission failure scenarios after you have added the fixes mentioned above?

Since the main change in this pr was to put everything inside a mainDB.atomic block so that a form would not get submitted if email sending fails i do not think that it would be necessary testing the mainDB.atomic() block since that is peewee's behavior.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Arohasina Arohasina Arohasina requested changes

@bakobagassas bakobagassas bakobagassas requested changes

@JohnCox2211 JohnCox2211 Awaiting requested review from JohnCox2211

Requested changes must be addressed to merge this pull request.

Assignees

@Kafui123 Kafui123

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@Kafui123 @Arohasina @bakobagassas