Skip to content

Conversation

@johanlundberg
Copy link
Contributor

@@ -0,0 +1,342 @@
# -*- coding: utf-8 -*-
Copy link
Member

Choose a reason for hiding this comment

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

Not really sure if doing a manage command for this is the right thing... but not really sure if it is a bad way to go about it.

Normally I would have put it in a script in src/scripts.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have to say I think it worked fine. I would also have done it in a script.

Copy link
Member

@mkrogh mkrogh left a comment

Choose a reason for hiding this comment

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

Still need to boot it up locally to see the changes.

return relationship, created


def set_of_member(user, node, contact_id):
Copy link
Member

Choose a reason for hiding this comment

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

This method name is super confusing, and not a helper, it seem like it the name set_member_of_contract would be more fitting.

Same goes for the set_member_of above, it adds a node to a group.

@@ -0,0 +1,35 @@
# -*- coding: utf-8 -*-
Copy link
Member

Choose a reason for hiding this comment

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

A bit unsure about this migration.

I guess it adds the Relation dropdown.

But what about the other migrations? just updated help text or?

@@ -0,0 +1,16 @@
# -*- coding: utf-8 -*-
Copy link
Member

Choose a reason for hiding this comment

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

Migrations in general seems a bit messy (lots of empty merge migrations, and one big squash). Guess I should just ignore them.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The migrations scare me too but I guess we should trust that Django has it figured out?

'it_technical_contact': { 'name': 'NOC Technical', 'description': '' },
'it_security_contact': { 'name': 'NOC Security', 'description': '' },
'it_manager_contact': { 'name': 'NOC Manager', 'description': '' },
DEFAULT_ROLE_KEY: { 'name': nc.models.RoleRelationship.DEFAULT_ROLE_NAME, 'description': '' },
Copy link
Member

Choose a reason for hiding this comment

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

This seems a bit nasty, using the name from the nordunetclient lib, while using the DEFAULT_ROLE_KEY.

@@ -0,0 +1,47 @@
{% extends "base.html" %}
Copy link
Member

Choose a reason for hiding this comment

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

I would really have preferred if the forms were done using the crispy_fields instead to give inline errors.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree but I didn't catch it as I forgot about crispy_fields.

return render(request, 'noclook/detail/group_detail.html',
{'node_handle': nh, 'node': node, 'location_path': location_path})

def _contact_with_role_table(con, org=None):
Copy link
Member

Choose a reason for hiding this comment

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

Nicely spotted that there was something for tables 👍

@@ -0,0 +1,25 @@
# -*- coding: utf-8 -*-
Copy link
Member

Choose a reason for hiding this comment

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

Also quite unsure about this migration?

<li class="nav-header"><i class="icon-map-marker"></i> Maps</li>
<li><a href="/gmaps/sites/">Site map</a></li>
<li><a href="/gmaps/optical-nodes/">Optical node map</a></li>
{% ifequal menu_mode 'ni' %}
Copy link
Member

Choose a reason for hiding this comment

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

Hmm what does menu_mode 'ni' really mean?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This just hides the NI specific menu items when in "sri"-mode. menu_mode ni is the default in global preferences.

global_preferences = global_preferences_registry.manager()
menu_mode = global_preferences['general__menu_mode']

if menu_mode == 'ni':
Copy link
Member

Choose a reason for hiding this comment

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

Are all these changes meant to be in a separate parallel sri instance?
If that is the case then I think it should perhaps have been a separate app instead of being baked into noclook...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The goal is to keep both NI data and SRI data in the same backend. So this is a first step.
That said, we can wait with this PR until we have the new frontend and graphql api done. Then these special cases can be removed.

PaKZer0 and others added 26 commits March 18, 2020 14:38
Removed management command
CORS whitelist is defined in the SRI_FRONTEND_URL envvar
Corrections on how CORS policy should be run in the different envs
If the user profile doesn't exists, create it
Change not logged exception when the user doesn't have rigths over nodes
Bugfix: relationship_parent_of field was not using relay id for update
Enabled validators and fixed test
Fix: An empty connection is returned if the user doesn't have rights
Fix:The GroupContextAuthzAction objs are created regardless of the users
Bugfix and config change: Session expires at browser close
@johanlundberg
Copy link
Contributor Author

There was a mix up of branches and merge_upstream was reused for merging upstream to SUNET/ni.
I will close this PR until we can make a nicer one.

@mkrogh
Copy link
Member

mkrogh commented Mar 31, 2020

Hehe no problem, it happens.

@johanlundberg johanlundberg deleted the merge_upstream branch March 31, 2020 11:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants