group alias need 1:1 mapping to group #2826

pkking · 2023-07-29T10:08:56Z

This PR try to fix: #2825 also fortunately fix: #2563

add unique contraint to fas_name and add migration script generated by alembic revision --autogenerate
I think macro fas_group_href canbe replaced by onfig.GROUP_INFO.link.forma which is more configurable
When creating a new alias for a group, if the group already has one alias, popup a flash and redirect to the already existed group

praiskup · 2023-07-31T14:03:22Z

frontend/coprs_frontend/coprs/views/groups_ns/groups_general.py

+                fas_group, group.name)
+        else:
+            message = "Group {} is activated in the System under the alias {} ".format(
+                fas_group, alias)


:-) how do you trigger this one actually? Is there a reproducer?

:-) how do you trigger this one actually? Is there a reproducer?

Yeah, i just go /groups/activate/<group_name> twice with two different alias
The second submit will lead to a 404 page

AAAh, this. Yeah, I totally misunderstood the thing you want to fix (I though you want to map 2 external groups to a single Copr group, and it is vice versa).

Could you please return "400 Bad Request" if user visits /groups/activate/<existing_group> for the second time? There's no such link, so that must be an error anyway (processing the request seems just redundant).

Will simplely return 400 be a good experience?
Or just redirect user to the existed group?

Not sure. Do you think users can get to the route once it is activated? If yes, redirecting is indeed a better option.

Not sure. Do you think users can get to the route once it is activated? If yes, redirecting is indeed a better option.

Seems no possible unless they know the route and play with it like me :)
But return 400 or redirect will not waste anything, so why not?

praiskup · 2023-07-31T14:04:48Z

frontend/coprs_frontend/coprs/templates/groups/user_fas_groups.html

@@ -37,7 +37,7 @@ <h1>
    <tbody>
    {% for team in teams %}
      <tr>
-        <td><a href="{{ fas_group_href(team) }}">
+        <td><a href="{{ config.GROUP_INFO.link.format(name=team) }}">


Is this #2563?

Is this #2563?

Don‘t know this ticket before :) but in our case, we should be redirect to openEuler group info page instead of fas_group stuff, so i think fas_group_href can be replaced with GROUP_INFO.link 😄

Yeah, I think that's exactly #2563 (forgotten hard-coded link). Can you use Fixes: #2563, too?

praiskup · 2023-07-31T14:07:54Z

Nit that I can fix myself for the PR - but would be helpful to write commit messages like:

<component>: <what you are doing>

_Why_ (rather how, it's the commit itself) are doing that. This should be readable and
describe the background story.

<tag>: value (e.g. Fixes: #XYZ)
<tag>: value

pkking · 2023-07-31T14:48:27Z

Nit that I can fix myself for the PR - but would be helpful to write commit messages like:
<component>: <what you are doing>

_Why_ (rather how, it's the commit itself) are doing that. This should be readable and
describe the background story.

<tag>: value (e.g. Fixes: #XYZ)
<tag>: value

I tried to describe it in the newly pushed commit message, any advice appreciated 😃

praiskup · 2023-08-01T05:14:34Z

alembic revision --autogenerate

This was a correct action (adding the index via migration); can you add the migration script into this PR?

pkking · 2023-08-01T09:55:44Z

alembic revision --autogenerate

This was a correct action (adding the index via migration); can you add the migration script into this PR?

Ah i forget to push this file 🐷, Done

frontend/coprs_frontend/alembic/versions/daa62cd0743d_add_unique_constraint_to_fas_group.py

praiskup · 2023-08-01T16:27:18Z

frontend/coprs_frontend/coprs/views/groups_ns/groups_general.py

+        # can't have more than one alias for a group
+        if group.name != alias:
+            message = "Group {} already activated as alias {} ".format(
+                fas_group, group.name)


If we go with error 400 or redirect, this if/else condition is redundant. While on this, would you mind making the flash message "green" with flash("message", "success") :-) so it is more visible? The grey color doesn't look like something actually happened...

praiskup · 2023-08-02T04:19:09Z

frontend/coprs_frontend/coprs/views/groups_ns/groups_general.py

        return flask.redirect(url_for(
-            "groups_ns.list_projects_by_group", group_name=alias))
+            "groups_ns.list_projects_by_group", group_name=group.name))


This is certainly OK and better behavior than before, thank you!

What I thought meant before was that we should redirect the user (or error 400) much earlier, before the user starts even thinking about how to name the group (it is wasting of time, the group is already activated so nothing will happen). Something like:

def activate_group(fas_group): if "fas group is activated": return flask.redirect(...)

That's sounds a good idea, will tweak the code :)

This is certainly OK and better behavior than before, thank you!

What I thought meant before was that we should redirect the user (or error 400) much earlier, before the user starts even thinking about how to name the group (it is wasting of time, the group is already activated so nothing will happen). Something like:

def activate_group(fas_group): if "fas group is activated": return flask.redirect(...)

Done

praiskup · 2023-08-02T04:20:27Z

+1 (note the missing thing above, but still good enough)

praiskup · 2023-08-02T08:04:11Z

frontend/coprs_frontend/coprs/views/groups_ns/groups_general.py

-        db.session.add(group)
+        group = UsersLogic.create_group_by_fas_name(fas_group, alias)
+        # db.session.add(group) is done by UsersLogic.create_group_by_fas_name()
+        db.session.flush()


I bet flush() is not needed. I'd drop the commented-out line with db.session.add() (NB adding the same object twice shouldn't hurt).

You are right, commit() will unconditionally call flush() before commit

I bet flush() is not needed. I'd drop the commented-out line with db.session.add() (NB adding the same object twice shouldn't hurt).

But why should we still keep add() since it was done by create_group_by_fas_name

Yeah, add() is redundant, I have no objections to removing it.

Yeah, add() is redundant, I have no objections to removing it.

I totally misunderstand you :)
Done

@praiskup

When i was developing group supported in OIDC, i found some wired hehavior in group activated in fedora-copr#2825, So i submitted this PR, and @praiskup also give advice a TODO so i include the fix as well This PR try to fix: fedora-copr#2825 1. add unique contraint to fas_name and add migration script generated by alembic revision --autogenerate 2. I think macro fas_group_href canbe replaced by config.GROUP_INFO.link.forma which is more configurable 3. When creating a new alias for a group, if the group already has one alias, popup a flash and redirect to the already existed group Signed-off-by: Li Chaoran <[email protected]>

praiskup

thank you

praiskup · 2023-08-03T05:51:40Z

NOTE: Needs manual merge.

praiskup · 2023-08-04T06:31:30Z

Merged as: bddee02

Thank you.

pkking requested a review from praiskup July 29, 2023 10:09

FrostyX assigned praiskup Jul 31, 2023

praiskup reviewed Jul 31, 2023

View reviewed changes

pkking force-pushed the fix_group_constraint branch from 82339c8 to fe562e2 Compare July 31, 2023 14:46

pkking requested a review from praiskup July 31, 2023 14:48

praiskup mentioned this pull request Aug 1, 2023

frontend: add relationship between actions and projects/builds #2805

Merged

3 tasks

pkking force-pushed the fix_group_constraint branch from fe562e2 to 744e2d3 Compare August 1, 2023 09:54

github-advanced-security bot found potential problems Aug 1, 2023

View reviewed changes

frontend/coprs_frontend/alembic/versions/daa62cd0743d_add_unique_constraint_to_fas_group.py Fixed Show fixed Hide fixed

pkking force-pushed the fix_group_constraint branch from 744e2d3 to 5647ba1 Compare August 1, 2023 09:58

praiskup reviewed Aug 1, 2023

View reviewed changes

pkking force-pushed the fix_group_constraint branch from 5647ba1 to 20546b8 Compare August 2, 2023 01:16

pkking requested a review from praiskup August 2, 2023 01:37

praiskup reviewed Aug 2, 2023

View reviewed changes

pkking force-pushed the fix_group_constraint branch 3 times, most recently from b1d1f4d to 931e2a9 Compare August 2, 2023 07:54

praiskup reviewed Aug 2, 2023

View reviewed changes

pkking force-pushed the fix_group_constraint branch from 931e2a9 to 2ea10ff Compare August 3, 2023 01:10

pkking requested a review from praiskup August 3, 2023 01:43

praiskup approved these changes Aug 3, 2023

View reviewed changes

praiskup added the do-manual-merge-please label Aug 3, 2023

praiskup closed this Aug 4, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

group alias need 1:1 mapping to group #2826

group alias need 1:1 mapping to group #2826

pkking commented Jul 29, 2023 •

edited

Loading

praiskup Jul 31, 2023

pkking Aug 1, 2023

praiskup Aug 1, 2023

praiskup Aug 1, 2023

pkking Aug 1, 2023

praiskup Aug 1, 2023

pkking Aug 2, 2023

praiskup Jul 31, 2023

pkking Jul 31, 2023

praiskup Aug 1, 2023

praiskup commented Jul 31, 2023

pkking commented Jul 31, 2023

praiskup commented Aug 1, 2023 •

edited

Loading

pkking commented Aug 1, 2023

praiskup Aug 1, 2023

pkking Aug 2, 2023

praiskup Aug 2, 2023

pkking Aug 2, 2023

pkking Aug 2, 2023

praiskup commented Aug 2, 2023

praiskup Aug 2, 2023

pkking Aug 2, 2023

pkking Aug 2, 2023 •

edited

Loading

praiskup Aug 2, 2023

pkking Aug 3, 2023

praiskup left a comment

praiskup commented Aug 3, 2023

praiskup commented Aug 4, 2023

group alias need 1:1 mapping to group #2826

group alias need 1:1 mapping to group #2826

Conversation

pkking commented Jul 29, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

praiskup commented Jul 31, 2023

pkking commented Jul 31, 2023

praiskup commented Aug 1, 2023 • edited Loading

pkking commented Aug 1, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

praiskup commented Aug 2, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pkking Aug 2, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

praiskup left a comment

Choose a reason for hiding this comment

praiskup commented Aug 3, 2023

praiskup commented Aug 4, 2023

pkking commented Jul 29, 2023 •

edited

Loading

praiskup commented Aug 1, 2023 •

edited

Loading

pkking Aug 2, 2023 •

edited

Loading