Early Bird Registration for DrupalCon Portland 2024 is open! Register by 23:59 PST on 31 March 2024, to get $100 off your ticket.
Comment | File | Size | Author |
---|---|---|---|
#17 | 3171393-17.patch | 10.87 KB | liuchanggang |
Comment | File | Size | Author |
---|---|---|---|
#17 | 3171393-17.patch | 10.87 KB | liuchanggang |
Comments
Comment #2
liuchanggang CreditAttribution: liuchanggang commentedComment #3
liuchanggang CreditAttribution: liuchanggang commentedThis patch needs rebase when 3171199 is merged.
Thanks
Comment #4
yas@liuchanggang
Thank you for the refactoring. Could you please re-create the patch due to the recent merge?
Comment #5
liuchanggang CreditAttribution: liuchanggang commented@yas,
I have rebased and retested it. It is ready to merge.
Thanks
Chang
Comment #6
liuchanggang CreditAttribution: liuchanggang commentedChanged status updating function.
Comment #7
yas@liuchanggang
Thank you for the update. I tested the patch but it didn't work as we expected.
Status Code: deleted
Could you please double-check the behaviors accordingly?
Comment #8
liuchanggang CreditAttribution: liuchanggang commented@yas,
Question: Do we want to show the items of DELETED status?
By now, if we click the Refresh button, Deleted VPC Peering Connections shows up on list page and same for Failed ones.
By AWS rule, failed VPC Peering Connections stay for 48 hours before disappearing. Deleted items have other rules
Do we want to change this behavior?
Thanks
Comment #9
yas@liuchanggang
Could you please give us the test scenario(s) and the expected result(s)?
Thanks
Comment #10
liuchanggang CreditAttribution: liuchanggang commented@yas,
1, Created a few of AWS VPC peering connections, they could have Failed or Pending status.
2, Delete those with pending status. They would change to Deleted status.
3, Click Refresh button, we can see those failed/deleted VPC connections on list page. They should disappear in 24 hours from API output.
4, During the first 24 hours, when users click the Edit/Delete button/link, it will redirect to list page and show warning message
"The VPC Peering Connection Name has deleted status. It can't be edited or deleted."
"The VPC Peering Connection Name has failed status. It can't be edited or deleted."
5, after 24 hours, they are not in API output any longer, when we click Edit/Delete, entity would be removed and redirect to list page showing warning message
"The VPC Peering Connection Name has already been deleted."
Comment #11
liuchanggang CreditAttribution: liuchanggang commented@yas
I updated the warning messages to be
''The VPC Peering Connection test1 has failed status. During this state, it cannot be accepted, rejected, or deleted. The failed VPC peering connection remains visible to the requester for 2 hours."
"The VPC Peering Connection test2 has deleted status. The VPC peering connection remains visible to the party that deleted it for 2 hours, and visible to the other party for 2 days. If the VPC peering connection was created within the same AWS account, the deleted request remains visible for 2 hours."
Comment #12
yas@liuchanggang
Thank you for the update. I tested the patch and it gives an error. That's fine as we expected.
And what about the behavior when we click the
Accept
button? It showsAccess denied
. We expect the same error message as theEdit
orDelete
button.Comment #13
liuchanggang CreditAttribution: liuchanggang commented@yas,
I updated VpcPeeringConnectionAcceptForm to have same update logic.
Thanks
Comment #14
yas@liuchanggang
Thank you for the update. I tested the patch but the result was the same as my previous comment at #12. Could you please double-check it?
Comment #15
liuchanggang CreditAttribution: liuchanggang commented@yas,
I see the issue is that even after status is updated to Deleted, we still see the Accept button and when clicking it, we see denied errors.
Accept button shouldn't show on page when status is deleted. I resolved the problem by adding
self::clearCacheValue();
I have updated the patch.
Thanks
Comment #16
baldwinlouie CreditAttribution: baldwinlouie commentedI'm worried about
self::clearCacheValue()
. For example,updateAllResourceList
also callsclearCacheValue
.We can get into a scenario, where the cache is cleared multiple times during one operation. When that happens, the site will slow down.
Would it make more sense to put it in
updateSingleEntity()
?That way, we know it won't be called multiple times when
updateAllResourceList
is calledComment #17
liuchanggang CreditAttribution: liuchanggang commented@baldwinlouie
Thanks for insight. I reviewed all those update entity functions and removed unnecessary clearCacheValue().
Comment #18
yas@liuchanggang
Thank you for the update. I tested the patch and it looks working as we expected.
@baldwinlouie
What do you think?
Comment #19
baldwinlouie CreditAttribution: baldwinlouie commented@liuchanggang, Thank you for the updated patch. It looks good to me now.
Comment #20
yas@baldwinlouie
Thank you for your review. I'll merge the patch to
8.x-1.x
and8.x-2.x
and close this issue asFixed
.Comment #21
yas