Skip to content

Add achievement form validation and footer UI improvement - #465

Open
bansalrujul07 wants to merge 2 commits into
meswaramuthu:mainfrom
bansalrujul07:fix/form-validation
Open

Add achievement form validation and footer UI improvement #465
bansalrujul07 wants to merge 2 commits into
meswaramuthu:mainfrom
bansalrujul07:fix/form-validation

Conversation

@bansalrujul07

Copy link
Copy Markdown

Which issue does this PR close?

Rationale for this change

The achievement submission/edit forms were allowing incomplete or invalid data to be submitted without proper feedback to the user. Most validations were only handled on the backend, which made the form experience confusing and less user-friendly.

This PR improves the overall form handling experience by adding proper client-side validation, inline error messages, accessibility improvements, and better UI feedback for invalid fields. These changes help users understand issues immediately before submission and improve data quality across the platform.

What changes are included in this PR?

  • Added client-side validation for achievement forms.
  • Prevented empty or invalid form submissions.
  • Added minimum and maximum character limits for relevant fields.
  • Added validation to prevent future dates in achievement date fields.
  • Added certificate file type validation for supported formats.
  • Added meaningful inline error messages below invalid inputs.
  • Highlighted invalid fields visually for better feedback.
  • Improved accessibility using labels and aria-related attributes.
  • Ensured valid submissions continue working normally with existing Flask backend routes.

Files updated

  • templates/teacher_achievements_2.html
  • templates/submit_achievements.html
  • templates/base.html
  • static/styles.css

##New file added

  • static/js/achievement-form-validation.js

Are these changes tested?

Yes.
The following scenarios were tested locally:

  • Empty form submission
  • Very short/invalid text input
  • Future date validation
  • Invalid certificate file type upload
  • Valid authenticated form submission flow

Are there any user-facing changes?

Yes.
Users will now see:

  • Inline validation error messages
  • Better visual feedback for invalid fields
  • Improved accessibility and form usability
  • Cleaner and more responsive footer styling-

These changes improve the overall experience while submitting achievements.

@vercel

vercel Bot commented May 23, 2026

Copy link
Copy Markdown

@bansalrujul07 is attempting to deploy a commit to the 007's projects Team on Vercel.

A member of the Team first needs to authorize it.

@github-actions

Copy link
Copy Markdown

Thanks for creating a PR for your Issue! ☺️

We'll review it as soon as possible.
In the meantime, please double-check the file changes and ensure that all commits are accurate.

If there are any unresolved review comments, feel free to resolve them. 🙌🏼

Comment thread templates/teacher_achievements_2.html Outdated
@entelligence-ai-pr-reviews

Copy link
Copy Markdown
Contributor

Confidence Score: 4/5 - Mostly Safe

Likely safe but review recommended — the PR successfully adds achievement form validation and footer UI improvements, but teacher_achievements_2.html has a minor correctness issue where toggleAchievementFields will fire twice per change event due to the retained inline onchange attribute on line 122 coexisting with a separately bound event listener. This double-firing is classified as a NIT-level correctness concern rather than a critical bug, but it could cause subtle UI state flickering or unexpected behavior in the toggle logic depending on how the function manages state. The rest of the changes appear clean with no logic, security, or data-integrity concerns identified.

Key Findings:

  • In teacher_achievements_2.html, the inline onchange="toggleAchievementFields()" attribute on line 122 was not removed when an external event listener was added, causing toggleAchievementFields to execute twice per user interaction — this could produce idempotent no-ops or toggle-back-and-forth bugs depending on the function's internal state management.
  • All 5 changed files were reviewed with no critical, significant, or high-risk issues found, suggesting the validation logic and footer UI changes are fundamentally sound.
  • No security concerns, data-loss risks, or crash-inducing logic errors were identified across the PR's scope.
Files requiring special attention
  • templates/teacher_achievements_2.html

@entelligence-ai-pr-reviews

Copy link
Copy Markdown
Contributor

EntelligenceAI PR Summary

Removes inline onchange event handler from the achievement_type select element in the teacher achievements template.

  • Removed onchange="toggleAchievementFields()" attribute from achievement_type select in templates/teacher_achievements_2.html
  • Decouples dynamic field toggling from inline HTML, suggesting migration to external JS addEventListener or removal of the toggling functionality

Confidence Score: 4/5 - Mostly Safe

Safe to merge — this PR makes a focused, low-risk change to templates/teacher_achievements_2.html by removing the inline onchange="toggleAchievementFields()" attribute from the achievement_type select element, which is a reasonable step toward separating concerns between HTML and JavaScript. The change itself introduces no new logic, no security surface, and no runtime errors on its own. However, the PR summary acknowledges that the toggleAchievementFields() functionality may no longer be wired up, which means dynamic field toggling could silently break for users unless the corresponding addEventListener migration has been completed elsewhere — this warrants a quick verification that the JS behavior is preserved.

Key Findings:

  • The removal of onchange="toggleAchievementFields()" from the achievement_type select in teacher_achievements_2.html is a clean inline-handler decoupling, which is a positive style improvement and reduces XSS-adjacent risks from inline event handlers.
  • No corresponding addEventListener wiring for toggleAchievementFields is confirmed in this PR — if the external JS file does not already listen for changes on achievement_type, the dynamic field toggling UX will silently regress without any JavaScript error, making this a potential hidden functional breakage.
  • The PR title references 'form validation and footer UI improvement' but the actual diff appears limited to the single onchange attribute removal — the broader scope implied by the title is not reflected in the described change, suggesting either incomplete implementation or a mismatch between title and actual diff.
Files requiring special attention
  • templates/teacher_achievements_2.html

@bansalrujul07

Copy link
Copy Markdown
Author

@Eswaramuthu
I have resolved the duplicate onchange handler issue by removing the inline onchange attribute from the achievement_type select field.

Also, verified locally that:

  • the addEventListener in achievement-form-validation.js is still handling the change event correctly
  • dynamic field toggling continues to work as expected
  • valid file types like .pdf are accepted
  • invalid file types like .txt are rejected properly

@bansalrujul07

Copy link
Copy Markdown
Author

Hi @Eswaramuthu ,

I hope you're doing well. I wanted to follow up on this PR. I addressed the requested changes and updated the PR around 3 weeks ago. Could you please review it when you have time?

Please let me know if any further modifications are needed from my side. Thank you for your time and effort in maintaining the project!

Sign up for free to 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.

Add Form Validation and User-Friendly Error Messages in Achievement Submission Forms

1 participant