Skip to content

526 contrasting confirmation messages - #571

Open
Kafui123 wants to merge 27 commits into
developmentfrom
526Contrasting_Confirmation_Messages
Open

526 contrasting confirmation messages#571
Kafui123 wants to merge 27 commits into
developmentfrom
526Contrasting_Confirmation_Messages

Conversation

@Kafui123

@Kafui123Kafui123 commented Apr 2, 2026

Copy link
Copy Markdown

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. Flash message errors are considered and both back-end error and front-end error UI are done fro both developers and users.
  • 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.
  • Front-end flash message is more responsive, stacked

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, flash success message would appear
  • if you want to break it and see an error flash, you can remove a parameter for one of those functions that send an email to see the error: createOverloadFormAndFormHistory , emailDuringBreak(checkForSecondLSFBreak
  • If the form got created, but no email was sent, the form would not get created and an error flash will appear.
Screenshot 2026-04-02 145031Screenshot 2026-04-02 145042

@bakobagassasbakobagassas left a comment

Copy link
Copy Markdown

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

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.

Comment threadapp/logic/emailHandler.py Outdated
Comment threadapp/static/js/laborStatusForm.js
Comment threadtests/code/test_adminManagement.py Outdated
Comment threadtests/code/test_adminManagement.py Outdated
Comment threadtests/code/test_adminManagement.py Outdated
Comment threadapp/static/js/laborStatusForm.js
@Arohasina

Copy link
Copy Markdown
Contributor

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?

Comment threadapp/logic/statusFormFunctions.py
@Kafui123

Copy link
Copy Markdown
Author

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.

@ArohasinaArohasina left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

looks good !

@Kafui123
Kafui123 requested a review from MImran2002May 1, 2026 16:30
Comment threadapp/logic/statusFormFunctions.py
Comment threadapp/models/__init__.py Outdated
@Kafui123
Kafui123 requested a review from MImran2002May 5, 2026 19:20
@MImran2002MImran2002 self-assigned this Jun 8, 2026

@MImran2002MImran2002 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

there are whitespaces, some changes I will revert to reduce the code change scope that are not related to the issue description

Comment threadapp/logic/statusFormFunctions.py
Comment threadapp/logic/statusFormFunctions.py
Comment threadapp/static/js/laborStatusForm.js Outdated
@MImran2002
MImran2002force-pushed the 526Contrasting_Confirmation_Messages branch from be5e0a1 to 5b82836CompareJune 11, 2026 15:14
@nahom70nahom70 added epic A project that will require multiple sub-tasks to complete and removed epic A project that will require multiple sub-tasks to complete labels Jul 1, 2026
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants

@Kafui123@Arohasina@MImran2002@bakobagassas@nahom70