add pagination to results table #96

KWMORALE · 2020-04-15T14:15:52Z

Close issue #39 #37

ITAYC0HEN · 2020-04-15T15:11:25Z

For some reason, I can't make it work with docker-compose.dev.yml:

dev-frontend_1  | Starting the development server...
dev-frontend_1  | 
dev-frontend_1  | Failed to compile.
dev-frontend_1  | 
dev-frontend_1  | ./src/QueryResultsStatus.js
dev-frontend_1  | Module not found: Can't resolve 'react-js-pagination' in '/app/src'

but it does work with the regular one :) The solution looks good in terms of UI, and anything else :D
I want to test it when running a query but all my 3 instances are broken now :(

Good work!

msm-code · 2020-04-15T15:30:59Z

@ITAYC0HEN had the same problem, you need to do a rebuild first (docker-compose -f docker-compose.dev.yml build). That's because this PR adds a new npm dependency (react-js-pagination).

msm-code · 2020-04-15T15:35:18Z

Looks good!

But there are a few things that don't make sense after this change:

query page will still try to load every result in memory (I've opened the page, and it downloaded all 10k results even though I just stayed on the first page) - it should be downloaded lazily on-demand
query page should display the number of results instead of displaying numbre of downloaded results. This is already a part of a different issue ( Mquery is counting results when displaying previous jobs #37 ), so if you don't feel like it feel free to fix it in a different PR.

ITAYC0HEN · 2020-04-15T15:41:24Z

@ITAYC0HEN had the same problem, you need to do a rebuild first (docker-compose -f docker-compose.dev.yml build). That's because this PR adds a new npm dependency (react-js-pagination).

Thanks, fixed this :)

msm-code

Please try to use lazy loading in the table

ITAYC0HEN · 2020-04-15T20:51:13Z

If you're up for lazy load then you can reconsider pagination in favor of. lazy load scrolling (like Facebook, Twitter feeds, VT results)

msm-code · 2020-04-15T21:00:05Z

You forgot mwdb!

Yeah, that's a viable solution too. Anything that will make it possible to reduce a number of queries to the backend for a finished large query.

I prefer paging, but I can see lazy load working here too.

msm-code · 2020-04-17T14:53:24Z

src/mqueryfront/src/QueryPage.js

+                });
+
+                if (newShouldRequest) {
+                    let nextTimeout =


Querying every 50ms may be too extreme. Maybe just hardcode this to 1000ms?

msm-code · 2020-04-17T14:55:32Z

src/mqueryfront/src/QueryPage.js

+            })
+            .catch(() => {
+                this.setState({
+                    shouldRequest: false,


Is this variable used anywhere? If not, we can remove this safely.

msm-code · 2020-04-17T14:59:48Z

src/mqueryfront/src/QueryPage.js

+
+                if (
+                    ["done", "cancelled", "failed", "expired"].indexOf(
+                        response.data.job.status


response.data.job probably can be made a local variable

msm-code · 2020-04-17T15:00:08Z

src/mqueryfront/src/QueryPage.js

+            .then((response) => {
+                let newShouldRequest = true;
+
+                if (


maybe move setState above this, and return in if, instead of using a temprary vraiable here?

msm-code · 2020-04-17T15:01:28Z

src/mqueryfront/src/QueryPage.js

+                let newShouldRequest = true;
+
+                if (
+                    ["done", "cancelled", "failed", "expired"].indexOf(
+                        response.data.job.status
+                    ) !== -1
+                ) {
+                    if (response.data.job.files_processed >= response.data.job.total_files) {
+                        newShouldRequest = false;
+                    }
+                }
+
+                this.setState({
+                    job: response.data.job,
+                });
+
+                if (newShouldRequest) {
+                    let nextTimeout =
+                        response.data.matches.length >= LIMIT ? 50 : 1000;
+                    this.timeout = setTimeout(
+                        () => this.loadJob(),
+                        nextTimeout
+                    );
+                }


All my suggestions in one comment:

Suggested change

let newShouldRequest = true;

if (

["done", "cancelled", "failed", "expired"].indexOf(

response.data.job.status

) !== -1

) {

if (response.data.job.files_processed >= response.data.job.total_files) {

newShouldRequest = false;

}

}

this.setState({

job: response.data.job,

});

if (newShouldRequest) {

let nextTimeout =

response.data.matches.length >= LIMIT ? 50 : 1000;

this.timeout = setTimeout(

() => this.loadJob(),

nextTimeout

);

}

let job = response.data.job;

this.setState({

job: job,

matches: response.data.matches,

});

let doneStatuses = ["done", "cancelled", "failed", "expired"];

let isDone = doneStatuses.indexOf(job.status) !== -1;

let processedAll = job.files_processed >= job.total_files;

if (isDone && processedAll) {

return;

}

this.timeout = setTimeout(() => this.loadJob(), 1000);

After this rewrite it also shows a bug (that I think was there even before your changes) - if the job is cancelled/failed but it didn't process all files, mquery will keep querying the backend. Maybe it should be || instead of &&?

msm-code · 2020-04-17T15:06:50Z

src/mqueryfront/src/QueryResultsStatus.js

@@ -63,13 +64,29 @@ class QueryResultsStatus extends Component {
    constructor(props) {
        super(props);

+        this.state = {
+            activePage: 1,
+            itemPerPage: 20,


Plural:

Suggested change

itemPerPage: 20,

itemsPerPage: 20,

msm-code · 2020-04-17T15:07:07Z

src/mqueryfront/src/QueryResultsStatus.js


        if (this.props.job.status === "expired") {
            return ReturnExpiredJob(this.props.job.error);
        }

+        // var indexOfLastMatch = this.state.activePage * this.state.itemPerPage;


Remove unnecessary comments

msm-code · 2020-04-17T15:07:18Z

src/mqueryfront/src/QueryResultsStatus.js

+                    <div>
+                        <Pagination
+                            activePage={this.state.activePage}
+                            itemsCountPerPage={this.state.itemPerPage}


Suggested change

itemsCountPerPage={this.state.itemPerPage}

itemsCountPerPage={this.state.itemsPerPage}

msm-code · 2020-04-17T15:07:35Z

src/mqueryfront/src/QueryResultsStatus.js

+    handlePageChange(pageNumber) {
+        this.setState({ activePage: pageNumber });
+        this.sendResultsActivePage(pageNumber);
+


unnecessary empty line

msm-code · 2020-04-17T15:07:54Z

src/mqueryfront/src/QueryPage.js

+
+    loadMatches() {
+        const LIMIT = 20;
+        let OFFSET = (this.state.activePage - 1) * 20 + 1;


msm-code · 2020-04-17T15:10:28Z

Nitpick: doing a new query should reset active page number (if i go to the 8th page, run a new query, pagination will automatically start at the 8th page)

msm-code · 2020-04-17T15:11:04Z

And also: please rebase your changes on the master, since there are merge conflicts

msm-code · 2020-04-17T15:58:40Z

src/app.py

        files_processed=job.get("files_processed", 0),
+        files_matched=job.get("files_matched", 0),


Suggested change

files_processed=job.get("files_processed", 0),

files_matched=job.get("files_matched", 0),

files_processed=int(job.get("files_processed", 0)),

files_matched=int(job.get("files_matched", 0)),

msm-code · 2020-04-17T18:42:01Z

src/mqueryfront/src/QueryResultsStatus.js

@@ -164,6 +184,19 @@ class QueryResultsStatus extends Component {
                        </thead>
                        <tbody>{matches}</tbody>
                    </table>
+                    {lenMatches > 20 && (
+                        <div>


I think div is unnecessary here?

msm-code · 2020-04-20T12:45:12Z

src/mqueryfront/src/QueryPage.js

+                let doneStatuses = ["done", "cancelled", "failed", "expired"];
+                let isDone = doneStatuses.indexOf(job.status) !== -1;
+                let processedAll = job.files_processed >= job.total_files;
+                if (isDone && processedAll) {


Still &&? If nothing else changed, this will cause infinite loop of requests.

I've also confirmed it locally. Test plan:

start a big query

stop it in a middle such that files_processed < total_files (remember that they're strings)

observe that requests are repeated in the loop every 1s

msm-code

good to go as soon as tests pass

* Add pagination to results table

KWMORALE requested a review from msm-code April 15, 2020 14:16

msm-code suggested changes Apr 15, 2020

View reviewed changes

KWMORALE marked this pull request as draft April 16, 2020 12:34

KWMORALE requested a review from msm-code April 17, 2020 14:00

KWMORALE marked this pull request as ready for review April 17, 2020 14:04

msm-code suggested changes Apr 17, 2020

View reviewed changes

KWMORALE and others added 11 commits April 17, 2020 18:43

add pagination to results table

941e264

Update QueryResultsStatus.js

0028cda

add total matched files variable / fix increasing matches

ff91db5

fix inreasing matches

8aa79ee

Update daemon.pt

1b91a64

Update query status

aa9444d

Update activePage child to parrent callback

14e99b6

loading per page

4e76217

Update QueryPage.js

6a19bc9

Update QueryPage.js

b1f19ac

after rebase to master

5d7b3cc

KWMORALE force-pushed the feature/pagination branch from d4cc2d5 to 5d7b3cc Compare April 17, 2020 17:34

fix updating active page for new query

37dda14

KWMORALE requested a review from msm-code April 17, 2020 18:24

msm-code reviewed Apr 17, 2020

View reviewed changes

KWMORALE added 2 commits April 20, 2020 10:56

Update QueryRecentsStatus and QueryPage

8f49a92

prettier QueryPage.js

e3a8521

KWMORALE requested a review from msm-code April 20, 2020 09:01

Remove unused variable from QueryPage.js

f9adf66

msm-code suggested changes Apr 20, 2020

View reviewed changes

KWMORALE added 3 commits April 20, 2020 15:06

Update QueryPage.js

ad06943

Update db.py

8dbf0c3

Update db.py

5cf2374

msm-code approved these changes Apr 20, 2020

View reviewed changes

msm-code merged commit fe49b19 into master Apr 20, 2020

msm-code deleted the feature/pagination branch April 20, 2020 15:16

This was referenced Apr 28, 2020

Mquery is counting results when displaying previous jobs #37

Closed

Add pagination or lazy-load to results table #39

Closed

KWMORALE added a commit to KWMORALE/mquery that referenced this pull request Jul 29, 2020

add pagination to results table (CERT-Polska#96)

06b39e6

* Add pagination to results table

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add pagination to results table #96

add pagination to results table #96

KWMORALE commented Apr 15, 2020 •

edited

Loading

ITAYC0HEN commented Apr 15, 2020

msm-code commented Apr 15, 2020

msm-code commented Apr 15, 2020

ITAYC0HEN commented Apr 15, 2020

msm-code left a comment

ITAYC0HEN commented Apr 15, 2020

msm-code commented Apr 15, 2020

msm-code Apr 17, 2020

msm-code Apr 17, 2020

msm-code Apr 17, 2020

msm-code Apr 17, 2020

msm-code Apr 17, 2020

msm-code Apr 17, 2020

msm-code Apr 17, 2020

msm-code Apr 17, 2020

msm-code Apr 17, 2020

msm-code Apr 17, 2020

msm-code Apr 17, 2020

msm-code commented Apr 17, 2020

msm-code commented Apr 17, 2020

msm-code Apr 17, 2020

msm-code Apr 17, 2020

msm-code Apr 20, 2020

msm-code Apr 20, 2020

msm-code left a comment

	itemsCountPerPage={this.state.itemPerPage}
	itemsCountPerPage={this.state.itemsPerPage}

		files_processed=job.get("files_processed", 0),
		files_matched=job.get("files_matched", 0),

add pagination to results table #96

add pagination to results table #96

Conversation

KWMORALE commented Apr 15, 2020 • edited Loading

ITAYC0HEN commented Apr 15, 2020

msm-code commented Apr 15, 2020

msm-code commented Apr 15, 2020

ITAYC0HEN commented Apr 15, 2020

msm-code left a comment

Choose a reason for hiding this comment

ITAYC0HEN commented Apr 15, 2020

msm-code commented Apr 15, 2020

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

msm-code commented Apr 17, 2020

msm-code commented Apr 17, 2020

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

msm-code left a comment

Choose a reason for hiding this comment

KWMORALE commented Apr 15, 2020 •

edited

Loading