Comment Search

Defect Metrics

Statistics on the results
Ranking
Defect Report
Classification
Defect Report
  • 1077 Defects:
  • 435 major defects, and
  • 642 other defects
  • from 4201 comments

Search Criteria

Search for comments

Search Criteria

Project
Comment content
Review PermaId
After
Before
Comment Author
Search Type
Search Type:
Review State
Review State:
Metrics:
Metric Ranking
Metric Classification
Submit
  •  

Comment Results

Review Name Created Custom Fields Content
CR-163 12 Apr 2017

??? waarom deze specifieke en niet de algemene ???
CR-163 12 Apr 2017 Classification: Improvement desirable Ranking: Minor

Er zal wel ergens vhTest.Utils.APPS.TRANSP.dto.LeveringAdres).EenBelg staan vermoed ik .. en als het nog niet zo is .. maak ze maar

CR-142 13 Apr 2017 Ranking: Major Classification: Factually incorrect

PropertyNaam in ECON is "Verpakkingtype"
CR-142 13 Apr 2017 Classification: Factually incorrect Ranking: Major

PropertyNaam in ECON is "Corpuswanddikte"
CR-142 13 Apr 2017 Ranking: Major Classification: Factually incorrect

PropertyNaam in ECON is "Matmateriaal"
en ook op de lijnen hieronder :-P
CR-179 18 Apr 2017 Classification: Missing Ranking: Major

Gelieve de bewuste id in de foutmelding op te nemen en indien mogelijk ook welke type bericht het dan wel was, kwestie om zo duidelijk mogelijke foutmelding te kunnen geven.
ps : Enkel eigennamen of het begin van een zin begint met een hoofdletter

CR-179 18 Apr 2017

?? mag deze method niet weg daar de response controleur kan gebruikt worden ??
CR-179 18 Apr 2017 Ranking: Major Classification: Risk-prone

Beter een $$$Not(BevatFout) zodoende 0 en "" hetzelfde terug geven.
CR-179 18 Apr 2017

Lijkt mij beter om VerzendData uit te breiden met BevatFout en Foutmelding zodoende er maar één object moet doorgegeven worden. Idem dito voor de CancelContainer..
CR-179 18 Apr 2017 Ranking: Major Classification: Risk-prone

Gelieve het gedrag en het gewenste resultaat iets duidelijk in de test te vermelden zodoende er kan gecontroleerd worden dat de test setup wel is volgens het vooropgestelde gedrag. De volgende stap is dat dat de code voldoet aan het vooropgesteld gedrag van de test.
CR-179 18 Apr 2017 Classification: Not conforming to standards Ranking: Major

Utils mogen classmethods zijn
CR-179 18 Apr 2017 Ranking: Major Classification: Editorial

GeefVerzendData zegt weinig over de wat voor verzend data het is ? Als het echt niet uitmaakt gelieve dan een ##class(vhTest.Dummy) te gebruiken als het wel van belang is gelieve dan een iets duidelijkere benaming te kiezen.
CR-179 18 Apr 2017

White space

CR-179 18 Apr 2017 Ranking: Minor Classification: Not conforming to standards

macro $$$false gebruiken ipv 0
CR-174 18 Apr 2017 Classification: Factually incorrect Ranking: Major

IngegevenKenmerkenApi transient zetten zodoende dit geen impact heeft op de storage
CR-174 18 Apr 2017

Denk niet dat deze nodig op de fake ?
CR-163 18 Apr 2017

Ik zou toch opteren voor andere data zodoende er een verschil tussen de GLSParcelNumber als de GLSUnitNumber zodat we zeker zijn we het juiste veld overgekopieerd hebben.
Idem dito voor de TrackAndTraceUrlVoorKlant en dienen voor VH
CR-163 18 Apr 2017 Ranking: Minor Classification: Not conforming to standards

Assert methods mogen geprivatiseerd worden
CR-184 20 Apr 2017 Classification: Improvement desirable Ranking: Minor

Lijkt mij beter om de URL te gaan bepalen in de afzonderlijke converters ipv hier nog weet te moeten hebben van vhisie3 en vhisie4.
CR-167 20 Apr 2017

:thumbs up:
CR-190 21 Apr 2017 Ranking: Minor Classification: Not conforming to standards

private?
CR-190 21 Apr 2017

Is het niet nodig om het gedrag van deze method te verduidelijken dmv een paar testjes? Of door op te nemen in reeds bestaande tests? En ja, ik weet dat het een lokale (private) method is en dat ge unittests schrijft en geen methodtests en jadajadajada, maar voor een leek zoals ik is het niet onmiddellijk duidelijk hoe het gewijzigde gedrag het probleem zoals omschreven in de titel oplost. Voilà.

Let the rebuttal begin...
CR-191 21 Apr 2017 Ranking: Minor Classification: Improvement desirable

Private?
CR-189 21 Apr 2017

Spijtig dat we altijd zoveel testfiles wijzigen, zouden beter systeem moeten hebben om er minder afhankelijk van te worden. De unittest in deze commit is al een stapje in de goede richting.
CR-163 21 Apr 2017

De parcel en unit id zijn gebaseerd op zelfde veld uit de verzend data. ze moeten dus de zelfde waarde hebben...
idem voor de t&t url
CR-188 26 Apr 2017 Classification: Not testable Ranking: Minor
  • Niet alle commits van ICT242 zijn opgenomen in deze review
  • Geen testen ==> needs resolution
  • Ik zie geen inhoudelijke wijzigingen, enkel code generatie aanpassingen (bv. package name, cdata)
CR-188 27 Apr 2017

Niet alle commits van ICT242 zijn opgenomen : 'k weet het - kzou de SVN repo hier ook moeten aan toevoegen ofwel die code in de common steken ben er nog niet 100% wat hier mee te doen.
Geen testen ==> needs resolution -> De testen zitten dus in de svn-repo
CR-196 27 Apr 2017 Classification: Editorial Ranking: Minor

Ik zou het eerder anders benoemen qua foutmelding :

Ofwel expliciet testen op aantal lijnen in de winkelkar : en melden dat er geen lijnen zijn

Ofwel de melding veranderen naar 'Er kon geen leverdatum bepaald worden voor winkelkar 'Den_ID', mogelijks zitten er geen lijnen in de winkelkar'
CR-178 27 Apr 2017 Classification: Factually incorrect Ranking: Major

Properties renamen met storage moet je opletten dat de properties niet onder de nieuwe naam een locatie innemen
CR-178 27 Apr 2017 Classification: Not conforming to standards Ranking: Major

Nog zo'n gevalleke
CR-148 08 May 2017

ik heb een sterke voorkeur om deze berekening af te zonderen, om verschillende redenen.
ik denk vooral aan de methods : LBX.GeefUitsparingCode() en LBX.GeefRugwandMateriaal()
Graag effe bespreken met WV.
CR-148 08 May 2017 Ranking: Minor Classification: Not conforming to standards

Rugwand sifon : niet ophalen uit Materiaal van Z1, maar van X
CR-148 08 May 2017

idem hierboven
CR-148 08 May 2017

Opmerking: zie EconNaarApps.LBX
CR-214 11 May 2017

Ook een property MailReplyTo bijvoegen (omdat de meeste mails vanuit de server verstuurd worden)
Dit principe moeten we meer en meer gaan gebruiken, om te vermijden dat de replies naar "CacheAdmin@..." worden gestuurd.
CR-214 11 May 2017 Classification: Not conforming to standards Ranking: Minor

Het "Resultaat" vind ik nogal kort door de bocht :

Technisch beschouwd :
Het resultaat van "Render" is een pdf-stream
Het resultaat van RenderAndFile is een bestand (of een error) --> ObjResultaat.BestandVolledigeNaam en ObjResultaat.ErrorMessage
Het resultaat van RenderAndMail is een OK of een error --> ObjResultaat.ErrorMessage
Het resultaat van RenderAndPrint is een OK of een error --> ObjResultaat.ErrorMessage

Conclusie:
ook al is "RenderAndFile" voorlopig de enige die resultaat MET inhoud teruggeeft, toch zou ik het result-object dus specifiek benoemen, iets zoals hierboven,
eerder dan RenderResultaat.DocNaam
CR-214 11 May 2017

Twee belangrijke opmerkingen (dit is de vhisie van WV en JCL):

  • input params zijn enerzijds te beperkt en anderzijds te specifiek : "MailOpdracht" -> beter een algemene "Opdracht" met ook "Render-opties" object [ Te bespreken met WV/JCL]
  • OrderData is OK, maar de structuur moet een "propere" dto zijn die AX moet invullen, en wij zullen er in caché dan wel de bizarre "pxMemo"-structuur (e.d.) van maken
    Let wel: VH legt de propere structuur vast (eventueel met overleg met ADU)


Zelfde opmerking geldt uiteraard voor Offerte- en LeveringService.
CR-217 12 May 2017

In dit soort unittest steeds een positieve case en een negatieve case behouden (dit is een algemene regel)
Hier komt dit neer op : minstens één testmethod ZONDER exceptie en één MET exceptie.

Ge moet in deze test niks meer aanpassen, aangezien TIPON volledig uitgefaseerd wordt.
dus : Review OK

CR-200 16 May 2017 Classification: Factually incorrect Ranking: Major

moet dit niet https://www.vanhoecke.be worden ???
CR-200 16 May 2017 Classification: Extra (superfluous) Ranking: Minor

Beter een deploy klasse maken die het oude config item delete ...
CR-203 16 May 2017 Ranking: Minor Classification: Editorial

HIer niet beter de valuelist van de enumeratie gebruiken zodoende we duidelijk kunnen zien dat alle lade dieptes van toepassing ..
CR-206 16 May 2017 Classification: Factually incorrect Ranking: Major

Lijkt mij toch niet overeen te komen APPS.TRANSP.GLS.dto en APPS.TRANSP.GLS.GLSv1.data
CR-206 16 May 2017

one liner

CR-206 16 May 2017

one liner
CR-206 16 May 2017 Ranking: Major Classification: Risk-prone

Dus met de IsGelukt is false mag je doen wat je wil..

Maar ik zou zeggen indien Timeout --> Er was een timeout probeer later eens opnieuw en in het ander geval UnitIdNumber : [ KlantInfo ] - Foutbericht..
CR-207 16 May 2017 Ranking: Major Classification: Ambiguous

Taal ????
CR-213 16 May 2017 Classification: Editorial Ranking: Major

Kuist de kak eens op
CR-213 16 May 2017 Ranking: Major Classification: Editorial

Zinloze commentaar opkuisen
CR-227 17 May 2017 Ranking: Minor Classification: Not conforming to standards

$$$Not
CR-227 17 May 2017

testje voor 0 palletten toevoegen