Projects Phase Save Did Nothing - edmozley/freeitsm GitHub Wiki
Fixed in 3.5.1 · Reported in #175 by @LvNixia · Present in 3.3.0, 3.4.0 and 3.5.0 · Module: Projects
On a project's Plan tab, + Add a phase (or editing an existing phase) opened the dialog, but Save did nothing. Pressing Enter did nothing either. The dialog stayed open and nothing was saved.
The cause was two dialogs on the same page giving their parts the same names. When the page's code asked for "the Save button", it got the Save button from the stakeholder dialog instead, so the phase dialog's own Save button was never wired up.
While fixing it, a second clash of the same kind turned up: Announce disruption on a project's Connections tab ignored the title you typed and answered "Give the maintenance a title."
Both are fixed. Nothing wrong was ever saved: the server refused every bad request these bugs sent.
The reporter diagnosed this exactly, down to the element ids and the fix. That made it a quick fix.
- Plan → + Add a phase (or the pencil on an existing phase): fill it in, press Save - nothing. No error, no request, the dialog stays open. Enter in the name box: the same.
- Connections → Announce disruption: type a title, press Save - "Give the maintenance a title.", however many times you tried.
- People → a stakeholder's Place / Edit: this one did save. But each Save also sent a second request, for a nameless phase, which the server refused. You would not normally have noticed.
Every part of a web page that the code needs to find again gets a name, called its id. The rule is that an id is used once per page. The code then asks the page for "the element called psSave" and gets exactly one thing back.
The project page holds many dialogs, all present in the page from the start and shown when needed. Two pairs of them broke the rule:
| Dialog | Its part | Clashed with |
|---|---|---|
| Stakeholder (People tab) | the header name, the error line, the Save button: psName, psError, psSave
|
the phase dialog's name box, error line and Save button, with the same three ids |
| Ask AI panel | its header: paTitle
|
the Announce disruption dialog's title box, also paTitle
|
When two things share an id, the browser does not complain. It quietly returns whichever comes first in the page. The stakeholder dialog comes before the phase dialog, and the Ask AI panel comes before the announce dialog, so the newer dialog lost both times.
document.getElementById() returns the first element in document order with that id. Duplicates are invalid HTML, but nothing reports them: no console warning, no failed lookup.
In projects/view.php the stakeholder modal (#prjStakeModal) is rendered before the stage modal (#prjStageModal). Two scripts each wired "their" Save button at start-up:
// assets/js/projects-tools.js
document.getElementById('psSave').addEventListener('click', saveStake);
// assets/js/projects-view.js
document.getElementById('psSave').addEventListener('click', saveStage);
document.getElementById('psName').addEventListener('keydown', e => { if (e.key === 'Enter') { ... saveStage(); } });Both lookups found the stakeholder button. So:
- the stage modal's Save button had no listener at all, and Enter was bound to a
<span>that never receives keystrokes; - the stakeholder Save ran
saveStakeandsaveStage.saveStagereadpsName- now the stakeholder header<span>, whose.valueisundefined- and posted a stage with an empty name toapi/projects/stage_save.php.ProjectsService::saveStage()refuses a blank name ("Give it a name."), so nothing was created.
The announce clash is less obvious, because it looks like it works. The Ask AI panel's header is a <div id="paTitle">. openAnnounce() ran getElementById('paTitle').value = '', which on a <div> doesn't throw: it just adds a plain JavaScript property called value to the div. saveAnnounce() then read that property back, '', and posted an empty title. The real input, further down the page, was never read. statusPlannedSave() refused the empty title with "Give the maintenance a title.", which was what you saw.
The first diagnosis of the announce bug here was wrong. Reading the code, it looked like
.valueon a<div>would beundefinedand.trim()would throw. Running the page showed it was'', becauseopenAnnounce()had set that property moments earlier. The symptom in the table above comes from the run, not from reading the code.
The issue named the phase dialog. Checking every id on the rendered page turned up the second clash, on the Ask AI panel, which came in with 3.3.0's Ask AI. Both are now fixed, and the page has no repeated ids.
The same check over every PHP page in the codebase found three more files with a repeated id. None of them is a real bug:
| Page | Id | Why it is fine |
|---|---|---|
auth/login.php |
localLoginForm, username, password
|
the two copies are in opposite branches of an if/else; only one is ever sent |
self-service/tickets.php |
tkFilter |
the same: one for the personal view, one for the team view |
reporting/intune/index.php |
drillCount |
the second is JavaScript rebuilding the heading's contents, replacing the first |
The reporter's suggestion, taken as written, plus the second clash:
-
Stakeholder dialog: every id renamed from
ps*topstk*(pstkName,pstkPower,pstkInterest,pstkStance,pstkKeep,pstkError,pstkSave), inprojects/view.phpandassets/js/projects-tools.js. Only three ids clashed, but the whole set was renamed, so the stakeholder dialog has its own prefix and the next phase field cannot clash with it. -
Ask AI panel header:
paTitle→paHeading, with itsaria-labelledby. No script looked it up by id, so that was the only change. - The phase and announce dialogs keep their ids, so
assets/js/projects-view.jsdid not change.
| File | Change | |
|---|---|---|
| 🟥 | projects/view.php |
stakeholder modal ids ps* → pstk*; Ask AI header paTitle → paHeading; assets/js/projects-tools.js cache-buster bumped to v12 |
| 🟦 | assets/js/projects-tools.js |
openStake(), saveStake() and the Save listener use the pstk* ids |
The real project page (an active demo project with stakeholders and announcements switched on) was opened in headless Chrome, signed in as an administrator. Every save request was intercepted and answered with a stub error, so nothing was written to the database. The test then clicked through each dialog the way a person would and recorded which requests left the page.
| Old code (control) | Fixed code | |
|---|---|---|
| Repeated ids on the page |
psName, psError, psSave, paTitle
|
none |
| Phase dialog: Save | no request | one api/projects/stage_save.php, with the typed name |
| Phase dialog: Enter in the name | no request | one api/projects/stage_save.php, with the typed name |
| Stakeholder: Save |
api/projects/tools.php member update plus a api/projects/stage_save.php with an empty name |
api/projects/tools.php member update only |
| Announce: Save |
api/projects/tools.php announce with "title":""
|
api/projects/tools.php announce with the typed title |
The left-hand column is the control: the same test, run against the code as it was before the fix, reproduces every symptom.
Not tested: a real save going all the way to the database (the requests were stubbed on purpose, because the test ran against real data). The request bodies are exactly what the unchanged server code already accepts, and nothing on the server changed. Not checked on a phone, where these dialogs are the same markup.
Update to 3.5.1 and hard-refresh the browser (Ctrl-F5) so the new assets/js/projects-tools.js loads. Phases, stakeholders and disruption announcements then save normally. There is nothing to repair: no phase, stakeholder or announcement was saved wrongly while the bug was live.
Related: Projects · Projects - Developer Guide · Bugs resolved