AGM Location Change 4th and final PR

JazzarKarim commented 11 months ago

Issue #: /bcgov/entity#18161

Description of changes:

Removed the option to cancel payment in case AGM Location change payment was unsuccessful
Misc. fixes

~Note: The work for this ticket is mostly done. However, I won't merge until filer and schema are merged and verifying that everything looks good. I believe things should be fine in the filings history list.~ Can merge now since I can file and it looks good.

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of the business-filings-ui license (Apache 2.0).

JazzarKarim commented 11 months ago

Update: Filer ticket has been merged. Everything looks fine! filer working

severinbeauvais commented 11 months ago

/gcbrun

bcregistry-sre commented 11 months ago

Temporary Url for review: https://business-filings-dev--pr-556-2djj8nd1.web.app

SB says: for example, https://business-filings-dev--pr-556-2djj8nd1.web.app/BC0870754

severinbeauvais commented 11 months ago

@JazzarKarim Check the red validation bars. Should they be the full height of the section? Also watch their top/bottom radii.

severinbeauvais commented 11 months ago

@JazzarKarim I noticed the SaveErrorDialog (and properties/methods) have been removed. Aren't they needed in case of a pay error, and with the ability to retry?

JazzarKarim commented 10 months ago

@JazzarKarim Check the red validation bars. Should they be the full height of the section? Also watch their top/bottom radii.

You're right Sev. Pretty sure the UX folks won't like that. I'll see if I can fix it.

JazzarKarim commented 10 months ago

@JazzarKarim I noticed the SaveErrorDialog (and properties/methods) have been removed. Aren't they needed in case of a pay error, and with the ability to retry?

Sev, your question made me notice something important. For other no fee filings (like continuation out), we don't show the StaffPaymentDialog. So, I connected with Yui and asked him if this was also the case here. He said yes.

So, I went ahead and removed the dialog and everything associated with it.

severinbeauvais commented 10 months ago

Can you have a quick look at this please?

severinbeauvais commented 10 months ago

~Hey, I'm unable to file an AGM Location Change from the temp URL, possibly because agmLocation is empty?~ should be fixed now

severinbeauvais commented 10 months ago

Sev, your question made me notice something important. For other no fee filings (like continuation out), we don't show the StaffPaymentDialog. So, I connected with Yui and asked him if this was also the case here. He said yes.

So, I went ahead and removed the dialog and everything associated with it.

Great catch! I've added the same todo item to the AGM Extension ticket.

(I hope we don't suddenly add a fee to this filing though, hahaha.)

JazzarKarim commented 10 months ago

Can you have a quick look at this please?

What's wrong here Sev?

codecov[bot] commented 10 months ago

Codecov Report

Merging #556 (8339703) into main (1ac27d5) will decrease coverage by 92.45%. Report is 31 commits behind head on main. The diff coverage is n/a.

:exclamation: Current head 8339703 differs from pull request most recent head 480e5ce. Consider uploading reports for the commit 480e5ce to get more accurate results

@@            Coverage Diff            @@
##             main   #556       +/-   ##
=========================================
- Coverage   92.44%      0   -92.45%     
=========================================
  Files         171      0      -171     
  Lines        2793      0     -2793     
  Branches      309      0      -309     
=========================================
- Hits         2582      0     -2582     
+ Misses        210      0      -210     
+ Partials        1      0        -1

see 170 files with indirect coverage changes