fixes issue 2177

[sumitdaga]

No description provided.

[maxceem]

Thanks @sumitdaga

Generally, the approach looks good. Functionality for posts on the Dashboard also works good except a small issue listed bellow. Post reloading for phases need some fixes and improvements.

1. When I click notification for non-yet-loaded comment, posts on dashboard are reloaded good. But not always scrolling works. I guess what can be tried is to put bigger timeout for scrolling, maybe 100 instead of 0. Maybe you have any other idea?

2. When we create a post on the phase and phase already exists we should reload all the phases because it's a lot of requests, and we can reload posts for one phase.

   

   Here is how we can load posts for one phase and members for it's post https://github.com/appirio-tech/connect-app/blob/dev/src/projects/actions/projectDashboard.js#L49-L64. Note, it could be needed if some new member created that post. I guess this part of the code could packed in one method and reused in both places.

3. In case when a phase has been created and we posted there we should reload all the phases. Currently, looks like it doesn't work, see demo video https://monosnap.com/file/4jMU7aBoo1h3mmjkodzCoEwqlYqem3

   Also, the current implementation looks not complete. As this logic https://github.com/appirio-tech/connect-app/blob/dev/src/projects/actions/projectDashboard.js#L47-L69 also loads members for posts. So if we create a post with a new member it wouldn't be loaded in current implementation.

Thanks.

[maxceem]

@sumitdaga how is it going? When do you think you would be able to complete this issue?

[sumitdaga]

@maxceem.
Believe it or not , I have spent almost entire today working on it ...It's quite complicated ... Hopefully tomorrow I will be able to finish

[maxceem]

@sumitdaga oh, ok. Which one of the 3 items above is the most complicated?

[sumitdaga]

@maxceem there are primarily 3 more things that needs to be addressed addition to the above

1. when we are on project plan tab and there is a notification for a new comment (which is not yet on redux store) then the new comment is not loaded (even with my fixes so far) .

2. on project plan tab when there is a hash in url it scrolls to it on every update to the component (something you said is not desirable) . Note that this was an existing issue.

3. Showing loading indicators when only phases are re-loaded in the project plan tab (i don't think this was already implemented) and then loading indicator when only posts for one phase are being loaded ...

and then there are other smaller things to check (and some which we may have missed still) .... and it takes some amount of time to verify a fix!

[maxceem]

I see, thanks for the detailed update @sumitdaga.

[sumitdaga]

@maxceem
btw i think the loadPhaseFeed method i am using https://github.com/appirio-tech/connect-app/blob/dev/src/projects/actions/phasesTopics.js#L37 from here loads the feed along with members (FYI for your concern about loading members when phase/phasePosts are loaded)

[sumitdaga]

@maxceem i am almost done!
but as of now i cannot add posts to a new phase ? the discussions tab is just empty ...can you please verify if its working for you ?

[maxceem]

@sumitdaga works good to me at the moment https://monosnap.com/file/c23EAxyTylOPUh6q2vPapqFGFFbwZ2

[sumitdaga]

@maxceem not to an existing phase
if you add a new phase and then open the discussions tab ...and the strange thing is it was working earlier

[maxceem]

@sumitdaga yeah, that's strange. Checking...

[maxceem]

@sumitdaga works in this project for me https://connect.topcoder-dev.com/projects/8048/plan. Could you please check.

[sumitdaga]

@maxceem i am unable to push changes
i have enabled two factor auth in my github account ...i guess i will need to be added again to the group ?

[maxceem]

@sumitdaga sent you invitation again. Though it's strange, as you push changes to your fork, but just in case.

[sumitdaga]

@maxceem
oh sorry you are right, so after adding 2 factor authentication i couldn't login to github on my terminal with username/password (my origin url was https based) and i changed the origin url to ssh and it worked ...

ok so about the fix:
this is what my general approach looks like - to load a feed/post/phase which is not yet loaded i have put the relevant handleUrlHash functions in the component which renders that particular post/phase etc so that scroll happens only once (same for expanding phase, i put the handleUrlHash function in ProjectStage component, so that it gets expanded only once) when it gets loaded for the first time while the actual loading of the phase/post is handled in ProjectInfoContainer
I think i have handled the loadingIndicator issue wherever it was needed
To check all the possibilites , you need to check if a feed can be scrolled to from home (/projects) page, dashboard tab and other tabs ...similarly for phases/posts

there are many changes, so goes without saying you will need to check it carefully so that i have not missed something or created some new bug ...

and of course if the fix works and all is ok please consider the prize on this one :)

[maxceem]

@sumitdaga thank you for the details. I'll check it out tomorrow, regarding all your points :-)

[maxceem]

Posting it here just not to forget some case:

1. Dashboard:

   1. new post is already loaded
   2. new post is not yet loaded
   3. new topic is already loaded
   4. new topic is not yet loaded

2. Phases:

   1. new post is already loaded for already loaded phase
   2. new post is not yet loaded for already loaded phase
   3. new post is not yet loaded for not yet loaded phase

[sumitdaga]

and check all the above from these pages /projects, projects/${id} (dashboard tab), projects/${id}/plan (project plan tab) ...since the behevaiour depends on which component is already mounted or not mounted

[maxceem]

Here is my testing:

1. Dashboard (from Project Plan):

   1. new post is already loaded - works https://monosnap.com/file/nKf1IG1gx2BtvVWDqkVylF90DyhTYi
   2. new post is not yet loaded - almost works - ❌ DIDN'T SCROLL https://monosnap.com/file/BRvnyyr7tTrgQEjTAV0UzlJtvBDTQO
   3. new topic is already loaded - works https://monosnap.com/file/9Byx8kVhICnTbq6OBsN5S7KYE2IeZZ
   4. new topic is not yet loaded - almost works - ❌ DIDN'T SCROLL https://monosnap.com/file/h7NogN9GsE6ecla6As3OiEU4mlw1IZ

2. Phases:

   1. new post is already loaded for already loaded phase - almost works - ❌ RELOAD PHASE with already loaded post - https://monosnap.com/file/jMDs843lGLdbUzi44NM403n8YwCUhq
   2. new post is not yet loaded for already loaded phase - almost works - ❌ reload posts for ALL pahses instead of only one - https://monosnap.com/file/T4kj6CUODMgQPxG69HC8FkrnQqAgRG
   3. new post is not yet loaded for not yet loaded phase - works - https://monosnap.com/file/2rPsrr9lMoCUQshhvB8VurWPB6kHSz

Issue

When I open the specification tab on any other phase it automatically scrolls to the phase I've clicked in the notification before https://monosnap.com/file/fchpLZhvGbIDXRlN7wc2NoVt0tZvxa This is an existent issue, I know you attempted to fix it, could you please check?

[sumitdaga]

@maxceem
i have fixed all other issues except

When I open the specification tab on any other phase it automatically scrolls to the phase I've clicked in the notification before

the above issue is happening because the specification tab renders the SpecSection (https://github.com/appirio-tech/connect-app/blob/dev/src/projects/detail/components/SpecSection.jsx#L430) component which is wrapped in ScrollToAnchors method ...which causes scrolling if there is a URL hash

i am not sure how to fix this as specSection is used at multiple other places ...do you have a suggestion ?

[sumitdaga]

I think the quickest fix would be to add disableAutoScrolling flag

@maxceem oh darn , you are right ...i have fixed all issues i think !
let me know if you see any more issues!

thanks

[maxceem]

Something is with my internet, so I cannot upload video. But I did the same actions as before.

1. Dashboard (from Project Plan):

   1. new post is already loaded - ❌ I've been immediately scrolled to the post - smooth scrolling doesn't work
   2. new post is not yet loaded - ❌ I haven't been scrolled to the post - scrolling doesn't work
   3. new topic is already loaded - ❌ I've been immediately scrolled to the post - smooth scrolling doesn't work
   4. new topic is not yet loaded - ❌ I haven't been scrolled to the topic - scrolling doesn't work

2. Phases (from Dashboard):

   1. new post is already loaded for already loaded phase - ✅ no unnecessary loading now - ❌ I've been immediately scrolled to the post smooth scrolling doesn't work
   2. new post is not yet loaded for already loaded phase - ✅ works perfectly - loads only posts for one phase including members and scrolls smoothly
   3. new post is not yet loaded for not yet loaded phase - looks good, ❌ but it doesn't load Timeline tab for a newly added phase https://monosnap.com/file/ktOT0e1QcB8dRVjcW19NbFehwyboz0. After reading Timeline is there, also it's shown after creating in another browser. Also, it scrolls to the correct place, though scrolling is almost immediate, not sure if it's just because the CPU is busy with something at the same moment.

[sumitdaga]

@maxceem
let me know if everything is fine now?

[maxceem]

@sumitdaga Implementation for loading phases looks like working good!

Test results

1. Dashboard (form Project Plan):

   1. new post is already loaded - ✅ works - https://monosnap.com/file/yi6o18DLq8Tdxd6D4LEHFhnMlXCtxu
   2. new post is not yet loaded - ❌ doesn't scroll - https://monosnap.com/file/UpQ5sTRriG3LNNvDXRAe5dpUSpcPhd
   3. new topic is already loaded - ✅ works - https://monosnap.com/file/33MNu7JmRSiScq8TOgkm3J3H9M4mar
   4. new topic is not yet loaded - ❌ doesn't scroll - https://monosnap.com/file/5ON8Qv74T4BIFSProWfmD94qj6ZuPd

2. Phases (form Dashboard):

   1. new post is already loaded for already loaded phase - ✅ works
   2. new post is not yet loaded for already loaded phase - ✅ works
   3. new post is not yet loaded for not yet loaded phase - ✅ works https://monosnap.com/file/PStGBeOfSZwSBEPV2NabVtIeX44AAF

3. Phases (form Project Plan):

   1. new post is already loaded for already loaded phase - ❌ doesn't do anything https://monosnap.com/file/fiqIw0dUr5fctyFNAwmaqjKy15DGRB
   2. new post is not yet loaded for already loaded phase - ❌ doesn't do anything https://monosnap.com/file/otoO9CWZkRCJi4rFLowSPYAjFWBC04
   3. new post is not yet loaded for not yet loaded phase - ❌ doesn't do anything https://monosnap.com/file/8yJ3QL60l1Gu5kiUhnIQL4HYkBmLIo

Issues

1. If I have some URL to the phase post and open Timeline with that post, it automatically scrolls to the Timeline tab https://monosnap.com/file/YdL8fjQmUABCO01NrwUCalSDn2gp1r. Expected behavior: it shouldn't scroll to the Timeline tab when we open it.

[sumitdaga]

@maxceem
this should do it ...i am not sure of the exact reason but my best guess is that using withRouter does not always trigger changes when location prop changes so i have added location as a prop from the top down so that at least these changes are triggered ...
let me know if its working now ..or you think it can be better handled!

[vikasrohit]

@maxceem @sumitdaga we have similar usage of location prop at multiple places to fix similar problems few examples are https://github.com/appirio-tech/connect-app/blob/dev/src/components/TopBar/ProjectsToolBar.js#L199, https://github.com/appirio-tech/connect-app/blob/dev/src/routes/metadata/components/MetaDataLayout.jsx#L16 and https://github.com/appirio-tech/connect-app/blob/dev/src/components/TopBar/ProjectToolBar.js#L79. However, I think we don't need to pass it through to the bottom of the hierarchy. Its only the component, which is blocking the re-rendering on location change event, that is need to be passed location as prop. Could you please check if passing location only to the super parent which is blocking the re-rendering, works?

[sumitdaga]

@vikasrohit @maxceem
i have passed the location prop from and to the minimum possible places for the issue to get fixed (to the best of my knowledge)
i checked the entire heirarchy and found where all it was blocked

[maxceem]

@sumitdaga most cases are fixed now, only two left.

Test results

1. Dashboard (form Project Plan):

   1. new post is already loaded - ✅ works -https://monosnap.com/file/9QRhi0CJWeILERrk87sboseqHVLqpa
   2. new post is not yet loaded - ❌ doesn't scroll - https://monosnap.com/file/8AJbDNYaYKXufZdkOhJjkQmkLRwMSr
   3. new topic is already loaded - ✅ works - https://monosnap.com/file/fYoBBRWPdxYBg2kzBomObREiT2utOk
   4. new topic is not yet loaded - ❌ doesn't scroll - https://monosnap.com/file/82SaJYGNTbSPJm1z8C4MKedLbyN8fA

2. Phases (form Dashboard):

   1. new post is already loaded for already loaded phase - ✅ works - https://monosnap.com/file/eLw5ohr8DbSxNE1j6NX0HFMLuu1xMi
   2. new post is not yet loaded for already loaded phase - ✅ works - https://monosnap.com/file/rdAaEWK8xKDriKJiJ7mTyfMrvBfJ3T
   3. new post is not yet loaded for not yet loaded phase - cannot test now because of some server issue

3. Phases (form Project Plan):

   1. new post is already loaded for already loaded phase - ✅ works - https://monosnap.com/file/iOeZQpqWHH5FrBEjOeKhoauonMYt1a
   2. new post is not yet loaded for already loaded phase - ✅ works - https://monosnap.com/file/9Ghzztx6nFf9Da7nPxKOxq72QbDOI1
   3. new post is not yet loaded for not yet loaded phase - cannot test now because of some server issue

I was testing with the latest commits:

[maxceem]

Could you please check if passing location only to the super parent which is blocking the re-rendering, works?

@vikasrohit it looks like we have to update for every component which has shouldComponentUpdate and stops re-renders.

@vikasrohit @maxceem
just a suggestion , it may be worth its while to check if a custom withRouterHOC fixes the issue because for anyone who is not aware this becomes very frustrating (i was yesterday :))

Good idea. Though as I see from the react-router documentation, the way it's done by @sumitdaga is the recommended way https://github.com/ReactTraining/react-router/blob/v4.1.1/packages/react-router/docs/guides/blocked-updates.md

Also, as per discussion in a related issue (as not only us come across with it) the other way of implementing withRouter HOC would be complicated and prone to various issues, see comment.

[sumitdaga]

@maxceem
all issues should be fixed now, or at least i hope :)

[maxceem]

@sumitdaga can wee keep here componentWillMount? As we use init to build comments inside Feed, and I guess it's better to keep it as before. So comments are rendered maybe faster.

[sumitdaga]

@maxceem
oh i did not intend to remove it, i was testing something and forgot to add it back ...should i put the init function inside componentWillMount (like before) or componentDidMount (which is also present)

[maxceem]

@sumitdaga please, keep it as it was before.

[maxceem]

Thanks @sumitdaga I've tested and everything works good.

Test results

1. Dashboard (form Project Plan):

   1. new post is already loaded - ✅ works - https://monosnap.com/file/Q7GVqlD5u9R5QP58sDbVreIVnmzseW
   2. new post is not yet loaded - ✅ works - https://monosnap.com/file/dd0BIuk3yxg8a3dauZER7HAXQ8d2wV
   3. new topic is already loaded - ✅ works - https://monosnap.com/file/KPI915QUwxUXToQfBDM60LEzfMWe3i
   4. new topic is not yet loaded - ✅ works - https://monosnap.com/file/H9ACmj4P0ny0sw763DexZMP2Zv15DS

2. Phases (form Dashboard):

   1. new post is already loaded for already loaded phase - ✅ works - https://monosnap.com/file/YMtbqfetvHU0no0ObFe18PKeSag03G
   2. new post is not yet loaded for already loaded phase - ✅ works - https://monosnap.com/file/DNEceMXRsnqstTjD4Gtd2lxCj7rk36
   3. new post is not yet loaded for not yet loaded phase - ✅ works - https://monosnap.com/file/ljM5MZxhzL4Bb10vnijKGwqp2DaeVs (NOTE I guess in the future we may consider improving this case and load only new phases, as currently it takes some time to load all the phases before the scrolling works. But as this case should be very rare, we should fine for now).

3. Phases (form Project Plan):

   1. new post is already loaded for already loaded phase - ✅ works - https://monosnap.com/file/BMgJXlDGRvZyV52CENc36lAyZrT8TW
   2. new post is not yet loaded for already loaded phase - ✅ works - https://monosnap.com/file/1Wv8BhTI5QvJg7rrKkCccABjUCdMmr
   3. new post is not yet loaded for not yet loaded phase - ✅ works - https://monosnap.com/file/lwbCseFKpuUMwRkLFxPiaUDLYddJJF (NOTE I guess in the future we may consider improving this case and load only new phases, as currently, it takes some time to load all the phases before the scrolling works. But as this case should be very rare, we should fine for now).

Enhancement

The only thing I see that the way with passing location and calling withRouter on different levels - is extremely prone to bugs. So on the second thoughts, I agree with @vikasrohit this, should be fixed some other way, the best with introducing a new HOC like withLocation which would always update when the location is changed.

So we can conclude this PR at this moment. But instead of merging it to DEV, we would keep it in a separate branch and try to achieve the same with a custom withLocation component.

[maxceem]

Note, this PR would be merged to cf17-notifications-reload-subject branch for now.

[maxceem]

This PR works good. Though before merging it to DEV we would make some code improvements, as the current version is prone to bugs.