[MM-52887] Fix panic if todo is not part of a project

[hanzei]

Summary

According to https://docs.gitlab.com/ee/user/todos.html there are todos that are not part of a project. For example, member access requests for groups are only tied to the group, not a project. This PR fixes the NPE by checking if a project is there first.

There are no tests that cover the outgoing API requests, and hence the changes in this PR are not covered by tests.

Ticket Link

https://mattermost.atlassian.net/browse/MM-52887
Fixes #372

[codecov]

Codecov Report

Patch coverage has no change and project coverage change: -0.02 ⚠️

Comparison is base (4a527b4) 31.20% compared to head (8acc4f4) 31.19%.

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #378      +/-   ##
==========================================
- Coverage   31.20%   31.19%   -0.02%     
==========================================
  Files          21       21              
  Lines        3778     3780       +2     
==========================================
  Hits         1179     1179              
- Misses       2479     2481       +2     
  Partials      120      120              

Impacted Files	Coverage Δ
server/plugin.go	15.02% <0.00%> (-0.05%)	⬇️

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

[Willyfrog]

LGTM thanks for the quick fix

[mickmister]

Thanks for addressing this @hanzei. I just have one request to make the code even more defensive, by checking that the todo itself is not nil

[hanzei]

@mickmister Your suggestion seems quite defensive to me, but I don't mind adding that additional layer of protection. Even though I slightly changed it to skip all nil entries.

[DHaussermann]

@hanzei I tried testing this but I must have misunderstood something. On the existing release I cannot reproduce the panic.
I created a 2nd group that my test user has access to and I have ToDo in multiple projects from multiple groups.

I don't see the crash when trying to pull back the data and the use of the lock field filters all the data as expected.

Is there something I've missed in the repro steps?

[hanzei]

@DHaussermann In GitLab, users can request access to a group (https://docs.gitlab.com/ee/user/group/#request-access-to-a-group). The group admin then gets a todo item added to their list. On master, the plugin should record a panic in the logs once that todo got added. Can you confirm that?

[hanzei]

@DHaussermann Do you need anything in order to test the PR?

[DHaussermann]

@hanzei for a reason I never managed to figure out... Most of my test groups are all missing the request link.
I was eventually able to find one that worked as expected and reproduced the panic 👍
Thanks for providing the additional context.

I did test this and the panic is resolved. For the user affected with the pending request the /todo becomes functional again.
The one draw back is that there does not seem to be a message body implemented for the RHS content here.

And the shorthand in the ToDo list is missing the name of the requester

Please advise if you have any thoughts on next steps. I don't know how much bandwidth it would take to implement a solution for these 2 UI gaps. But, given that if this happened it causes a panic and renders /todo unusable, I'm inclined to accept the PR as is and open follow-up issues in the repo if it requires any significant effort.

[DHaussermann]

@hanzei Do you have any thoughts you can share on this? Either implementing a solution to the UX gaps or accepting the solution as is. I'm happy to accept this and create follow-up tasks if you don't have the bandwidth to add any more to this.

[Willyfrog]

@DHaussermann I think it is ok to create follow-up tickets

[DHaussermann]

Tested and passed

• Ensured the panic is resolved when the event occurs
• todo will become functional again
• Moved UX issues to UX gap when group membership is requested  #383 so this can be merged.

LGTM!

[DHaussermann]

Sorry @hanzei - I meant to approve with the comment Monday ☝️

Please merge when you have a chance.