Digital guide API foundation and adapted toilets

[24bartixx]

What has been made

• digital guide models modified and upgraded to support other tiles
• adapted toilet models
• adapted toilets repo
• adapted toilets screen
• compared all of the toilets (88) with the digital guide website

Why did I structure models like this

Digital guide response -> Levels -> Regions

Such models and endpoints structure are used by multiple unimplemented tiles in the digital guide. Therefore, it will be efficient to fetch this necessary data once instead of doing it on expanding individual tiles

I am familiar with most of the endpoints and db structure of this API and want to connect all of the implemented features once they are implemented. I think I can make it efficient.

Why doors are not implemented?

Doors details are used in a few tiles which is why it's better to implement them separately

Fun fact

Asked about what I do in this student organization by my family, I could answer toilets hehe

[tomasz-trela]

Good job on the task! I noticed a few things that could be refined.

[tomasz-trela]

Now LGTM!

[24bartixx]

Ready to merge?

[mikolaj-jalocha]

Good work here. That was a good idea with implementing Level it could reduce some complexities later on. Please write documentation about how it works and how to use it, some diagrams are welcome as well 😄

Some part of code are imo hard to read and understand but that's the nature of this API and we can't do much with it unless some better responses'd come :/ I'm leaning towards some kind of wrapper even more rn 😆

Below I left few minor comments and suggestions of mine.

[mikolaj-jalocha]

lgtm, good job!

with merging let's wait for Szymon's approval

[simon-the-shark]

so good job here, but uh - this is complex shit - but the api is pretty nasty structured so this isn't our fault.

As far as I understand we load levels and regions and then all other data is somehow grouped by it. It seems like a fine idea to load it up-front.
At first I was skeptical, but as far as I understand, we would need to get this and group by it in any other case too.
So as far as I can tell it, this looks like a fine job done. Maybe I'll think how to improve it in some future, but for now we can go with it and merge it.

My biggest concern now is the "NotFull" things - does it only mean that we load without proper url, only id and need to get the url from the other endpoint? Are there any other differences bewteen full and not full models?? If there's no difference than this, then I would refactor the whole thing completely and load the proper url in some new DigitalGuidePhoto widget. Unless there are some extra differences I've missed. Cause basically duplicating every models is nasty.

Apart from it, few small notes I've picked.

Tell me what you think

[simon-the-shark]

So this applies to both This and Olivier's PR (and basically all previous ones too): is there any difference between original models and the "Extended" / "Full" ones besides the fact that we need to load image urls from our photo's ids?

If not, then I would change our approach completely, cause now we're duplicating the models. I have a pretty nice idea in my opinion which would be a new custom widget DigitalGuideImage that loads the url itself using some imageRepository that takes id and load the url. But this would be done directly in this widget and not in ALL repositories like this is done now. And we wouldn't repeat all the models. Also the image can show some nice loading state - no problem here.

But maybe I'm missing something. Tell me what you think and do you undestand my idea? Also Am I right that this can be done this way and are there more differences between "NotFull" and "Extended" models?

I can help to do this if you don't understand my idea and let's coordinate the thing so the two of you won't do it twice and duplicate the same work.

[24bartixx]

I will implement the mentioned improvements soon. No worries!

@simon-the-shark Regarding your question I think it's a decent idea and probably there aren't any contraindications right now. However, the concept of the DigitalGuideImage widget and related refactoring of models for simplicity should be performed in another task. Each model pairs (Extended / NotFull) have to be analyzed separately. If the issue is the image URL solely, we should implement your idea. Otherwise, figure out a better idea because I don't like the current model structure in the digital guide either

[simon-the-shark]

Yeah. It can be a separate task

[24bartixx]

Please let me know whether I put too many commits. Should I do less commits with more changes or such separation is alright

[simon-the-shark]

Please let me know whether I put too many commits. Should I do less commits with more changes or such separation is alright

So about the commits first:

I don’t care veery much, cause I’m gonna probably squash them all to one commit before merging to main anyway. But since, you’re asking:

• This is a matter of convention. Some people (or companies or teams) prefer very „atomic” changes. Very small, very detailly described and due to that, extremelly fragmented into milion commits

• personally I think the most important is finding balance between atomic and yet human readable. Too much commits is just sometimes clutter that doesn’t give much value for the person who reads it. If some things are part of a bigger feature/subfeature or smth I’m okey with having them in one bigger commit.

• One thing that is probably more widely accepted as something not so good are commits like this:

  • „apply pr suggestions”
  • improvements
  • minor changes
  • „fix fix”


• this is zero value for the people that reads them. This just means nothing. When possible commit messages should be more about functionality the commit add or change.

  • If you want to be a PRO git user that has very clean commits history, PR suggestions should very rarely be applied as new commits. Most small changes/suggestions can be added to already existing commits and then the branch hiisotry can be forced-pushed - this way we don’t have meaningless follow-up commits on the PR. Only warning is that u can’t force push branch when u collaborate with someone there. But if u own the branch in 100% (like in most of our feature branches), u can force push. (Using —force-with-lease is a bit more safe and gives the same results)
  • but for this you would have to get into more mechanisms like interactive rebase, git fixup, git ammend. There’s also unofficial tool for this: git absorb but this doesn’t always fully works as expected.

Hopefully u get what I mean. As I said, every company have its own convention. I don’t care here that much, cause I prefer to do a squash before merging anyway - but this again is a matter of convention. Some people prefer simple rebase that preserves all atomic commits.

[simon-the-shark]

Oh it looks goood now. I like it. Good job. Last small notes and we're merging

[24bartixx]

• fixup

Thank You for such an insightful explanation, I found it very informative

[simon-the-shark]

• fixup

Thank You for such an insightful explanation, I found it very informative

Always happy to help

[simon-the-shark]

Sorry I'm still blocking this, but one thing I've noticed in the recent changes. I hope we can get it merged soon. Also u have some conflicts with main that u need to resolve first :)

[simon-the-shark]

adaptedToiletsData could be of a type that also has IList, like IMap<int, IList<AdaptedToilet>>, and then you wouldn't need to do that if, right? If I'm not mistaken?