Fix "Reprocess" operation for reports

Is anyone interested in these kinds of patches?

@jbk Do you use the Reprocess feature? Does it work for you as is?

Yes, we're interested in these kind of patches.

For some reason I did not get any emails about this MR at all until now… Not sure what is wrong with GitLab again…

@ePirat @ToddShepard What about the merge request?

Hello @ePirat, @ToddShepard,
What are you thoughts on this merge request?
I can allocate some time on CrashDragon next week, perhaps we can make some progress?
(I also have a much more complex MR I'd like to discuss)
Regards

Hi, I unfortunately do not know that much about this code, @ToddShepard had a quick look too but does not really remember the specific reason for why it was done in the way it was here either. That said if it works fine for your setup, it might be fine, after all it should only affect reprocessing reports anyway…

We definitely appreciate MRs on CrashDragon, problem is more lack of time and code familiarity to review them.

@ePirat My objective is to keep the differences between my code and yours to a minimum.

I found a problem with the GORM in the new code, but I don't have a good solution.
It would be great if we could discuss it at some point

Hello @ePirat @ToddShepard @jbk

I want to ask your opinion.

We have made multiple changes to the code. Some changes might be useful to VLC, others likely not. Maintaining our project as a "soft fork" (regularly rebasing) is becoming time-consuming. Merging some of these changes back into your project would lessen the work required. However, I totally understand that you might not be interested in our changes (which I can describe in more details).

Therefore, I ask: should we just hard fork the project (and give up the possibility to merge changes back) or should we discuss our changes to see how much work could be merged upstream?

Regards

Can you provide a repository with your changes? That might be easiest so I can look through them and check which of them we can easily upstream.

Hi,

I have also patched ReprocessReport(), but in a much older version. I just wanted to highlight that it is critical that the GORM query is checked for errors. If the report doesn't exist, an uninitialized report is passed on to processReport() where processing fails and the report is deleted. However, since the report is uninitialized ALL reports are deleted. This was learnt the hard way from users trying to reprocess deleted reports... I believe it's still an issue in the current code base.

I've made the following change:

- database.DB.Where("id = ?", c.Param("id")).First(&Report)

+ if err := database.Db.Where("id = ?", c.Param("id")).Preload("Product").Preload("Version").First(&Report).Error; err != nil { 
+     c.AbortWithError(http.StatusNotFound, errors.New("the report does not exist")) 
+     return 
+ }

A similar error check is also missing in GetReport().

@ePirat @ToddShepard
Did you have a chance to take a look at this patch?

Fix "Reprocess" operation for reports

Merge request reports

Activity