search

JujaLabs / itevents

Resource to subscribe on it events
Apache License 2.0
7 stars 5 forks source link

issue-111 #114

Closed IgorMaksymov closed 8 years ago

IgorMaksymov commented 8 years ago

На проверку, connect to #111

vaa25 commented 8 years ago

@TestExecutionListeners для чего тут?

vaa25 commented 8 years ago
  1. GlobalExceptionHandler, EventNotFoundException are redundant
  2. Misprint in validUnassignReaon
  3. Why absence of reason is invalid case?
  4. shouldNotUnassignAssignUserFromEventIfParamLengthIsNotValid should be divided in two test cases
IgorMaksymov commented 8 years ago

@vaa25 1) where you see GlobalExceptionHandler ? you updated the branch? 3) why not? we need it so it should not be null 4) ok

vaa25 commented 8 years ago

@vkuchyn does unassign reason is necessary for us?

vkuchyn commented 8 years ago

@vaa25 I think it's not neccessary

vaa25 commented 8 years ago

@IgorMaksymov can you add test with empty unassign reason&? @AndriyBaibak 10 min

vaa25 commented 8 years ago

@vkuchyn if it not necessary, how shall we receive its absence in controller and other layers? As null or as empty string?

IgorMaksymov commented 8 years ago

@vkuchyn my vision of feature: user just click OK at Controller receives empty string "", but not null.

vkuchyn commented 8 years ago

@vaa25 this branch has conflicts - need to resolve

vaa25 commented 8 years ago

@vkuchyn really they are, but earlier they weren't

vkuchyn commented 8 years ago

@vaa25 I see, that could happens - just kindly ask commiter to fix them. Someone have to do them. Less changes in PR - less probability of conflicts. The quicker author make fixes - less probability of conflicts

vaa25 commented 8 years ago

@IgorMaksymov please resolve conflicts

IgorMaksymov commented 8 years ago

@vaa25 fixed @romach review pls

IgorMaksymov commented 8 years ago

@vaa25 @romach all problems fixed, review pls

vaa25 commented 8 years ago

Return previous PASSED, because of its illegal removing. Merge quality not reviewed.

romach commented 8 years ago
vkuchyn commented 8 years ago

@romach after merge, please inform PM to close ticket

IgorMaksymov commented 8 years ago

@vkuchyn who shall assign you to task, after finish?

romach commented 8 years ago

@vkuchyn I accidentally closed it. Have I to reopen it or I can only inform PM?

vkuchyn commented 8 years ago

@IgorMaksymov noone. Need to inform PM to close the task.

vkuchyn commented 8 years ago

@romach just inform

IgorMaksymov commented 8 years ago

@vkuchyn we dont have such rules in workflow, we need to add them also REVIEWER assigns you to the task, rule 3.3.3.3 we should update workflow with new rules

romach commented 8 years ago

@AndriyBaibak time spent: 30m @AndriyBaibak issue is done

vkuchyn commented 8 years ago

@IgorMaksymov ok, thanks - will fix the rules

romach commented 8 years ago

@IgorMaksymov no, I did it.