fix: refresh ursadb page and add alert about successful submit #448

mickol34 · 2025-02-27T10:00:17Z

Your checklist for this pull request

I've read the contributing guideline.
I've tested my changes by building and running mquery, and testing changed functionality (if applicable)
I've added automated tests for my change (if applicable, optional)
I've updated documentation to reflect my change (if applicable)

What is the current behaviour?

Currently theres no feedback to user about successful submit to UrsaDB page.

What is the new behaviour?

This PR introduces new alert about successful POST request and clears form upon submission

Test plan

Click through new page and check for any unexpected behavior

Closing issues

fixes #issuenumber

msm-cert · 2025-03-04T13:39:08Z

src/mqueryfront/src/indexFiles/IndexPage.js

@@ -130,11 +269,14 @@ class IndexPageInner extends Component {
                        <div className="my-2">{`Newest file in the queue: ${this.state.newestFile}`}</div>
                    )}
                    {this.state.oldestFile && (
-                        <div className="my-2">{`Oldest file in the queue`}</div>
+                        <div className="my-2">{`Oldest file in the queue: ${this.state.oldestFile}`}</div>


🤔 missed that previously

msm-cert · 2025-03-04T13:57:16Z

src/mqueryfront/src/indexFiles/IndexPage.js

@@ -130,11 +269,14 @@ class IndexPageInner extends Component {
                        <div className="my-2">{`Newest file in the queue: ${this.state.newestFile}`}</div>


Looking at it more deeply, the message looks like:
Newest file in the queue: 2025-03-04T12:58:25.030027
The date format is a bit over the top (I know it's the ISO format). JS is not very good at formatting dates, but maybe we
can convert it to 2025-03-04 12:58 at least?

Should I also remove seconds or maybe they are still relevant? For now maybe I'll leave seconds, later this can easily be changed by changing slicing index.

msm-cert · 2025-03-04T14:07:05Z

src/mqueryfront/src/indexFiles/IndexPage.js

+    fileOrFiles(length) {
+        return `file${length > 1 ? "s" : ""}`;
+    }


Nice, I wondered about suggesting this refactoring in the previous PR.

But it's usually handled a bit differently, with a helper like pluralize:

const pluralize = (count, noun) => `${count} ${noun}${count !== 1 ? "s" : ''}`;

This is a bit more generouns, as you can use it for things other than file. More importantly, you don't have to do

${this.state.alertShowFileLen} ${this.fileOrFiles(this.state.alertShowFileLen)}!

and instead can just

${pluralize("files", this.state.alertShowFileLen}}!

which is nice I guess

Yeah, I'll generalize this function to all nouns, but I think it would be better to leave first ${count} out of this function.

msm-cert · 2025-03-04T14:09:55Z

src/mqueryfront/src/indexFiles/IndexPage.js

+                    )}
+                    {this.state.alertShowCleared && (
+                        <IndexClearedPage
+                            msg={`Successfully cleared ${


Successfully cleared false file from queue default!

I think the variable here should be alertShowCleared?

I'm not sure if I understand - this alert won't show if alertShowCleared is false (this variable is boolean). Variable alertShowFileLen stores amount of submitted files and this is the value we want to return in feedback. Is something not presenting correctly on your version?

step one: add 1 file

step two: add 2 files

now there are three files in the queue (verify by select * from queuedfile;)
step three: clear queue
this should clear three fules, but the result is either:

or

depending on if the previous banner was closed. Also in the latter situation, I can now add one file again, and get:

so I believe there is some variable confusion here 🦆

You're right that my comment was wrong, I think that the issue is that handleClearQueue doesn't set FileLen? But I leave this to you, anyway something's wrong I belive

Yes, I mistakenly used the same variable used to store sent files to also show amount of cleared files. Unfortunately I think the best way to handle this issue was to simplify cleared queue alert not to use any numbers, since it's non-obvious how many files are currently in queue after submitting some amount (and later if queued files would happen to change status server-side whilst page would not update).

msm-cert · 2025-03-04T14:11:40Z

src/mqueryfront/src/indexFiles/IndexPage.js

                            {`Add to queue${
                                fileLen > 0
-                                    ? ` (${fileLen} file${
-                                          fileLen > 1 ? "s" : ""
-                                      })`
+                                    ? ` (${fileLen} ${this.fileOrFiles(
+                                          fileLen
+                                      )})`
                                    : ""


The same comment about pluralize here - it will work if you reorder it to add N files to queue which also makes it a bit more idiomatic english

msm-cert · 2025-03-04T14:14:07Z

src/mqueryfront/src/App.css

+    top: 50%;
+    left: 50%;
+    transform: translate(-50%, -50%);
+    z-index: auto;


isn't z-index: auto the default value? Unless there's some inheritance going on here.

Leftover from modal styling 😁 Will clear this.

msm-cert · 2025-03-13T14:55:01Z

src/mqueryfront/src/indexFiles/IndexPage.js

+                    )}
+                    {this.state.alertShowCleared && (
+                        <IndexClearedPage
+                            msg={`Successfully cleared ${


step one: add 1 file

step two: add 2 files

now there are three files in the queue (verify by select * from queuedfile;)
step three: clear queue
this should clear three fules, but the result is either:

or

depending on if the previous banner was closed. Also in the latter situation, I can now add one file again, and get:

so I believe there is some variable confusion here 🦆

You're right that my comment was wrong, I think that the issue is that handleClearQueue doesn't set FileLen? But I leave this to you, anyway something's wrong I belive

fix: refresh ursadb page and add alert about successful submit

a543faa

mickol34 requested a review from msm-cert February 27, 2025 10:00

mickol34 added 2 commits February 27, 2025 11:01

remove unnecessary ID

ab5afb7

feat: add modal and clear alert to index page

95560c3

msm-cert reviewed Mar 4, 2025

View reviewed changes

mickol34 added 2 commits March 11, 2025 11:38

fix: date-formatting and plural form refactor

977df9f

lint: prettier

e9a3422

msm-cert requested changes Mar 13, 2025

View reviewed changes

mickol34 added 2 commits March 14, 2025 11:58

fix: remove fileLen from clear alert

55e120b

fix: remove debug button + prettier

5eda1aa

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: refresh ursadb page and add alert about successful submit #448

fix: refresh ursadb page and add alert about successful submit #448

mickol34 commented Feb 27, 2025 •

edited

Loading

msm-cert Mar 4, 2025

msm-cert Mar 4, 2025

mickol34 Mar 11, 2025

msm-cert Mar 4, 2025

mickol34 Mar 11, 2025

msm-cert Mar 4, 2025

mickol34 Mar 11, 2025

msm-cert Mar 13, 2025

mickol34 Mar 14, 2025

msm-cert Mar 4, 2025

msm-cert Mar 4, 2025

mickol34 Mar 11, 2025

msm-cert Mar 13, 2025

		@@ -130,11 +269,14 @@ class IndexPageInner extends Component {
		<div className="my-2">{`Newest file in the queue: ${this.state.newestFile}`}</div>

fix: refresh ursadb page and add alert about successful submit #448

Are you sure you want to change the base?

fix: refresh ursadb page and add alert about successful submit #448

Conversation

mickol34 commented Feb 27, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mickol34 commented Feb 27, 2025 •

edited

Loading