Skip to content

  • Projects
  • Groups
  • Snippets
  • Help
  • Sign in / Register
Minds Frontend
Minds Frontend
  • Project overview
  • Repository
  • Issues 426
  • Merge Requests 56
  • CI / CD
  • Security & Compliance
  • Packages
  • Wiki
  • Snippets
  • Members
  • Collapse sidebar
  • Graph
  • Charts
  • Create a new issue
  • Jobs
  • Commits
  • Issue Boards
  • Minds
  • Minds FrontendMinds Frontend
  • Merge Requests
  • !702

Open
Opened 4 days ago by Ben Hayward@benhayward.ben
Report abuse

Fixed pattern matching for external and internal page links #2133

  • Overview 3
  • Commits 7
  • Pipelines 5
  • Changes 16
0/1 thread resolved

Summary

Pattern matching for the /p/ pages is broken on production. To replicate, go and click the iOS link in the footer.

Our system is seeing the URL itunes.apple.com/us/app/minds-com/id961771928%3Fmt Seeing that it contains '/p' and thinking that it is an internal page

This happened because the system was looking simply for indexOf('p/')

For this fix I have upgraded that check to a regex check, in a commonly held service.

image

Steps to test

Ignore the first iPhone link - that is invalid data that for now is trapped in the sandbox. (If there is now only 1 link for iPhone, include it in the test, we may have found the missing schema and fixed).

image

  1. Not logged in, check the links on the bottom footer, ensure they're all working
  2. Log in, check the links on the newsfeed footer
  3. Check the links on the actual /p/ pages, like https://www.minds.com/p/terms-of-service/
  4. Check the pro footer hasn't been effected https://www.minds.com/pro

Estimated Regression Scope

The components touched by this are: Footer, register homepage and newsfeed.

I think the worst-case scenario would be a failure in the Regex logic causing another pattern matching issue.

Edited 3 days ago by Ben Hayward
Request to merge fix/external-pages-regex-2133 into master
The source branch is 4 commits behind the target branch
Open in Web IDE
Pipeline #104439214 passed with warnings for bbd796c7 on fix/external-pages-regex-2133
              Requires approval from QA. Approved by
              Emiliano Balbuena
              Emiliano Balbuena
              Mark Harding
              Mark Harding
              Marcelo Rivera
              Marcelo Rivera
              Guy Thouret
              Guy Thouret
              Olivia Madrid
              Olivia Madrid
              Rami Albatal
              Rami Albatal
              Ready to be merged automatically. Ask someone with write access to this repository to merge this request

              Mentions #2133

              Deletes source branch

              • Ben Hayward @benhayward.ben added Squad::Yellow Status::Requires Changes scoped labels 4 days ago

                added scoped labels

              • Ben Hayward @benhayward.ben added 1 commit 4 days ago

                added 1 commit

                • 6527b1eb - Update footer.component.spec.ts

                Compare with previous version

              • Ben Hayward @benhayward.ben added 2 commits 4 days ago

                added 2 commits

                • 4abf88a3 - Fixed regex issue apparent on sanbox
                • 4a17a6f9 - Merge branch 'fix/external-pages-regex-2133' of gitlab.com:minds/front into...

                Compare with previous version

              • Ben Hayward
                Ben Hayward @benhayward.ben · 4 days ago
                Developer

                Still an issue with that regex.

                !702 (diffs)

                Seems to be disregarding the starting char ^

              • Ben Hayward @benhayward.ben added 1 commit 4 days ago

                added 1 commit

                • a06f22a5 - Turned regex.test into path.match(regex) because of weird error

                Compare with previous version

              • Ben Hayward @benhayward.ben added 1 commit 3 days ago

                added 1 commit

                • bbd796c7 - Fixed RexExp issue

                Compare with previous version

              • Ben Hayward @benhayward.ben changed the description 3 days ago

                changed the description

              • Ben Hayward @benhayward.ben added Status::BuddyReview scoped label and automatically removed Status::Requires Changes label 3 days ago

                added scoped label and automatically removed label

              • Emiliano Balbuena
                Emiliano Balbuena @edgebal started a thread on the diff 2 hours ago
                src/app/common/services/pages.service.ts
                1 import { Injectable } from '@angular/core';
                2
                3 /**
                4 * Shared logic between internal 'p/' page loading components.
                5 * e.g. header, footer.
                6 *
                7 * @author Ben Hayward
                8 */
                9 @Injectable()
                10 export class PagesService {
                11 private internalPageRegex: RegExp = /^p\/.+$/gm; // matches 'p/' in first and second position.
                • Emiliano Balbuena
                  Emiliano Balbuena @edgebal · 2 hours ago
                  Developer

                  Since you're just using this regular expression for testing, it can be simplified to:

                  /^\/?p\/./

                  That will match:

                  • Start of the line
                  • An optional slash
                  • The letter p
                  • A slash
                  • A character that's not a line terminator

                  g and m modifiers are unnecessary since you're not needing the matches information neither testing a multi-line string.

                  image

                  Edited by Emiliano Balbuena 2 hours ago
                • Please register or sign in to reply
              • Emiliano Balbuena @edgebal approved this merge request 1 hour ago

                approved this merge request

              • Emiliano Balbuena
                Emiliano Balbuena @edgebal · 1 hour ago
                Developer

                Looks good! Internal links seem to be working OK on the sandbox site.

              Please register or sign in to reply
              0 Assignees
              None
              None
              Milestone
              None
              Time tracking
              No estimate or time spent
              2
              Labels
              Squad::Yellow Status::BuddyReview
              Lock merge request
              Unlocked
              11
              11 participants
              user avatar
              Mark Harding
              user avatar
              Marcelo Rivera
              user avatar
              Guy Thouret
              user avatar
              Olivia Madrid
              user avatar
              Rami Albatal
              user avatar
              Xander Miller
              user avatar
              Martin Santangelo
              Reference: minds/front!702

              Now you can access the merge request navigation tabs at the top, where they’re easier to find.

              More information and share feedback