NullTraining / reviewz

..
1 stars 2 forks source link

[Behat/app context] Check if current user is organizer #181 #182

Closed HcsOmot closed 6 years ago

HcsOmot commented 6 years ago

There was a bug in the feature file for Event Application Context which was a false positive - user who was not an organization organizer was able to create a new event for organization.

Closes #181

msvrtan commented 6 years ago

Organizer check should be part of Handler, not Application context...

But for instance, it would make sense to put it in DomainContext on that step

HcsOmot commented 6 years ago

Seeing there is no handler for creating new events, should we merge this as is for now?

I can add the command and handler for creating events and update the relevant contexts in a separate PR.

Or I can just add the handler to this PR in additional commits.

msvrtan commented 6 years ago

Ooouch, how did we miss that one :(

Yeah, please add Command/Handlers

HcsOmot commented 6 years ago

UPDATE:

I've added command & handler for creating new Events, and implemented LocationRepository based on how @msvrtan created EventRepository and OrganizationRepository.

I haven't changed anything in Behat context files just yet, because I'm not sure that CreateEventHandler is the place to perform the checks for current user's permissions (is the current user an organizer of some organization and can they create a new event).

I think this kind of check should be done somewhere else, but still not sure where and how.

Perhaps controller isn't such a bad place for this ATM? We haven't decided if this restriction should be part of the core business rules. If it is, we can always modify the command and handler to include the user who is trying to create the event and handle that in the Handler, however we see fit.

Please leave feedback on this, as we need to decide on where and how to implement these check before this gets merged to master.

@msvrtan @div3r @zgperak @flackovic @leovujanic

msvrtan commented 6 years ago

I think handler is the best place to check if user is allowed to add an event.

Of course, there might be more places where we will need to add checks as well but handler is for sure a good one ...

HcsOmot commented 6 years ago

Ok, I'll add event creator to command. :+1:

HcsOmot commented 6 years ago

~Note to self:~ ~We need negative scenario tests to prove users who aren't organizers can't create events.~

HcsOmot commented 6 years ago

fixed.

msvrtan commented 6 years ago

@HcsOmot fire away!