Feature/logs paging active #318

jordibc · 2018-04-24T16:34:53Z

Add a "get more logs" button that loads logs from previous pages.

There are no changes to the dataStore configuration, we just use it appropriately in the main application to show older logs on request.

It introduces two new variables to the status: logsFilter: function that selects the relevant logs (we use it to show only the logs that include the selected datasets). logsPageLast: index of the last page that we have retrieved so far. The Log dialog now contains a new action, a flat button that allows it to load logs from previous pages. There are new semantics for the getLogs() function: if it returns null, then there are no more log pages (so we do not need to request more). Also, the pages are a positive number, that is, page 1 is the page previous to the last one (instead of it being page -1).

Remove the optimization for not retrieving the dataStore multiple times unnecessarily. It turns out that it is negligible, and thus it is better to have a simpler code.

So, when concatenated, the position of the last log is predictable (it is in position 0), and also of the most recent log (it is in the last position of the array).

When requesting a new log page in the log dialog, it will now show a button that includes the date of the last log retrieved so far.

So we show the correct date in the button to get older log pages.

…t order

tokland · 2018-05-16T11:05:44Z

src/DataSets/DataSets.component.js

@@ -74,6 +74,10 @@ const DataSets = React.createClass({
        };
    },

+    tr(text, variables={}) {


Very minor: variables={} is idiomatic Python for keyword args, but AFAIK in JS is idiomatic to write the spaces: variables = {}

tokland · 2018-05-16T11:08:31Z

src/DataSets/DataSets.component.js

+        if (Array.isArray(datasets)) {
+            const ids = datasets.map(ds => ds.id);
+            const idsSet = new Set(ids);
+            this.state.logsFilter = log => log.datasets.some(ds => idsSet.has(ds.id));


Imperatively modifying this.state is forbidden in React (except the initial set in a constructor) -> this.setState({...}).

tokland · 2018-05-16T11:25:59Z

src/DataSets/DataSets.component.js

+        }
+
+        let title = this.tr("logs");
+        if (Array.isArray(datasets)) {


This kind of checks is a hint that a refactor is needed. I see openLogs is called on the global button onClick, it gets the mouse event, which is what is being compared here. I'd create a separate openAllLogs method for that action.

tokland · 2018-05-16T11:27:53Z

src/DataSets/DataSets.component.js

+    addLogs(pages) {
+        // Add logs from the given log pages (to the already loaded logs if any).
+        if (!Array.isArray(pages))
+            pages = [this.state.logsPageLast + 1];


To avoid this check+rebinding of pages, we can make sure the caller sends the correct values. I think the check is needed in onClick={this.addLogs}.

tokland · 2018-05-16T11:28:41Z

src/DataSets/DataSets.component.js

+                this.setState({logsPageLast: -1});
+            } else {
+                const logsOldestDate = logs.length > 0 ? logs[logs.length - 1].date : null;
+                logs = _(logs).filter(this.state.logsFilter).value();


Avoid rebinding/reuse of variables, create new const values with descriptive names -> const filteredLogs = ...

tokland · 2018-05-16T11:30:25Z

src/DataSets/DataSets.component.js

@@ -303,21 +332,37 @@ const DataSets = React.createClass({

        const logActions = [
            <FlatButton
-                label={this.getTranslation('close')}
+                label={(this.state.logsPageLast < 0 ? this.tr("logs_no_older") : this.tr("logs_older")) +


Too much logic for a prop, we can use a variable.

tokland · 2018-05-17T09:54:55Z

src/DataSets/log.js

+    const store = await getStore();
+    const logsPageCurrent = await store.get('logs-page-current').catch(() => 0);
+    const pagesNames = pages.map(n => 'logs-page-' + mod(logsPageCurrent - n, maxLogPages));
+    const logs = await Promise.all(pagesNames.map(name => store.get(name).catch(() => null)));


I've seen there is a way to avoid 404 on non-existing pages. The store has a method to get the current keys. Thus we could also remove the catch, which might mask real problems (network issues/implementation bugs). Just an idea:

const storeKeys = new Set(await store.getKeys()); const existingPageNames = pagesNames.filter(name => storeKeys.has(name)); const logs = await Promise.all(existingPageNames.map(name => store.get(name)));

tokland · 2018-05-17T09:59:21Z

src/DataSets/DataSets.component.js

@@ -303,21 +332,37 @@ const DataSets = React.createClass({

        const logActions = [
            <FlatButton
-                label={this.getTranslation('close')}
+                label={(this.state.logsPageLast < 0 ? this.tr("logs_no_older") : this.tr("logs_older")) +


Related to the 404 avoidance for non-existing pages, I was thinking we could use the same idea: since we already know the existing pages in the store, we can hide the button if we are sure there is no more data to load.

tokland · 2018-05-17T10:01:41Z

src/DataSets/DataSets.component.js

@@ -303,21 +332,37 @@ const DataSets = React.createClass({

        const logActions = [
            <FlatButton


A question about the UX, @adrianq : does it make sense to move this action inside the logs box, as a center button? this way the user only sees it when it really makes sense.

adrianq · 2018-05-17T12:43:38Z

@tokland I talked to @jordibc and it looks like he is not gonna have time before Tuesday for this. Can you please implement the changes yourself in this branch? I will make a final review afterwards and have a look at the UX...

- Apply styling (spaces/indent) - Create wrapper methods openAllLogs/addNextLog - Render loadMoreLogs button only if necessary - Move loadMoreLogs button to logs list - Abstract logs.getCurrentPage - Refactor getLogs - Fix: Clear always next page in the circular buffer - Format date in logs rendering.

tokland · 2018-05-18T12:38:25Z

I've applied all my proposed changes and some extra refactor.

Also implements the "sharing: save only on changes" feature, which requires EyeSeeTea/d2-ui#20

…aging-active

tokland · 2018-05-23T11:00:41Z

Task "Sharing: Move logging to parent component, so it logs only once when dialog is closed." -> Done. Ready to test again.

jordibc added 7 commits April 23, 2018 13:22

Cosmetics: nicer indentation

c4b87ad

Cosmetics: simplify the api slightly

5664a83

Remove the optimization for not retrieving the dataStore multiple times unnecessarily. It turns out that it is negligible, and thus it is better to have a simpler code.

Cosmetics

5192e1c

Load log pages with the last page first

59da995

So, when concatenated, the position of the last log is predictable (it is in position 0), and also of the most recent log (it is in the last position of the array).

Show a more clear message about loading older logs

022bb00

When requesting a new log page in the log dialog, it will now show a button that includes the date of the last log retrieved so far.

Fix loading of the oldest log first

666db78

So we show the correct date in the button to get older log pages.

jordibc requested a review from adrianq April 24, 2018 16:34

jordibc added the testing label Apr 24, 2018

tokland added the in progress label Apr 24, 2018

Make it the responsability of getLogs() to return them in recent-firs…

8796f5e

…t order

adrianq removed the in progress label Apr 27, 2018

adrianq requested a review from tokland May 16, 2018 09:05

tokland requested changes May 17, 2018

View reviewed changes

tokland added 2 commits May 18, 2018 13:42

Use new SharingDialog prop <onSave> to log on changes

ecb4fc7

tokland added 3 commits May 18, 2018 14:40

Merge remote-tracking branch 'origin/development' into feature/logs-p…

bb65913

…aging-active

Save unique log when sharing dialog is closed with changes

a518416

Use also userAccesses field in Sharing

2ae1742

adrianq merged commit 7333909 into development May 24, 2018

adrianq removed the testing label May 24, 2018

adrianq deleted the feature/logs-paging-active branch May 10, 2019 08:35

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature/logs paging active #318

Feature/logs paging active #318

jordibc commented Apr 24, 2018

tokland May 16, 2018

tokland May 16, 2018

tokland May 16, 2018

tokland May 16, 2018

tokland May 16, 2018

tokland May 16, 2018

tokland May 17, 2018

tokland May 17, 2018 •

edited

Loading

tokland May 17, 2018 •

edited

Loading

adrianq commented May 17, 2018

tokland commented May 18, 2018 •

edited

Loading

tokland commented May 23, 2018

		@@ -303,21 +332,37 @@ const DataSets = React.createClass({

		const logActions = [
		<FlatButton

Feature/logs paging active #318

Feature/logs paging active #318

Conversation

jordibc commented Apr 24, 2018

tokland May 16, 2018

Choose a reason for hiding this comment

tokland May 16, 2018

Choose a reason for hiding this comment

tokland May 16, 2018

Choose a reason for hiding this comment

tokland May 16, 2018

Choose a reason for hiding this comment

tokland May 16, 2018

Choose a reason for hiding this comment

tokland May 16, 2018

Choose a reason for hiding this comment

tokland May 17, 2018

Choose a reason for hiding this comment

tokland May 17, 2018 • edited Loading

Choose a reason for hiding this comment

tokland May 17, 2018 • edited Loading

Choose a reason for hiding this comment

adrianq commented May 17, 2018

tokland commented May 18, 2018 • edited Loading

tokland commented May 23, 2018

tokland May 17, 2018 •

edited

Loading

tokland May 17, 2018 •

edited

Loading

tokland commented May 18, 2018 •

edited

Loading