Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Refreshing sample data #394

Merged
merged 17 commits into from
Feb 6, 2023
Merged

Refreshing sample data #394

merged 17 commits into from
Feb 6, 2023

Conversation

rowan04
Copy link
Member

@rowan04 rowan04 commented Nov 8, 2022

Resolves #384

  • Sanitised data so it's not based on real data
  • Added multiple NGIs with sites and services
  • Added more users with unique dummy emails

@rowan04 rowan04 marked this pull request as ready for review November 11, 2022 11:12
@rowan04 rowan04 requested a review from a team as a code owner November 11, 2022 11:13
@gregcorbett
Copy link
Member

Looks good 😄 , the only other comment I have is - can you check the roles the Users have allign with the type of entity the role is over (see the "Role Action Map" page on GOCDB instances)?

i.e. Donna Butler has the "COD Staff" role over NGI_CH, but "COD Staff" is a Project entity role.

@rowan04
Copy link
Member Author

rowan04 commented Dec 14, 2022

The commit just done is to do with the site names - I will hopefully be able to fix the user roles tomorrow morning

@rowan04 rowan04 requested a review from gregcorbett December 15, 2022 14:06
@gregcorbett
Copy link
Member

can you check the roles the Users have allign with the type of entity the role is over (see the "Role Action Map" page on GOCDB instances)?

I also now realise the sample User / Roles never made much sense (for example, it has always had Site level roles over NGIs), I assumed I had fixed this, but it seems that change has become trapped in #282

It would be good to have a saner sample User / Role mapping given you are refreshing the sample data already.

The "Role Action Map" can be a little confusing (it's just confused me for a while resulting in me going down a platypus hole in the code).

Taking this as an example:

https://github.com/rowan04/gocdb/blob/8f5eb4772541a090ff04847d8022c666d21f1c16/lib/Doctrine/deploy/sampleData/UsersAndRoles.xml#L24-L28

"Site Administrator" is a Site level RoleType and should only be applied to a Site entity (i.e. the ON_ENTITY element should container a Site name, not an NGI name).

Having a Site level role does grant actions over NGIs and Projects (which is how we end up with lines like this in the "Role Action Map".

image

but the "Site Administrator" only be applied to a site entity.

Does that make sense?

@rowan04
Copy link
Member Author

rowan04 commented Dec 15, 2022

When I tried to make the ON_ENTITY element a sitename, it would produce an error saying something like "no NGI's found with sitename". That's what i tried to do originally for roles like site administrator but had to change them all back to NGI's.

@rowan04
Copy link
Member Author

rowan04 commented Dec 15, 2022

When I tried to make the ON_ENTITY element a sitename, it would produce an error saying something like "no NGI's found with sitename". That's what i tried to do originally for roles like site administrator but had to change them all back to NGI's.

Would you like me to reproduce the error?

@gregcorbett
Copy link
Member

When I tried to make the ON_ENTITY element a sitename, it would produce an error saying something like "no NGI's found with sitename". That's what i tried to do originally for roles like site administrator but had to change them all back to NGI's.

Would you like me to reproduce the error?

That's weird, yes please 😄

@rowan04
Copy link
Member Author

rowan04 commented Dec 15, 2022

When I tried to make the ON_ENTITY element a sitename, it would produce an error saying something like "no NGI's found with sitename". That's what i tried to do originally for roles like site administrator but had to change them all back to NGI's.

Would you like me to reproduce the error?

That's weird, yes please 😄

nongis

@rowan04
Copy link
Member Author

rowan04 commented Dec 15, 2022

When I tried to make the ON_ENTITY element a sitename, it would produce an error saying something like "no NGI's found with sitename". That's what i tried to do originally for roles like site administrator but had to change them all back to NGI's.

Would you like me to reproduce the error?

That's weird, yes please 😄

nongis

this was the code
torch

@rowan04
Copy link
Member Author

rowan04 commented Dec 15, 2022

And I'm heading home now. I guess we'll return to this next year :)

@gregcorbett gregcorbett mentioned this pull request Jan 3, 2023
Merged
@gregcorbett
Copy link
Member

When I tried to make the ON_ENTITY element a sitename, it would produce an error saying something like "no NGI's found with sitename". That's what i tried to do originally for roles like site administrator but had to change them all back to NGI's.

Would you like me to reproduce the error?

That's weird, yes please 😄

nongis

this was the code torch

ah, looks like <ENTITY_TYPE>group</ENTITY_TYPE> is the culprit. Going a bit deeper it looks like UsersAndRoles.xml is based on an older version of the API output from GOCDB's get_user method 😢

Looking at the output now, <ENTITY_TYPE> is either site, ngi, project, servicegroup.

I'm not sure what's going on with <ENTITY_TYPE> equalling servicegroup. We don't seem to have a AddServiceGroupRoles.php equivalent, but we do have some sample ServiceGroupRoles.. I think it would be good to tackle #373 at the same time as this I think. Though we should probably just delete ServiceGroupRoles.xml and give additional servicegroup roles to the users in UsersAndRoles.xml.

@gregcorbett
Copy link
Member

I think it would be good to tackle #373 at the same time as this I think. Though we should probably just delete ServiceGroupRoles.xml and give additional servicegroup roles to the users in UsersAndRoles.xml.

Having just noticed #393 exists 🤭, lets not conflate the two - so let's not worry about servicegroups at all here :)

@rowan04
Copy link
Member Author

rowan04 commented Jan 3, 2023

When I tried to make the ON_ENTITY element a sitename, it would produce an error saying something like "no NGI's found with sitename". That's what i tried to do originally for roles like site administrator but had to change them all back to NGI's.

Would you like me to reproduce the error?

That's weird, yes please 😄

nongis

this was the code torch

ah, looks like <ENTITY_TYPE>group</ENTITY_TYPE> is the culprit. Going a bit deeper it looks like UsersAndRoles.xml is based on an older version of the API output from GOCDB's get_user method 😢

Looking at the output now, <ENTITY_TYPE> is either site, ngi, project, servicegroup.

I'm not sure what's going on with <ENTITY_TYPE> equalling servicegroup. We don't seem to have a AddServiceGroupRoles.php equivalent, but we do have some sample ServiceGroupRoles.. I think it would be good to tackle #373 at the same time as this I think. Though we should probably just delete ServiceGroupRoles.xml and give additional servicegroup roles to the users in UsersAndRoles.xml.

I see (I think!)

@rowan04
Copy link
Member Author

rowan04 commented Jan 3, 2023

I think it would be good to tackle #373 at the same time as this I think. Though we should probably just delete ServiceGroupRoles.xml and give additional servicegroup roles to the users in UsersAndRoles.xml.

Having just noticed #393 exists 🤭, lets not conflate the two - so let's not worry about servicegroups at all here :)

Ok.

This was referenced Jan 12, 2023
Merged
Closed
Closed
@rowan04 rowan04 requested a review from gregcorbett January 17, 2023 11:20
@rowan04
Copy link
Member Author

rowan04 commented Jan 17, 2023

I found that the INSTALL.md information about the sample data is now incorrect: https://github.com/GOCDB/gocdb/blob/f6d4a1b750eecc12c2ca4aa4f38a496154f27b29/INSTALL.md#optional-deploy-sample-data
However it will need to be edited for the remove simple sample data (#415) issue as it also contains information about the simple sample data, so should this file be left for that pull request or should I edit the bit about the normal sample data here?

@gregcorbett
Copy link
Member

I found that the INSTALL.md information about the sample data is now incorrect:

Good spot :)

should I edit the bit about the normal sample data here?

Yes please, else we run the risk of forgetting about it :)

@rowan04 rowan04 mentioned this pull request Jan 18, 2023
Merged
@gregcorbett
Copy link
Member

This is looking great 😄, one final thing - cna you remove any CIC Staff roles from UsersAndRoles.xml. That is a legacy role that we no longer use.

@rowan04
Copy link
Member Author

rowan04 commented Feb 6, 2023

This is looking great 😄, one final thing - cna you remove any CIC Staff roles from UsersAndRoles.xml. That is a legacy role that we no longer use.

done

gregcorbett
gregcorbett approved these changes Feb 6, 2023
View reviewed changes
Copy link
Member

@gregcorbett gregcorbett left a comment

Choose a reason for hiding this comment

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

LGTM 😄

@gregcorbett
Copy link
Member

Rebasing

rowan04 added 17 commits February 6, 2023 11:25
@rowan04 @gregcorbett
Refreshing sample data
b0b7181 
- Sanitised data so it's not based on real data
- Added multiple NGIs with sites and services
- Added more users with unique dummy emails
@rowan04 @gregcorbett
Removes one of the NGIs from EGI Project
8d6bcbb 
- Removes the NGI_HU NGI from the EGI Project
- Changes the NGI_HU NGI and its sites to have Local Scope (not EGI)
@rowan04 @gregcorbett
Addresses Codacy Problems
3de8887 
- Fixes indentation issues
- Fixes multi-line function call
- Moves else statement to same line as closing brace
@rowan04 @gregcorbett
Fix incorrect country code for Slovenian NGI
d988ddd 
- Fixes an issue in multiple files where the Slovenian NGI was wrongly stated as NGI_SL not NGI_SI
@rowan04 @gregcorbett
Add space after IF keyword
0500473 
- From codacy review
@rowan04 @gregcorbett
Change names of some sites to move data away from real data
40b6a46 
- Multiple sites' names changed to unmirror real data
- Their corresponding service endpoints and users were hence edited
@rowan04 @gregcorbett
Change User Roles to align with role entity
9a69eab 
- edited some of the user roles and information about them
- this is to make the roles users have make sense
@rowan04 @gregcorbett
Rename AddGroupRoles.php and AddEgiRoles.php
4a2811d 
- AddGroupRoles.php renamed to AddNGIRoles.php to be more generic and clear
- AddEgiRoles.php renamed to AddProjectRoles.php to be more generalised
- AddProjectRoles.php no longer assumes it's working on the EGI project to allow it to work on other projects
- DeploySampleDataRunner.php updated with the new file names
@rowan04 @gregcorbett
Update user roles to align with the correct entity
f72a2a4 
- updated ENTITY_TYPE for user roles to allow for roles to be set to sites and projects, as well as NGIs
- user roles were updated so that they made more sense - for example to allow site roles to be held over the site entity, instead of over the ngi entity
@rowan04 @gregcorbett
Update from Codacy analysis
2baa2fa 
- Add space after If statement on two occasions
- to improve readability
@rowan04 @gregcorbett
Fixes users not in EGI project having roles over it
096b8af 
- Users in NGI_HU which is not a part of the EGI project still had roles over/in the EGI project
- to fix this, check added in AddProjectRoles.php so that only project roles over the EGI project were added to the EGI project
- And users in NGI_HU had any roles over the project entity removed, as NGI_HU isn't part of a project
@rowan04 @gregcorbett
Updates to UsersAndRoles.xml
9e142a2 
- some basic fixes:
- role changes and edits to some users
- fixes to incorrect emails/names/dns for some users
@rowan04 @gregcorbett
Add project lookup when adding project roles
a39c1b9 
- when adding project roles, the project is looked up first
- this allows for roles to be added to any project, not just an egi project
- the project entity stated is checked to ensure it referes to exactly one project
- the docstring was edited accordingly, and api output removed as it was unnecessary
@rowan04 @gregcorbett
Fix small issues
98fa03d 
- Add space after IF keyword
- Fix incorrect CERTDN for user Mariah Morgan
@rowan04 @gregcorbett
Remove unnecessary if statement in AddNGIRoles.php
75bc70a 
- Removes an unnecessary if statement that checked if roles were of egi entity AFTER they had been checked for ngi entity
- Also fixes an incorrect comment at the ngi entity check
@rowan04 @gregcorbett
Update INSTALL.md to contain up-to-date information about the sample …
4f3a551 
…data

- Update INSTALL.md to have more information about the sample data refresh
@rowan04 @gregcorbett
Removes CIC staff roles from example users
a555835 
- CIC staff is a legacy role no longer used
- so any sample users with the role have had the role removed
- some changes to existing roles to make more sense were also made, e.g. giving each ngi a manager
@gregcorbett gregcorbett force-pushed the 384-refresh_sample_data branch from ab9f7ba to a555835 Compare February 6, 2023 11:26
@gregcorbett gregcorbett merged commit 045aedb into GOCDB:dev Feb 6, 2023
@rowan04 rowan04 mentioned this pull request Feb 7, 2023
Merged
@rowan04 rowan04 mentioned this pull request Sep 28, 2023
Open
@gregcorbett gregcorbett mentioned this pull request Oct 2, 2023
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gregcorbett gregcorbett gregcorbett approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Refresh sample data so that it is not based on real data
2 participants
@rowan04 @gregcorbett