search

RaiderIO / raiderio-addon

RaiderIO AddOn
https://raider.io/addon
Other
40 stars 21 forks source link

add raid achievement label #167

Closed camclark closed 3 years ago

camclark commented 3 years ago

Meet request #131 with noted colours.

image

Note: that this change would be nice to have with MAINS_RAID_PROGRESS also. At this time there is more work required:

ghost commented 3 years ago

DeepCode failed to analyze this pull request

Something went wrong despite trying multiple times, sorry about that. Please comment this pull request with "Retry DeepCode" to manually retry, or contact us so that a human can look into the issue.

camclark commented 3 years ago

Looks to be failing automated testing, but I'm wondering if its a problem with test coverage. Since previous tooltip changes have also failed.

Can I get a review please @Vladinator89 @jahraphael

jahraphael commented 3 years ago

Hi @camclark, thanks for taking a look at this. However, this PR will not work as the addon simply doesn't have the right data to support this feature yet.

Proper handling for CE/AOTC needs to account for more than just whether the user has killed the last boss, as CE/AOTC are time-bounded achievements that you can't earn after a certain point. So when Sanctum comes out we would want to ensure that CE/AOTC are only showing on the previous raid progress records (i.e. CN) if they were actually earned then. On top of that CE/AOTC are account-wide achievements, and we'd want it to properly track its presence across region boundaries like our "Main's Progress" does.

So, alas, it's not feasible for anyone other than us to implement it in the addon at this time.

All that said, I do think this may be something we get to before 9.1.

camclark commented 3 years ago

Makes sense to me. Would be handy just to implement a store of AOTC/CE from your backend that can do the required checks.

Happy to help with implementation in the future. Feel free to ping me, @jahraphael.

Backend services are closed source aren't they?