#238 [frontend] fix copr_url() template macro for custom method
Merged 6 years ago by clime. Opened 6 years ago by praiskup.
Unknown source copr_url_macro_fix  into  master

@@ -221,7 +221,23 @@

      {{- url_for(view, username=copr.user.name, coprname=copr.name, **kwargs) }}

    {%- else %}

      {#- Only the listed views have merged functions for regular and group projects. Once all views have it, @TODO remove the workaround #}

-     {%- if view in ["coprs_ns.copr_add_package", "coprs_ns.copr_new_package", "coprs_ns.copr_rebuild_package", "coprs_ns.copr_packages", "coprs_ns.copr_package", "coprs_ns.copr_edit_package", "coprs_ns.copr_fork", "coprs_ns.copr_delete_package", "coprs_ns.copr_create_module", "coprs_ns.copr_module_raw", "coprs_ns.generate_module_repo_file", "coprs_ns.copr_modules", "coprs_ns.copr_module", "coprs_ns.copr_package_icon", "coprs_ns.copr_rebuild_all_packages"] %}

+     {%- if view in ["coprs_ns.copr_add_package",

+                     "coprs_ns.copr_new_package",

+                     "coprs_ns.copr_rebuild_package",

+                     "coprs_ns.copr_packages",

+                     "coprs_ns.copr_package",

+                     "coprs_ns.copr_edit_package",

+                     "coprs_ns.copr_fork",

+                     "coprs_ns.copr_delete_package",

+                     "coprs_ns.copr_create_module",

+                     "coprs_ns.copr_module_raw",

+                     "coprs_ns.generate_module_repo_file",

+                     "coprs_ns.copr_modules",

+                     "coprs_ns.copr_module",

+                     "coprs_ns.copr_add_build_custom",

+                     "coprs_ns.copr_new_build_custom",

+                     "coprs_ns.copr_package_icon",

+                     "coprs_ns.copr_rebuild_all_packages"] %}

        {%- set group_view = view %}

      {%- else %}

        {%- set group_view = view.split(".")[0] + ".group_" + view.split(".")[-1] %}

Offtopic: My apologies for having this workaround in the copr_url() template macro. There used to be a condition whether the project is owned by a group and rendering the appropriate URL for it, on every link ... which was even worse than this hack.

I think that we should update the rest of the group views (not listed in the macro), so we can remove the workaround. I can do it since it is on my personal backburner for a long time.

This indeed fixes the issue. Thank you for the quick fix!

Pull-Request has been merged by clime

6 years ago

With some additional logic we could list all the routes (app.url_map.iter_rules()),
and check whether the group_* prefixed route exists. But that code wouldn't
belong to template anymore.

With the hack, I only dislike the fact that it "promotes" the worse approach; so I would
rather see a reversed approach where "merged routes" work just fine, and not-yet
merged routes are on "whitelist". That way we could iteratively shrink the white-list
once time permits. Also new submitter would clearly see that adding new methods
on white-list is not really the best thing ...

With the hack, I only dislike the fact that it "promotes" the worse approach; so I would
rather see a reversed approach where "merged routes" work just fine, and not-yet
merged routes are on "whitelist".

I agree, the reversed approach would be better. When I wrote this code I wanted to show that it is possible to do it this way, that regular and group views can be merged and that it is IMHO a better approach than the current one. The list contained only a few methods, so it was easier to just whitelist them. However, It has been there for a long time and it really seems to be fine because AFAIK new views are made merged.

With some additional logic we could list all the routes (app.url_map.iter_rules()),
and check whether the group_* prefixed route exists. But that code wouldn't
belong to template anymore.

This is a cool idea, too bad that I haven't implemented it this way. I will make the effort and take care of solving it properly, i.e. merge everything.

Metadata