Edit position page by NYABUTOA · Pull Request #664 · BCStudentSoftwareDevTeam/lsf

NYABUTOA · 2026-08-04T16:03:08Z

Issue Description -

Added a Revise Position page for admins to edit a position's details and description.

Changes -

Added revisepositionpage.html, the new revise page. It's a form pre-filled with the position's title, WLS level, last revision date, revised-by, and all of its existing description sections so admins aren't starting from a blank page.
Added revisepositionpage.js so admins can add or remove description sections
Added revisepositionpage.css for the section-row and action-button styling the form needed that Bootstrap didn't already cover.
Added a link on the "Revise Position" button in managePositions.html so it takes you to the revise page for that exact position.

Test -

Went to Manage Positions and clicked "Revise Position" on a row, confirmed it opens the revise page and pulls in that position's title, code, WLS level, last revision date, revised-by, and its existing sections correctly.
Clicked "Add Section" a few times and confirmed each click adds a fresh, empty section row.
Clicked "Remove Section" and confirmed it only removes that one row, not the others.
Clicked "Cancel" and confirmed it takes you back to Manage Positions for the right department.
Reloaded the page to make sure the pre-filled data still shows up the same way.

… its going to work because its not going to be active its going to be pending so i need to work my way around it

…e revise page

…ition and make changes.these are the bones, html,css,and js

…ion so the revise position page actually works. Submitting the form now creates a new pending (Requested) revision of the position with updated title, WLS, and description sections.

BhushanSah · 2026-08-04T17:35:31Z

        transaction.rollback()
-
+
+@pytest.mark.integration


These tests still create PositionHistory records with description="", but the model no longer has a description field and now requires revisedBy.

Can you update the tests to use revisedBy instead? Several creates in this file may fail because revisedBy is required and not provided.

… revision is immediately visible instead of redirecting back to a list that only shows Active positions.

brightfietsop-ux · 2026-08-04T21:09:35Z

+    revisionDate = None
+    if revisionDateParam:
+        try:
+            revisionDate = datetime.strptime(revisionDateParam, '%Y-%m-%d').date()


Currently, when a section is added and saved, it works. But if the page is reloaded to check if it saves in the database, it breaks with an unexpected error. I believe it might be a routing issue or simply simply just an exception handler problem

brightfietsop-ux · 2026-08-04T21:12:31Z

+        position=position,
+        sections=sections
+    )
+


make sure the WLS number entered should not be more than 6. until it gets revised to the new 1-3 system. currently, any number input is accepted, even if the number is a 10000 etc.

conwelld · 2026-08-05T17:10:06Z

On the revise positions page, there should be alimit to how high wls can go right now it can go infinitely

conwelld · 2026-08-05T17:14:51Z

There may need to be a character limit, or change the size of the heading, so for long position titles it does not require a lot of scrolling

conwelld · 2026-08-05T17:16:53Z

I feel like the error here makes it harder for the user to return to where they caused that error, consider rerouting it to the manage positions page

conwelld · 2026-08-05T17:17:52Z

For these buttons I feel like cancel is not the right language, because if someone wanted to just go back they may think their changes are not saved, but i see both cases working

MImran2002 · 2026-08-05T19:05:35Z

@@ -0,0 +1,105 @@
+{% extends "base.html" %}
+


when I look into it your page has html tags being display as well that means we need a more sophisticated text editor. I did a bit of digging and there is a editor in emailtemplate.html. And from your python you also need to have a restriction to only allow certain tags to prevent the text-editor from being hackable.

@nahom70 this will be useful for you too

This looks like the right direction. CKEditor is now loaded on the revise position page, and the JS initializes it for both existing sections and newly added sections. I also saw the backend sanitizer in getPositions.py, which is important because the editor alone does not make the submitted HTML safe.

Can we add a small test for sanitizeDescriptionHTML()? Since this content will be rendered as HTML later, I think we should make sure allowed tags are preserved and unsafe tags/attributes are stripped before this is merged.

…ut plus a server-side check, since the field previously accepted any value.

…ound in the email template. Section title/content are now passed through an allow-list HTML sanitizer before saving.

brightfietsop-ux · 2026-08-06T19:34:30Z

+          <section id="sectionsContainer">
+            {%- for section in sections %}
+              <div class="description-section-row">
+                <div class="form-group">


So far, the font and the format are small issues that need to be addressed. There are lots of inconsistencies in it

BhushanSah · 2026-08-06T19:41:35Z

@@ -0,0 +1,105 @@
+{% extends "base.html" %}
+


This looks like the right direction. CKEditor is now loaded on the revise position page, and the JS initializes it for both existing sections and newly added sections. I also saw the backend sanitizer in getPositions.py, which is important because the editor alone does not make the submitted HTML safe.

Can we add a small test for sanitizeDescriptionHTML()? Since this content will be rendered as HTML later, I think we should make sure allowed tags are preserved and unsafe tags/attributes are stripped before this is merged.

BhushanSah · 2026-08-06T19:53:28Z



+@main_bp.route('/department/<org>/<account>/positions/<positionCode>/revise', methods=['GET', 'POST'])
+def revisePosition(org, account, positionCode):


Can we add an authorization check here before allowing the user to view or save a revision?

managePositions checks whether the user is a labor admin or a supervisor connected to this department, but this revise route does not appear to have the same check. Since this route can create a new position revision on POST, a user who knows the URL may be able to revise a position without going through Manage Positions.

BhushanSah · 2026-08-06T19:53:50Z

+        return render_template('errors/404.html'), 404
+
+    position = getPosition(dept, positionCode)
+


I think this route should reuse the same department access logic as managePositions.

Something like this would keep the behavior consistent:

if not g.currentUser.isLaborAdmin: if not SupervisorDepartment.select().where( (SupervisorDepartment.supervisor == g.currentUser.supervisor) & (SupervisorDepartment.department == dept.departmentID) ).exists(): return render_template('errors/403.html'), 403

BhushanSah · 2026-08-06T19:54:27Z

+tagPattern = re.compile(r'<(/?)\s*([a-zA-Z][a-zA-Z0-9]*)((?:\s+[^<>]*)?)\s*/?>')
+hrefPattern = re.compile(r'href\s*=\s*(["\'])(https?:.*?|mailto:.*?|/.*?)\1', re.IGNORECASE)
+
+def sanitizeDescriptionHTML(value):


Can we add a small test for this sanitizer?

Since section content is edited with CKEditor and later rendered as HTML, it would be good to verify that allowed tags like <p>, <ul>, <li>, and <strong> stay, but unsafe tags/attributes like <script>, onclick, and unsafe links are removed.

BhushanSah · 2026-08-06T19:55:26Z

    return positionDescriptionSections

+allowedDescriptionTags = {'p', 'br', 'strong', 'b', 'em', 'i', 'u', 'ul', 'ol', 'li', 'a', 'h1', 'h2', 'h3', 'h4', 'h5', 'h6'}
+tagPattern = re.compile(r'<(/?)\s*([a-zA-Z][a-zA-Z0-9]*)((?:\s+[^<>]*)?)\s*/?>')


This sanitizer is a good start, but parsing HTML with regex can be fragile.

Could we either add stronger tests around tricky HTML cases, or use a dedicated HTML sanitizer if the project already has one available? The main thing I want to avoid is saving content with unsafe attributes or malformed tags that later get rendered with |safe.

BhushanSah · 2026-08-06T19:57:10Z

+                request.form.getlist('sectionTitle[]'),
+                request.form.getlist('sectionContent[]')
+            )
+            flash('Position revision saved.', 'success')


After saving a revision, should this redirect instead of rendering the page from the POST request?

Right now, refreshing the browser after a successful save could submit the POST again and create another requested revision. A redirect after save would avoid duplicate revisions from refresh.

BhushanSah · 2026-08-06T19:57:35Z

+        revisedBy=revisedBy
+    )
+
+    for order, (sectionTitle, sectionContent) in enumerate(zip(sectionTitles, sectionContents)):


Can we validate that sectionTitles and sectionContents have the same length before using zip()?

If one list is longer than the other, zip() will silently drop the extra values. Since this is saving a revision, it would be safer to catch that case and return an error instead of losing part of the submitted form.

BhushanSah · 2026-08-06T19:57:55Z

+        if (event.target.classList.contains('remove-section-btn')) {
+            var row = event.target.closest('.description-section-row');
+            var textarea = row.querySelector('textarea[name="sectionContent[]"]');
+            var editor = textarea && CKEDITOR.instances[textarea.id];


Can we make sure this reliably destroys the CKEditor instance before removing a section?

The textareas in the template do not have explicit IDs, but this lookup depends on textarea.id. If CKEditor does not set the ID the way we expect, the editor instance may not be destroyed before the row is removed.

MImran2002 · 2026-08-07T13:40:03Z

+            </div>
+          </div>
+
+          <h3 class="description-header">Description</h3>


Section Title still shows html tags

MImran2002 · 2026-08-07T13:50:40Z

+              <a href="{{ url_for('main.managePositions', org=department.ORG, account=department.ACCOUNT) }}" class="btn btn-default">
+                Cancel
+              </a>
+              <button type="submit" class="btn btn-primary">Save Revision</button>


Save Revision doesn't portray the full picture save revision mean they can come back and edit. it should be submit revision.

MImran2002 · 2026-08-07T17:14:51Z

+              <button type="submit" class="btn btn-primary">Save Revision</button>
+            </div>
+          </div>
+


We didn't considered for the draft stage where our user instead of submit can save and leave only to come back. This means we need to add Draft as a status. This also mean we can use this draft and not change its composite key but its other fields. Moreover, in case the revisiondate move another day we can create a new one with different revision date base on the filter that the revision date is recent and draft exist if not we create a new one.

MImran2002 · 2026-08-07T17:16:20Z

+                request.form.getlist('sectionTitle[]'),
+                request.form.getlist('sectionContent[]')
+            )
+            flash('Position revision saved.', 'success')


submitted for submission and saved for saving a draft

MImran2002 · 2026-08-07T17:46:49Z

+    function initEditor(row) {
+        var textarea = row.querySelector('textarea[name="sectionContent[]"]');
+        if (textarea) {
+            CKEDITOR.replace(textarea);


if you use this here: CKEDITOR.replace(textarea, {
allowedContent:
'p br strong b em i u ul ol li a[href] h1 h2 h3 h4 h5 h6'
}); you don't need to create a function, you don't need to sanitize the value in flask as this can sanitize

MImran2002 · 2026-08-07T17:47:50Z

+
+    function initEditor(row) {
+        var textarea = row.querySelector('textarea[name="sectionContent[]"]');
+        if (textarea) {


the CKeditor that we add need other features for the use to add like list, bold, italic, font size and so on

MImran2002 · 2026-08-07T17:48:10Z

+    var sectionsContainer = document.getElementById('sectionsContainer');
+    var sectionRowTemplate = document.getElementById('sectionRowTemplate');
+
+    function initEditor(row) {


we should also remove the insetplaceholder as we don't need it.

MImran2002 · 2026-08-07T17:49:44Z

+            {%- endfor %}
+          </section>
+
+          <button type="button" id="addSectionBtn" class="btn btn-default">Add Section</button>


this button should be green button

MImran2002 · 2026-08-07T17:50:31Z

+          <div class="row revise-actions">
+            <div class="col-xs-12 text-left">
+              <a href="{{ url_for('main.managePositions', org=department.ORG, account=department.ACCOUNT) }}" class="btn btn-default">
+                Cancel


cancel should be red and there should be a new button for submit which should be green too.

NYABUTOA added 5 commits August 3, 2026 16:07

Created the revise position page bones i still need to figure out how…

7c7fc83

… its going to work because its not going to be active its going to be pending so i need to work my way around it

added a connection so that we are able to press the button and see th…

1f7f422

…e revise page

This is the revise position page where one should be able to edit pos…

48e83ec

…ition and make changes.these are the bones, html,css,and js

DETELETD THIS DUPLICATE FILES

496a93f

Implemented the revisePosition route and createPositionRevision funct…

557e211

…ion so the revise position page actually works. Submitting the form now creates a new pending (Requested) revision of the position with updated title, WLS, and description sections.

BhushanSah requested changes Aug 4, 2026

View reviewed changes

BhushanSah assigned NYABUTOA Aug 4, 2026

NYABUTOA requested a review from johnlolonga19 August 4, 2026 17:48

Make revise position stay on the page after saving so the new pending…

8cddd37

… revision is immediately visible instead of redirecting back to a list that only shows Active positions.

brightfietsop-ux requested changes Aug 4, 2026

View reviewed changes

fixed the merge conflicts

1d902d3

MImran2002 requested changes Aug 5, 2026

View reviewed changes

NYABUTOA added 2 commits August 6, 2026 12:47

Cap WLS level at 6 Adds a max=6 bound on the revise-position form inp…

a52dab1

…ut plus a server-side check, since the field previously accepted any value.

Replaced the plain description textareas with CKEditor that is also f…

9799cde

…ound in the email template. Section title/content are now passed through an allow-list HTML sanitizer before saving.

brightfietsop-ux reviewed Aug 6, 2026

View reviewed changes

BhushanSah requested changes Aug 6, 2026

View reviewed changes

MImran2002 requested changes Aug 7, 2026

View reviewed changes



		@main_bp.route('/department/<org>/<account>/positions/<positionCode>/revise', methods=['GET', 'POST'])
		def revisePosition(org, account, positionCode):

		return render_template('errors/404.html'), 404

		position = getPosition(dept, positionCode)

		transaction.rollback()


		@pytest.mark.integration

Uh oh!

Conversation

NYABUTOA commented Aug 4, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

conwelld commented Aug 5, 2026

Uh oh!

conwelld commented Aug 5, 2026

Uh oh!

conwelld commented Aug 5, 2026

Uh oh!

conwelld commented Aug 5, 2026

Uh oh!

MImran2002 Aug 5, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

NYABUTOA commented Aug 4, 2026 •

edited

Loading

MImran2002 Aug 5, 2026 •

edited

Loading