Skip to content
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.

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

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

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

mickol34
Copy link
Collaborator

@mickol34 mickol34 commented Feb 27, 2025

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

@mickol34 mickol34 requested a review from msm-cert February 27, 2025 10:00
@@ -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>
Copy link
Member

Choose a reason for hiding this comment

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

🤔 missed that previously

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

Choose a reason for hiding this comment

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

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?

Comment on lines +86 to +88
fileOrFiles(length) {
return `file${length > 1 ? "s" : ""}`;
}
Copy link
Member

Choose a reason for hiding this comment

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

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

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

Choose a reason for hiding this comment

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

Successfully cleared false file from queue default!

I think the variable here should be alertShowCleared?

Comment on lines 258 to 263
{`Add to queue${
fileLen > 0
? ` (${fileLen} file${
fileLen > 1 ? "s" : ""
})`
? ` (${fileLen} ${this.fileOrFiles(
fileLen
)})`
: ""
Copy link
Member

Choose a reason for hiding this comment

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

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

top: 50%;
left: 50%;
transform: translate(-50%, -50%);
z-index: auto;
Copy link
Member

Choose a reason for hiding this comment

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

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants