UI template without links for Organisation Details #53

SocFoot · 2023-12-22T14:52:04Z

model and template

template model and spec

Context

In this pr we are introduce the first part of organisation details we don't have data yet for Special educational needs and disabilities and additional details.
Our goal is to have the result looking the closest to the demo app

Changes proposed in this pull request

We created a component, we created a page and test related to the page.

Guidance to review

Check demo app and compare it to the screenshoot or local

Link to Trello card

Ticket

Things to check

If the code removes any existing feature flags, a data migration has also been added to delete the entry from the database
This code does not rely on migrations in the same Pull Request
If this code includes a migration adding or changing columns, it also backfills existing records for consistency
If this code adds a column to the DB, decide whether it needs to be in analytics yml file or analytics blocklist
API release notes have been updated if necessary
If it adds a significant user-facing change, is it documented in the CHANGELOG?
Required environment variables have been updated or added to the Azure KeyVault

Screenshots

app/assets/stylesheets/application.sass.scss

app/controllers/claims/pages_controller.rb

app/helpers/navigation_bar_helper.rb

spec/factories/gias_schools.rb

elceebee

A few translation things which aren't a big deal, but I definitely think the organisation needs to be the School model, not the GiasSchool.

I've tested it locally and it works as expected.

app/components/navigation_bar.html.erb

app/views/claims/pages/index.html.erb

spec/features/personas/sign_in_as_a_claims_user_persona_spec.rb

spec/system/home_page_spec.rb

CatalinVoineag

Good work. I'm a bit confused on the user flow we need here. Maybe you didn't pull the latest version of main. But I expected to have a organisations#show controller and route here so we can redirect the user to the organisations details from here if he has multiple organisations or from here if he has only one organisation.

Happy to jump on a call for more detail.

app/components/navigation_bar.rb

app/views/claims/pages/index.html.erb

app/controllers/claims/pages_controller.rb

app/helpers/navigation_bar_helper.rb

app/views/claims/pages/index.html.erb

app/assets/stylesheets/application.sass.scss

app/helpers/navigation_bar_helper.rb

app/views/claims/pages/index.html.erb

app/views/layouts/application.html.erb

spec/features/personas/sign_in_as_a_claims_user_persona_spec.rb

spec/system/claims/view_organisations_spec.rb

spec/system/home_page_spec.rb

app/controllers/claims/pages_controller.rb

app/components/application_component.rb

app/views/claims/pages/index.html.erb

Comments and suggestions only, don't want to be a blocker

CatalinVoineag · 2024-01-03T13:09:06Z

Good work. I'm a bit confused on the user flow we need here. Maybe you didn't pull the latest version of main. But I expected to have a organisations#show controller and route here so we can redirect the user to the organisations details from here if he has multiple organisations or from here if he has only one organisation.

Happy to jump on a call for more detail.

This has been changed to schools#index now for the claims domain #64

Nitemaeric · 2024-01-04T12:02:22Z

app/controllers/claims/pages_controller.rb

@@ -1,2 +1,5 @@
 class Claims::PagesController < ApplicationController
+  def index
+    @user = User.first


I've just realised. Is this the correct page?

The organisation details should be for the /schools/:school_id page, right?

app/views/claims/pages/index.html.erb

spec/system/home_page_spec.rb

Nitemaeric

The user redirection flow will be

Sign In -> Organisations -> Organisation (if a user only has 1 Org)

This will also handle the case of when a user lands on the Organisations page and they will be redirected.

For now, we'll keep the root path in, but we can remove it once we know what we want to do for sure.

Nitemaeric · 2024-01-04T17:25:20Z

app/views/claims/pages/index.html.erb

Please remove this deletion from the set of changes.

This also has not been addressed.

Nitemaeric · 2024-01-04T17:25:36Z

app/controllers/claims/pages_controller.rb

This file also doesn't need to change

This hasn't been addressed.

Nitemaeric · 2024-01-04T17:26:19Z

config/routes/claims.rb

@@ -1,7 +1,7 @@
 scope module: :claims, as: :claims, constraints: { host: ENV["CLAIMS_HOST"] } do
-  root to: "pages#index"
+  root to: "schools#show"


Please leave this as pages#index. We will update the after_sign_in_path in another PR.

It wasn't clear, it would be good in the future to put these indication in the ticket.

spec/system/claims/pages/home_spec.rb

Nitemaeric · 2024-01-04T18:37:41Z

app/controllers/claims/pages_controller.rb

This hasn't been addressed.

Nitemaeric · 2024-01-04T18:37:53Z

app/views/claims/pages/index.html.erb

This also has not been addressed.

app/views/claims/schools/show.html.erb

config/locales/en.yml

Nitemaeric · 2024-01-05T09:29:48Z

app/views/claims/schools/show.html.erb

+<div class="govuk-grid-row">
+<br><br>
+    <div class="govuk-grid-column-two-thirds">
+    <h1 class="govuk-heading-l"><%= t("claims.pages.index.heading") %></h1>


Why is this page pointing to the index controller action? 😅

spec/system/claims/view_school_spec.rb

Nitemaeric · 2024-01-05T09:32:06Z

spec/system/home_page_spec.rb

I think you can remove the added methods from this spec.

Nitemaeric · 2024-01-05T15:24:45Z

config/locales/en.yml

@@ -71,6 +71,15 @@ en:
      heading: Placements news and updates
    contents: Contents
  claims:
+    schools:
+      index:


Suggested change

index:

show:

Nitemaeric · 2024-01-05T15:25:13Z

spec/system/claims/view_school_spec.rb

@@ -0,0 +1,54 @@
+require "rails_helper"
+
+RSpec.feature "School Page" do


Suggested change

RSpec.feature "School Page" do

RSpec.describe "School Page", type: :system do

Nitemaeric · 2024-01-05T15:25:31Z

spec/system/claims/view_school_spec.rb

+require "rails_helper"
+
+RSpec.feature "School Page" do
+  after { Capybara.app_host = nil }


Once the above is added, this is already handled by the Rails Helper.

Nitemaeric · 2024-01-05T15:26:19Z

spec/system/claims/view_school_spec.rb

+  def given_i_am_on_the_claims_site
+    Capybara.app_host = "http://#{ENV["CLAIMS_HOST"]}"
+  end


This is already handled by the services test helper that is included into System specs.

Nitemaeric

Thanks for going through each of the issues!

model and template template model and spec rubocop fix test fix request changes lint request changes, add empty claim page, update test and routes request changes

SocFoot force-pushed the add_organisation_details branch 3 times, most recently from ad32226 to 3063772 Compare December 22, 2023 15:46

SocFoot requested review from elceebee, CatalinVoineag, gms-gs and JamieCleare2525 and removed request for elceebee and CatalinVoineag December 22, 2023 16:02

JamieCleare2525 reviewed Dec 22, 2023

View reviewed changes

app/assets/stylesheets/application.sass.scss Outdated Show resolved Hide resolved

JamieCleare2525 reviewed Dec 22, 2023

View reviewed changes

app/controllers/claims/pages_controller.rb Outdated Show resolved Hide resolved

JamieCleare2525 reviewed Dec 22, 2023

View reviewed changes

app/helpers/navigation_bar_helper.rb Outdated Show resolved Hide resolved

JamieCleare2525 reviewed Dec 22, 2023

View reviewed changes

spec/factories/gias_schools.rb Outdated Show resolved Hide resolved

elceebee previously requested changes Jan 2, 2024

View reviewed changes

CatalinVoineag reviewed Jan 2, 2024

View reviewed changes

SocFoot force-pushed the add_organisation_details branch 5 times, most recently from 84277e7 to e488d09 Compare January 2, 2024 17:53

JamieCleare2525 reviewed Jan 3, 2024

View reviewed changes

app/controllers/claims/pages_controller.rb Outdated Show resolved Hide resolved

app/helpers/navigation_bar_helper.rb Outdated Show resolved Hide resolved

app/views/claims/pages/index.html.erb Outdated Show resolved Hide resolved

Nitemaeric reviewed Jan 3, 2024

View reviewed changes

elceebee reviewed Jan 3, 2024

View reviewed changes

app/components/application_component.rb Outdated Show resolved Hide resolved

app/views/claims/pages/index.html.erb Outdated Show resolved Hide resolved

SocFoot force-pushed the add_organisation_details branch from e488d09 to 9a26afd Compare January 3, 2024 11:23

elceebee self-requested a review January 3, 2024 12:26

SocFoot force-pushed the add_organisation_details branch from 9a26afd to 86f1ebd Compare January 3, 2024 13:04

SocFoot force-pushed the add_organisation_details branch from 86f1ebd to a53d41b Compare January 3, 2024 17:26

SocFoot force-pushed the add_organisation_details branch 3 times, most recently from 328d802 to 8e90969 Compare January 4, 2024 11:49

Nitemaeric requested changes Jan 4, 2024

View reviewed changes

SocFoot changed the title ~~template without links~~ add_organisation_details template without links Jan 4, 2024

SocFoot changed the title ~~add_organisation_details template without links~~ UI template without links for Organisation Details Jan 4, 2024

SocFoot force-pushed the add_organisation_details branch from 8e90969 to 1522e51 Compare January 4, 2024 16:19

SocFoot requested review from a team as code owners January 4, 2024 16:19

SocFoot force-pushed the add_organisation_details branch from 1522e51 to c672260 Compare January 4, 2024 17:17

Nitemaeric requested changes Jan 4, 2024

View reviewed changes

SocFoot force-pushed the add_organisation_details branch from 3d59381 to b0060c9 Compare January 4, 2024 18:17

Nitemaeric requested changes Jan 4, 2024

View reviewed changes

SocFoot force-pushed the add_organisation_details branch from 709e0d1 to 3ec8e22 Compare January 4, 2024 19:06

Nitemaeric requested changes Jan 5, 2024

View reviewed changes

SocFoot force-pushed the add_organisation_details branch 2 times, most recently from 876c929 to 8e74878 Compare January 5, 2024 15:24

Nitemaeric reviewed Jan 5, 2024

View reviewed changes

SocFoot force-pushed the add_organisation_details branch from 8e74878 to 224d5d3 Compare January 5, 2024 15:35

Nitemaeric approved these changes Jan 5, 2024

View reviewed changes

SocFoot force-pushed the add_organisation_details branch from 224d5d3 to 5a41526 Compare January 5, 2024 15:39

template without links

5a41526

model and template template model and spec rubocop fix test fix request changes lint request changes, add empty claim page, update test and routes request changes

SocFoot merged commit 9f4e937 into main Jan 5, 2024
3 checks passed

SocFoot deleted the add_organisation_details branch January 5, 2024 15:41

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

UI template without links for Organisation Details #53

UI template without links for Organisation Details #53

SocFoot commented Dec 22, 2023 •

edited

Loading

elceebee left a comment

CatalinVoineag left a comment

CatalinVoineag commented Jan 3, 2024 •

edited

Loading

Nitemaeric Jan 4, 2024

Nitemaeric left a comment

Nitemaeric Jan 4, 2024

Nitemaeric Jan 4, 2024

Nitemaeric Jan 4, 2024

Nitemaeric Jan 4, 2024

Nitemaeric Jan 4, 2024

SocFoot Jan 4, 2024

Nitemaeric Jan 4, 2024

Nitemaeric Jan 4, 2024

Nitemaeric Jan 5, 2024

Nitemaeric Jan 5, 2024

Nitemaeric Jan 5, 2024

Nitemaeric Jan 5, 2024

Nitemaeric Jan 5, 2024

Nitemaeric Jan 5, 2024

Nitemaeric left a comment

		@@ -0,0 +1,54 @@
		require "rails_helper"

		RSpec.feature "School Page" do

	RSpec.feature "School Page" do
	RSpec.describe "School Page", type: :system do

UI template without links for Organisation Details #53

UI template without links for Organisation Details #53

Conversation

SocFoot commented Dec 22, 2023 • edited Loading

Context

Changes proposed in this pull request

Guidance to review

Link to Trello card

Things to check

Screenshots

elceebee left a comment

Choose a reason for hiding this comment

CatalinVoineag left a comment

Choose a reason for hiding this comment

CatalinVoineag commented Jan 3, 2024 • edited Loading

Choose a reason for hiding this comment

Nitemaeric left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Nitemaeric left a comment

Choose a reason for hiding this comment

SocFoot commented Dec 22, 2023 •

edited

Loading

CatalinVoineag commented Jan 3, 2024 •

edited

Loading