"Report an issue" button link broken for subpages.

[jackorp]

When a user clicks "Report an issue" on the main page of a given topic, the user is taken to Github issues with the correct title (e.g., from https://developer.fedoraproject.org/tech/languages/go/go-installation.html the button link is valid)

However, when a user clicks on the button on the subpage of a topic, the section is not present (e.g., https://developer.fedoraproject.org/tech/languages/go/go-packages.html)
Examples of the bug are:

• : ruby ror-installation.md content#401
• : docker docker-images.md content#398

One solution would be to parse the link if the required section info is not present in the subpages.

[dirschn]

The issue is that a lot of the md files are missing section attributes. For example, see
start/sw/web-app/about.md vs start/sw/web-app/rails.md

I noticed adding section to each page causes a couple issues, the first being multiple logos in "The top blue thing" because of this:

website/_includes/jumbo-content.html

Lines 3 to 9 in 616a734

	{% for p in site.pages %}
	{% if p.subsection == page.subsection %}
	{% if p.section %}
	<img class="img-rounded" src="{{ p.subsection | subsection_logo }}">
	{% endif %}
	{% endif %}
	{% endfor %}

The other issue I noticed is that it the order of the links is semi-dependent on whether or not there is a section attribute:

website/_layouts/content.html

Lines 18 to 31 in 616a734

	{% for p in sorted_pages %}
	{% if p.subsection == page.subsection %}
	{% if p.section %}
	<a href="{{p.url}}" class="list-group-item {% if p.url == page.url %} active {% endif %}">{{ p.title }}</a>
	{% endif %}
	{% endif %}
	{% endfor %}
	{% for p in sorted_pages %}
	{% if p.subsection == page.subsection %}
	{% if p.section %}{% else %}
	<a href="{{p.url}}" class="list-group-item {% if p.url == page.url %} active {% endif %}">{{ p.title }}</a>
	{% endif %}
	{% endif %}
	{% endfor %}

That won't be as much of an issue if they all have it though, at that point we could just remove the whole second for loop.

One idea I had to avoid an overly large refactor is to add a github-title attribute to the markdown files and use that here instead of section.

As I'm new here I wanted to document this and ask what the preferred solution would be before going in and changing a bunch of things.

[jackorp]

Ok, great you went through it and documented it! This has shown multiple problems when I looked at the linked liquid templates closer.

github-title sounds ok short term, if you want to go ahead and add that to the md files, feel free to. However, it complicates stuff, adds new variable people will have to track is present with new content etc... Just to be clear, I wouldn't want that to be around for longer than necessary.

With what you investigated a more proper fix IMO is: add sections to all the files [0], delete the second loop, delete the inner if (that one seems only for the purpose of ordering) and put order properly into all files. The pages are actually sorted via order first, so it all should work (though preferably some testing whether it is true should be done). That is a larger refactor, that's true.

[0] Hmm, the sections are only needed for the GH issue link if we delete the inner if...

On top of that, we should validate the YAML headers, because one can discover pages that exist but are not show due to wrong YAML header: https://github.com/developer-portal/content/pull/457/files . So another work item would be to create a CI step to validate that all the YAML is present and correct. Thinking out loud now, that should be that section is correct depending on directory it is in, similarly with subsection.

That CI should IMO be not depend on GH actions. Maybe some script in developer-portal/tools or a standalone gem, or a part of the test suite? I'd like it to be runnable locally without a problem, so that one can validate this before even pushing.

But I also had the idea of inheritance, where you could put a yaml file into such bottom dirs and have them appended to YAML contents of individual files. I think we'd need a custom plugin, and honestly, for us it sounds like overengineering. Just throwing ideas out there.

If something is unclear feel free to ask. I edited this back and forth so something might be a bit confusing.

I wanted to document this

That is honestly insanely helpful, thanks.

[dirschn]

With what you investigated a more proper fix IMO is

I'm totally fine with doing the proper fix, I agree that github-title is not an ideal solution. All in, both solutions would end up being about the same amount of work anyway.

On top of that, we should validate the YAML headers

I might need some guidance on implementing the YAML header validation, though. I haven't worked with this kind of system before so I'm not sure how exactly that would be done.

But I also had the idea of inheritance

This does sound more robust but like you said maybe a little over engineered- especially if a new plugin is needed. I would think after this refactor it wouldn't be more than a couple files at a time as people add to or edit a guide.

[jackorp]

I might need some guidance on implementing the YAML header validation, though. I haven't worked with this kind of system before so I'm not sure how exactly that would be done.

No problem.

Now, I haven't worked with validating yaml in jekyll and I am not aware of any solution that would make this easier, so we are going to have to answer the "how" ourselves. (Though feel free to search rubygems.org/git{lab,hub} whether someone already solved it for us, my cursory glance on rubygems.org did not reveal anything)

So first things first, I'd look at jekyll docs, they have some wisdom on this: https://jekyllrb.com/docs/front-matter/
i was able to track reading front matter to this method: https://github.com/jekyll/jekyll/blob/5ae029f9cd565c77d97fcadada2d98d6cf24632b/lib/jekyll/convertible.rb#L37

It looks like they are just capturing content using regex. But that might be a detail we don't care about in the end. Simply thanks to the fact the Page object has the front matter parsed out and probably available: https://github.com/jekyll/jekyll/blob/5ae029f9cd565c77d97fcadada2d98d6cf24632b/lib/jekyll/convertible.rb#L45 this method should be called when initializing a page: https://github.com/jekyll/jekyll/blob/5ae029f9cd565c77d97fcadada2d98d6cf24632b/lib/jekyll/page.rb#L49C7-L49C16

In general I think not copy pasting logic from jekyll and instead reusing it will be the best approach.

Not sure if this would be simple without a plugin but it should be rather simple one that's not too dissimilar to what we have already like https://github.com/developer-portal/website/blob/master/_plugins/permalinks.rb

I remembered that I test a mock jekyll site using Ruby code and cucumber in the jekyll-git-authors plugin: https://gitlab.com/jackorp/jekyll-git-authors/-/blob/5efdeb82ecc820e2a8889f9b5e416e02b878fcb6/features/step_definitions/render_output.rb

Of interest is the block When('Jekyll-git-authors generates authors') do that contains the necessary logic for generating a site using jekyll in Ruby instead of the command line and therefore triggering the plugin. I think this can be used in RSpec, cucumber, test-unit, and other approaches. We can also inject config to enable plugin only in testing env and not use it by default to not have to wait on it when generating sites in e.g. writing new content.

And the checking of the YAML itself might be as simple as pulling out the fields from what I assume will be a method on page returning some hash, and checking that the fields 1) exist 2) have expected data in them, or at least expected format (i.e. that order is a numeric value).

Feel free to ask for clarification or any other questions.

[dirschn]

That all sounds good, I'm (slowly) working on a PR.

[dirschn]

Hey @jackorp, sorry for going dark for so long! Switched jobs and moved, etc. I think I have a PR ready, but I'm not sure exactly how to do it, because there's changes to this repo and the content repo that both need to be merged at the same time. Is there a way to do that? I've never worked with subprojects/submodules before.

I'll mention, I read through the thread here again and I was thinking about inheritance on the way home today, then did not implement it because I forgot by the time I arrived... I think what I have though is the simplest solution. Not super DRY, though.

[jackorp]

sorry for going dark for so long!

Happy to hear you're still working on it! :)

I do not know how exactly the PRs are depending on each other. Generally we can have it "broken" a little while until we finalize merging each one into their respective repos. Next release is in circa 2 weeks.

Now, with that in mind:
If /website PR depends on changes in the /content repo, there is quite a simple way forward. In git, a submodule is the same as any file basically, but it gets a bit of a special treatment, so that when you do git add . it doesn't go into the submodule and doesn't commit the subrepo's files as the main repo's files. When you git add the_submodule_folder, git just marks down the commit of the submodule at that time.

So perhaps a way forward is, put both of them up, I can review PR/content in context of the PR/website, merge content in, then you can use the PR/website to add a commit which bumps the submodule commit. Usually that would go something like this:

$ # $PWD == website
$ cd content
$ git checkout master && git pull
$ cd ..
$ git add content
$ git commit -m "Update content." 

But we build /content and /website as one unit by hand, so at the time of build, no matter what the PR dependency relationships are, we can make sure all required bits are in before doing that.

On top of that, once we merge, I can just do a build and push into the staging sites: https://developer.stg.fedoraproject.org/ to not have the main one borked while observing the changes.

I think what I have though is the simplest solution. Not super DRY, though.

I'll be happy to take a look.

[dirschn]

Okay, that all sounds good. I'll put up PRs and we can just see how it goes.

[jackorp]

The changes were pushed and should be synced no later than in an hour here: https://developer.stg.fedoraproject.org/

[jackorp]

I'll go make an announcement later notifying of the changes to the Fedora Developer Portal mailing list. Let's give it a week, if no one complains that there is messed up output somewhere, I'd call it a resolved issue and push it as-is to the prod version.

[jackorp]

Right, the developer.stg.fedoraproject.org now lists all sections and subsections. I'll revert the changes for now and let's figure out what to do next.

[dirschn]

I'll take a look when I get a chance, not entirely sure when that'll be but hopefully within the next week.

[jackorp]

I had some free time to look at it closer. We were able to simplify the template into least amount that's needed from what I observed in devel container: #142 . And I found out we can do what we want via _config.yml that sets the variables on our side instead of on the side of /content: #140 .

If you figure out some enhancements or more fine-tuned category names, I'll welcome a PR.

More on #143