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

request: API does not always include who on review entries #3857

Open
jberry-suse opened this issue Sep 19, 2017 · 4 comments
Open

request: API does not always include who on review entries #3857

jberry-suse opened this issue Sep 19, 2017 · 4 comments
Labels
API Things regarding our API Bug Frontend Things related to the OBS RoR app

Comments

@jberry-suse
Copy link
Contributor

Issue/Feature description

The tool I am creating to parse historical request data for metrics has revealed some seemingly inconsistent or broken data being returned via the API. From the request that I have analyzed it seems to occur when a review is either obsoleted or left new while request was placed in a final state such as accepted. Tho who values can be seen in the web interface and the history entries (which is likely what the web interface represents) and clearly there were performed by a someone, but seem to be arbitrarily missing in the returned data.

Expected result

The who would always be present.

How to Reproduce

osc api '/request/$reqid' for a request effected by this issue

Further information

Some examples (only showing relevant parts of request XML):

<request id="505869" creator="dimstar">
  <review state="new" when="2017-06-23T12:17:00" by_group="opensuse-review-team">
    <comment/>
  </review>
  <review state="new" when="2017-06-23T13:51:04" by_project="openSUSE:Leap:42.3:Staging:D">
    <comment>Being evaluated by staging project "openSUSE:Leap:42.3:Staging:D"</comment>
  </review>
</request>

One can see that staging-bot opened the review since it accepted the factory-staging review, but interestingly I see no way that staging-bot would have opened the factory-staging review since that is done automatically since the project includes factory-staging as a reviewer in the config.

  <review state="accepted" when="2017-06-23T11:48:15" who="staging-bot" by_group="factory-staging">
    <comment>Picked openSUSE:Leap:42.3:Staging:D</comment>
    <history who="staging-bot" when="2017-06-23T13:51:05">
      <description>Review got accepted</description>
      <comment>Picked openSUSE:Leap:42.3:Staging:D</comment>
    </history>
  </review>

Similarly this can be seen when a review is obsoleted since the project was removed.

<request id="454014" creator="leaper">
  <review state="obsoleted" when="2017-02-02T02:18:01" by_project="openSUSE:Leap:42.3:Staging:adi:5">
    <comment>Being evaluated by staging project "openSUSE:Leap:42.3:Staging:adi:5"</comment>
    <history who="jberry_factory" when="2017-02-02T06:28:55">
      <description>Review got obsoleted</description>
      <comment>reviewer got removed</comment>
    </history>
  </review>
</request>

Again, somehow the who is no longer present although it can be inferred from the factory-staging accept entry who.

  <review state="accepted" when="2017-02-02T01:36:48" who="jberry_factory" by_group="factory-staging">
    <comment>Picked openSUSE:Leap:42.3:Staging:adi:5</comment>
    <history who="jberry_factory" when="2017-02-02T02:18:21">
      <description>Review got accepted</description>
      <comment>Picked openSUSE:Leap:42.3:Staging:adi:5</comment>
    </history>
  </review>

Also note that I cannot re-create the loss of who by creating a fresh request and walking through these cases, so there is likely more to it.

As an aside, using <history> to indicate an event that takes place more recently than the primary <review> tag is a bit backwards although I can see what it is trying to convey.

So far I have found such occurrences in the following requests and analyzed a few to notice these cases, but that is not to say this is the complete set of cases. I would be rather curious to know what sort of code could cause this type of problem. I will likely try to workaround these cases since the who can be inferred since these are "related" requests due to being part staging workflow, but really this should be resolved. If I find that some are missing more I may just have to ignore them for the time being.

  • 285047
  • 285059
  • 312231
  • 321423
  • 339131
  • 360718
  • 361128
  • 366048
  • 390130
  • 390152
  • 390176
  • 390179
  • 390201
  • 390205
  • 390220
  • 390254
  • 390261
  • 390289
  • 390294
  • 390305
  • 390313
  • 390319
  • 390325
  • 390326
  • 390328
  • 390331
  • 390351
  • 390359
  • 390376
  • 390391
  • 390396
  • 390413
  • 390415
  • 390417
  • 390421
  • 390423
  • 390425
  • 390427
  • 390429
  • 390431
  • 390433
  • 390435
  • 390437
  • 390441
  • 390443
  • 390445
  • 390447
  • 390451
  • 390452
  • 390456
  • 390459
  • 390468
  • 390469
  • 390472
  • 390486
  • 390497
  • 390498
  • 390515
  • 390521
  • 390533
  • 390544
  • 390549
  • 390553
  • 390567
  • 390586
  • 390590
  • 390614
  • 390622
  • 390626
  • 390636
  • 390637
  • 390638
  • 390639
  • 390690
  • 390703
  • 390706
  • 390709
  • 390714
  • 390719
  • 390741
  • 390742
  • 390755
  • 390759
  • 390763
  • 390764
  • 390772
  • 390789
  • 390802
  • 390805
  • 390811
  • 390815
  • 390820
  • 390835
  • 390870
  • 391089
  • 391119
  • 391123
  • 391124
  • 391127
  • 391133
  • 391134
  • 391141
  • 391152
  • 391167
  • 391175
  • 391192
  • 391208
  • 391231
  • 391236
  • 391240
  • 391245
  • 391248
  • 391253
  • 391254
  • 391255
  • 391265
  • 391268
  • 391283
  • 391293
  • 391296
  • 391300
  • 391305
  • 391324
  • 391325
  • 391330
  • 391359
  • 391360
  • 391367
  • 391379
  • 391384
  • 391388
  • 391389
  • 391395
  • 391418
  • 391419
  • 391424
  • 391436
  • 391444
  • 391445
  • 391446
  • 391453
  • 391475
  • 391493
  • 391501
  • 391502
  • 391503
  • 391505
  • 391507
  • 391511
  • 391512
  • 391513
  • 391514
  • 391518
  • 391520
  • 391522
  • 391524
  • 391525
  • 391527
  • 391535
  • 391536
  • 391544
  • 391548
  • 391557
  • 391565
  • 391571
  • 391577
  • 391579
  • 391590
  • 391591
  • 391593
  • 391594
  • 391602
  • 391604
  • 391606
  • 391609
  • 391611
  • 391613
  • 391615
  • 391617
  • 391619
  • 391622
  • 391624
  • 391626
  • 391629
  • 391631
  • 391633
  • 391635
  • 391637
  • 391639
  • 391641
  • 391644
  • 391646
  • 391648
  • 391652
  • 391655
  • 391657
  • 391665
  • 391666
  • 391667
  • 391668
  • 391669
  • 391674
  • 391675
  • 391697
  • 391701
  • 391713
  • 391718
  • 391719
  • 391720
  • 391724
  • 391726
  • 391730
  • 391745
  • 391750
  • 391753
  • 391756
  • 391757
  • 391786
  • 391788
  • 391792
  • 391793
  • 391816
  • 391824
  • 391825
  • 391826
  • 391827
  • 391830
  • 391831
  • 391835
  • 391836
  • 391841
  • 391848
  • 391849
  • 391850
  • 391862
  • 391863
  • 391864
  • 391872
  • 391894
  • 391896
  • 391900
  • 391901
  • 391920
  • 391925
  • 391929
  • 391936
  • 391939
  • 391949
  • 391965
  • 391970
  • 391973
  • 391974
  • 391991
  • 391992
  • 392015
  • 392021
  • 392056
  • 392057
  • 392076
  • 392078
  • 392473
  • 392574
  • 392738
  • 392740
  • 392744
  • 392763
  • 392787
  • 392904
  • 392905
  • 392907
  • 392933
  • 392939
  • 392956
  • 392957
  • 392958
  • 392975
  • 392976
  • 392991
  • 393061
  • 393065
  • 393066
  • 393085
  • 393086
  • 393087
  • 393098
  • 393114
  • 393157
  • 393191
  • 393193
  • 393195
  • 393197
  • 393200
  • 393201
  • 393202
  • 393203
  • 393211
  • 393248
  • 393249
  • 393251
  • 393252
  • 393262
  • 393267
  • 393275
  • 393300
  • 393312
  • 393387
  • 393405
  • 394291
  • 394293
  • 394294
  • 395714
  • 395715
  • 395717
  • 395719
  • 395721
  • 395722
  • 395723
  • 395725
  • 395726
  • 395728
  • 395729
  • 395732
  • 395735
  • 395737
  • 395738
  • 395739
  • 395740
  • 395741
  • 395742
  • 395743
  • 395744
  • 395745
  • 395749
  • 395750
  • 395751
  • 395752
  • 395753
  • 395755
  • 395756
  • 395757
  • 395760
  • 395761
  • 395762
  • 395763
  • 395767
  • 395768
  • 395769
  • 395770
  • 395771
  • 395772
  • 395773
  • 395775
  • 395777
  • 395778
  • 395779
  • 395780
  • 395781
  • 395784
  • 395785
  • 395786
  • 395789
  • 395790
  • 395791
  • 395792
  • 395793
  • 395795
  • 395796
  • 395797
  • 395798
  • 395801
  • 395803
  • 395818
  • 395819
  • 395820
  • 395822
  • 395823
  • 395825
  • 395826
  • 395829
  • 395830
  • 395831
  • 395832
  • 395834
  • 395836
  • 395838
  • 395839
  • 395840
  • 395842
  • 395845
  • 395847
  • 395848
  • 395849
  • 395851
  • 395855
  • 395859
  • 395861
  • 395862
  • 395865
  • 395867
  • 395870
  • 395871
  • 395872
  • 395873
  • 395875
  • 395879
  • 395880
  • 395884
  • 395885
  • 395886
  • 395888
  • 395890
  • 395891
  • 395892
  • 395893
  • 395894
  • 395900
  • 395902
  • 395903
  • 395904
  • 395905
  • 395906
  • 395907
  • 395911
  • 395912
  • 395916
  • 395917
  • 395920
  • 395923
  • 395925
  • 395930
  • 395931
  • 395932
  • 395934
  • 395936
  • 396060
  • 396062
  • 396125
  • 396412
  • 396886
  • 397063
  • 397082
  • 397083
  • 397084
  • 397085
  • 397087
  • 397088
  • 397089
  • 397091
  • 397099
  • 397108
  • 397114
  • 397542
  • 397593
  • 397688
  • 397759
  • 397786
  • 397801
  • 397813
  • 397815
  • 397820
  • 404227
  • 418986
  • 422078
  • 422104
  • 422166
  • 423278
  • 424410
  • 425719
  • 436621
  • 444187
  • 454014
  • 456939
  • 483292
  • 489399
  • 489401
  • 489463
  • 492259
  • 493643
  • 495655
  • 495700
  • 495702
  • 497506
  • 501795
  • 502594
  • 505869
  • 509509
@ChrisBr
Copy link
Member

ChrisBr commented Sep 20, 2017

Just looked briefly at it but could be similar to what I fixed recently for @lchiquitto. Have a look at
#3123 and
#3736

So could be that it is already fixed but of course still wrong for old requests (maybe we can fix it with a migration?)

@jberry-suse
Copy link
Contributor Author

Hmm, looking at the changes to test case it does not include who there either:

@@ -108,8 +108,8 @@ def test_parse_bigger
    <state name="review" who="Iggy" when="2012-11-07T21:13:12">
      <comment>No comment</comment>
    </state>
 -  <review state="new" by_user="adrian"/>
 -  <review state="new" by_group="test_group"/>
 +  <review state="new" when="2017-09-01T09:11:11" by_user="adrian"/>
 +  <review state="new" when="2017-09-01T09:11:11" by_group="test_group"/>
    <review state="accepted" when="2012-11-07T21:13:12" who="tom" by_user="tom">
      <comment>review1</comment>
    </review>

So I would imagine that bit is not fixed since it seems the primary target of that issue was the when. I ran these queries in the last few days so certainly exists in production data. If that needs to be changed in db it would be much appreciated.

@jberry-suse jberry-suse changed the title /request/$reqid does not always include who on review entries even though it should request: API does not always include who on review entries even though it should Sep 22, 2017
@jberry-suse jberry-suse changed the title request: API does not always include who on review entries even though it should request: API does not always include who on review entries Sep 22, 2017
@bgeuken bgeuken added Bug Frontend Things related to the OBS RoR app P3 labels Sep 25, 2017
@adrianschroeter
Copy link
Member

reviews in state "new" may indeed not have a who, when these are default reviewers.

Do you have an example where a who is missing and the state got changed?

@jberry-suse
Copy link
Contributor Author

I included examples of reviews that were not automatically added (like reviews), but were added by someone.

 <review state="new" when="2017-06-23T13:51:04" by_project="openSUSE:Leap:42.3:Staging:D">

That review is set by the staging tools and therefore was performed by a user. In the other couple hundred thousand requests I processed it was there.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
API Things regarding our API Bug Frontend Things related to the OBS RoR app
Projects
None yet
Development

No branches or pull requests

6 participants