Change SoftDelete for AverageRatings, Ratings and QuartzJobs.

[alrom]

No description provided.

[VadymLevkovskyi]

See no issues in logic. Few code suggestion were mentioned in comments, though they might be out of scope

[DmyMi]

@VadymLevkovskyi I like the IDto interface idea as it will do magic to simplify our code in some places. But I have a question for future:
Currently we do not really follow REST convention and do not have ids in request path for most updates as we put it in dto body.
But what if we finally decide to refactor to be more RESTy, is there maybe some recommendation we can do to both use the interface magic and follow convention?

[VadymLevkovskyi]

@VadymLevkovskyi I like the IDto interface idea as it will do magic to simplify our code in some places. But I have a question for future: Currently we do not really follow REST convention and do not have ids in request path for most updates as we put it in dto body. But what if we finally decide to refactor to be more RESTy, is there maybe some recommendation we can do to both use the interface magic and follow convention?

@DmyMi - it's a good point I hadn't thought before. Let's investigate.
You'd like to have "id-less" dto's passed from front-end to presentation layer and (as I get it) there are "id-full" dto's taken from persistence layer (I'm talking about EF side, not ES, mainly because ES part is not analized deep enough).

So main question (as for me) - do we really need that "id-less" dto's?

If yes - we need 2 sets of dto's (with/without id field),
if not - have dto with id field that is sometimes ignored.

So naive solution for 1st variant:
NotificationUIDto : IDtoNoId<Notification> and NotificationDbDto : IDto<Notification> classes with duplicated fields.
Pro - DB and UI dto's could evolve independently, contra - lot of fields duplication.

Mechanical "improvement" of 1st variant:
NotificationUIDto : IDtoNoId<Notification> and NotificationDbDto : NotificationUIDto, IDto<Notification> classes where duplication is solved thanks to inheritance. As for me, this approach introduces too high coupling between UI and DB dto's, so better to avoid it.

Solution for 2nd variant:
single NotificationDto : IDto<Notification> where .Id field could be taken into account or not. Same issue with DB and UI dto's independency and additional one - you would never trust you .Id field - as you don't know whether it was populated or not.

Slightly improved solution for 2nd variant - rehydrate UI dto.Id from query string using custom model binder (one generic implementation could be enough). Have to check, but expect it to be simple enough - first use default model binder, and then add id from ModelBindingContext data. Registering such binders (all generics for concrete models) might be a tricky point, though reflection/code generation could help. Contra - it requires writing extra code. Pro - you're assured that .Id field is always correct.
(Moreover, I've even saw in internet approach where svc.GetById() - that we have as initial code in some actions - was performed directly in model binder. I have mixed feelings about this way - as it moves logic on UI level, but in kinda "elegant" way, "pretty ugly", I'd say )) )

These're possible solutions I can see, and, pay attention, that "magical Dto" interfaces (mentioned initially) aren't really the obstacle.

---

And here are (finally) my recommendations:

• I'd prefer to have duplicated dto's for UI and DB, even if they looks similar. DB is not UI, right?
• I'd prefer to have .Id encapsulated into dto via automatic handling of .Id on UI level - custom model binders, mainly because they introduce other neat possibilities such as "full-fledged" (cross-fields - i.e. first name + last name, or complex fields - i.e. email) validation (that could be reused on code level - again emails), and I don't like all that metaprogramming (attribute-powered) stuff (not reusable on C# code level).

Unfortunately both my suggestions requires extra coding, so please "choose your poison" )

--
Feel free to ping me in Teams, if you'd like to discuss it in voice

[DmyMi]

do we ready need to track deleted quartz jobs? They they are either useful and exist in db or not usefull and we can safely delete it completely.

[alrom]

Ok. I'll fix it in the next PR.

[sonarqubecloud]

SonarCloud Quality Gate failed.   

0 Bugs
0 Vulnerabilities
0 Security Hotspots
1 Code Smell

53.8% Coverage
0.0% Duplication

The version of Java (11.0.20) you have used to run this analysis is deprecated and we will stop accepting it soon. Please update to at least Java 17.
Read more here

Catch issues before they fail your Quality Gate with our IDE extension SonarLint