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

Display operation types #175

Merged
merged 1 commit into from Sep 2, 2015

Conversation

Projects
None yet
2 participants
@ZeHiro
Copy link
Contributor

ZeHiro commented Aug 29, 2015

Known issue : no label in english (and no default display), don't know why....
I also need to implent types in the generation of dummy operation for proper types.
In fact thie PR is more to say "hello, I have new things to publish, can you wait for it before the new version of kresus ;)"

@ZeHiro

This comment has been minimized.

Copy link
Contributor Author

ZeHiro commented Aug 31, 2015

@bnjbvr One final test is needed, and will be done tonight. Then it's ready 😊

@bnjbvr

This comment has been minimized.

Copy link
Collaborator

bnjbvr commented Aug 31, 2015

Thanks for the pull request! I actually need to merge really soon (hopefully today), as some bank-side changes can provoke some data loss (see also #135 and #169). Don't worry, when your PR is ready, I can do a small version upgrade with your changes (I'll try to have more frequent updates).

In the meanwhile, I can provide you with some details:

  • in the operations list table, the sum of columns width (col-sm-2 == 2 columns wide) must be no more than 12, otherwise bootstrap can't handle it.
  • there is no need for a locale/en.js, because the default translation (that is, english) is directly encoded at the places where it's used. For instance: t('translation.key') || 'Default message in English'.
  • trailing spaces in routes.coffee
  • randInt has the same effect as rand in the mock file

Other than that, the PR looks good, thanks for doing this.

@ZeHiro

This comment has been minimized.

Copy link
Contributor Author

ZeHiro commented Aug 31, 2015

@bnjbvr Good for me.

@bnjbvr

This comment has been minimized.

Copy link
Collaborator

bnjbvr commented Aug 31, 2015

I get an error on startup when running your incoming branch. Can you make sure it runs fine the first time, please? (sorry this won't go in this release, but will be eligible for the next one, happening around the next Cozy meetup, that would be September 17th). Cheers!

@bnjbvr

This comment has been minimized.

Copy link
Collaborator

bnjbvr commented Aug 31, 2015

First time:

[2015-08-31 23:08:10:996] error - models/operationtype | .zy/kresus/build/server/models/operationtype.js:80 | Error: 3 is undefined, please contact a kresus maintainer
/home/ben/code/cozy/kresus/build/server/models/operationtype.js:81
      return ListOperationType["0"].id;
                                   ^
TypeError: Cannot read property 'id' of undefined
    at Function.OperationType.getOperationTypeID (/home/ben/code/cozy/kresus/build/server/models/operationtype.js:81:36)
    at /home/ben/code/cozy/kresus/build/server/lib/accounts-manager.js:243:43
    at null._onTimeout (/home/ben/code/cozy/kresus/build/server/lib/sources/mock.js:155:14)
    at Timer.listOnTimeout (timers.js:110:15)

Then, subsequent runs:

      docType = this.getDocType();
                     ^
TypeError: undefined is not a function
    at Object.pouchdbDataAdapter.updateAttributes (/home/ben/code/cozy/kresus/node_modules/cozy-db-pouchdb/lib/pouchmodel.js:101:22)
    at Function.Model.updateAttributes (/home/ben/code/cozy/kresus/node_modules/cozy-db-pouchdb/lib/model.js:92:27)
    at /home/ben/code/cozy/kresus/build/server/models/operationtype.js:35:32
    at /home/ben/code/cozy/kresus/node_modules/cozy-db-pouchdb/lib/model.js:200:18
    at /home/ben/code/cozy/kresus/node_modules/cozy-db-pouchdb/lib/pouchmodel.js:295:18
    at /home/ben/code/cozy/kresus/node_modules/cozy-db-pouchdb/node_modules/pouchdb/node_modules/pouchdb-mapreduce/utils.js:17:9
    at process._tickCallback (node.js:355:11)

Looks like there is some race condition in here...

@ZeHiro

This comment has been minimized.

Copy link
Contributor Author

ZeHiro commented Aug 31, 2015

By first time, you mean "with a fresh cozy install?"

@bnjbvr

This comment has been minimized.

Copy link
Collaborator

bnjbvr commented Sep 1, 2015

No, I meant "right before I run the patched app", so I had accounts, operations, etc. But I guess the issue would show up also if you don't have any accounts.

@ZeHiro

This comment has been minimized.

Copy link
Contributor Author

ZeHiro commented Sep 1, 2015

I patched it again, and it seems fixed. i used americano instead of ../db for the declation of the model, and my test plaftorm already had the docType declared, so the error wasn't raised. I tested this morning on a fresh Vagrant image (empty DB), and everything went well.
But I observed a CSS problem in firefox 40 for fedora, but not in gnome browser. The borders of the colums are not well displayed now. Any hint?

@bnjbvr

This comment has been minimized.

Copy link
Collaborator

bnjbvr commented Sep 1, 2015

Regarding the CSS issue, I don't see any possible reason, sorry. What do you mean by "not well displayed"?

@ZeHiro

This comment has been minimized.

Copy link
Contributor Author

ZeHiro commented Sep 1, 2015

I have no border on some columns. But on other I can see them.

@bnjbvr

This comment has been minimized.

Copy link
Collaborator

bnjbvr commented Sep 1, 2015

Does it also happen with recent versions of Firefox? (Aurora / Dev Edition, or Nightly?)

@ZeHiro

This comment has been minimized.

Copy link
Contributor Author

ZeHiro commented Sep 1, 2015

I haven't tried yet

@ZeHiro

This comment has been minimized.

Copy link
Contributor Author

ZeHiro commented Sep 1, 2015

The CSS thing is a Firefox problem, it I change the width of the window, the borders are displayed again.

@ZeHiro

This comment has been minimized.

Copy link
Contributor Author

ZeHiro commented Sep 1, 2015

Seems good to me, at last. I even tested on the demo.cozy.cc instance, non error.

@@ -108,6 +108,7 @@ class OperationDetails extends React.Component {
<ul>

This comment has been minimized.

@ZeHiro

ZeHiro Sep 1, 2015

Author Contributor

colSpan should be 5 on line 107

@bnjbvr

This comment has been minimized.

Copy link
Collaborator

bnjbvr commented Sep 2, 2015

I've tested this morning. Had to change a few things to make it work on the standalone version, but now it seems to work everywhere. Thanks a lot for this contribution, changes to the frontend aren't trivial. If you have any feedback on what would make frontend development easier (documentation, another code structure, etc.), please let me know :)
Merged into incoming.

@bnjbvr bnjbvr added the incoming label Sep 2, 2015

@ZeHiro

This comment has been minimized.

Copy link
Contributor Author

ZeHiro commented Sep 2, 2015

Cool 😊
For this change, it wasn't really difficult, as I just copied what is done for categories.
BTW, I have on another branch (https://github.com/ZeHiro/kresus/tree/incoming-select-operation-type), the modification f operation type which seem to be ready. Could you tell me what modifications you did so that I incorporate them in this branch?

@bnjbvr

This comment has been minimized.

Copy link
Collaborator

bnjbvr commented Sep 2, 2015

The only thing that bothers me is that there's a race condition at startup: if the operation types are loaded after the accounts manager ended loading new operations, the new operations won't have a type set. For weboob it's not a problem, as it's quite slow to download operations; but for the mock backend, it is and I get a lot of warnings that the operation types are unknown. Can be fixed in another patch, though.

Fantastic that you've started another patch! I'll publish my changes this morning, so that you can rebase and move forward :)

@bnjbvr bnjbvr merged commit 4f722aa into kresusapp:incoming Sep 2, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment