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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

馃悰 [Minor] Console prints error #318

Closed
ulfgebhardt opened this issue Feb 10, 2019 · 11 comments
Closed

馃悰 [Minor] Console prints error #318

ulfgebhardt opened this issue Feb 10, 2019 · 11 comments
Assignees
Labels
bug something is wrong in Cypht core core module set

Comments

@ulfgebhardt
Copy link
Contributor

ulfgebhardt commented Feb 10, 2019

馃悰 Bugreport

[Minor] Console prints error

This is prob. caused by the switch to cash.js (?)

image

Version & Environment

Rev: 2f9f16b66004e197a001eee46d80653c7449505b

OS: [Chrome beta]

Note: Using InLineView

@jasonmunro jasonmunro self-assigned this Feb 10, 2019
@jasonmunro jasonmunro added bug something is wrong in Cypht core core module set labels Feb 10, 2019
@jasonmunro
Copy link
Member

I can't seem to reproduce. Can you tell me on what page or during what action this happens? I need to reproduce it in debug mode so I can capture the trace leading up to the error. Thanks!

@ulfgebhardt
Copy link
Contributor Author

Directly after login - on the "Everything" Page; On "Contacts", on "History" ... so basically everywhere.
-> Maybe its a timing issue?

@ulfgebhardt
Copy link
Contributor Author

ulfgebhardt commented Feb 10, 2019

image

image

@jasonmunro
Copy link
Member

I see the problem, AND I see that I disabled keyboard shortcuts while working on cash.js and forgot to enable it! Either way this code is dodgy and I don't like it. We were jumping through hoops to load JS specific references in the body of the page that are defined in a JS include in the page, then on DOM load trying to force that data into being "real" JS hoping the include is present in time which is just daft.

I have a solution which fixes all this by embedding real JS in the page with keywords instead of actual JS functions we need to call. In the JS include we just translate the keywords into the function name and all this goes away. Running tests now and should have a fix committed shortly.

@jasonmunro
Copy link
Member

Should be fixed :)

jasonmunro added a commit that referenced this issue Feb 10, 2019
* origin/master:
  clean up, and add some keyboard shortcut tests
  more keyboard shortcut fixes
  fix keyboard shortcuts module set javascript issue #318
  add an install file
  include branch info in subject where applicable
  limit travis builds to the master branch for now
@ulfgebhardt
Copy link
Contributor Author

<3

@jasonmunro
Copy link
Member

FYI I added some selenium tests around keyboard shortcuts after this fix and found out we have some cross browser issues because the default META control key is not as compatible across systems as I thought. Still working out how I want to fix that but I'm leaning toward just changing the defaults :).

@ulfgebhardt
Copy link
Contributor Author

I actually did not know there are Keyboard shortcuts ;-)
Where can I read the list of keys?

Also I have to thank you for taking my request, I did a while back, seriously about tests. I think it has contributed alot to current stability of cypht - would you say writing the test is while your worth - have they caught errors regularly for you?

@jasonmunro
Copy link
Member

If you enable the keyboard shortcuts module set you will find a "shortcuts" link in the Settings section that shows you the keys and actions, and lets you set your own for each action.

The UI tests have been great. It took some work getting the initial flow going but now it's easy to add a new suite of tests. The tests are actually in python because 1. I know python and 2. it has good selenium bindings to make controlling the browser easy. With Travis and browserstack in the mix, we can (and do) test against IE/Edge/FF/Chrome/Safari.

Unit tests are great, but just because a block of code behaves the way you expect does not mean the app as a whole does :). Not to mention unit tests only exercise server side code while the UI tests include the client side bits. One of the best tests we have is sending ourselves an E-mail then navigating to the unread page and opening it. The test code itself for that is simple, but bootstrapping Travis properly was a lot of work.

@ulfgebhardt
Copy link
Contributor Author

Haha yes, I know Travis is a pain - especially since you can only test after commit & push - but in my experience its quite powerful once properly set up.

@jasonmunro
Copy link
Member

Travis is a pain but absolutely worth it. I try to minimize breakage before pushing with this: https://github.com/jasonmunro/cypht/blob/master/scripts/commit_check.sh It runs sanity checks on all the PHP, JS, and CSS files, and it runs all our unit tests and UI tests.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug something is wrong in Cypht core core module set
Projects
None yet
Development

No branches or pull requests

2 participants