Add long_ids (original, uncompressed id's) in ManageIQ.gridChecks

[AparnaKarve]

Noticed that ManageIQ.gridChecks now contains compressed id's instead of the non-compressed ids (or long ids).

This seems like a recent change...

and has introduced a bug, because newer screens like Physical Servers and Generic Object Definitions, use the id to pass it to the REST API for a Delete operation

Since the API does not recognize the compressed id, it causes a 404 error.

The change in the PR fixes the issue.
Although, I would need @karelhala input here, because we did not need this code before.

@karelhala Any idea what changed? Also should this be fixed some place else (and not here)?

[himdel]

@karelhala Should we even have the short ids in gtl?

Looks like gaprindashvili is the last release with 2 kinds of ids so trying to figure out if we can't just make id the long version instead, instead of adding code we'll need to undo when we drop the cid support.

(Note to self: If this is merged as is (using long_id), add this as a TODO item to #2482.)

[AparnaKarve]

Should we even have the short ids in gtl?

I don't think we should - short ids will lead to 404 errors if used in API calls.

trying to figure out if we can't just make id the long version instead

I think that is what is required.

@himdel If you are OK with this change, should I go ahead and fix the JS specs?

#2482 would have fixed this for us. But in the interim, do you have an alternative if we decide not to go with this proposed solution?
Thanks.

[himdel]

@AparnaKarve I'm fine with your code as it is :)..

I was just wondering if we can make the GTL use the long ones by default (in id, not long_id) so that we don't actually have to use the long_id field since it's probably going away (because the value will live in id).

But if that's a non-trivial change (or if there are still places relying on the short id), no objections to just merging this PR, we just have to ping Tim to undo it when removing the compressed ids :).

[karelhala]

@himdel @AparnaKarve well the short ID is used in some screens for correct redirect (sadly I do not remeber which ones). But sure we can move the long_id to id field and see if something breaks. I guess either those screens which required short ID will behave same way with long ID or they were fixed to use long ID anyways. So feel free to remove the short ID, couple of screens might get broken, but that's easy to fix afterwards.

[AparnaKarve]

So feel free to remove the short ID, couple of screens might get broken, but that's easy to fix afterwards.

@karelhala For now, I have made s/id/long_id/changes only in the onItemSelect function because that's where I need them at the moment.
I think the other areas can wait if there are no bugs in those areas related to compressed/uncompressed ids.

[himdel]

@AparnaKarve Sorry, I lied.. turns out, long_id is currently being sent as a number in the report data, not a string.

That means we can't reliably use long_id in JS (would break for regions where the number is large enough for the whole long_id to be longer than js INT_MAX) :(.

---

Would using miqUncompressedId to convert the id field to the long value work for you instead?

(Or we can try fixing report_data so that long_id always comes as a string.)

(trying to figure out which of these is the least breaking change for gaprindashvili..)

[himdel]

.. Never mind, turns out changing long_id to string should not affect anything else..

So #2902 should help, and with that one, I think this can go in as is (haven't tested yet).

[AparnaKarve]

@himdel Thanks for #2902
One way to test this would be to try a Delete on a Generic Object Definition record.

[miq-bot]

Checked commits AparnaKarve/manageiq-ui-classic@8c913fa~...ba218a8 with ruby 2.3.3, rubocop 0.47.1, haml-lint 0.20.0, and yamllint 1.10.0
0 files checked, 0 offenses detected
Everything looks fine. 🏆

[miq-bot]

Checked commits AparnaKarve/manageiq-ui-classic@8c913fa~...ba218a8 with ruby 2.3.3, rubocop 0.47.1, haml-lint 0.20.0, and yamllint 1.10.0
0 files checked, 0 offenses detected
Everything looks fine. 👍

[himdel]

LGTM, verified I can delete GOD objects :) 👍

@AparnaKarve this is present in gaprindashvili as well, right?
Adding gaprindashvili/yes unless you disagree :).

[AparnaKarve]

Adding gaprindashvili/yes

Yes please. Thank you!

[simaishi]

Gaprindashvili backport details:

$ git log -1
commit 1bd1a914bbde940828ed08ef752f912e1cfe4b59
Author: Martin Hradil <[email protected]>
Date:   Mon Dec 4 17:47:10 2017 +0000

    Merge pull request #2791 from AparnaKarve/fix_miq_gridchecks_ids
    
    Add `long_id`s (original, uncompressed id's) in ManageIQ.gridChecks
    (cherry picked from commit 25a619e2f40a7b1575ceb2c2d2ba4a8380a37ff5)

[JPrause]

@miq-bot add_label blocker