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 links in emails #56

Merged
merged 3 commits into from
Jun 4, 2021
Merged

Fix links in emails #56

merged 3 commits into from
Jun 4, 2021

Conversation

githinho
Copy link
Contributor

@githinho githinho commented Jun 4, 2021

No description provided.

@githinho githinho self-assigned this Jun 4, 2021
@linear
Copy link

linear bot commented Jun 4, 2021

@codecov
Copy link

codecov bot commented Jun 4, 2021

Codecov Report

Merging #56 (21d3f4a) into master (e27008c) will not change coverage.
The diff coverage is 92.85%.

Impacted file tree graph

@@            Coverage Diff            @@
##             master      #56   +/-   ##
=========================================
  Coverage     78.92%   78.92%           
  Complexity      151      151           
=========================================
  Files            35       35           
  Lines           522      522           
  Branches         20       20           
=========================================
  Hits            412      412           
  Misses          100      100           
  Partials         10       10           
Impacted Files Coverage Δ
...pnet/mailservice/service/impl/mail/AbstractMail.kt 70.37% <50.00%> (ø)
...ampnet/mailservice/config/ApplicationProperties.kt 100.00% <100.00%> (ø)
...ailservice/service/impl/LinkResolverServiceImpl.kt 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b9967c0...21d3f4a. Read the comment docs.

src/main/resources/application.properties Outdated Show resolved Hide resolved
com.ampnet.mailservice.mail.wallet-activated-path=dash/wallet
com.ampnet.mailservice.mail.manage-project-path=manage_project
com.ampnet.mailservice.mail.overview-path=overview
com.ampnet.mailservice.mail.manage-project-path=dash/project

Choose a reason for hiding this comment

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

This is suspicious. Why project when the "manage project" screen is here: https://staging.ampnet.io/ampnet/dash/projects/5aeda05e-1ba4-4be9-8601-deb89cf9cc24/edit?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is renamed to project-path, and it is used like this: dash/project + /{projectUuid}.
There is no needed to force the user to edit screen.

Choose a reason for hiding this comment

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

dash/project route doesn't exist. dash/projects and dash/projects/:uuid do. (singular -> plural) is the issue.

Copy link
Contributor Author

@githinho githinho Jun 4, 2021

Choose a reason for hiding this comment

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

What is the difference between dash/projects/:uuid and dash/offers/:uuid? And which one to use?

Copy link

@mightymatth mightymatth Jun 4, 2021

Choose a reason for hiding this comment

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

dash/projects shows all the projects (and groups) that can be managed from a project owner's perspective. (suited for project owners and group owners/managers).

dash/offers shows the list of all active projects that are available to invest in (suited for investors).

dash/offers/:uuid and dash/projects/:uuid are mirrored and show the project page for investors. The mirroring thing has a purpose.

We wanted to split responsibility for investors (offers) and project maintainers (projects).

I wanted to make it look better from the project manager perspective:

Bad navigation (pay attention to offers segment):

  • Projects screen: dash/projects
  • Project screen: dash/offers/:uuid
  • Project edit screen dash/projects/:uuid/edit

Better navigation:

  • Projects screen: dash/projects
  • Project screen: dash/projects/:uuid
  • Project edit screen dash/projects/:uuid/edit

It depends on perspective which route is semantically better.
In your case of managing a project, it is better to route to dash/projects or dash/projects/:uuid.

Does it help?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Awesome! I will use: dash/projects + /:uuid.

@githinho githinho merged commit 0ab71e3 into master Jun 4, 2021
@githinho githinho deleted the sd-127-fix-links-in-email branch June 4, 2021 11:31
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.

3 participants