Closed Preeticp closed 5 years ago
Thank you @Preeticp for this contribution!
It appears that no tests have been added or updated in this PR.
Automated tests give us confidence in shipping reliable software. Please add some as part of this change.
If you are an admin or the reviewer of this PR and you are sure that no test is needed then you can use the command /ok-without-tests
as a comment to make the status green.
For more information please head over to official documentation. You can find there how to configure the plugin.
The #
used for comments in YAML doesn't work in backend. I ran this PR locally and the resulting response has the #
in it. @Preeticp Can you please remove those two lines instead of commenting them out? We don't really need them anyway :)
@jarifibrahim I have removed the 2 statements can you please check if it's alright now?
Thanks @Preeticp. I've update the test files.
Merging #2353 into master will increase coverage by
0.01%
. The diff coverage isn/a
.
@@ Coverage Diff @@
## master #2353 +/- ##
==========================================
+ Coverage 70.19% 70.21% +0.01%
==========================================
Files 171 171
Lines 16919 16919
==========================================
+ Hits 11876 11879 +3
+ Misses 3892 3891 -1
+ Partials 1151 1149 -2
Impacted Files | Coverage Δ | |
---|---|---|
controller/workitem.go | 79.22% <0%> (+0.42%) |
:arrow_up: |
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 1bdd012...af99b03. Read the comment docs.
/ok-without-tests
@kwk I raised this PR as I felt we should not have "to be implemented" statements that are user facing. Can you please review this PR?
As discussed with you @kwk, the ideal resolution for this is to have only one of the options available to the end user, that is, we retain either 'is related to' or 'relates to' as both essentially mean the same. For this, as mentioned, we need to do this: ...we treat all link types as bidirectional...To remove one of them we have to introduce the concept of unidirectional links. Not sure if that is even the right word to use...
and we need to discuss the implementation with the UI team. @nimishamukherjee, @sahil143, @divyanshi, can you please let us know your views on this? If required we could discuss this in the upcoming cabal.
I would like to stay honest because I fear that without that honesty we make things more complicated for the user to understand.
This isn't about being honest. If we haven't implemented something we shouldn't show it to the user. Showing we just haven't implemented it yet
is so wrong.
What if we add a button and when you click on it, it shows a pop up with we haven't implemented it yet
. We would be honest if we did that, but we should never do that. If we haven't implemented something, the end user should not see it. We don't show the TODOs
to the user :)
@jarifibrahim please wait for the Cabal. We cannot do anything about the two link types being there because they stem from just one link type and only describe a forward and backwards/reversed direction. And since the feature is already being rolled out and doing no harm, we can at least document it so the user doesn't feel confused which to pick.
Let's discuss in the Cabal next week as @Preeticp suggested (https://github.com/fabric8-services/fabric8-wit/pull/2353#issuecomment-441225921).
[test]
This PR removes a 'to be implemented' statement from the description of the link type.