Skip to content

handle 400 search event errors Issue #57 #62

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

Merged
malvpatel merged 4 commits into from
May 18, 2024
Merged

handle 400 search event errors Issue #57 #62

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
9124a03
handle 400 search event errors Issue #57
malvpatel May 18, 2024
ab45bd0
Merge remote-tracking branch 'origin/main' into handle_400_search_errors
malvpatel May 18, 2024
909c5d0
Merge remote-tracking branch 'origin/main' into handle_400_search_errors
malvpatel May 18, 2024
3a6831a
show alert instead of triggering fallback on 403 error for search eve…
malvpatel May 18, 2024
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
9 changes: 9 additions & 0 deletions html/classhelper.css
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -116,3 +116,12 @@ table tr:hover {
flex-grow: 1;
}

.search-error input {
border: 2px solid red;
}

.search-error > .error-message {
color: red;
font-size: 14px;
margin-left: 4px;
}
55 changes: 54 additions & 1 deletion html/classhelper.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -607,6 +607,16 @@ class ClassHelper extends HTMLElement {
search.addEventListener("click", (e) => {
e.preventDefault();
let fd = new FormData(form);

let hasError = this.popupRef.document.getElementsByClassName("search-error").item(0);
if (hasError != null) {
let current = fd.get(hasError.dataset.errorField)
let prev = hasError.dataset.errorValue;
if (current === prev) {
return;
}
}

this.dispatchEvent(new CustomEvent("search", {
detail: {
value: fd
Expand Down Expand Up @@ -1163,7 +1173,7 @@ class ClassHelper extends HTMLElement {
}

/** method when search is performed within classhelper, here we need to update the classhelper table with search results
* @param {URL | string} apiURL
* @param {URL} apiURL
* @param {HelpUrlProps} props
* @throws {Error} when fetching or parsing data from roundup rest api fails
*/
Expand Down Expand Up @@ -1198,6 +1208,42 @@ class ClassHelper extends HTMLElement {
throw new Error(message, { cause: error });
}

if (!resp.ok && resp.status === 400) {
// In the error message we will have the field name that caused the error.
// and the value that caused the error, in a double quoted string
// <some text> "(value)" <some text> "(key)", this regex is a capture group
// that captures the value and key in the error message.
let regexCaptureDoubleQuotedString = /"(.*?)"/g;
let iterator = json.error.msg.matchAll(regexCaptureDoubleQuotedString);
let results = Array.from(iterator);

if (results.length == 2) {
let value = results[0][1];
let field = results[1][1];

// Find the input element with the name of the key
let input = this.popupRef.document.getElementsByName(field).item(0);
if (input) {
let parent = input.parentElement;
parent.classList.add("search-error");
parent.dataset.errorValue = value;
parent.dataset.errorField = field;
// remove if there was already an error message
parent.getElementsByClassName("error-message").item(0)?.remove();
let span = document.createElement("span");
span.classList.add("error-message");
span.textContent = `Invalid value: ${value}`;
parent.appendChild(span);
return;
}
}
}
Copy link
Contributor

@rouilj rouilj May 18, 2024

Choose a reason for hiding this comment

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

I like providing error feedback to the user. I'm inclined to accept this even though it does expose
some issues.

In theory the json message can be translated. This method requires that the structure of the message
(value occurs before key and both are quoted strings) doesn't change. It also requires that there is only
one message that can be returned with a matching structure. If either of those requirements are not met,
I expect

this.popupRef.document.getElementsByName(field).item(0);

will not match anything and it looks like your code handles that case. That would lead to a missing
error message.

This means different feedback (UI) for the same issue depending on the language. IIRC the REST interface messages you are using are not currently translated. So this isn't an issue at the moment. It does create a
close coupling between a human readable (vs. machine readable) message generated from
the back end and the correct opertation of the classhelper popup. This will require some additional
care.

Please add a test to @BharathKanama test file. Have the test rigger an error that causes this code to
run and verify that the error message is displayed. You will probably need to log in as the demo user
and do a search expected to trigger the error.

One other area for translation is the use of:

span.textContent = `Invalid value: ${value}`;

This is a phrase and isn't as simple as translating a word. E.G. in some language this might have
to be expressed with a structure like "Value ${value} is not correct".
Adding the literal string: Invalid value: ${value} to the translation template and then expanding the
result should work. But this is more complex than just single word translations which have been
used up to this point and exposes some aditional risk.

So I say keep this code as is for the time being.


if (!resp.ok && resp.status === 403) {
this.popupRef.alert(json.error.msg);
return;
}

if (!resp.ok) {
let message = `Unexpected response\n`;
message += `url: ${apiURL.toString()}\n`;
Expand Down Expand Up @@ -1228,6 +1274,13 @@ class ClassHelper extends HTMLElement {
const popupBody = this.popupRef.document.body;
const pageIndex = selfPageURL.searchParams.get("@page_index");

// remove any previous error messages
let errors = Array.from(popupDocument.getElementsByClassName("search-error"));
errors.forEach(element => {
element.classList.remove("search-error");
element.getElementsByClassName("error-message").item(0)?.remove();
});

const oldPaginationFrag = popupDocument.getElementById("popup-pagination");
let newPaginationFrag = this.getPaginationFragment(prevPageURL, nextPageURL, pageIndex, props.pageSize, collection.length);
popupBody.replaceChild(newPaginationFrag, oldPaginationFrag);
Expand Down