Skip to content

Edit position page #664

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
NYABUTOA wants to merge 9 commits into
base: department-portal-base
Choose a base branch
from
Open

Edit position page #664

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
7c7fc83
Created the revise position page bones i still need to figure out how…
NYABUTOA Aug 3, 2026
1f7f422
added a connection so that we are able to press the button and see th…
NYABUTOA Aug 4, 2026
48e83ec
This is the revise position page where one should be able to edit pos…
NYABUTOA Aug 4, 2026
496a93f
DETELETD THIS DUPLICATE FILES
NYABUTOA Aug 4, 2026
557e211
Implemented the revisePosition route and createPositionRevision funct…
NYABUTOA Aug 4, 2026
8cddd37
Make revise position stay on the page after saving so the new pending…
NYABUTOA Aug 4, 2026
1d902d3
fixed the merge conflicts
NYABUTOA Aug 4, 2026
a52dab1
Cap WLS level at 6 Adds a max=6 bound on the revise-position form inp…
NYABUTOA Aug 6, 2026
9799cde
Replaced the plain description textareas with CKEditor that is also f…
NYABUTOA Aug 6, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
43 changes: 40 additions & 3 deletions app/controllers/main_routes/departmentPortal.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,11 +1,11 @@
from datetime import datetime

from flask import g, render_template, request, send_file
from flask import flash, g, render_template, request, send_file
from peewee import DoesNotExist

from app.controllers.main_routes import main_bp
from app.logic.download import makePositionDescriptionPDF
from app.logic.getPositions import getPosition, getPositions, getPositionDescriptionSections
from app.logic.getPositions import createPositionRevision, getPosition, getPositions, getPositionDescriptionSections
from app.models.department import Department
from app.models.positionHistory import PositionHistory
from app.models.supervisorDepartment import SupervisorDepartment
Expand Down Expand Up @@ -67,6 +67,43 @@ def downloadPositionDescription(org, account, positionCode):



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

@BhushanSah BhushanSah Aug 6, 2026

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.

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.

try:
dept = Department.get(Department.ORG == org, Department.ACCOUNT == account)
except (NameError, DoesNotExist):
return render_template('errors/404.html'), 404

position = getPosition(dept, positionCode)

@BhushanSah BhushanSah Aug 6, 2026

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.

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

if not position:
return render_template('errors/404.html'), 404

if request.method == 'POST':
wls = request.form.get('wls', type=int)
if wls is None or not (0 <= wls <= 6):
flash('WLS level must be between 0 and 6.')
else:
position = createPositionRevision(
position,
g.currentUser.fullName,
request.form.get('positionTitle'),
wls,
request.form.getlist('sectionTitle[]'),
request.form.getlist('sectionContent[]')
)
flash('Position revision saved.', 'success')

@BhushanSah BhushanSah Aug 6, 2026

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.

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.

@MImran2002 MImran2002 Aug 7, 2026

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.

submitted for submission and saved for saving a draft


sections = getPositionDescriptionSections(position)

return render_template(
'main/revisepositionpage.html',
department=dept,
position=position,
sections=sections
)


@main_bp.route('/department/<org>/<account>/positions', methods=['GET'])
def managePositions(org, account):
try:
Expand All @@ -87,4 +124,4 @@ def managePositions(org, account):
department = dept,
department_name = dept.DEPT_NAME,
positions = positions
)
)
53 changes: 52 additions & 1 deletion app/logic/getPositions.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
import re
from app.models.positionHistory import PositionHistory
from app.models.positionDescriptionSection import PositionDescriptionSection
from datetime import date

def getActivePositions(dept):
"""
Expand Down Expand Up @@ -52,7 +54,56 @@ def getPositionDescriptionSections(position):
positionDescriptionSections = list(PositionDescriptionSection.select()
.where(PositionDescriptionSection.position == position)
.order_by(PositionDescriptionSection.order.asc()))

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*/?>')

@BhushanSah BhushanSah Aug 6, 2026

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.

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.

hrefPattern = re.compile(r'href\s*=\s*(["\'])(https?:.*?|mailto:.*?|/.*?)\1', re.IGNORECASE)

def sanitizeDescriptionHTML(value):

@BhushanSah BhushanSah Aug 6, 2026

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.

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.

"""
Strips any HTML tag not in allowedDescriptionTags, and drops all attributes
except a safe href on <a> tags, since section content is rendered with |safe.
"""
if not value:
return ''

def replaceTag(match):
closingSlash, tag, attrs = match.groups()
tag = tag.lower()
if tag not in allowedDescriptionTags:
return ''
if tag == 'a' and not closingSlash:
hrefMatch = hrefPattern.search(attrs)
return f'<a href="{hrefMatch.group(2)}">' if hrefMatch else '<a>'
return f'<{closingSlash}{tag}>'

return tagPattern.sub(replaceTag, str(value))

def createPositionRevision(position, revisedBy, positionTitle, wls, sectionTitles, sectionContents):
"""
Creates a new pending (Requested) revision of a position, copying forward its
department and position code, and replaces its description sections with the
given titles/contents. Returns the newly created PositionHistory row.
"""
newPosition = PositionHistory.create(
positionTitle=positionTitle,
positionCode=position.positionCode,
department=position.department,
status="Requested",
wls=wls,
revisionDate=date.today(),
revisedBy=revisedBy
)

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

@BhushanSah BhushanSah Aug 6, 2026

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.

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.

PositionDescriptionSection.create(
position=newPosition,
sectionTitle=sanitizeDescriptionHTML(sectionTitle),
sectionContent=sanitizeDescriptionHTML(sectionContent),
order=order
)

return newPosition

12 changes: 12 additions & 0 deletions app/static/css/revisepositionpage.css
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
/* Revise position form - styles not covered by Bootstrap */
.description-section-row {
background: #fff;
border: 1px solid #e6e6e6;
padding: 1rem;
border-radius: 6px;
margin-bottom: 1rem;
}

.revise-actions {
margin-top: 1.5rem;
}
32 changes: 32 additions & 0 deletions app/static/js/revisepositionpage.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,32 @@
$(document).ready(function () {
var sectionsContainer = document.getElementById('sectionsContainer');
var sectionRowTemplate = document.getElementById('sectionRowTemplate');

function initEditor(row) {

@MImran2002 MImran2002 Aug 7, 2026

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.

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

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

@MImran2002 MImran2002 Aug 7, 2026

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.

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

CKEDITOR.replace(textarea);

@MImran2002 MImran2002 Aug 7, 2026

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.

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

}
}

sectionsContainer.querySelectorAll('.description-section-row').forEach(initEditor);

document.getElementById('addSectionBtn').addEventListener('click', function () {
var fragment = sectionRowTemplate.content.cloneNode(true);
var row = fragment.querySelector('.description-section-row');
sectionsContainer.appendChild(fragment);
initEditor(row);
});

sectionsContainer.addEventListener('click', function (event) {
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];

@BhushanSah BhushanSah Aug 6, 2026

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.

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.

if (editor) {
editor.destroy(true);
}
row.remove();
}
});
});
5 changes: 2 additions & 3 deletions app/templates/main/managePositions.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,9 +33,8 @@ <h1 class="text-center">{{ department_name }} Positions</h1>
<td>{{ position.wls }}</td>
<td>{{ position.revisionDate }}</td>
<td>
<a href="/department/{{ department.ORG }}/{{ department.ACCOUNT }}/positions/{{ position.positionCode }}"
class="btn btn-success view-btn">View</a>
<button class="btn btn-primary request-btn" data-position-id="{{ position.id }}">Revise Position</button>
<a href="{{ url_for('main.postionDescription', org=department.ORG, account=department.ACCOUNT, positionCode=position.positionCode) }}" class="btn btn-success">View</a>
<a href="{{ url_for('main.revisePosition', org=department.ORG, account=department.ACCOUNT, positionCode=position.positionCode) }}" class="btn btn-primary">Revise Position</a>
Comment thread
NYABUTOA marked this conversation as resolved.
</td>
</tr>
{% endfor %}
Expand Down
106 changes: 106 additions & 0 deletions app/templates/main/revisepositionpage.html
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,106 @@
{% extends "base.html" %}

@MImran2002 MImran2002 Aug 5, 2026 •
edited
Loading

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.

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. image

@nahom70 this will be useful for you too

@BhushanSah BhushanSah Aug 6, 2026

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.

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.

{% block scripts %}
{{ super() }}
<script type="text/javascript" src="{{ url_for('static', filename='js/ckeditor/ckeditor.js') }}"></script>
<script type="text/javascript" src="{{ url_for('static', filename='js/revisepositionpage.js') }}?u={{ lastStaticUpdate }}"></script>
<link rel="stylesheet" type="text/css" href="/static/css/individualPositions.css?u={{ lastStaticUpdate }}" />
<link rel="stylesheet" type="text/css" href="/static/css/revisepositionpage.css?u={{ lastStaticUpdate }}" />
{% endblock %}

{% block app_content %}
<div class="department-header-container">
<h1 class="department-header">Revise {{ position.positionTitle }}</h1>
</div>

<div class="container-fluid position-container">
<div class="row">
<div class="col-12">

<form method="POST" action="{{ url_for('main.revisePosition', org=department.ORG, account=department.ACCOUNT, positionCode=position.positionCode) }}">

<div class="position-information">
<div class="form-group row">
<label class="col-sm-3 col-form-label" for="positionTitle">Position Title:</label>
<div class="col-sm-9">
<input type="text" class="form-control" id="positionTitle" name="positionTitle" value="{{ position.positionTitle }}" required>
</div>
</div>

<div class="form-group row">
<label class="col-sm-3 col-form-label">Position Code:</label>
<div class="col-sm-9">
<p class="form-control-static">{{ position.positionCode }}</p>
</div>
</div>

<div class="form-group row">
<label class="col-sm-3 col-form-label" for="wls">WLS Level:</label>
<div class="col-sm-9">
<input type="number" class="form-control" id="wls" name="wls" min="0" max="6" value="{{ position.wls }}" required>
</div>
</div>

<div class="form-group row">
<label class="col-sm-3 col-form-label">Last Revision Date:</label>
<div class="col-sm-9">
<p class="form-control-static">{{ position.revisionDate }}</p>
</div>
</div>

<div class="form-group row">
<label class="col-sm-3 col-form-label">Revised By:</label>
<div class="col-sm-9">
<p class="form-control-static">{{ position.revisedBy }}</p>
</div>
</div>
</div>

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

@MImran2002 MImran2002 Aug 7, 2026

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.

Section Title still shows html tags

Image

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

@brightfietsop-ux brightfietsop-ux Aug 6, 2026

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.

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

<label>Section Title</label>
<input type="text" class="form-control" name="sectionTitle[]" value="{{ section.sectionTitle }}">
</div>
<div class="form-group">
<label>Section Content</label>
<textarea class="form-control" name="sectionContent[]" rows="4">{{ section.sectionContent }}</textarea>
</div>
<button type="button" class="btn btn-danger btn-sm remove-section-btn">Remove Section</button>
</div>
{%- endfor %}
</section>

<button type="button" id="addSectionBtn" class="btn btn-default">Add Section</button>

@MImran2002 MImran2002 Aug 7, 2026

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.

this button should be green button


<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

@MImran2002 MImran2002 Aug 7, 2026

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.

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

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

@MImran2002 MImran2002 Aug 7, 2026

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.

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

</div>
</div>

@MImran2002 MImran2002 Aug 7, 2026

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.

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.

</form>

</div>
</div>
</div>

<template id="sectionRowTemplate">
<div class="description-section-row">
<div class="form-group">
<label>Section Title</label>
<input type="text" class="form-control" name="sectionTitle[]" value="">
</div>
<div class="form-group">
<label>Section Content</label>
<textarea class="form-control" name="sectionContent[]" rows="4"></textarea>
</div>
<button type="button" class="btn btn-danger btn-sm remove-section-btn">Remove Section</button>
</div>
</template>
{% endblock %}