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.

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

Fix missing / non-array user.ranks() issue on v17development/flarum-support #114

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

hackerESQ
Copy link

Related to v17development/flarum-support-feedback#14 which causes support page to break.

Changes proposed in this pull request:
This change adds a conditional check to make sure the user.ranks() is actually loaded. If the rank is falsey, it does not try to add user rank to the PostUser view.

Reviewers should focus on:

    if (user.ranks()) { // <-- added this conditional
      header_node.children = header_node.children
        .concat(
          user
            .ranks()
            .reverse()
            .splice(0, amt)
            .map((rank) => {
              return <span className="Post-Rank">{rankLabel(rank)}</span>;
            })
        )
        .filter(function (el) {
          return el.tag !== undefined;
        });
    }

Screenshot
This is how the support tickets page was breaking:
image

And here's the console log showing the ranks() trying to reverse() which is not available since it's not an array:
image

Confirmed

  • Frontend changes: tested on a local Flarum installation.
  • Backend changes: tests are green (run composer test).

Required changes:
N/A

Copy link
Member

@imorland imorland left a comment

Choose a reason for hiding this comment

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

Hello and thank you for the PR!

Looks good to me, but can you please remove the dist files from this? In order to keep merging as straightforward as possible, the js will be automatically built once merged :)

@hackerESQ
Copy link
Author

Not sure if that's the right way to do that. But let me know if that works.

@DavideIadeluca
Copy link
Member

Hi @hackerESQ thanks for your contribution! This branch still has conflicts that must be resolved. I suggest that you git reset the branch back to the first commit you made and then force push to remote.

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.

None yet

3 participants