proteinevolution / Toolkit

The MPI Bioinformatics Toolkit
https://toolkit.tuebingen.mpg.de
Apache License 2.0
63 stars 14 forks source link

Develop has several issues #254

Closed vikramalva closed 6 years ago

vikramalva commented 6 years ago

Forwarding, selection etc. are broken. I have no free time to test the Toolkit presently and I am afraid we are just accumulating bugs. Please urgently reset the Toolkit to: https://github.com/zy4/Toolkit/commit/4f9b7f09e0a92ac5d7ac2fb84878cd0478dfda41.

vikramalva commented 6 years ago

Some issues that I found within 5-10 minutes of testing:

bug1

vikramalva commented 6 years ago

bug2

vikramalva commented 6 years ago

Positioning of the menu and unnecessary scrollbars

bug3

UPDATE: should be fixed now - Felix

vikramalva commented 6 years ago

I will just stop here, but there are several others.

vikramalva commented 6 years ago

Toolbar is missing..

bug4

zy4 commented 6 years ago

are you planning to push to production today? Because otherwise I can fix all of these today. There will always be bugs like this but this is no reason to panic. As far as I can see, these bugs are css and off-by-one-errors but nothing special. And regarding the workflow: if you really need to change something today, let's say some update message, you can simply merge a separate branch on rye and push it back to master. But these big commits needed to get merged because otherwise we can hardly proceed in other big tasks like the frontend rewrite. The reason for this being that simply Felix was working on files which I deleted in the other branch!

zy4 commented 6 years ago

Plus I squashed all of the recent commits so that reverting (not resetting) is easy.

vikramalva commented 6 years ago

No, but I can only start to test the Toolkit again after I return to Germany. And when I return I will have to hit the ground running to deal with other important deadlines. All search tools are essentially broken. Clicking on the visualization map doesn't take me to the alignments. When I select sequences on a different page of the data table and subsequently go somewhere else they are not remembered. If I select one sequence and hit foward, not one but but many are forwarded. There many other issues. I would prefer to have Master in a clean state and we should revert back to my last commit.

zy4 commented 6 years ago

There is some trade-off to be paid: you saw the size of the merges. Either we have a very painful process to resolve merge conflicts (and you would have to test thrice or more times the whole toolkit) or we run through some mild debugging which I would completely do myself. Again: if you need to push to master, just modify in a different branch and merge it. If you need to merge a bugfix which exists only in master: just git cherry-pick and push back to master.

Master will be clean this evening. These are minor bugs.

vikramalva commented 6 years ago

I just pushed IE related changes to Rye and had to do it via a not so straightforward process. I could of course merge bug fixes directly to master on Rye through new branches, but this would result in many unwanted issues. I may not be able to push back to master easily without doing a pull.

zy4 commented 6 years ago

they were merged first, you could just pull them from master via git cherry-pick commithash

I see your points and from your perspective it is understandable to me that:

From a developer's perspective, I made my points quite clear. We are under heavy construction, still. This is due to the fact that the current code base is not extensible. Only to the price of following all the anti-patterns.

As for pushing back, you can do the following: git push origin master:newbranchname But in this case I see no point in doing so because you would not add anything new to master, you are just behind.

vikramalva commented 6 years ago

My only aim is to keep master as clean as possible and we should test branches thoroughly before merging them to master to avoid this sort of situations.

vikramalva commented 6 years ago

Loading is broken:

bug6

zy4 commented 6 years ago

Should we set up a "develop" branch then? How can we speed up code reviewing?

zy4 commented 6 years ago

We can do something like https://github.com/zurb/foundation-sites They have the "develop" branch as main protected branch and master is separate.

vikramalva commented 6 years ago

Good idea! That way I would not need to test Master thoroughly so often.

zy4 commented 6 years ago

I am going to set it up later. We need to find some workflow for the time when we make it open-source, anyways.

vikramalva commented 6 years ago

Selection and forwarding for all search tools are broken; Please compare with production. There seems to be no real correlation between what is selected and what is forwarded. Selection syn between data tables and alignments is broken for all search tools. If one selects a hit on any page of the data table, they should also be selected in the alignments section. I think I would need days of tests to have all of this working again.

vikramalva commented 6 years ago

Please have the HEAD of the develop branch point to https://github.com/zy4/Toolkit/commit/2904f1bdfb1862b4b0ca56e945a1682595157bc8. We could push all important fixes directly to the develop branch.

felixgabler commented 6 years ago

Mental note: Maybe we should also think about implementing tests in the frontend. Forwarding, selecting and more could be tested in a more automated fashion to make it easier for all of us. There has to be a way to rid us of the unnecessary function testing to some degree.

vikramalva notifications@github.com schrieb am Mi., 10. Jan. 2018, 15:07:

Please have the HEAD of the develop branch point to 2904f1b https://github.com/zy4/Toolkit/commit/2904f1bdfb1862b4b0ca56e945a1682595157bc8. We could push all important fixed directly to the develop branch.

— You are receiving this because you were assigned.

Reply to this email directly, view it on GitHub https://github.com/zy4/Toolkit/issues/254#issuecomment-356611976, or mute the thread https://github.com/notifications/unsubscribe-auth/AaEXSAq3RcWsvHhSE9jN4NjDR-UsZxDOks5tJMQmgaJpZM4RZDNU .

zy4 commented 6 years ago

What about css, though?

felixgabler commented 6 years ago

I hope that there will not be such a big refactoring in the near future. Furthermore CSS is quickly tested. Or at least quicker than all of the js.

Seung-Zin Nam notifications@github.com schrieb am Mi., 10. Jan. 2018, 15:25:

What about css, though?

— You are receiving this because you were assigned.

Reply to this email directly, view it on GitHub https://github.com/zy4/Toolkit/issues/254#issuecomment-356617048, or mute the thread https://github.com/notifications/unsubscribe-auth/AaEXSPO6jH3yN8uf-5JwFqFcJP-aEyfwks5tJMhVgaJpZM4RZDNU .

zy4 commented 6 years ago

Please have the HEAD of the develop branch point to 2904f1b. We could push all important fixes directly to the develop branch.

you mean HEAD of master, right?

vikramalva commented 6 years ago

Yes head of master. And I meant we could push import fixes directly to master, while most other stuff should go through the develop branch.

vikramalva commented 6 years ago

I found a bug in PSI-BLAST on production that must have crept recently; I had tested these things thoroughly in the past.

Hit 1 reappears again after hit 500. We should try to fix this soon, before fixing the others. https://toolkit.tuebingen.mpg.de/#/jobs/10165566

bug7

zy4 commented 6 years ago

For me it does not repeat

vikramalva commented 6 years ago

I tested it more and it doesn't happen every time. Something to keep in mind.

zy4 commented 6 years ago

I think this is due to side-effects which I was about to reduce.

zy4 commented 6 years ago

branches are set up

zy4 commented 6 years ago

Could you sbt-release since you pushed to production?

vikramalva commented 6 years ago

To the current master on GitHub?

zy4 commented 6 years ago

I already did it. It's alright! We should just keep up the workflow...

felixgabler commented 6 years ago

I have a question about the missing toolbar. Do we really want to show it for smaller devices? We already have a off-canvas menu for these cases

zy4 commented 6 years ago

I also think that it should not be there

vikramalva commented 6 years ago

I find it really practical to have, as I use the Toolkit in a browser window half the size of my screen and I don't want to always use the off-canvas menu for this. I would therefore like have it.

toolbar1

toolbar3

felixgabler commented 6 years ago

We could reduce the breakpoint to only hide it for small screens but display it for medium-sized ones.

vikramalva notifications@github.com schrieb am Do., 11. Jan. 2018, 18:54:

I find it really practical to have, as I use the Toolkit in a browser window half the size of my screen and I don't want to always use the off-canvas menu for this. I would therefore like have it.

[image: toolbar1] https://user-images.githubusercontent.com/20257466/34839047-9c2a9b94-f700-11e7-91d1-fd8bb13e4186.png

[image: toolbar2] https://user-images.githubusercontent.com/20257466/34839060-a41245dc-f700-11e7-9ece-4803f394ee57.png

— You are receiving this because you were assigned.

Reply to this email directly, view it on GitHub https://github.com/zy4/Toolkit/issues/254#issuecomment-357008217, or mute the thread https://github.com/notifications/unsubscribe-auth/AaEXSEPpGBEPtm_g4NtdvDTxvafRA_Xtks5tJkrNgaJpZM4RZDNU .

vikramalva commented 6 years ago

Isn't it this way on production?

toolbar4

zy4 commented 6 years ago

Clicking on the visualization map doesn't take me to the alignments

this bug was settled outside of our codebase - it is in hhviz.pl wondering whether we should include this into the repo because there is javascript code embedded

vikramalva commented 6 years ago

Sure and I should mention that it used by HHblits, HHpred and HHomp. Also, clicking on a bar at the bottom of the map should load all unloaded alignments up until the alignment corresponding to that bar, as on production.

vikramalva commented 6 years ago

I think the map is also broken for PSI-BLAST, which uses a different script.

felixgabler commented 6 years ago

If it is already this way, we should think about reworking our responsiveness. I will look into it

vikramalva commented 6 years ago

Also, don't try to further optimize the size for mobile devices. I and @anjestephens spent quite some time on this and fixed the smallest size such that the results page of various tools are not broken. We don't want to spend time on optimizing the pages of every single tool for mobile devices.

smallest

anjestephens commented 6 years ago

I don't know if I did everything right to get up to date but I have issues as well. image (The tabs are not generated, the content is limited to the input field)

felixgabler commented 6 years ago

Are there still issues?

zy4 commented 6 years ago

@anjestephens please try to execute sbt clean and sbt cleanFiles @felixgabler yes, I am going to fix them this WE.

anjestephens commented 6 years ago

@zy4 Actually I freshly cloned the project because I had merging issues and thought it was time to refresh anyways. But I'll try that.

vikramalva commented 6 years ago

@felixgabler most of the issues (e.g. with selection and forwarding) that I reported earlier still exist. There are some CSS issues too.

zy4 commented 6 years ago

@anjestephens ... also clear the browser cache if you haven't already

zy4 commented 6 years ago

@vikramalva can you check again? If something is still missing / not working, please open a separate issue for it.

felixgabler commented 6 years ago

If I understand correctly, all the issues have been singled out into separate issues. Forwarding/Selecting into the checkbox component (#260) and css into #265. If this is correct, we could close this issue