Skip to content

Conversation

@resmo
Copy link
Member

@resmo resmo commented Feb 20, 2018

Statically passing account (and domainid) results in a conflict when in project view because projectid and account can not be used together.

Removing the 2 params. fixes the issue. (some whitespace fixes along)

@resmo resmo changed the title CLOUDSTACK-10299: UI: fix error in network listing in project mode [4.11] CLOUDSTACK-10299: UI: fix error in network listing in project mode Feb 20, 2018
@resmo resmo added this to the 4.11.1 milestone Feb 20, 2018
}

/*$.ajax({
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You have gone over some commented code. What about removing it?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't see a reason why to remove comments, I wanted to fix the issue. Whitespace cleanup was made by my editor automatically.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well.. the commented code is not used. So, there is your reason to remove it. The less "code" (lines) for our eyes to see, the better.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see your point. cleaning up commented out code makes sense.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great. Thanks!

@rafaelweingartner
Copy link
Member

@resmo thanks for the contributions, but I have a doubt. You say it (the PR) is aimed at 4.11, but you opened a PR against master branch. Which one do you want?

@resmo resmo changed the base branch from master to 4.11 February 21, 2018 06:40
@resmo
Copy link
Member Author

resmo commented Feb 21, 2018

@rafaelweingartner changed the base to 4.11

@borisstoyanov
Copy link
Contributor

@blueorangutan package

@blueorangutan
Copy link

@borisstoyanov a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress.

$.ajax({
url: createURL('listRemoteAccessVpns'),
data: {
account: g_account,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this call only used in the project view?
I mean, if I list networks and I am out of the project view, should not ACS filter by these data? Or, is it done automatically in the server side when the users is logged-in?

@blueorangutan
Copy link

Packaging result: ✔centos6 ✔centos7 ✔debian. JID-1728

Copy link
Contributor

@borisstoyanov borisstoyanov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@resmo Can we have some screenshots please?

@resmo
Copy link
Member Author

resmo commented Feb 21, 2018

@borisstoyanov before or after the patch? I attached the screenshot to the issue in jira. Didn't make one after the patch, as you see "nothing" (no error).

@borisstoyanov
Copy link
Contributor

@resmo both if you have. If no, new stuff is what's interesting :)

@resmo
Copy link
Member Author

resmo commented Feb 26, 2018

Before

selection_149

After

selection_151

@resmo
Copy link
Member Author

resmo commented Feb 26, 2018

ready for review

Copy link
Contributor

@borisstoyanov borisstoyanov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@DaanHoogland
Copy link
Contributor

@borisstoyanov @rafaelweingartner is this merge ready?

@rafaelweingartner
Copy link
Member

There is still one open question of mine there. It is about some parameters that were removed. I am asking if that removal can affect the loading of information for the default view (view outside of project).

@resmo
Copy link
Member Author

resmo commented Feb 27, 2018

@rafaelweingartner I couldn't test the view with having existing remote vpns configured. But my assumption was that the api is working identically as other list apis returning the results of the current owner.

@rafaelweingartner
Copy link
Member

Got it. I think this one is ready to be merged then.

@DaanHoogland
Copy link
Contributor

anything holding you back, @rafaelweingartner ?

@rafaelweingartner
Copy link
Member

Nothing... I was busy with some other things.
I am merging this one now

@rafaelweingartner rafaelweingartner merged commit 0bb20a7 into apache:4.11 Feb 27, 2018
rafaelweingartner added a commit that referenced this pull request Feb 27, 2018
[4.11] CLOUDSTACK-10299: UI: fix error in network listing in project mode
@resmo resmo deleted the fix/4.11/CLOUDSTACK-10299 branch September 21, 2018 14:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants