dirkschumacher / iUPB

A popular app for the University of Paderborn, Germany [project is dead]
http://www.i-upb.de
GNU General Public License v3.0
4 stars 2 forks source link

Mensa API v2 #70

Closed MichaelWhi closed 10 years ago

MichaelWhi commented 10 years ago

saves more info from the csv in our db including localized descriptions. also introduces an ordering, inspired by the sorting in MensaUPB

dirkschumacher commented 10 years ago

Dear Mr. Whittaker,

I just talked to QA and they are not happy merging failing commits.

Thank you D. Schumacher

MichaelWhi commented 10 years ago

Tests aren't really broken, but have wrong expectations ... I will try out the release later and fix the tests tmrw

dirkschumacher commented 10 years ago

But that means you didn't write tests for your code :)

MichaelWhi commented 10 years ago

status update: now running in production. will merge this after adding tests :)

MichaelWhi commented 10 years ago

open todos:

dirkschumacher commented 10 years ago

I see if I find some time tonight to work on it.

ironjan commented 10 years ago

A long post - sorry in advance ;)

First of all an API call (e.g. restaurants/Mensa.json) should directly return an array of menus instead of an array of wrapped menus. [0]

Also I'd propose the following changes to the menu entries:

The new API probably should rename description to name and discard the attribute name like it used right now. Prices probably could be split into three attributes.

One thing I'm missing is the so called Tara attribute. I added it as "pricesPerKilo" since it's used like ("Tara" -> price per kg, "Kein Tara" -> price per portion)

Badges and types should imho be constants like allergenes - that enables API users to easily

Of course using constants instead of translated strings would mean more documentation and a little bit of work on the client side (client side includes the web interface) but I think it's worth it.

The field order_info should be a client side decision. Thus this field should be removed.

Now:

[
    { 
        "menu": {
            "date": "2014-03-04",
            "description": "Kartoffelgnocchi mit Hackfleischsauce vom Schwein",
            "name": "Mittagessen",
            "type": "Essen",
            "price": "Stud. 1,60 / Bed. 2,70 / Gast 3,20",
            "counter": null,
            "allergens": [
                "2",
                "15",
                "A3",
                "A7",
                "A9",
                "38",
                "A15"
            ],
            "order_info": 0,
            "badges": []
        }
    }
]

Proposed:

...
  [
    {
        "date": "2014-03-05",
        "name": "Germknödel mit Kirschen und Vanillesauce",
        "type": "LUNCH",
        "priceStudents": 1.05,
        "priceWorkers": 2.15,
        "priceGuests": 2.65,
        "counter": null,
        "allergens": [
            "1",
            "2",
            "4",
            "15",
            "A1",
            "A3",
            "A7"
        ],
        "badges": [
            "VEGETARIAN"
        ],
        "pricesPerKilo": false
    }
]

[0] Right now the API calls (from me) look like this:

@Get("restaurants/Mensa.json")
MenuWrapper[] getMenus()

class MenuWrapper{
Menu menu;
// setters, getters
}

class Menu{
 // put menu attributes here
}
ironjan commented 10 years ago

Also there should be a way to get all menus for all restaurants (could not find an API call for that). the structure of this json could look like:

[
{
"restaurant":"Mensa",
"menus":[…]
},
{
"restaurant":"Hotspot",
"menus":[…]
}]

also restaurant identifier as constants (see last comment)

dirkschumacher commented 10 years ago

looks good to me

dirkschumacher commented 10 years ago

Ok I fixed the tests, but @MichaelWhi let's try to not break tests and to also write more tests. iUPB has already gotten quite a monster with 0 test coverage.

MichaelWhi commented 10 years ago

OK. Reg. Tests: I think I will not have time to maintain tests for trivial features with complex data sources that are constantly changing... However, I'll try my best :) Reg. the changes: good ideas, make sense for native apps. Nevertheless, we have to make sure that the API is self-explaining and kind'a hyper-media/rest-style. My goal is still that all small pet projects and touchdevelop demos still work easily with a couple simple Json calls ;) So we have to be careful with constants or batch API calls. Maybe we just need some more routes or some parameters to alter the representation. I think I'll have time on Friday to work on iUPB again.

ironjan commented 10 years ago

Just to let you know: since the STW wants to provide a json API soon, I added some proposals to https://etherpad.cs.uni-paderborn.de/epad/p/speiseplan

This comment is here because my proposal is based upon iUPBs API but keeps in mind that the STW API mainly is a provider for clients (either iUPB, MensaUPB, an STW app). Also I extended some things (opening times). I'd prefer to let someone look at that design before I'm implementing a prototype to test it or the STW implements it.

It would be a shame to base the json API on the CSV "API".

MichaelWhi commented 10 years ago

zur Info, ich habe auf Basis deiner Anmerkungen mal folgendes Format vorbereitet:

 {
        "date": "2014-03-05",
        "name": "Germknödel mit Kirschen und Vanillesauce",
        "category": "Eintopf 1" // optional
        "type": "LUNCH",
        "prices": {                        // Preisinfos in einem Hash
                "per_100g": false,
                "students": 1.05
                "staff": 2.15,
                "guests": 2.65,
                "currency": "€"
        },
        price: "pro 100g: Studenten 1,05€ / ...", // vorformulierter String, wie bisher
        "counter": null,
        "allergens_raw": [   // Rohdaten zum Umwandeln mit Hilfe der off. Zuordnung
            "1",
            "2"
        ],
        "allergens": [ // passend lokalisiert
            "Farbstoff", 
            "konserviert"
        ],
        "badges": [    // passend lokalisiert
            "vegetarisch"
        ]
        "badges_raw": [   // Rohdaten zum Umwandeln mit Hilfe der off. Zuordnung
            "3"
        ]
    }

Ich bin mir wegen Konstanten/übersetzten Werten noch nicht so sicher... Da die Konstanten bzw. Werte ja nicht von uns abhängen, sondern auch vom STWPB geändert werden können, würde das u. U. auch nicht vollständig sein. Was hältst du von "badges_translated" und "badges_raw" oder iwie so? vllt ein raw-Parameter für Allergene und Badges? Wir würden schon gerne in der API die Daten haben, damit man auch schneller mal was daraus bauen kann und nicht sooft den AppCode ändern muss (das is ja fast bei nativen Apps noch schwieriger...)

Die Ideen mit Öffnungszeiten etc. finde ich auch gut... Das sieht aber schon fast wie REST/Hypermedia aus, da könnte man auch das Restaurant mit den Infos und Öffnungszeiten als API-Einstiegspunkt nehmen und vom Restaurant aus dann die Menüs anzeigen....

dirkschumacher commented 10 years ago

Trotzdem sollte es schon getestet werden, bevor du es pushed. Alle Tests nachträglich decken eh nur sehr defensiv die Änderungen ab

ironjan commented 10 years ago

Ich glaube, das currency Feld kann weggelassen werden; counter auch, da diese "dank des Speiseleitsystems nicht mehr notwendig sind" und somit nicht mehr vom STW gepflegt werden (Wert dürfte immer null sein).

Eventuell könnte man das beide Vorschläge vereinen, um die Redundanzen (x, x_raw) herauszunehmen.

Ich denke mal, dass die "non_raw" Felder on-the-fly aus den "raw"-Feldern im selben Aufruf gebaut werden bzw. dass die Zuordnung "raw" -> "non_raw" hinterlegt ist. Hier Ausschnitte aus dem Pull-Request, wie es möglich sein könnte:

// translation for en added
data["prices"] = { "de" => "#{einheit}Stud. #{('%.02f' % stud_price).sub(".", ",")} / Bed. #{('%.02f' % staff_price).sub(".", ",")} / Gast #{('%.02f' % guests_price).sub(".", ",")}"
"en" =>  "#{einheit}Students #{('%.02f' % stud_price).sub(".", ",")} / Employees #{('%.02f' % staff_price).sub(".", ",")} / Guests #{('%.02f' % guests_price).sub(".", ",")}",
"raw" => {
        "per_100g" => per100g,
        "students" => '%.02f' % stud_price,
        "staff" => '%.02f' % staff_price,
        "guests" => '%.02f' % guests_price,
        } 
}

// ...

data["allergens"] = additives.map do |additive|
       case additive.try(:strip)
          when "1"
            {"de" => "Farbstoff", "en" => "artificial coloring", "raw" => "1"}
          when "2"
            {"de" => "konserviert", "en" => "preserved", "raw" => "2"}

// ...

Je nachdem, wo und wie das JSON generiert $root/Mensa Annahme: Deutsche Lokalisierung gewünscht, optional: "lang=de", "lang=en"

{
    "date": "2014-03-05",
    "name": "Germknödel mit Kirschen und Vanillesauce",
    "category": "Eintopf 1",
    "type": "LUNCH",
    "prices": "pro 100g: Studenten 1,05€ / ...",
    "allergens": [
        "Farbstoff",
        "konserviert"
    ],
    "badges": [
        "vegetarisch"
    ]
}

GET $root/Mensa?raw=true

(Unschön, aber auch möglich: "lang=raw")

{
    "date": "2014-03-05",
    "name": {
        "german": "Germknödel mit Kirschen und Vanillesauce",
        "english": "Germknödel mit Kirschen und Vanillesauce"
    },
    "category": "Eintopf 1",
    "type": "LUNCH",
    "prices": {
        "per_100g": false,
        "students": 1.05,
        "staff": 2.15,
        "guests": 2.65
    },
    "allergens": [
        "1",
        "2"
    ],
    "badges": [
        "3"
    ]
}
MichaelWhi commented 10 years ago

FYI: i will find time end of the week to rework this and put a draft online, sry for the delay

MichaelWhi commented 10 years ago

Sorry for the long delay, was too busy recently... I will find time this week