Skip to content
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

Add All RPC Endpoints #80

Merged

Conversation

tobiasjungmann
Copy link
Collaborator

@tobiasjungmann tobiasjungmann commented Jul 17, 2022

This PR adds the endpoints in the proto file for all the endpoints presented in (for the "barrier free" requests) the issue "Implementing Existing Endpoints". Which attributes are returned in which request has been taken over from the backend. The corresponding data types were taken from the models in this campus backend. The http suffix is also taken from the old campus backend.

I decided to add the base for all endpoints to reduce the number of PRs which only purpose is to alter the proto file.
The Implementation itself will be done in smaller PRs which are much easier to review.

It would be great if someone with experience with the old backend can check whether I have included the correct attributes and the correct web request addresses. Primarily, I am not 100% sure with the structure of the endpoint "getAreaFacilitiesByBuildingNr".

sizeCache protoimpl.SizeCache
unknownFields protoimpl.UnknownFields

Id string `protobuf:"bytes,1,opt,name=id,proto3" json:"id,omitempty"`
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since I have no idea, how the building ids look like (I dont have access to the database):

  • What is the number of Buildings in this relationship?
  • Could you send them to me and would adding another field for the new (NavigaTUM) ID be possible (assuming that our IDs are not 1:1 compatible?

The reason for this comment is that we identified this as a blocking issue in TUM-Dev/Campus-Android#1462

Since the RoomId, RoomCode, BuildingNr and ArchId is already present, this should not be a huge issue, but I would need some more information about what field does mean what..

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can only give an overview over that type: The building_nr has the type varchar with length 8, but I don't know how the ids look like in the database. The database scheme itself is published in the db-models-branch in this Repo.

I can add this new field in this implementation, how would it look like? Is it a string or an Integer?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Example values which I would expect (from similar-ish naming):

ArchId: "27543"
RoomCode: "5407.01.760D"
BuildingNr: "5407"
RoomId: internal Id; just used in this API

Is this assumption right?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From my point of view, at least the datatypes would match this assumption. @joschahenningsen ca you provide any example data?

@tobiasjungmann tobiasjungmann marked this pull request as draft July 21, 2022 09:57
@tobiasjungmann tobiasjungmann changed the title Add Barrier Free Endpoints Add All RPC Endpoints Jul 21, 2022
@tobiasjungmann tobiasjungmann marked this pull request as ready for review July 27, 2022 07:55
api/CampusService.proto Outdated Show resolved Hide resolved
api/CampusService.proto Outdated Show resolved Hide resolved
api/CampusService.proto Outdated Show resolved Hide resolved
api/CampusService.proto Outdated Show resolved Hide resolved
api/CampusService.proto Outdated Show resolved Hide resolved
api/CampusService.proto Outdated Show resolved Hide resolved
api/CampusService.proto Outdated Show resolved Hide resolved
@joschahenningsen
Copy link
Member

Can you have a look at why the staticcheck is failing? Did you regenerate the .proto changes?

@tobiasjungmann
Copy link
Collaborator Author

I didn't rerun generate after merging, now they are passing again.

Copy link
Member

@joschahenningsen joschahenningsen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, thank you!

@joschahenningsen joschahenningsen merged commit c404335 into TUM-Dev:main Aug 3, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants