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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix for DPs not loading in tab when task change not detected #1511

Merged
merged 5 commits into from
Jul 30, 2018

Conversation

ascobie
Copy link
Member

@ascobie ascobie commented Jul 27, 2018

No description provided.

@@ -30,7 +30,8 @@ export class TaskDependencyBrowserComponent implements OnChanges {
this._tasks.clear();
}

if (changes.jobId || ComponentUtils.recordChangedId(changes.task)) {
// Task initially loaded with no dedendsOn. Change to task properties was not handled here.
if (changes.jobId || ComponentUtils.recordChangedId(changes.task) || changes.task) {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tim, is this OK?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hhm, I don't think so, here it will recompute every time the task change(which is every 5 seconds as it polls for a new one)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok, need something to trigger the first time its reloaded then. The first time there are no DPs, second time they are there. I could do something with the InitialChange or FirstChange or whatever that property is called. Or add the task change to another conditional that only executes once.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah so maybe there is a bug in recordChangedId because that should have triggered I believe

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

recordChangedId only seems to care about the task ID changing. Not the properties of the task.

@codecov
Copy link

codecov bot commented Jul 27, 2018

Codecov Report

Merging #1511 into master will increase coverage by <.01%.
The diff coverage is 83.33%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1511      +/-   ##
==========================================
+ Coverage   60.75%   60.75%   +<.01%     
==========================================
  Files        1070     1070              
  Lines       25064    25068       +4     
  Branches     3456     3458       +2     
==========================================
+ Hits        15228    15231       +3     
- Misses       9836     9837       +1
Impacted Files Coverage Δ
app/utils/component-utils/component-utils.ts 90% <100%> (+1.11%) ⬆️
...dency-browser/task-dependency-browser.component.ts 97.29% <66.66%> (-1.32%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 68b39c9...a852c91. Read the comment docs.

@@ -30,12 +30,18 @@ export class TaskDependencyBrowserComponent implements OnChanges {
this._tasks.clear();
}

if (changes.jobId || ComponentUtils.recordChangedId(changes.task)) {
// Task initially loaded with no dedendsOn. Change to task properties was not handled here.
if (changes.jobId || ComponentUtils.recordChangedId(changes.task) || this._hasUnloadedDependencies) {
this._loaded = 0;
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This only gets called twice now if it needs to be. The first time and the next time if there was a change in the DP property

@ascobie
Copy link
Member Author

ascobie commented Jul 30, 2018

confirmed that this is now fixed.

@timotheeguerin timotheeguerin merged commit 6b17e10 into master Jul 30, 2018
@timotheeguerin timotheeguerin deleted the fix/dependancy-status-css branch July 30, 2018 23:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants