balvi / cuba-component-declarative-controllers

CUBA component that allows to write generic features for a Controller and use them in a declarative way
5 stars 6 forks source link

updated to support EntityCombinedScreen #3 #6

Open genthalili opened 6 years ago

genthalili commented 6 years ago

Hi,

I'm proposing a solution to support EntityCombinedScreen, please feel free to update at my forked repo.

Zynde commented 6 years ago

Oh great. Thanks. I've not had the time to go back and do this myself.

bresche commented 6 years ago

Thanks for the pull request. I looked at the code and it has duplicates. It would be better to move the code into super class (AbstractAnnotationDispatcherBean). Could you create some unit tests?

genthalili commented 6 years ago

Hi @bresche ,

Could you explain more how to avoid this duplicated code?

bresche commented 6 years ago

Pull up Member from BrowseAnnotationDispatcherBean,CombinedAnnotationDispatcherBean to AbstractAnnotationDispatcherBean and make methods generic:

  protected Collection<T> getSupportedCombinedAnnotationExecutors(Annotation annotation) {
        Collection<BrowseAnnotationExecutor> annotationExecutors = getCombinedAnnotationExecutors();
        return (Collection<T>) getSupportedAnnotationExecutors(annotationExecutors, annotation);
    }
genthalili commented 6 years ago

Removed duplicated code and added tests, however tests are not working, I have a Null exception, could you assis on this ?

genthalili commented 6 years ago

@bresche, any suggestion to fix this ?

bresche commented 6 years ago

I'll take a look at it.

bresche commented 6 years ago

Hi, I put some mocks in your test spec. Now you can use them in spock style and create some good test cases.

genthalili commented 6 years ago

Thanks! I think you covered the basic testings, that's super! Can we consider merging this pull request ?

bresche commented 6 years ago

Unfortunately, not quite. We need more tests. The CombinedAnnotationDispatcherBean class is not well tested and contains code which is commented. I have slightly increased the test code coverage.

Zynde commented 6 years ago

Has this progressed any further?

bresche commented 6 years ago

There are still 2 classes to test.