-
Notifications
You must be signed in to change notification settings - Fork 5
Ees 5954 On Publication Changed #5754
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
Conversation
711d39b
to
a802e99
Compare
…e events instead of the methods
…ignify that it was a stream of changes. But it's too easy to transpose xxxChanged which is the name of the events and event handler etc
…so check for the events (or lack of them where appropriate).
…e event that was raised is expected rather than if the entire publication entity is correct.
a802e99
to
5917e6f
Compare
src/GovUk.Education.ExploreEducationStatistics.Admin/Events/PublicationChangedEventDto.cs
Outdated
Show resolved
Hide resolved
...ucation.ExploreEducationStatistics.Admin.Tests/Services/PublicationServicePermissionTests.cs
Show resolved
Hide resolved
namespace GovUk.Education.ExploreEducationStatistics.Admin.Tests.Services | ||
namespace GovUk.Education.ExploreEducationStatistics.Admin.Tests.Services; | ||
|
||
public class PublicationServiceTests | ||
{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This goes against one of our backend style guide rules because this change to use files scoped namespace is being made where there's something like a 28 line change here turning into a 5672 line change.
Change existing files to use file scoped namespace declarations only if there are significant line changes or if files are small, to minimise the impact on readability of pull requests.
We've discussed this as a team previously at Dev team meetings and we do eventually want to switch all files to use file scoped namespace but for now we agreed to only use it on new files or switch where the majority of the existing code is changing.
As well as minimising the impact on readability of pull requests, it's also to reduce the chance of causing a conflict with anyone else with changes to the same code already in flight, and because we appreciate the history and being able to tell what issues certain lines refer to.
The same comment applies to PublicationService.cs where there's even less lines changing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm guessing this was discussed at a meeting I wasn't at?
- It barely affects the PR if you turn off whitespace changes
- It won't affect other PRs if everyone is also converting the files as we go
At the moment, nothing is changing so we don't move towards our goal at all. We just kick the can down the road, no?
I'm happy to raise this in a meeting but it's difficult to if I'm not invited to those meetings.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, the fact that this file is thousands of lines long is a problem in itself! 🤣
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes it's been discussed prior to you starting at the Dev's team meeting, probably on multiple occasions I think. Those conversations led us to having that reference in the style guide about it.
So far we've either not reached an agreement or found a convenient time on if/when we should go ahead with the conversion of all files.
The Dev's team meeting would be the place to discuss it again and you should be invited to that?
I don't think you need to undo this change, although you could if you want, just to fit in with the style guide. I wouldn't make further changes like this until we've had chance to discuss it again though.
Note: There are 4 steps to this PR which I have left as easy to understand separate commits
Renamed Mock Builder asserts from xxCalled to xxRaised to refer to the events instead of the methods
Very subtle change: I had originally named the Topics xxxChanges to signify that it was a stream of changes. But it's too easy to transpose xxxChanged which is the name of the events and event handler etc
Add new Event Raiser function - to raise a Publication Changed event
Utilise the new event in Publication Service. Update some tests to also check for the events (or lack of them where appropriate).