TracksApp / tracks

Tracks is a GTD™ web application, built with Ruby on Rails
https://www.getontracks.org/
GNU General Public License v2.0
1.18k stars 537 forks source link

Quickly marking several repeating todo instances as done gives wrong UI feedback #1706

Closed C-Otto closed 10 years ago

C-Otto commented 10 years ago

This may be related to #1444

Before: Three repeating todos (a,b,c) set to daily. For all three a corresponding instance is shown as an open todo.

sqlite> select * from todos;
1|1|1|a||2014-07-03 20:53:31.704885|2014-07-03 20:53:31.699464||1||active|1|2014-07-03 20:53:31.730812|
2|1|1|b||2014-07-03 20:53:37.629295|2014-07-03 20:53:37.626726||1||active|2|2014-07-03 20:53:37.646524|
3|1|1|c||2014-07-03 20:53:44.699796|2014-07-03 20:53:44.697092||1||active|3|2014-07-03 20:53:44.714920|

When I now complete all three of those instances at nearly the same time (using Firebug, to simulate the slow server mentioned in #1444), I get the following results.

The database now gives:

sqlite> select * from todos;
1|1|1|a||2014-07-03 20:53:31.704885|2014-07-03 20:53:31.699464|2014-07-03 20:56:13.702625|1||completed|1|2014-07-03 20:56:13.705743|
2|1|1|b||2014-07-03 20:53:37.629295|2014-07-03 20:53:37.626726|2014-07-03 20:56:13.867744|1||completed|2|2014-07-03 20:56:13.869509|
3|1|1|c||2014-07-03 20:53:44.699796|2014-07-03 20:53:44.697092|2014-07-03 20:56:14.037687|1||completed|3|2014-07-03 20:56:14.039503|
4|1|1|a||2014-07-03 20:56:13.729303|2014-07-03 23:00:00.000000||1||active|1|2014-07-03 20:56:13.745716|
5|1|1|b||2014-07-03 20:56:13.887435|2014-07-03 23:00:00.000000||1||active|2|2014-07-03 20:56:13.906613|
6|1|1|c||2014-07-03 20:56:14.056360|2014-07-03 23:00:00.000000||1||active|3|2014-07-03 20:56:14.071667|
dnrce commented 10 years ago

1444 is against Tracks 2.2.2 but this is against master, right?

C-Otto commented 10 years ago

Correct.

lrbalt commented 10 years ago

could you tell something about your setup? Is this apache + passenger?

And are a,b,c todos that belong to three different rec todos? I do not see the column names, but I think a belongs to rec todo 1, b belongs to rec todo 2 and c belongs to rec todo 3. Am I correct?

lrbalt commented 10 years ago

I think the problem is a name collision in the returned ajax. We know that the processing of the three requests went correct, since the reload shows the correct situation. It must be on the client side, i.e. javascript side

When your server returns the generated js from your three requests, all three define the same functions, in particular html_for_todo() (in todos/toggle_check.js). So, todo c is probably returned last and it redefines the html_for_todo() of the js for a and b, effectively causing a and b use the html_for_todo of c.

This must probably happen for the other functions too, but since a,b and c are in the same project/context you do not notice this (i.e. all updates are in the same container)

lrbalt commented 10 years ago

I have put the js from toggle_check in an object with a unique name. Could you test to see if this fixes your problem. It is in the branch fix1706

C-Otto commented 10 years ago

Hmhm, any tips how to migrate from 2.2 (release) to master?

lrbalt commented 10 years ago

You can find the upgrade instructions in /doc of the master branch here:

https://github.com/TracksApp/tracks/blob/master/doc/upgrading.textile

On Wed, Aug 6, 2014 at 9:22 PM, Carsten Otto notifications@github.com wrote:

Hmhm, any tips how to migrate from 2.2 (release) to master?

Reply to this email directly or view it on GitHub: https://github.com/TracksApp/tracks/issues/1706#issuecomment-51383690

C-Otto commented 10 years ago

Thanks! I tested the branch, it fixes the problem.

C-Otto commented 10 years ago

Before you close it: The same bug also appears when I mark the repeating todos as done (so that they do not repeat anymore). I manually clicked the boxes for my a,b,c jobs and saw a,b,b in the list at the bottom of the page. Reloading, as with the original bug, shows the correct results.

lrbalt commented 10 years ago

That was to be expected :-) I have only fixed the js for marking todos complete. We need to do this for all js that is returned after an ajax call.

C-Otto commented 10 years ago

OK then :) :+1:

lrbalt commented 10 years ago

I have modified most relevant js being returned for ajax calls to be embedded in their own object. For tracks 3 the complete ui will be changed, so all of this will go away anyway :-)

Could you test a little? If all goes well (inclusing Travis), I'll merge to master

C-Otto commented 10 years ago

I see no problem, thanks!

C-Otto commented 10 years ago

Changing the context now gives me a red box: "There was an error retrieving from server: parsererror". The database is updated correctly, so I just need to reload the page. This seems to be a display error related to the fixes for this bug.

If you want, I can investigate this further (later, at home), just tell me so.

C-Otto commented 10 years ago

Same for setting "show from".

C-Otto commented 10 years ago

And the name. I guess about any response now gives this behavior.

lrbalt commented 10 years ago

I see these regressions too. I have some fixes and refactorings in the works.

lrbalt commented 10 years ago

did you try with the latest fixes? I saw that I had them pushed to master already. I can change the context without the error. Travis does not show any, to this fix related, errors.

lrbalt commented 10 years ago

I went ahead and merged into master, so please try master...

C-Otto commented 10 years ago

Now clicking on the edit link (which shows the pen symbol, but also the alt text "edit" overlayed on top) gives "There was an error retrieving from server: error". The server log does not contain anything related to such clicks.

lrbalt commented 10 years ago

Did you regenerated the assets? I.e. rake assets:precompile RACK_ENV=production

C-Otto commented 10 years ago

Yes, that does not help.

lrbalt commented 10 years ago

uhm, yes it helps? or no, it still fails?

lrbalt commented 10 years ago

it looks to me that it must be something with your setup, since Travis is green on editing a todo. Could you empty the cache of your browser?

C-Otto commented 10 years ago

Let me rephrase: "Yes, I regenerated them (again). That does not help, though."

With Chrome (I use Firefox normally) I see a broken image indicator above the correct symbols. In the case of the star icon (leftmost) I see both the filled and the unfilled star.

lrbalt commented 10 years ago

Very strange, I am not seeing this. Broken images should not be caused by the recent changes, so I still suspect something on your end. Could you restart the webserver (apache?) and do a clean reload (ctrl-f5)?

C-Otto commented 10 years ago

I did both of that already (and tried it again). No change. According to my server's log file, I have /tracks/assets/blank-b19beff0c9103bf0a6951fc3326409f5.png (this exists) and /assets/blank-229cca7af663d834577d5460a8f9955d.png (this does not exist). However, with the /tracks/ prefix both files exist on disk. So the issue is that a non-prefixed path is used. However, I set up the subdir correctly in site.yml.

C-Otto commented 10 years ago

Let me clarify: If the webserver were asked for the missing file (blank-229...) WITH the prefix, it would find it. However, without the prefix a 404 error code is returned and the client does not get the requested image.

lrbalt commented 10 years ago

perhaps it could be that the subdir setting in config/application.rb is not correct anymore.

Could you try changing the line in application.rb to

config.relative_url_root = '/subdir'

and try again after regenerating the assets?

lrbalt commented 10 years ago

I found that you need to change config/config.ru too. I have changed master with these fixes. Could you try on latest master?

C-Otto commented 10 years ago

With those patches I get 'Not Found: /' as the only output on the configured web address, so not even a Passenger error page.

lrbalt commented 10 years ago

And is passenger configures for subdirs?

https://www.phusionpassenger.com/documentation/Users%20guide%20Apache.html#deploying_rack_to_sub_uri

Reinier Balt

On Wed, Aug 13, 2014 at 06:34 PM, Carsten Ottonotifications@github.com, wrote:

With those patches I get 'Not Found: /' as the only output on the configured web address, so not even a Passenger error page.

— Reply to this email directly or view it on GitHub.

C-Otto commented 10 years ago

Yes, it is. The setup has been running for a while now. I just updated from 2.2.2 to master, and switched between WEBrick and Apache.

C-Otto commented 10 years ago

When leaving out 863d780ad0a6c89a8d820ea516d4aa84f277b468 it works again. So I am at 2757c88c0bcd5723f52e22ae156aab95416068eb now and see most of the images. However, the 'blank.png' (#1713) still is missing.

Updating the name, due date, context now works again. Thanks for fixing!

dnrce commented 10 years ago

@C-Otto I'm a little confused reading back over this ticket because it was being mixed up a bit with #1713. Has this issue been fixed?

C-Otto commented 10 years ago

I guess the core bug is fixed, but it needs to be applied to all places: "We need to do this for all js that is returned after an ajax call." (Irbalt, 07.08.). The display issues also mentioned in #1713 are not really related to this bug, so feel free to ignore such references.

lrbalt commented 10 years ago

"We need to do this for all js that is returned after an ajax call." I have fixed all the relevant js cases, so imho this issue can be closed

C-Otto commented 9 years ago

Could you delete branch 'fix1706' please? It is merged already.